[FFmpeg-devel] [PATCH] libavfilter/f_select: switch to activate and properly handle EOF pts
Li-Heng Chen
lihengc at netflix.com
Wed Sep 14 21:03:19 EEST 2022
On Wed, Sep 14, 2022 at 8:14 AM Nicolas George <george at nsup.org> wrote:
>
> Li-Heng Chen (12022-09-13):
> > This patch solves a potential EOF pts bug that can be triggered with other filters: when placing select filter before fps filter, the EOF pts in `f_select` always indicate the last input frame regardless of the frame selected. This may cause unwanted duplication of the last-selected-frame in `vf_fps`. Switching the filtering process from `filter_frame` to `activate` allows to properly set EOF pts by ff_outlink_set_status.
>
> Thanks for the patch. A few comments.
>
Thanks for the comment, Nicolas! I've attached a new patch file which
should address the comments. I also want to mention that this patch is
similar to another one Paul had done for setpts filter:
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=2a546fb7d5722c306dd42c715137e5e493b0d5be
> First, please wrap this paragraph. Also, the patch seems to have been
> corrupted by the mail software, please look into it.
>
> Also, please explain the logic you used to set the end timestamp. It
> seems you are using some kind of weird test to use the timestamp of the
> fist not-selected frame.
>
The attached patch should have the commit message wrapped, and I
have also added a comment in the code to explain the logic:
/* Two conditions that current pts could be the EOF pts:
* - If the current frame N is not selected while the previous
* frame (N-1) is selected, frame N-1 could be the last frame,
* hence we update select->eof_pts.
* - Last input frame: we have EOF status, and the current (last)
* frame is selected */
> In fact, I have doubt about the validity of this patch at all. You will
> notice that the select filters does not alter the timestamps of the
> frames. The frames that are not selected are skipped, but they count for
> the timing. I do not think the final frames should behave differently.
>
This patch does not change the frame timestamps. Instead, we added
another variable select->eof_pts to track possible EOF pts. It sets the
EOF pts as the pts *after* the last selected frame (e.g. if the last frame
selected by the filter is frame 20, EOF pts is set as the pts of frame 21)
> > This bug can be reproduced by the ffmpeg cmd below (bitstreams in
> > fate-suite can reproduce this issue):
> >
> > ffmpeg -y -i /path/ffmpeg/fate-suite/h264/bbc2.sample.h264 -vf select='gte(n\,0)*gte(24\,n)',fps=25/1 out.y4m
>
> I do not think it shows a bug. If you want to truncate the stream, the
> select filter is not the recommended choice, use trim.
>
I have also tested the trim filter, which does not present this bug. However,
if you do -vf select='eq(n\,24)',fps=25/1 on the above example, this selected
frame will also be duplicated 21 times, which is also not an expected behavior
for the select filter.
> >
> > Signed-off-by: Li-Heng Chen <lihengc at netflix.com>
> > ---
> > libavfilter/f_select.c | 49 +++++++++++++++++++++++++++++++++---------
> > 1 file changed, 39 insertions(+), 10 deletions(-)
> >
> > diff --git a/libavfilter/f_select.c b/libavfilter/f_select.c
> > index 1cfe2d59e5..e01f476b5b 100644
> > --- a/libavfilter/f_select.c
> > +++ b/libavfilter/f_select.c
> > @@ -33,6 +33,7 @@
> > #include "libavutil/opt.h"
> > #include "libavutil/pixdesc.h"
> > #include "avfilter.h"
> > +#include "filters.h"
> > #include "audio.h"
> > #include "formats.h"
> > #include "internal.h"
> > @@ -159,6 +160,7 @@ typedef struct SelectContext {
> > double select;
> > int select_out; ///< mark the selected output pad index
> > int nb_outputs;
> > + int64_t eof_pts;
> > } SelectContext;
> >
> > #define OFFSET(x) offsetof(SelectContext, x)
> > @@ -215,6 +217,7 @@ static int config_input(AVFilterLink *inlink)
> >
> > select->bitdepth = desc->comp[0].depth;
> > select->nb_planes = is_yuv ? 1 : av_pix_fmt_count_planes(inlink->format);
> > + select->eof_pts = AV_NOPTS_VALUE;
> >
> > for (int plane = 0; plane < select->nb_planes; plane++) {
> > ptrdiff_t line_size = av_image_get_linesize(inlink->format, inlink->w, plane);
> > @@ -336,7 +339,7 @@ static void select_frame(AVFilterContext *ctx, AVFrame *frame)
> > if (isnan(select->var_values[VAR_START_T]))
> > select->var_values[VAR_START_T] = TS2D(frame->pts) * av_q2d(inlink->time_base);
> >
> > - select->var_values[VAR_N ] = inlink->frame_count_out;
> > + select->var_values[VAR_N ] = inlink->frame_count_out - 1;
> > select->var_values[VAR_PTS] = TS2D(frame->pts);
> > select->var_values[VAR_T ] = TS2D(frame->pts) * av_q2d(inlink->time_base);
> > select->var_values[VAR_POS] = frame->pkt_pos == -1 ? NAN : frame->pkt_pos;
> > @@ -409,17 +412,43 @@ static void select_frame(AVFilterContext *ctx, AVFrame *frame)
> > select->var_values[VAR_PREV_T] = select->var_values[VAR_T];
> > }
> >
> > -static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> > +static int activate(AVFilterContext *ctx)
> > {
> > - AVFilterContext *ctx = inlink->dst;
> > + int ret, status;
> > SelectContext *select = ctx->priv;
> > + AVFilterLink *inlink = ctx->inputs[0];
> > + AVFilterLink *outlink = ctx->outputs[0];
> > + AVFrame *in;
> > + int64_t pts;
> >
> > - select_frame(ctx, frame);
> > - if (select->select)
> > - return ff_filter_frame(ctx->outputs[select->select_out], frame);
> > + FF_FILTER_FORWARD_STATUS_BACK(outlink, inlink);
> >
> > - av_frame_free(&frame);
> > - return 0;
> > + ret = ff_inlink_consume_frame(inlink, &in);
> > + if (ret < 0)
> > + return ret;
> > + if (ret > 0) {
> > + select_frame(ctx, in);
> > + if (select->select)
> > + return ff_filter_frame(ctx->outputs[select->select_out], in);
> > + }
> > + av_frame_free(&in);
> > +
> > + ret = ff_inlink_acknowledge_status(inlink, &status, &pts);
> > +
>
> > + if (((int64_t)select->var_values[VAR_N] - (int64_t)select->var_values[VAR_PREV_SELECTED_N] == 1) ||
>
> Do not bring back integer values that have been converted to float. The
> real value of var_values[VAR_N] is available; if
> var_values[VAR_PREV_SELECTED_N] is necessary too introduce a field to
> keep track of its exact value.
>
fixed in the attached patch
> > + (status == AVERROR_EOF && select->select))
> > + select->eof_pts = pts;
> > +
>
> > + if (ret && status == AVERROR_EOF) {
>
> The status has to be forwarded even if it is other than EOF.
>
fixed in the attached patch (I removed status == AVERROR_EOF)
> > + av_log(ctx, AV_LOG_TRACE, "N:EOF PTS:%"PRId64"\n",
> > + select->eof_pts);
> > + ff_outlink_set_status(outlink, status, select->eof_pts);
> > + return 0;
> > + }
> > +
> > + FF_FILTER_FORWARD_WANTED(outlink, inlink);
> > +
> > + return FFERROR_NOT_READY;
> > }
> >
> > static int request_frame(AVFilterLink *outlink)
> > @@ -467,7 +496,6 @@ static const AVFilterPad avfilter_af_aselect_inputs[] = {
> > .name = "default",
> > .type = AVMEDIA_TYPE_AUDIO,
> > .config_props = config_input,
> > - .filter_frame = filter_frame,
> > },
> > };
> >
> > @@ -475,6 +503,7 @@ const AVFilter ff_af_aselect = {
> > .name = "aselect",
> > .description = NULL_IF_CONFIG_SMALL("Select audio frames to pass in output."),
> > .init = aselect_init,
> > + .activate = activate,
> > .uninit = uninit,
> > .priv_size = sizeof(SelectContext),
> > FILTER_INPUTS(avfilter_af_aselect_inputs),
> > @@ -522,7 +551,6 @@ static const AVFilterPad avfilter_vf_select_inputs[] = {
> > .name = "default",
> > .type = AVMEDIA_TYPE_VIDEO,
> > .config_props = config_input,
> > - .filter_frame = filter_frame,
> > },
> > };
> >
> > @@ -530,6 +558,7 @@ const AVFilter ff_vf_select = {
> > .name = "select",
> > .description = NULL_IF_CONFIG_SMALL("Select video frames to pass in output."),
> > .init = select_init,
> > + .activate = activate,
> > .uninit = uninit,
> > .priv_size = sizeof(SelectContext),
> > .priv_class = &select_class,
>
> Regards,
>
> --
> Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libavfilter-f_select-switch-to-activate-and-properly.patch
Type: application/octet-stream
Size: 5972 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220914/900a40fa/attachment.obj>
More information about the ffmpeg-devel
mailing list