[FFmpeg-devel] [PATCH] vf_framestep: add blend parameter for motion blur effect

[Paul B Mahol]

On 5/31/17, Matthias Troffaes <matthias.troffaes at gmail.com> wrote:
> Dear Moritz,
>
> On Wed, May 31, 2017 at 2:17 PM, Moritz Barsnick <barsnick at gmx.net> wrote:
>> On Wed, May 31, 2017 at 13:31:14 +0100, Matthias C. M. Troffaes wrote:
>>> @@ -2,6 +2,7 @@ Entries are sorted chronologically from oldest to
>>> youngest within each release,
>>>  releases are sorted from youngest to oldest.
>>>
>>>  version <next>:
>>> +- framestep filter: add blend parameter for motion blur effect
>>>  - deflicker video filter
>>
>> Did you read the text up there? You need to add your change to the
>> bottom of the "<next>:" list.
>
> Oh dear, that's embarrassing. I must have misread the instruction.
> Thanks for pointing out - I'll fix this.
>
>>> +    int frame_blend;    ///< how many frames to blend on each step
>> [...]
>>>  static const AVOption framestep_options[] = {
>>>      { "step", "set frame step",  OFFSET(frame_step), AV_OPT_TYPE_INT,
>>> {.i64=1}, 1, INT_MAX, FLAGS},
>>> +    { "blend", "number of frames to blend per step",
>>> OFFSET(frame_blend), AV_OPT_TYPE_INT, {.i64=1}, 1, 65535, FLAGS},
>>
>> Your maximum is too high to be assigned to an int, unless you properly
>> checked that any overflow still results in correct behaviour. The upper
>> limit probably needs to be "INT_MAX" (which would be 32767, but you can
>> use the constant).
>
> On various platforms (such as a standard 64-bit Fedora install),
> INT_MAX is 2147483647 (for example see
> https://www.tutorialspoint.com/c_standard_library/limits_h.htm) and
> that value *will* potentially overflow the code as the internal buffer
> requires all values across the blend added up to fit into a uint32_t.
> To avoid overflows, the maximum value for blend therefore needs to
> satisfy the following inequality:
>
> blend_max * pix_value_max <= 2^32 - 1
>
> and since the filter supports up to 16 bit formats, this gives the
> constraint:
>
> blend_max <= (2^32 - 1) / (2^16 - 1)
>
> which is satisfied by 65535. Technically 65537 would work as well.
>
> Shall I change AV_OPT_TYPE_INT into AV_OPT_TYPE_INT64 for both
> parameters, use INT64_MAX for the maximum value of "step", and
> UINT16_MAX for the maximum value of "blend"? That should eliminate a
> possible overflow on those platforms where INT_MAX is only 32767.
>
>>>      AVFilterContext *ctx = outlink->src;
>>> -    FrameStepContext *framestep = ctx->priv;
>>> -    AVFilterLink *inlink = ctx->inputs[0];
>>> +    const FrameStepContext *s = ctx->priv;
>>> +    const AVFilterLink *inlink = ctx->inputs[0];
>>>
>>>      outlink->frame_rate =
>>> -        av_div_q(inlink->frame_rate, (AVRational){framestep->frame_step,
>>> 1});
>>> +        av_div_q(inlink->frame_rate, (AVRational){s->frame_step, 1});
>>>
>>>      av_log(ctx, AV_LOG_VERBOSE, "step:%d frame_rate:%d/%d(%f) ->
>>> frame_rate:%d/%d(%f)\n",
>>> -           framestep->frame_step,
>>> +           s->frame_step,
>>>             inlink->frame_rate.num, inlink->frame_rate.den,
>>> av_q2d(inlink->frame_rate),
>>>             outlink->frame_rate.num, outlink->frame_rate.den,
>>> av_q2d(outlink->frame_rate));
>>> +
>>>      return 0;
>>>  }
>>
>> Isn't this just a rename and const-ification? This probably doesn't
>> belong into this patch (perhaps a separate one, if at all.) And it adds
>> an empty line, which certainly doesn't belong into a functional patch.
>
> Sure, I'll split it off into a separate patch. The use of "framestep"
> for the context is very confusing with frame_step already being used
> for the actual frame step value. Other filters use "s" for the context
> and this simply brings the code in line with naming conventions
> elsewhere. It seemed like a good thing to include, but I agree it's
> not functional, just code cleanup.
>
> Thank you for the prompt feedback!

This code does not belong in this filter.

Make new filter instead.