On 5/10/16, Kyle Swanson <k...@ylo.ph> wrote:
> 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' ?
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@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
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
