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

The old version of the filter had a problem where it would queue up
all of the duplicate frames required to fill a timestamp gap in a
single call to filter_frame. In problematic files - I've hit this in
webcam streams with large gaps due to network issues - this will queue
up a potentially huge number of frames. (I've seen it trigger the Linux
OOM-killer on particularly large pts gaps.)

This revised version of the filter using the activate callback will
generate at most 1 frame each time it is called, and respects output
links having the frame_wanted_out set to a negative number to indicate
that they don't want frames.

TODO: This is still a work in progress. It may have different behaviour
in some cases from the old fps filter. I have not yet implemented the
"eof_action" option, since I haven't figured out what it's supposed to
do.
---
 libavfilter/vf_fps.c | 398 +++++++++++++++++++++++++++++----------------------
 1 file changed, 230 insertions(+), 168 deletions(-)

Oops, I forgot to remove this bit from the changelog:

On Fri, 2018-02-16 at 15:02 -0500, Calvin Walton wrote:
> TODO: This is still a work in progress. It may have different
> behaviour
> in some cases from the old fps filter. I have not yet implemented the
> "eof_action" option, since I haven't figured out what it's supposed
> to
> do.

At this point I'm fairly confident that the behaviour matches the
existing filter, and that I've gotten the eof_action behaviour correct
as well.

Cheers!
Calvin.

Calvin Walton (2018-02-16):
> The old version of the filter had a problem where it would queue up
> all of the duplicate frames required to fill a timestamp gap in a
> single call to filter_frame. In problematic files - I've hit this in
> webcam streams with large gaps due to network issues - this will queue
> up a potentially huge number of frames. (I've seen it trigger the Linux
> OOM-killer on particularly large pts gaps.)
> 
> This revised version of the filter using the activate callback will
> generate at most 1 frame each time it is called, and respects output
> links having the frame_wanted_out set to a negative number to indicate
> that they don't want frames.

Thanks for the patch. It was something I had in my TODO list for a long
time. The code looks very good. Here are a few comments below. Of course
open to discussion.

> 
> TODO: This is still a work in progress. It may have different behaviour
> in some cases from the old fps filter. I have not yet implemented the
> "eof_action" option, since I haven't figured out what it's supposed to
> do.
> ---
>  libavfilter/vf_fps.c | 398 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 230 insertions(+), 168 deletions(-)
> 
> diff --git a/libavfilter/vf_fps.c b/libavfilter/vf_fps.c
> index dbafd2c35a..16e432db0b 100644
> --- a/libavfilter/vf_fps.c
> +++ b/libavfilter/vf_fps.c
> @@ -2,6 +2,7 @@
>   * Copyright 2007 Bobby Bingham
>   * Copyright 2012 Robert Nagy <ronag89 gmail com>
>   * Copyright 2012 Anton Khirnov <anton khirnov net>
> + * Copyright 2018 Calvin Walton <calvin.walton@kepstin.ca>
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -28,17 +29,12 @@
>  #include <float.h>
>  #include <stdint.h>
>  
> -#include "libavutil/common.h"
> -#include "libavutil/fifo.h"
> +#include "libavutil/avassert.h"
>  #include "libavutil/mathematics.h"
>  #include "libavutil/opt.h"
> -#include "libavutil/parseutils.h"
> -
> -#define FF_INTERNAL_FIELDS 1
> -#include "framequeue.h"
>  #include "avfilter.h"
> +#include "filters.h"
>  #include "internal.h"
> -#include "video.h"
>  
>  enum EOFAction {
>      EOF_ACTION_ROUND,
> @@ -49,17 +45,24 @@ enum EOFAction {
>  typedef struct FPSContext {
>      const AVClass *class;
>  
> -    AVFifoBuffer *fifo;     ///< store frames until we get two successive timestamps
> -
> -    /* timestamps in input timebase */
> -    int64_t first_pts;      ///< pts of the first frame that arrived on this filter
> -
> +    /* Options */
>      double start_time;      ///< pts, in seconds, of the expected first frame
> -
>      AVRational framerate;   ///< target framerate
>      int rounding;           ///< AVRounding method for timestamps
>      int eof_action;         ///< action performed for last frame in FIFO
>  
> +    /* Set during outlink configuration */
> +    int64_t  start_pts;     ///< pts of the expected first frame
> +
> +    /* Runtime state */
> +    int      status;        ///< buffered input status
> +    int64_t  status_pts;    ///< buffered input status timestamp
> +
> +    AVFrame *frames[2];     ///< buffered frames
> +    int      frames_count;  ///< number of buffered frames
> +
> +    int64_t  next_pts;      ///< pts of the next frame to output
> +
>      /* statistics */
>      int frames_in;             ///< number of frames on input
>      int frames_out;            ///< number of frames on output
> @@ -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().

>  
> -    s->first_pts    = AV_NOPTS_VALUE;
> +    s->start_pts    = AV_NOPTS_VALUE;
> +    s->status_pts   = AV_NOPTS_VALUE;
> +    s->next_pts     = AV_NOPTS_VALUE;
>  
>      av_log(ctx, AV_LOG_VERBOSE, "fps=%d/%d\n", s->framerate.num, s->framerate.den);
>      return 0;
>  }
>  
> -static void flush_fifo(AVFifoBuffer *fifo)
> +/* Remove the first frame from the buffer, returning it */
> +static AVFrame *shift_frame(FPSContext *s)
>  {
> -    while (av_fifo_size(fifo)) {
> -        AVFrame *tmp;
> -        av_fifo_generic_read(fifo, &tmp, sizeof(tmp), NULL);
> -        av_frame_free(&tmp);
> -    }
> +    AVFrame *frame;
> +
> +    /* Must only be called when there are frames in the buffer */
> +    av_assert1(s->frames_count > 0);
> +
> +    frame = s->frames[0];
> +    s->frames[0] = s->frames[1];
> +    s->frames[1] = NULL;
> +    s->frames_count--;
> +
> +    return frame;
>  }
>  
>  static av_cold void uninit(AVFilterContext *ctx)
>  {
>      FPSContext *s = ctx->priv;
> -    if (s->fifo) {
> -        s->drop += av_fifo_size(s->fifo) / sizeof(AVFrame*);
> -        flush_fifo(s->fifo);
> -        av_fifo_freep(&s->fifo);
> +
> +    AVFrame *frame;
> +
> +    /* Free any remaining buffered frames. This only happens if a downstream
> +     * filter has asked us to stop, so don't count them as dropped. */
> +    av_log(ctx, AV_LOG_DEBUG, "Discarding %d buffered frame(s) at exit.\n",
> +            s->frames_count);
> +    while (s->frames_count > 0) {
> +        frame = shift_frame(s);
> +        av_frame_free(&frame);
>      }
>  
>      av_log(ctx, AV_LOG_VERBOSE, "%d frames in, %d frames out; %d frames dropped, "
> @@ -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.

> +
> +    /* 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.

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

Nit: indentation.

> +    }
>  
>      return 0;
>  }
>  
> -static int request_frame(AVFilterLink *outlink)
> +/* Read a frame from the input and save it in the buffer */
> +static void read_frame(AVFilterContext *ctx, FPSContext *s, AVFilterLink *inlink, AVFilterLink *outlink)
>  {
> -    AVFilterContext *ctx = outlink->src;
> -    FPSContext        *s = ctx->priv;
> +    AVFrame *frame;
>      int ret;
> +    int64_t in_pts;
>  
> -    ret = ff_request_frame(ctx->inputs[0]);
> -
> -    /* flush the fifo */
> -    if (ret == AVERROR_EOF && av_fifo_size(s->fifo)) {
> -        int i;
> -        for (i = 0; av_fifo_size(s->fifo); i++) {
> -            AVFrame *buf;
> -
> -            av_fifo_generic_read(s->fifo, &buf, sizeof(buf), NULL);
> -            if (av_fifo_size(s->fifo)) {
> -                buf->pts = av_rescale_q(s->first_pts, ctx->inputs[0]->time_base,
> -                                        outlink->time_base) + s->frames_out;
> -
> -                if ((ret = ff_filter_frame(outlink, buf)) < 0)
> -                    return ret;
> -
> -                s->frames_out++;
> -            } else {
> -                /* This is the last frame, we may have to duplicate it to match
> -                 * the last frame duration */
> -                int j;
> -                int eof_rounding = (s->eof_action == EOF_ACTION_PASS) ? AV_ROUND_UP : s->rounding;
> -                int delta = av_rescale_q_rnd(ctx->inputs[0]->current_pts - s->first_pts,
> -                                             ctx->inputs[0]->time_base,
> -                                             outlink->time_base, eof_rounding) - s->frames_out;
> -                av_log(ctx, AV_LOG_DEBUG, "EOF frames_out:%d delta:%d\n", s->frames_out, delta);
> -                /* if the delta is equal to 1, it means we just need to output
> -                 * the last frame. Greater than 1 means we will need duplicate
> -                 * delta-1 frames */
> -                if (delta > 0 ) {
> -                    for (j = 0; j < delta; j++) {
> -                        AVFrame *dup = av_frame_clone(buf);
> -
> -                        av_log(ctx, AV_LOG_DEBUG, "Duplicating frame.\n");
> -                        dup->pts = av_rescale_q(s->first_pts, ctx->inputs[0]->time_base,
> -                                                outlink->time_base) + s->frames_out;
> -
> -                        if ((ret = ff_filter_frame(outlink, dup)) < 0)
> -                            return ret;
> -
> -                        s->frames_out++;
> -                        if (j > 0) s->dup++;
> -                    }
> -                    av_frame_free(&buf);
> -                } else {
> -                    /* for delta less or equal to 0, we should drop the frame,
> -                     * otherwise, we will have one or more extra frames */
> -                    av_frame_free(&buf);
> -                    s->drop++;
> -                }
> -            }
> -        }
> -        return 0;
> -    }
> +    /* Must only be called when we have buffer room available */
> +    av_assert1(s->frames_count < 2);
>  
> -    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.

>  
> -static int write_to_fifo(AVFifoBuffer *fifo, AVFrame *buf)
> -{
> -    int ret;
> +    /* Convert frame pts to output timebase */
> +    in_pts = frame->pts;

> +    frame->pts = av_rescale_q_rnd(in_pts, inlink->time_base,
> +            outlink->time_base, s->rounding);

Nit: indentation.

>  
> -    if (!av_fifo_space(fifo) &&
> -        (ret = av_fifo_realloc2(fifo, 2*av_fifo_size(fifo)))) {
> -        av_frame_free(&buf);
> -        return ret;
> -    }
> +    av_log(ctx, AV_LOG_DEBUG, "Read frame with in pts %"PRId64", out pts %"PRId64"\n",
> +            in_pts, frame->pts);
>  
> -    av_fifo_generic_write(fifo, &buf, sizeof(buf), NULL);
> -    return 0;
> +    s->frames[s->frames_count++] = frame;
> +    s->frames_in++;
>  }
>  
> -static int filter_frame(AVFilterLink *inlink, AVFrame *buf)
> +/* Write a frame to the output */
> +static int write_frame(AVFilterContext *ctx, FPSContext *s, AVFilterLink *outlink)
>  {
> -    AVFilterContext    *ctx = inlink->dst;
> -    FPSContext           *s = ctx->priv;
> -    AVFilterLink   *outlink = ctx->outputs[0];
> -    int64_t delta;
> -    int i, ret;
> +    AVFrame *frame;
> +    int ret;
> +    int dup = 0;
>  
> -    s->frames_in++;
> -    /* discard frames until we get the first timestamp */
> -    if (s->first_pts == AV_NOPTS_VALUE) {
> -        if (buf->pts != AV_NOPTS_VALUE) {
> -            ret = write_to_fifo(s->fifo, buf);
> -            if (ret < 0)
> -                return ret;
> -
> -            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);
> -                s->first_pts = av_rescale_q(first_pts, AV_TIME_BASE_Q,
> -                                                     inlink->time_base);
> -                av_log(ctx, AV_LOG_VERBOSE, "Set first pts to (in:%"PRId64" out:%"PRId64")\n",
> -                       s->first_pts, av_rescale_q(first_pts, AV_TIME_BASE_Q,
> -                                                  outlink->time_base));
> -            } else {
> -                s->first_pts = buf->pts;
> -            }
> +    av_assert1(s->frames_count == 2);
> +
> +    /* We haven't yet determined the pts of the first frame */
> +    if (s->next_pts == AV_NOPTS_VALUE) {
> +        if (s->frames[0]->pts != AV_NOPTS_VALUE) {
> +            s->next_pts = s->frames[0]->pts;
> +            av_log(ctx, AV_LOG_VERBOSE, "Set first pts to %"PRId64"\n",
> +                    s->next_pts);
>          } else {
>              av_log(ctx, AV_LOG_WARNING, "Discarding initial frame(s) with no "
> -                   "timestamp.\n");
> -            av_frame_free(&buf);
> +                    "timestamp.\n");
> +            frame = shift_frame(s);
> +            av_frame_free(&frame);
>              s->drop++;
> +            return AVERROR(EAGAIN);
>          }
> -        return 0;
>      }
>  
> -    /* now wait for the next timestamp */
> -    if (buf->pts == AV_NOPTS_VALUE || av_fifo_size(s->fifo) <= 0) {
> -        return write_to_fifo(s->fifo, buf);
> +    /* If the second buffered frame is acceptable for the next frame, drop the first. */
> +    if (s->frames[1]->pts <= s->next_pts) {
> +        frame = shift_frame(s);
> +        if (frame->pts == s->next_pts) {
> +            av_log(ctx, AV_LOG_DEBUG, "Dropping frame with pts %"PRId64"\n", frame->pts);
> +            s->drop++;
> +        }
> +        av_frame_free(&frame);
> +        return AVERROR(EAGAIN);
> +
> +    /* The second buffered frame is in the future, output the first */
> +    } else {
> +        frame = av_frame_clone(s->frames[0]);
> +        frame->pts = s->next_pts++;
> +
> +        /* If we're using a different pts than the frame's original, it's a dup */
> +        if (s->frames[0]->pts != frame->pts) {
> +            av_log(ctx, AV_LOG_DEBUG, "Duplicating frame with pts %"PRId64" to pts %"PRId64"\n",
> +                    s->frames[0]->pts, frame->pts);
> +            dup = 1;
> +        } else {
> +            av_log(ctx, AV_LOG_DEBUG, "Writing frame with pts %"PRId64"\n", frame->pts);
> +        }
> +

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

> +
> +        return ret;
>      }
> +}
>  
> -    /* number of output frames */
> -    delta = av_rescale_q_rnd(buf->pts - s->first_pts, inlink->time_base,
> -                             outlink->time_base, s->rounding) - s->frames_out ;
> +/* Write the last frame to the output */

> +static int write_frame_eof(AVFilterContext *ctx, FPSContext *s, AVFilterLink *outlink)
> +{
> +    AVFrame *frame;
> +    int ret;
> +    int dup = 0;
>  
> -    if (delta < 1) {
> -        /* drop everything buffered except the last */
> -        int drop = av_fifo_size(s->fifo)/sizeof(AVFrame*);
> +    av_assert1(s->frames_count == 1);
>  
> -        av_log(ctx, AV_LOG_DEBUG, "Dropping %d frame(s).\n", drop);
> -        s->drop += drop;
> +    /* Either we've already hit the status timestamp or it's in the past,
> +     * e.g. AV_NOPTS_VALUE, so drop the last frame. */
> +    if (s->status_pts <= s->next_pts) {
> +        frame = shift_frame(s);
> +        if (frame->pts == s->next_pts) {
> +            av_log(ctx, AV_LOG_DEBUG, "Dropping frame with pts %"PRId64"\n", frame->pts);
> +            s->drop++;
> +        }
> +        av_frame_free(&frame);
> +        return AVERROR(EAGAIN);
> +
> +    /* We haven't yet reached the timestamp of the status (EOF) */
> +    } else {
> +        frame = av_frame_clone(s->frames[0]);
> +        frame->pts = s->next_pts++;
> +
> +        /* If we're using a different pts than the frame's original, it's a dup */
> +        if (s->frames[0]->pts != frame->pts) {
> +            av_log(ctx, AV_LOG_DEBUG, "Duplicating frame with pts %"PRId64" to pts %"PRId64"\n",
> +                    s->frames[0]->pts, frame->pts);
> +            dup = 1;
> +        } else {
> +            av_log(ctx, AV_LOG_DEBUG, "Writing frame with pts %"PRId64"\n", frame->pts);
> +        }
>  
> -        flush_fifo(s->fifo);
> -        ret = write_to_fifo(s->fifo, buf);
> +        ret = ff_filter_frame(outlink, frame);
> +        if (ret >= 0) {
> +            s->frames_out++;
> +            s->dup += dup;
> +        }
>  
>          return ret;
>      }
> +}

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.

>  
> -    /* can output >= 1 frames */
> -    for (i = 0; i < delta; i++) {
> -        AVFrame *buf_out;
> -        av_fifo_generic_read(s->fifo, &buf_out, sizeof(buf_out), NULL);
> +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.

>  
> -        /* duplicate the frame if needed */
> -        if (!av_fifo_size(s->fifo) && i < delta - 1) {
> -            AVFrame *dup = av_frame_clone(buf_out);
> +    av_log(ctx, AV_LOG_DEBUG, "EOF is at pts %"PRId64"\n", s->status_pts);
> +}
>  
> -            av_log(ctx, AV_LOG_DEBUG, "Duplicating frame.\n");
> -            if (dup)
> -                ret = write_to_fifo(s->fifo, dup);
> -            else
> -                ret = AVERROR(ENOMEM);
> +static int activate(AVFilterContext *ctx)
> +{
> +    FPSContext   *s       = ctx->priv;
> +    AVFilterLink *inlink  = ctx->inputs[0];
> +    AVFilterLink *outlink = ctx->outputs[0];
>  
> -            if (ret < 0) {
> -                av_frame_free(&buf_out);
> -                av_frame_free(&buf);
> -                return ret;
> -            }
> +    int ret;
>  
> -            s->dup++;
> +    FF_FILTER_FORWARD_STATUS_BACK(outlink, inlink);
> +
> +    /* No buffered status: normal operation */
> +    if (!s->status) {
> +
> +        /* Read available input frames if we have room */
> +        while (s->frames_count < 2 &&

> +                ff_inlink_check_available_frame(inlink)) {

Nit: indentation.

> +            read_frame(ctx, s, inlink, outlink);
>          }
>  
> -        buf_out->pts = av_rescale_q(s->first_pts, inlink->time_base,
> -                                    outlink->time_base) + s->frames_out;
> +        /* We do not yet have enough frames to produce output */
> +        if (s->frames_count < 2) {
>  
> -        if ((ret = ff_filter_frame(outlink, buf_out)) < 0) {
> -            av_frame_free(&buf);
> -            return ret;
> +            ret = ff_inlink_acknowledge_status(inlink, &s->status, &s->status_pts);
> +            if (!ret) {
> +                /* If someone wants us to output, we'd better ask for more input */

> +                if (ff_outlink_frame_wanted(outlink) > 0)
> +                    ff_inlink_request_frame(inlink);

You can use FF_FILTER_FORWARD_WANTED().

> +
> +                return 0;
> +            }
> +            if (ret > 0) {
> +                /* Need to calculate the end timestamp in the output timebase */
> +                update_eof_pts(ctx, s, inlink, outlink);
> +            }
>          }
>  
> -        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.

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

> +            return ret;
> +        }
>      }
> -    flush_fifo(s->fifo);
>  
> -    ret = write_to_fifo(s->fifo, buf);
> +    /* There is a buffered status, flush frames and forward it */
> +    if (s->status) {
> +
> +        /* There are still buffered frames, so flush a frame */
> +        if (s->frames_count > 0) {
> +            if (s->frames_count == 2)
> +                ret = write_frame(ctx, s, outlink);
> +            else
> +                ret = write_frame_eof(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;
> +            }
> +        }
> +
> +        /* No frames left, so forward the status */
> +        if (s->frames_count == 0) {
> +            ff_outlink_set_status(outlink, s->status, s->next_pts);
> +            return 0;
> +        }
> +    }
>  
> -    return ret;
> +    return FFERROR_NOT_READY;
>  }
>  
>  static const AVFilterPad avfilter_vf_fps_inputs[] = {
>      {
>          .name         = "default",
>          .type         = AVMEDIA_TYPE_VIDEO,
> -        .filter_frame = filter_frame,
>      },
>      { NULL }
>  };
> @@ -324,8 +386,7 @@ static const AVFilterPad avfilter_vf_fps_outputs[] = {
>      {
>          .name          = "default",
>          .type          = AVMEDIA_TYPE_VIDEO,
> -        .request_frame = request_frame,
> -        .config_props  = config_props
> +        .config_props  = config_props,
>      },
>      { NULL }
>  };
> @@ -337,6 +398,7 @@ AVFilter ff_vf_fps = {
>      .uninit      = uninit,
>      .priv_size   = sizeof(FPSContext),
>      .priv_class  = &fps_class,
> +    .activate    = activate,
>      .inputs      = avfilter_vf_fps_inputs,
>      .outputs     = avfilter_vf_fps_outputs,
>  };

Regards,

On 16.02.2018 21:09, calvin.walton@kepstin.ca wrote:
> Oops, I forgot to remove this bit from the changelog:
> 
> On Fri, 2018-02-16 at 15:02 -0500, Calvin Walton wrote:
>> TODO: This is still a work in progress. It may have different
>> behaviour
>> in some cases from the old fps filter. I have not yet implemented the
>> "eof_action" option, since I haven't figured out what it's supposed
>> to
>> do.
> 
> At this point I'm fairly confident that the behaviour matches the
> existing filter, and that I've gotten the eof_action behaviour correct
> as well.

I have run this patch against some private test files with filter 
options "fps=1:round=near:eof_action=pass" and found no regressions. So 
eof_action seems to work fine, indeed.

Regards,
Tobias

Hi Nicolas,

On Sun, 2018-02-18 at 19:01 +0100, Nicolas George wrote:
> 
> Thanks for the patch. It was something I had in my TODO list for a
> long
> time. The code looks very good. Here are a few comments below. Of
> course
> open to discussion.

Thanks for the review. I'm going to spend some time going over it now.
I've noticed in my testing that I completely forgot to hook up the
'start_time' option, so expect a patch respin fixing that and
addressing your comments as well.

(I'll look at writing some tests for the start_time option too.)

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.