Re: [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, );
> > +/* 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 

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

[Calvin Walton]

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

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

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

[Tobias Rapp]

On 16.02.2018 21:09, calvin.wal...@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

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

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

[Nicolas George]

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 
>   * 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,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, , 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*);
> -

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

[calvin . walton]

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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] 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, 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(-)

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 
  * 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,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;
 
-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, , 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, "
@@ -128,194 +146,238 @@ static int config_props(AVFilterLink* link)
 
 link->time_base = av_inv_q(s->framerate);
 link->frame_rate= s->framerate;