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. -- Calvin Walton <calvin.wal...@kepstin.ca> _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel