[FFmpeg-devel] [PATCH] avfilter: add loudnorm

Paul B Mahol onemda at gmail.com
Tue May 10 20:45:53 CEST 2016


On 5/10/16, Kyle Swanson <k at ylo.ph> wrote:
> On Tue, May 10, 2016 at 4:33 AM, Paul B Mahol <onemda at gmail.com> wrote:
>> On 5/10/16, Kyle Swanson <k at ylo.ph> wrote:
>>> Hi,
>>>
>>> Updated patch attached. Thanks!
>>>
>>
>>
>> [...]
>>
>>>
>>> + at section loudnorm
>>> +
>>> +EBU R128 loudness normalization. Includes both dynamic and linear
>>> normalization modes.
>>> +Support for both single pass (livestreams, files) and double pass
>>> (files) modes.
>>> +This algorithm can target IL, LRA, and maximum true peak.
>>> +
>>> +To enable compilation of this filter you need to configure FFmpeg with
>>> + at code{--enable-libebur128}.
>>> +
>>> +The filter accepts the following options:
>>> +
>>> + at table @option
>>> + at item I, i
>>> +Set integrated loudness target
>>
>> Could you document range and default values here and below?
>>
>> [...]
>>
>>> +    ebur128_state *r128_in;
>>> +    ebur128_state *r128_out;
>>> +} LoudNormContext;
>>> +
>>> +enum {
>>> +    FIRST_FRAME,
>>> +    INNER_FRAME,
>>> +    FINAL_FRAME,
>>> +    LINEAR_MODE
>>> +};
>>
>> Can't you name those enums? And add NB as last one?
>
> Sure, I'll do that. What do you mean by `NB' ?

FRAME_NB, its just nitpicking, so when new field to enum is added you don't need
to update options table, because max value there is set to FRAME_NB-1.

>
>>
>> [...]
>>
>>> +static int config_input(AVFilterLink *inlink)
>>> +{
>>> +    AVFilterContext *ctx = inlink->dst;
>>> +    LoudNormContext *s = ctx->priv;
>>> +
>>> +    s->r128_in = av_malloc((size_t) sizeof(ebur128_state*));
>>> +    if (!s->r128_in)
>>> +        return AVERROR(ENOMEM);
>>> +    s->r128_in = ebur128_init(inlink->channels, inlink->sample_rate,
>>> EBUR128_MODE_I | EBUR128_MODE_S | EBUR128_MODE_LRA |
>>> EBUR128_MODE_SAMPLE_PEAK);
>>> +
>>
>> Can this fail? One should check return value.
>> Doesn't this leaks memory?
>
> I already check the malloc, and it doesn't look like ebur128_init()
> can fail in any way here.

But you overwrite s->r128_in.

>
>>
>>> +    s->r128_out = av_malloc((size_t) sizeof(ebur128_state*));
>>> +    if (!s->r128_out)
>>> +        return AVERROR(ENOMEM);
>>> +    s->r128_out = ebur128_init(inlink->channels, inlink->sample_rate,
>>> EBUR128_MODE_I | EBUR128_MODE_S | EBUR128_MODE_LRA |
>>> EBUR128_MODE_SAMPLE_PEAK);
>>
>> ditto.
>>
>> [...]
>>
>>> +static const AVFilterPad avfilter_af_loudnorm_outputs[] = {
>>> +    {
>>> +        .name = "default",
>>> +        .request_frame = request_frame,
>>> +        .type = AVMEDIA_TYPE_AUDIO,
>>
>> Vertical alignment please.
>>
>> [...]
>>
>> rest LGTM
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Thanks for the review. If someone can answer my question about the
> enum I'll send a new patch later today.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list