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

[Hendrik Leppkes]

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.

- Hendrik