[FFmpeg-devel] [PATCH 2/2] libavfilter/vf_fps: Rewrite using activate callback

[Calvin Walton]

Just a few comments on your review:

On Sun, 2018-02-18 at 19:01 +0100, Nicolas George wrote:
> > @@ -91,31 +94,46 @@ static av_cold int init(AVFilterContext *ctx)
> >  {
> >      FPSContext *s = ctx->priv;
> >  
> > -    if (!(s->fifo = av_fifo_alloc_array(2, sizeof(AVFrame*))))
> > -        return AVERROR(ENOMEM);
> > +    /* Pass INT64_MIN/MAX through unchanged to avoid special cases
> > for AV_NOPTS_VALUE */
> > +    s->rounding = s->rounding | AV_ROUND_PASS_MINMAX;
> 
> Since rounding is exposed as an option with explicit bounds, it would
> probably be better to keep AV_ROUND_PASS_MINMAX out of the field and
> only include it when actually calling av_rescale_q_rnd().

Makes sense, easy change.

> > @@ -128,194 +146,238 @@ static int config_props(AVFilterLink* link)
> >  
> >      link->time_base = av_inv_q(s->framerate);
> >      link->frame_rate= s->framerate;
> > -    link->w         = link->src->inputs[0]->w;
> > -    link->h         = link->src->inputs[0]->h;
> 
> Looks unrelated.

I can split this into a separate cleanup patch, then.

> > +
> > +    /* Convert the start time option to output timebase and save
> > it */
> > +    if (s->start_time != DBL_MAX && s->start_time !=
> > AV_NOPTS_VALUE) {
> > +        double first_pts = s->start_time * AV_TIME_BASE;
> > +        first_pts = FFMIN(FFMAX(first_pts, INT64_MIN), INT64_MAX);
> 
> It would probably better to issue an error when the value is out of
> representable range.

I'll make this a fatal error. I considered adjusting the range of
accepted values on the AVOption, but it would be tricky to get right,
with rounding issues and whatnot (and I'm not sure that using DBL_MAX
as an invalid/default value would still work).

> 
> > +        s->start_pts = av_rescale_q_rnd(first_pts, AV_TIME_BASE_Q,
> > +                link->time_base, s->rounding);
> 
> Nit: indentation.

Do you prefer the style of matching the indentation level of wrapped
parameters to the ( of the function? I can do that, I'll try to make it
consistent in the file.

> > -    return ret;
> > -}
> > +    ret = ff_inlink_consume_frame(inlink, &frame);
> > +    /* Caller must have run ff_inlink_check_available_frame first
> > */
> > +    av_assert1(ret);
> 
> Negative error codes must still be checked.

Ah, took me a moment looking at this function's return values again to
understand why this was needed. I'll add the error handling code.

> > +
> > +        ret = ff_filter_frame(outlink, frame);
> > +        if (ret >= 0) {
> > +            s->frames_out++;
> > +            s->dup += dup;
> > +        }
> 
> Minor nit: I would rather have "if (ret < 0) return ret;" and make
> the
> incrementation unconditional.

Making the incrementation unconditional simplifies the flow quite a
bit, I'll make that change.

> > [static int void write_frame_eof()]

> This whole function seems to implement the very same logic as
> write_frame() with just s->status_pts instead of s->frames[1]->pts. It
> should be factored.

I thought about doing this, but it'll make the conditionals quite a bit
more complicated. I'll spend some time trying to figure out a better
way to handle this.

> > +static void update_eof_pts(AVFilterContext *ctx, FPSContext *s, AVFilterLink *inlink, AVFilterLink *outlink)
> > +{
> > +    /* Convert status_pts to outlink timebase */
> > +    int eof_rounding = (s->eof_action == EOF_ACTION_PASS) ? AV_ROUND_UP : s->rounding;
> > +    s->status_pts = av_rescale_q_rnd(s->status_pts, inlink->time_base,
> > +            outlink->time_base, eof_rounding);
> 
> Nit: indentation. Also, I do not like the fact that s->status_pts can be
> in different time bases depending on the part of the code. I would
> prefer if ff_inlink_acknowledge_status() is used with a temp variable,
> passed as argument to update_eof_pts(), and only the s->status_pts is
> set.

This is something that was bugging me a bit as well, I'll make the
change.

> >  
> > -        s->frames_out++;
> > +        /* Buffered frames are available, so generate an output frame */
> > +        if (s->frames_count == 2 && ff_outlink_frame_wanted(outlink) >= 0) {
> 
> Do not check ff_outlink_frame_wanted() here: if the filter is activated
> and can produce a frame, produce it.

Alright.

> 
> > +            ret = write_frame(ctx, s, outlink);
> > +            /* Couldn't generate a frame: e.g. a frame was dropped */
> > +            if (ret == AVERROR(EAGAIN)) {
> > +                /* Schedule us to perform another step */
> > +                ff_filter_set_ready(ctx, 100);
> > +                return 0;
> > +            }
> 
> I do not like the use of EAGAIN for this. It may conflict with strange
> error codes returned by other filters. It should not happen, but it
> could. I would advice to define a specific error code for this file
> only, like FFERROR_REDO in libavformat/internal.h. Or, even better, add
> "int *again" as an extra argument to write_frame() and return the
> condition like that.

I like the solution with adding an extra return argument to the
function to keep this status separate from the error return codes. I'll
make this change.


-- 
Calvin Walton <calvin.walton at kepstin.ca>