[FFmpeg-devel] [PATCH 2/3] lavfi/loudnorm: add an internal libebur128 library

[Paul B Mahol]

On 11/10/16, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> On Thu, Nov 10, 2016 at 5:32 PM, Kyle Swanson <k at ylo.ph> wrote:
>> On Tue, Nov 8, 2016 at 9:39 AM, Kyle Swanson <k at ylo.ph> wrote:
>>> On Mon, Nov 7, 2016 at 6:00 PM, Marton Balint <cus at passwd.hu> wrote:
>>>>
>>>> On Fri, 4 Nov 2016, Marton Balint wrote:
>>>>
>>>>>
>>>>> On Thu, 3 Nov 2016, Hendrik Leppkes wrote:
>>>>>
>>>>>> On Mon, Oct 17, 2016 at 5:20 PM, Moritz Barsnick <barsnick at gmx.net>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Mon, Oct 17, 2016 at 17:09:15 +0200, wm4 wrote:
>>>>>>>>
>>>>>>>> Does this copy parts of libebur128 to FFmpeg?
>>>>>>>> Why?
>>>>>>>
>>>>>>>
>>>>>>> There was a long discussion regarding this patch:
>>>>>>>
>>>>>>> http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2016-April/192668.html
>>>>>>>
>>>>>>> (in summary: "please don't require yet another small external
>>>>>>> library,
>>>>>>> rather port it to ffmpeg and maintain it") leading to this one:
>>>>>>>
>>>>>>
>>>>>> The generic idea was not to just copy/paste an external library into
>>>>>> internal code, but extend the ebur128 code we already have - at least
>>>>>> that way we get code written by one of our maintainers, code he knows
>>>>>> and can properly maintain.
>>>>>
>>>>>
>>>>> I copied the external library because we needed an API. The way the
>>>>> internals work, I used the library code because it was simply easier,
>>>>> than
>>>>> factoring out f_ebur128 stuff, also there are some features which
>>>>> f_ebur128.c does not have (variable sample rate support), and there was
>>>>> the
>>>>> licensing issue GPL v.s. LGPL.
>>>>>
>>>>>> If you just copy the implementation of a library, you might as well
>>>>>> just use that library - thats what it exists for. Why do we want to
>>>>>> increase the maintenance burden of our project when other people (ie.
>>>>>> the authors of libebur128) are already doing it as well?
>>>>>
>>>>>
>>>>> In general I agree with people who think that for small code, it is
>>>>> better
>>>>> to integrate it into our codebase, because
>>>>> - it can benefit from features we already have (e.g. resampling)
>>>>> - makes it easier for developers to work on features based on this
>>>>> - code gets more review than code in a small 3rd party library
>>>>> - code and/or improvements have stronger requirements performance-wise
>>>>> - code is better audited (Coverity, etc)
>>>>>
>>>>> Yes, some additional maintenance burden is the price we pay for this,
>>>>> which is IMHO in this case is acceptable.
>>>>>
>>>>
>>>> Is it fine to apply, or we should put this to a vote?
>>>
>>> Give me another day to review the patch. Meant look at this last weekend.
>>
>> These patches look good to me. If we're going to do this, we really
>> need to keep the true peak mode in the libebur128 port. This is a huge
>> part of the R128 spec, and it's important that it stays in. Of course
>> redundant code is bad, so we'll need to update f_ebur128 as well
>> (which has a true peak requirement.) I already have a patch for
>> f_ebur128. Marton, it might be easier for you to update the patch to
>> include true peak mode but I could do it as well. It'd be interesting
>> to benchmark the libebur128 FIR resampler vs. swresample.
>
> If this gets re-added, please make it use our resampler unless there
> are good technical reasons against that (algorithm wise).
> Keeping an extra small resampler just for some measurements seems like
> something that can be surely avoided.

Yes, I also agree. Is such small resampler really needed?