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

[Nicolas George <george@nsup.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6652 bytes --]

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.

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.

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

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

> +        (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.

> +        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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: 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".