Re: [FFmpeg-devel] [PATCH 2/2] libavfilter/vf_fps: Rewrite using activate callback
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 w
Re: [FFmpeg-devel] [PATCH 2/2] libavfilter/vf_fps: Rewrite using activate callback
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
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
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, &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); > -
Re: [FFmpeg-devel] [PATCH 2/2] libavfilter/vf_fps: Rewrite using activate callback
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