On Tue, May 10, 2016 at 4:33 AM, Paul B Mahol <one...@gmail.com> wrote:
> On 5/10/16, Kyle Swanson <k...@ylo.ph> wrote:
>> Hi,
>>
>> Updated patch attached. Thanks!
>>
>
>
> [...]
>
>>
>> +@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
>> +@code{--enable-libebur128}.
>> +
>> +The filter accepts the following options:
>> +
>> +@table @option
>> +@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@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@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
