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

[wm4]

On Wed, 31 May 2017 20:59:07 +0200
Paul B Mahol <onemda at gmail.com> wrote:

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

I kind of agree. vf_framestep is not even supposed to be a framerate
conversion filter. It merely skips frames by a fixed pattern or
depending on frame type. It also can work on any format because it does
not touch the actual image data. This patch changes most of these things
(even if it's "optional").

Also, I wonder if we don't already have other filters for framerate
conversion. Though each filter with fundamentally different, you know,
filtering, should probably be separate.