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

[Kyle Swanson]

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' ?

>
> [...]
>
>> +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.

>
>> +    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.