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.wal...@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,
--
Nicolas George
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
