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

[Nicolas George]

Calvin Walton (2018-02-19):
> 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.
> ---
>  libavfilter/vf_fps.c | 383 
> ++-
>  1 file changed, 199 insertions(+), 184 deletions(-)

Thanks a lot. It looks very good. A few minor comments below, and I will
wait for the version where you fixed statistics.

> diff --git a/libavfilter/vf_fps.c b/libavfilter/vf_fps.c
> index dbafd2c35a..74500f59c9 100644
> --- a/libavfilter/vf_fps.c
> +++ b/libavfilter/vf_fps.c
> @@ -2,6 +2,7 @@
>   * Copyright 2007 Bobby Bingham
>   * Copyright 2012 Robert Nagy 
>   * Copyright 2012 Anton Khirnov 
> + * Copyright 2018 Calvin Walton 
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -28,17 +29,12 @@
>  #include 
>  #include 
>  
> -#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,25 @@ 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
> -
>  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  in_pts_off;///< input frame pts offset for start_time 
> handling
> +int64_t  out_pts_off;   ///< output frame pts offset for start_time 
> handling
> +
> +/* 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 +95,42 @@ static av_cold int init(AVFilterContext *ctx)
>  {
>  FPSContext *s = ctx->priv;
>  
> -if (!(s->fifo = av_fifo_alloc_array(2, sizeof(AVFrame*
> -return AVERROR(ENOMEM);
> -
> -s->first_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, , sizeof(tmp), NULL);
> -av_frame_free();
> -}
> +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(>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();
>  }
>  
>  av_log(ctx, AV_LOG_VERBOSE, "%d frames in, %d frames out; %d frames 
> dropped, "
> @@ -124,198 +139,198 @@ static av_cold void uninit(AVFilterContext *ctx)
>  
>  static int config_props(AVFilterLink* link)
>  {
> -FPSContext   *s = 

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

[Calvin Walton]

I've found one minor problem with this patch. The actual functionality
is correct, as best I can tell, but the stats collection
(dropped/duplicated frames) will under-report dropped frames in my
testing. I'm going to rework the stats collection code a bit, but I
expect this to be a relatively minor change.

I'll post the updated patch later this week, so please let me know if
you have any other changes I should incorporate.

Calvin.

On Mon, 2018-02-19 at 19:54 -0500, Calvin Walton wrote:
> 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.
> ---
>  libavfilter/vf_fps.c | 383 ++---
> --
>  1 file changed, 199 insertions(+), 184 deletions(-)

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Calvin Walton]

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.
---
 libavfilter/vf_fps.c | 383 ++-
 1 file changed, 199 insertions(+), 184 deletions(-)

diff --git a/libavfilter/vf_fps.c b/libavfilter/vf_fps.c
index dbafd2c35a..74500f59c9 100644
--- a/libavfilter/vf_fps.c
+++ b/libavfilter/vf_fps.c
@@ -2,6 +2,7 @@
  * Copyright 2007 Bobby Bingham
  * Copyright 2012 Robert Nagy 
  * Copyright 2012 Anton Khirnov 
+ * Copyright 2018 Calvin Walton 
  *
  * This file is part of FFmpeg.
  *
@@ -28,17 +29,12 @@
 #include 
 #include 
 
-#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,25 @@ 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
-
 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  in_pts_off;///< input frame pts offset for start_time handling
+int64_t  out_pts_off;   ///< output frame pts offset for start_time 
handling
+
+/* 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 +95,42 @@ static av_cold int init(AVFilterContext *ctx)
 {
 FPSContext *s = ctx->priv;
 
-if (!(s->fifo = av_fifo_alloc_array(2, sizeof(AVFrame*
-return AVERROR(ENOMEM);
-
-s->first_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, , sizeof(tmp), NULL);
-av_frame_free();
-}
+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(>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();
 }
 
 av_log(ctx, AV_LOG_VERBOSE, "%d frames in, %d frames out; %d frames 
dropped, "
@@ -124,198 +139,198 @@ static av_cold void uninit(AVFilterContext *ctx)
 
 static int config_props(AVFilterLink* link)
 {
-FPSContext   *s = link->src->priv;
+AVFilterContext *ctx = link->src;
+AVFilterLink *inlink = ctx->inputs[0];
+FPSContext   *s = ctx->priv;
 
 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;
 
-return 0;
-}
-
-static int request_frame(AVFilterLink *outlink)
-{
-AVFilterContext *ctx = outlink->src;
-FPSContext