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

[Matthias Troffaes]

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!

Kind regards,
Matthias