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

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

[Li-Heng Chen]

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.

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

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) ||
+        (status == AVERROR_EOF && select->select))
+        select->eof_pts = pts;
+
+    if (ret && 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,
-- 
2.32.1 (Apple Git-133)

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

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

[Nicolas George]

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

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

[Li-Heng Chen]

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

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

[Nicolas George]

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

Li-Heng Chen (12022-09-14):
> 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

It is similar in its form, but the logic is very different: Paul's
commit has the function evaluated for the final timestamps.

> 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 */

Thanks for explaining. Unfortunately, it confirms my analysis that your
logic is slightly flawed.

Consider a select filter that keeps only the frames with a PTS that is a
prime number. The frame with PTS 13 used to extend in the timeline of
the stream from 13 inclusive to 14 exclusive. Now it extends from 13
inclusive to 17 exclusive; the timestamp 14 was completely skipped.

There is no reason that a EOF arriving a little after that would change
that and make the filter re-consider 14: the frame 14 was skipped, its
PTS should not be considered.

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

Which means it sets the EOF PTS at the timestamp of a frame that was
dropped. This is not logical.

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

I consider it the correct behavior.

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

[FFmpeg-devel] Unsubscribe

[Sam Davis]

Unsubscribe

On Wed, Sep 14, 2022, 11:14 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.
>
> 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
> _______________________________________________
> 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".
>
_______________________________________________
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".

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

[Li-Heng Chen]

On Wed, Sep 14, 2022 at 11:15 AM Nicolas George <george@nsup.org> wrote:
>
> Li-Heng Chen (12022-09-14):
> > 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
>
> It is similar in its form, but the logic is very different: Paul's
> commit has the function evaluated for the final timestamps.
>
> > 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 */
>
> Thanks for explaining. Unfortunately, it confirms my analysis that your
> logic is slightly flawed.
>
> Consider a select filter that keeps only the frames with a PTS that is a
> prime number. The frame with PTS 13 used to extend in the timeline of
> the stream from 13 inclusive to 14 exclusive. Now it extends from 13
> inclusive to 17 exclusive; the timestamp 14 was completely skipped.
>

Please allow me to explain again with your example. That's say we have
an input with 20 frames (0-19), and we want to keep prime number frames
with select filter, -vf select='eq(n\,2)+eq(n\,3)+...+eq(n\,13)+eq(n\,17)'

In the activation function, select->eof_pts is updated to n at n=4, 6,
8, 12, 14, 18
However, select->eof_pts is passed into ff_outlink_set_status only at
n=20, since
ff_inlink_acknowledge_status returns 1.

> There is no reason that a EOF arriving a little after that would change
> that and make the filter re-consider 14: the frame 14 was skipped, its
> PTS should not be considered.
>

My point is to indicate EOF PTS to last-selected-frame (n=17) instead of
last-input frame (n=19). The reason why I added one to select->eof_pts is
because the existing EOF PTS is the pts value after the last frame (20).

> > 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)
>
> Which means it sets the EOF PTS at the timestamp of a frame that was
> dropped. This is not logical.
>

In the existing behavior, the EOF PTS is always the pts after the last
input frame.
For example, if the input to ffmpeg has 20 frames, the EOF PTS passed to the
next filter is the pts of the "21th" frame, no matter the 20th frame
is dropped or not.

> > 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.
>
> I consider it the correct behavior.
>

Regardless of my logic, this behavior looks weird to me. In my opinion,
-vf select='eq(n\,24),fps=25/1' should produce the same result as just having
-vf select='eq(n\,24)'. However, select='eq(n\,24),fps=25/1' generates
22 identical
frames while select='eq(n\,24)' only outputs one frame...

Thanks,
Li-Heng

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

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

[Nicolas George]

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

Li-Heng Chen (12022-09-14):
> Please allow me to explain again with your example. That's say we have
> an input with 20 frames (0-19), and we want to keep prime number frames
> with select filter, -vf select='eq(n\,2)+eq(n\,3)+...+eq(n\,13)+eq(n\,17)'
> 
> In the activation function, select->eof_pts is updated to n at n=4, 6,
> 8, 12, 14, 18
> However, select->eof_pts is passed into ff_outlink_set_status only at
> n=20, since
> ff_inlink_acknowledge_status returns 1.

I understood that. And I argue it is wrong.

> Regardless of my logic, this behavior looks weird to me. In my opinion,
> -vf select='eq(n\,24),fps=25/1' should produce the same result as just having
> -vf select='eq(n\,24)'. However, select='eq(n\,24),fps=25/1' generates
> 22 identical
> frames while select='eq(n\,24)' only outputs one frame...

Trick question:

testsrc=r=25:d=4,select='gte(n\,0)*gte(24\,n)+gte(n\,50)*gte(74\,n)',fps=25

How many frames do you expect on the output?

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

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

[Li-Heng Chen]

On Thu, Sep 15, 2022 at 2:41 AM Nicolas George <george@nsup.org> wrote:
>
> Li-Heng Chen (12022-09-14):
> > Please allow me to explain again with your example. That's say we have
> > an input with 20 frames (0-19), and we want to keep prime number frames
> > with select filter, -vf select='eq(n\,2)+eq(n\,3)+...+eq(n\,13)+eq(n\,17)'
> >
> > In the activation function, select->eof_pts is updated to n at n=4, 6,
> > 8, 12, 14, 18
> > However, select->eof_pts is passed into ff_outlink_set_status only at
> > n=20, since
> > ff_inlink_acknowledge_status returns 1.
>
> I understood that. And I argue it is wrong.
>
> > Regardless of my logic, this behavior looks weird to me. In my opinion,
> > -vf select='eq(n\,24),fps=25/1' should produce the same result as just having
> > -vf select='eq(n\,24)'. However, select='eq(n\,24),fps=25/1' generates
> > 22 identical
> > frames while select='eq(n\,24)' only outputs one frame...
>
> Trick question:
>
> testsrc=r=25:d=4,select='gte(n\,0)*gte(24\,n)+gte(n\,50)*gte(74\,n)',fps=25
>
> How many frames do you expect on the output?
>

Played with this cmd
-f lavfi -i testsrc=r=25:d=4 -vf
select='gte(n\,0)*gte(24\,n)+gte(n\,50)*gte(74\,n)',fps=25/1

1) Existing ffmpeg behavior: output 100 frames
- n=25-49 are duplicated from frame #24;
- n=75-99 duplicated from frame #74.
2) with my patch: output 75 frames due to the EOF pts change
-> n=25-49 are duplicated from frame #24.

I guess your argument is that the duplication behavior should be
consistent between [25,50) and [75,99)? That makes sense to me!

Interestingly, however, I also tried the cmd without the fps filter
-f lavfi -i testsrc=r=25:d=4 -vf
select='gte(n\,0)*gte(24\,n)+gte(n\,50)*gte(74\,n)'

The existing ffmpeg outputs 75 frames, where n=25-49 are duplicated
from frame #24. This matches the behavior in 2)

Best,
Li-Heng



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