Re: [FFmpeg-devel] [PATCH] libavfilter/f_select: switch to activate and properly handle EOF pts

[Li-Heng Chen <lihengc@netflix.com>]

[-- Attachment #1: Type: text/plain, Size: 8709 bytes --]

On Wed, Sep 14, 2022 at 8:14 AM Nicolas George <george@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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[-- Attachment #2: 0001-libavfilter-f_select-switch-to-activate-and-properly.patch --]
[-- Type: application/octet-stream, Size: 5972 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".