| Message ID | 20240313122425.92457-2-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avfilter: mark scale2ref as supporting dynamic sizes | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_fate_x86 | success | Make fate finished |
| yinshiyou/make_loongarch64 | warning | New warnings during build |
| andriy/make_x86 | warning | New warnings during build |
On Wed, 13 Mar 2024 13:24:25 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote: > - frame_changed = in->width != link->w || > - in->height != link->h || > - in->format != link->format || > - in->sample_aspect_ratio.den != link->sample_aspect_ratio.den || > - in->sample_aspect_ratio.num != link->sample_aspect_ratio.num || > - in->colorspace != link->colorspace || > - in->color_range != link->color_range; > + ret = ff_framesync_dualinput_get(fs, &main, &ref); > + if (ret < 0) > + return ret; > > - if (frame_changed) { > - link->format = in->format; > - link->w = in->width; > - link->h = in->height; > - link->sample_aspect_ratio.num = in->sample_aspect_ratio.num; > - link->sample_aspect_ratio.den = in->sample_aspect_ratio.den; > - link->colorspace = in->colorspace; > - link->color_range = in->color_range; > + if (ref) { > + reflink->format = ref->format; > + reflink->w = ref->width; > + reflink->h = ref->height; > + reflink->sample_aspect_ratio.num = ref->sample_aspect_ratio.num; > + reflink->sample_aspect_ratio.den = ref->sample_aspect_ratio.den; > + reflink->colorspace = ref->colorspace; > + reflink->color_range = ref->color_range; > > - config_props_ref(outlink); > - } > + ret = config_props(outlink); > + if (ret < 0) > + return ret; This change unintentionally dropped the `frame_changed` check to avoid reconfiguring the sws context unnecessarily. Fixed locally by reintroducing the check.
On 2024-03-13 05:54 pm, Niklas Haas wrote: > From: Niklas Haas<git@haasn.dev> > > This filter's existing design has a number of issues: > > - There is no guarantee whatsoever about the order in which frames are > pushed onto the main and ref link, due to this being entirely > dependent on the order in which downstream filters decide to request > frames from their various inputs. As such, there is absolutely no > synchronization for ref streams with dynamically changing resolutions > (see e.g. fate/h264/reinit-*). The only raison d'etre for scale2ref was to have some way to access another stream's parameters. Dynamic streams weren't really the focus. > - For some (likely historical) reason, this filter outputs its ref > stream as a second ref output, which is in principle completely > unnecessary (complex filter graph users can just duplicate the input > pin), but in practice only required to allow this filter to > "eventually" see changes to the ref stream (see first point). In > particular, this means that if the user uses the "wrong" pin, this > filter may break completely. This change is fine but you should note in the manual docs that this change has occurred (and when) as existing scripts will fail due to the surplus output pad. Regards, Gyan
On Wed, Mar 13, 2024 at 01:24:25PM +0100, Niklas Haas wrote: > From: Niklas Haas <git@haasn.dev> > > This filter's existing design has a number of issues: > > - There is no guarantee whatsoever about the order in which frames are > pushed onto the main and ref link, due to this being entirely > dependent on the order in which downstream filters decide to request > frames from their various inputs. As such, there is absolutely no > synchronization for ref streams with dynamically changing resolutions > (see e.g. fate/h264/reinit-*). > > - For some (likely historical) reason, this filter outputs its ref > stream as a second ref output, which is in principle completely > unnecessary (complex filter graph users can just duplicate the input > pin), but in practice only required to allow this filter to > "eventually" see changes to the ref stream (see first point). In > particular, this means that if the user uses the "wrong" pin, this > filter may break completely. > > - The default filter activation function is fundamentally incapable of > handling filters with multiple inputs cleanly, because doing so > requires both knowledge of how these inputs should be internally > ordered, but also how to handle EOF conditions on either input (or > downstream). Both of these are best left to the filter specific > options. (See #10795 for the consequences of using the default > activate with multiple inputs). > > Switching this filter to framesync fixes all three points: > > - ff_framesync_activate() correctly handles multiple inputs and EOF > conditions (and is configurable with the framesync-specific options) > - framesync only supports a single output, so we can (indeed must) drop > the redundant ref output stream > > Update documentation, changelog and tests to correspond to the new usage > pattern. > > Fixes: https://trac.ffmpeg.org/ticket/10795 > --- > Changelog | 2 + > doc/filters.texi | 10 +- > libavfilter/vf_scale.c | 130 ++++++++++++----------- > tests/filtergraphs/scale2ref_keep_aspect | 3 +- > 4 files changed, 76 insertions(+), 69 deletions(-) this causes ./ffplay --help to segfault [...]
On 3/13/2024 8:41 PM, Michael Niedermayer wrote: > On Wed, Mar 13, 2024 at 01:24:25PM +0100, Niklas Haas wrote: >> From: Niklas Haas <git@haasn.dev> >> >> This filter's existing design has a number of issues: >> >> - There is no guarantee whatsoever about the order in which frames are >> pushed onto the main and ref link, due to this being entirely >> dependent on the order in which downstream filters decide to request >> frames from their various inputs. As such, there is absolutely no >> synchronization for ref streams with dynamically changing resolutions >> (see e.g. fate/h264/reinit-*). >> >> - For some (likely historical) reason, this filter outputs its ref >> stream as a second ref output, which is in principle completely >> unnecessary (complex filter graph users can just duplicate the input >> pin), but in practice only required to allow this filter to >> "eventually" see changes to the ref stream (see first point). In >> particular, this means that if the user uses the "wrong" pin, this >> filter may break completely. >> >> - The default filter activation function is fundamentally incapable of >> handling filters with multiple inputs cleanly, because doing so >> requires both knowledge of how these inputs should be internally >> ordered, but also how to handle EOF conditions on either input (or >> downstream). Both of these are best left to the filter specific >> options. (See #10795 for the consequences of using the default >> activate with multiple inputs). >> >> Switching this filter to framesync fixes all three points: >> >> - ff_framesync_activate() correctly handles multiple inputs and EOF >> conditions (and is configurable with the framesync-specific options) >> - framesync only supports a single output, so we can (indeed must) drop >> the redundant ref output stream >> >> Update documentation, changelog and tests to correspond to the new usage >> pattern. >> >> Fixes: https://trac.ffmpeg.org/ticket/10795 >> --- >> Changelog | 2 + >> doc/filters.texi | 10 +- >> libavfilter/vf_scale.c | 130 ++++++++++++----------- >> tests/filtergraphs/scale2ref_keep_aspect | 3 +- >> 4 files changed, 76 insertions(+), 69 deletions(-) > > this causes > ./ffplay --help > to segfault Unrelated to this crash, but why does that command line output every single option from every single filter? It's several thousand printed lines. Shouldn't that be the output of --longhelp or similar, and leave --help to print some basic non filter specific options?
Niklas Haas: > From: Niklas Haas <git@haasn.dev> > > This filter's existing design has a number of issues: > > - There is no guarantee whatsoever about the order in which frames are > pushed onto the main and ref link, due to this being entirely > dependent on the order in which downstream filters decide to request > frames from their various inputs. As such, there is absolutely no > synchronization for ref streams with dynamically changing resolutions > (see e.g. fate/h264/reinit-*). > > - For some (likely historical) reason, this filter outputs its ref > stream as a second ref output, which is in principle completely > unnecessary (complex filter graph users can just duplicate the input > pin), but in practice only required to allow this filter to > "eventually" see changes to the ref stream (see first point). In > particular, this means that if the user uses the "wrong" pin, this > filter may break completely. > > - The default filter activation function is fundamentally incapable of > handling filters with multiple inputs cleanly, because doing so > requires both knowledge of how these inputs should be internally > ordered, but also how to handle EOF conditions on either input (or > downstream). Both of these are best left to the filter specific > options. (See #10795 for the consequences of using the default > activate with multiple inputs). > > Switching this filter to framesync fixes all three points: > > - ff_framesync_activate() correctly handles multiple inputs and EOF > conditions (and is configurable with the framesync-specific options) > - framesync only supports a single output, so we can (indeed must) drop > the redundant ref output stream > > Update documentation, changelog and tests to correspond to the new usage > pattern. > > Fixes: https://trac.ffmpeg.org/ticket/10795 > --- > Changelog | 2 + > doc/filters.texi | 10 +- > libavfilter/vf_scale.c | 130 ++++++++++++----------- > tests/filtergraphs/scale2ref_keep_aspect | 3 +- > 4 files changed, 76 insertions(+), 69 deletions(-) > > diff --git a/Changelog b/Changelog > index 069b8274489..bacda2524ea 100644 > --- a/Changelog > +++ b/Changelog > @@ -32,6 +32,8 @@ version <next>: > - DVD-Video demuxer, powered by libdvdnav and libdvdread > - ffprobe -show_stream_groups option > - ffprobe (with -export_side_data film_grain) now prints film grain metadata > +- scale2ref now only has a single output stream, and supports the framesync > + options > > > version 6.1: > diff --git a/doc/filters.texi b/doc/filters.texi > index e0436a5755c..07e8136adb3 100644 > --- a/doc/filters.texi > +++ b/doc/filters.texi > @@ -21555,9 +21555,9 @@ Deprecated, do not use. > Scale (resize) the input video, based on a reference video. > > See the scale filter for available options, scale2ref supports the same but > -uses the reference video instead of the main input as basis. scale2ref also > -supports the following additional constants for the @option{w} and > -@option{h} options: > +uses the reference video instead of the main input as basis. This filter also > +supports the @ref{framesync} options. In addition, scale2ref also supports the > +following additional constants for the @option{w} and @option{h} options: > > @table @var > @item main_w > @@ -21600,13 +21600,13 @@ Only available with @code{eval=frame}. > @item > Scale a subtitle stream (b) to match the main video (a) in size before overlaying > @example > -'scale2ref[b][a];[a][b]overlay' > +'[b][a]scale2ref[sub];[a][sub]overlay' > @end example > > @item > Scale a logo to 1/10th the height of a video, while preserving its display aspect ratio. > @example > -[logo-in][video-in]scale2ref=w=oh*mdar:h=ih/10[logo-out][video-out] > +[logo-in][video]scale2ref=w=oh*mdar:h=ih/10[logo-out] > @end example > @end itemize > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > index fc3b5a91e60..d4173b63097 100644 > --- a/libavfilter/vf_scale.c > +++ b/libavfilter/vf_scale.c > @@ -29,6 +29,7 @@ > > #include "avfilter.h" > #include "formats.h" > +#include "framesync.h" > #include "internal.h" > #include "scale_eval.h" > #include "video.h" > @@ -114,6 +115,7 @@ typedef struct ScaleContext { > struct SwsContext *isws[2]; ///< software scaler context for interlaced material > // context used for forwarding options to sws > struct SwsContext *sws_opts; > + FFFrameSync fs; ///< for scale2ref > > /** > * New dimensions. Special values are: > @@ -288,6 +290,9 @@ static av_cold int preinit(AVFilterContext *ctx) > if (ret < 0) > return ret; > > + if (ctx->filter == &ff_vf_scale2ref) > + ff_framesync_preinit(&scale->fs); > + > return 0; > } > > @@ -303,6 +308,8 @@ static const int sws_colorspaces[] = { > -1 > }; > > +static int do_scale2ref(FFFrameSync *fs); > + > static av_cold int init(AVFilterContext *ctx) > { > ScaleContext *scale = ctx->priv; > @@ -380,6 +387,7 @@ static av_cold int init(AVFilterContext *ctx) > if (!threads) > av_opt_set_int(scale->sws_opts, "threads", ff_filter_get_nb_threads(ctx), 0); > > + scale->fs.on_event = do_scale2ref; > return 0; > } > > @@ -389,6 +397,7 @@ static av_cold void uninit(AVFilterContext *ctx) > av_expr_free(scale->w_pexpr); > av_expr_free(scale->h_pexpr); > scale->w_pexpr = scale->h_pexpr = NULL; > + ff_framesync_uninit(&scale->fs); > sws_freeContext(scale->sws_opts); > sws_freeContext(scale->sws); > sws_freeContext(scale->isws[0]); > @@ -678,35 +687,16 @@ static int config_props(AVFilterLink *outlink) > flags_val); > av_freep(&flags_val); > > - return 0; > - > -fail: > - return ret; > -} > - > -static int config_props_ref(AVFilterLink *outlink) > -{ > - AVFilterLink *inlink = outlink->src->inputs[1]; > - > - outlink->w = inlink->w; > - outlink->h = inlink->h; > - outlink->sample_aspect_ratio = inlink->sample_aspect_ratio; > - outlink->time_base = inlink->time_base; > - outlink->frame_rate = inlink->frame_rate; > - outlink->colorspace = inlink->colorspace; > - outlink->color_range = inlink->color_range; > + if (ctx->filter != &ff_vf_scale2ref) > + return 0; > > - return 0; > -} > + if ((ret = ff_framesync_init_dualinput(&scale->fs, ctx)) < 0) > + return ret; > > -static int request_frame(AVFilterLink *outlink) > -{ > - return ff_request_frame(outlink->src->inputs[0]); > -} > + return ff_framesync_configure(&scale->fs); > > -static int request_frame_ref(AVFilterLink *outlink) > -{ > - return ff_request_frame(outlink->src->inputs[1]); > +fail: > + return ret; > } > > static void frame_offset(AVFrame *frame, int dir, int is_pal) > @@ -909,43 +899,49 @@ static int filter_frame(AVFilterLink *link, AVFrame *in) > return ret; > } > > -static int filter_frame_ref(AVFilterLink *link, AVFrame *in) > +static int do_scale2ref(FFFrameSync *fs) > { > - ScaleContext *scale = link->dst->priv; > - AVFilterLink *outlink = link->dst->outputs[1]; > - int frame_changed; > + AVFilterContext *ctx = fs->parent; > + ScaleContext *scale = ctx->priv; > + AVFilterLink *reflink = ctx->inputs[1]; > + AVFilterLink *outlink = ctx->outputs[0]; > + AVFrame *main, *ref, *out; > + int ret; > > - frame_changed = in->width != link->w || > - in->height != link->h || > - in->format != link->format || > - in->sample_aspect_ratio.den != link->sample_aspect_ratio.den || > - in->sample_aspect_ratio.num != link->sample_aspect_ratio.num || > - in->colorspace != link->colorspace || > - in->color_range != link->color_range; > + ret = ff_framesync_dualinput_get(fs, &main, &ref); > + if (ret < 0) > + return ret; > > - if (frame_changed) { > - link->format = in->format; > - link->w = in->width; > - link->h = in->height; > - link->sample_aspect_ratio.num = in->sample_aspect_ratio.num; > - link->sample_aspect_ratio.den = in->sample_aspect_ratio.den; > - link->colorspace = in->colorspace; > - link->color_range = in->color_range; > + if (ref) { > + reflink->format = ref->format; > + reflink->w = ref->width; > + reflink->h = ref->height; > + reflink->sample_aspect_ratio.num = ref->sample_aspect_ratio.num; > + reflink->sample_aspect_ratio.den = ref->sample_aspect_ratio.den; > + reflink->colorspace = ref->colorspace; > + reflink->color_range = ref->color_range; > > - config_props_ref(outlink); > - } > + ret = config_props(outlink); > + if (ret < 0) > + return ret; > > - if (scale->eval_mode == EVAL_MODE_FRAME) { > - scale->var_values[VAR_N] = link->frame_count_out; > - scale->var_values[VAR_T] = TS2T(in->pts, link->time_base); > + if (scale->eval_mode == EVAL_MODE_FRAME) { > + scale->var_values[VAR_N] = reflink->frame_count_out; > + scale->var_values[VAR_T] = TS2T(ref->pts, reflink->time_base); > #if FF_API_FRAME_PKT > FF_DISABLE_DEPRECATION_WARNINGS > - scale->var_values[VAR_POS] = in->pkt_pos == -1 ? NAN : in->pkt_pos; > + scale->var_values[VAR_POS] = ref->pkt_pos == -1 ? NAN : ref->pkt_pos; > FF_ENABLE_DEPRECATION_WARNINGS > #endif > + } > + } > + > + ret = scale_frame(ctx->inputs[0], main, &out); > + if (out) { > + return ff_filter_frame(outlink, out); > } > > - return ff_filter_frame(outlink, in); > + return ret; > } > > static int process_command(AVFilterContext *ctx, const char *cmd, const char *args, > @@ -973,9 +969,25 @@ static int process_command(AVFilterContext *ctx, const char *cmd, const char *ar > return ret; > } > > +static int activate(AVFilterContext *ctx) > +{ > + ScaleContext *scale = ctx->priv; > + return ff_framesync_activate(&scale->fs); > +} > + > static const AVClass *child_class_iterate(void **iter) > { > - const AVClass *c = *iter ? NULL : sws_get_class(); > + void *tmp = NULL; > + const AVClass *sws = sws_get_class(); > + const AVClass *fs = ff_framesync_child_class_iterate(tmp); This should be &tmp. It is probably the reason for Michael's segfault. Apart from that: It is easier if you simply used 0..2 for *iter (1== returned sws_get_class, 2 returned ff_framesync_child_class_iterate). > + const AVClass *c; > + if (!*iter) { > + c = sws; > + } else if (*iter == (void*)(uintptr_t)sws) { > + c = fs; > + } else { > + c = NULL; > + } > *iter = (void*)(uintptr_t)c; > return c; > } > @@ -985,6 +997,8 @@ static void *child_next(void *obj, void *prev) > ScaleContext *s = obj; > if (!prev) > return s->sws_opts; > + if (prev == s->sws_opts) > + return &s->fs; > return NULL; > } > > @@ -1082,12 +1096,10 @@ static const AVFilterPad avfilter_vf_scale2ref_inputs[] = { > { > .name = "default", > .type = AVMEDIA_TYPE_VIDEO, > - .filter_frame = filter_frame, > }, > { > .name = "ref", > .type = AVMEDIA_TYPE_VIDEO, > - .filter_frame = filter_frame_ref, > }, > }; > > @@ -1096,13 +1108,6 @@ static const AVFilterPad avfilter_vf_scale2ref_outputs[] = { > .name = "default", > .type = AVMEDIA_TYPE_VIDEO, > .config_props = config_props, > - .request_frame= request_frame, > - }, > - { > - .name = "ref", > - .type = AVMEDIA_TYPE_VIDEO, > - .config_props = config_props_ref, > - .request_frame= request_frame_ref, > }, > }; > > @@ -1117,5 +1122,6 @@ const AVFilter ff_vf_scale2ref = { > FILTER_INPUTS(avfilter_vf_scale2ref_inputs), > FILTER_OUTPUTS(avfilter_vf_scale2ref_outputs), > FILTER_QUERY_FUNC(query_formats), > + .activate = activate, > .process_command = process_command, > }; > diff --git a/tests/filtergraphs/scale2ref_keep_aspect b/tests/filtergraphs/scale2ref_keep_aspect > index f407460ec7c..d63968666a8 100644 > --- a/tests/filtergraphs/scale2ref_keep_aspect > +++ b/tests/filtergraphs/scale2ref_keep_aspect > @@ -1,5 +1,4 @@ > sws_flags=+accurate_rnd+bitexact; > testsrc=size=320x240 [main]; > testsrc=size=640x360 [ref]; > -[main][ref] scale2ref=iw/4:ow/mdar [main][ref]; > -[ref] nullsink > +[main][ref] scale2ref=iw/4:ow/mdar [main]
On Wed, Mar 13, 2024 at 08:43:58PM -0300, James Almer wrote: > On 3/13/2024 8:41 PM, Michael Niedermayer wrote: > > On Wed, Mar 13, 2024 at 01:24:25PM +0100, Niklas Haas wrote: > > > From: Niklas Haas <git@haasn.dev> > > > > > > This filter's existing design has a number of issues: > > > > > > - There is no guarantee whatsoever about the order in which frames are > > > pushed onto the main and ref link, due to this being entirely > > > dependent on the order in which downstream filters decide to request > > > frames from their various inputs. As such, there is absolutely no > > > synchronization for ref streams with dynamically changing resolutions > > > (see e.g. fate/h264/reinit-*). > > > > > > - For some (likely historical) reason, this filter outputs its ref > > > stream as a second ref output, which is in principle completely > > > unnecessary (complex filter graph users can just duplicate the input > > > pin), but in practice only required to allow this filter to > > > "eventually" see changes to the ref stream (see first point). In > > > particular, this means that if the user uses the "wrong" pin, this > > > filter may break completely. > > > > > > - The default filter activation function is fundamentally incapable of > > > handling filters with multiple inputs cleanly, because doing so > > > requires both knowledge of how these inputs should be internally > > > ordered, but also how to handle EOF conditions on either input (or > > > downstream). Both of these are best left to the filter specific > > > options. (See #10795 for the consequences of using the default > > > activate with multiple inputs). > > > > > > Switching this filter to framesync fixes all three points: > > > > > > - ff_framesync_activate() correctly handles multiple inputs and EOF > > > conditions (and is configurable with the framesync-specific options) > > > - framesync only supports a single output, so we can (indeed must) drop > > > the redundant ref output stream > > > > > > Update documentation, changelog and tests to correspond to the new usage > > > pattern. > > > > > > Fixes: https://trac.ffmpeg.org/ticket/10795 > > > --- > > > Changelog | 2 + > > > doc/filters.texi | 10 +- > > > libavfilter/vf_scale.c | 130 ++++++++++++----------- > > > tests/filtergraphs/scale2ref_keep_aspect | 3 +- > > > 4 files changed, 76 insertions(+), 69 deletions(-) > > > > this causes > > ./ffplay --help > > to segfault > > Unrelated to this crash, but why does that command line output every single > option from every single filter? It's several thousand printed lines. > Shouldn't that be the output of --longhelp or similar, and leave --help to > print some basic non filter specific options? I think the help handling should be consistent between the tools, iam not sure why it is not currently thx [...]
> This should be &tmp. It is probably the reason for Michael's segfault. > Apart from that: It is easier if you simply used 0..2 for *iter (1== > returned sws_get_class, 2 returned ff_framesync_child_class_iterate). Changed this function to: static const AVClass *child_class_iterate(void **iter) { void *tmp = NULL; switch ((uintptr_t)*iter) { case 0: *iter = (void*)(uintptr_t)1; return sws_get_class(); case 1: *iter = (void*)(uintptr_t)2; return ff_framesync_child_class_iterate(&tmp); } return NULL; }
Niklas Haas: >> This should be &tmp. It is probably the reason for Michael's segfault. >> Apart from that: It is easier if you simply used 0..2 for *iter (1== >> returned sws_get_class, 2 returned ff_framesync_child_class_iterate). > > Changed this function to: > > static const AVClass *child_class_iterate(void **iter) > { > void *tmp = NULL; > switch ((uintptr_t)*iter) { > case 0: > *iter = (void*)(uintptr_t)1; > return sws_get_class(); > case 1: > *iter = (void*)(uintptr_t)2; > return ff_framesync_child_class_iterate(&tmp); > } > > return NULL; > } Better make framesync_class un-static and return that directly. Also: It makes no sense for vf_scale's child_class_iterate and child_next to return framesync-objects (even a framesync context whose AVClass* has not been set!); but this is what your patch does. - Andreas
On Wed, Mar 13, 2024 at 01:24:25PM +0100, Niklas Haas wrote: > From: Niklas Haas <git@haasn.dev> > > This filter's existing design has a number of issues: > > - There is no guarantee whatsoever about the order in which frames are > pushed onto the main and ref link, due to this being entirely > dependent on the order in which downstream filters decide to request > frames from their various inputs. As such, there is absolutely no > synchronization for ref streams with dynamically changing resolutions > (see e.g. fate/h264/reinit-*). > > - For some (likely historical) reason, this filter outputs its ref > stream as a second ref output, which is in principle completely > unnecessary (complex filter graph users can just duplicate the input > pin), but in practice only required to allow this filter to > "eventually" see changes to the ref stream (see first point). In > particular, this means that if the user uses the "wrong" pin, this > filter may break completely. > > - The default filter activation function is fundamentally incapable of > handling filters with multiple inputs cleanly, because doing so > requires both knowledge of how these inputs should be internally > ordered, but also how to handle EOF conditions on either input (or > downstream). Both of these are best left to the filter specific > options. (See #10795 for the consequences of using the default > activate with multiple inputs). > > Switching this filter to framesync fixes all three points: > > - ff_framesync_activate() correctly handles multiple inputs and EOF > conditions (and is configurable with the framesync-specific options) > - framesync only supports a single output, so we can (indeed must) drop > the redundant ref output stream > > Update documentation, changelog and tests to correspond to the new usage > pattern. > > Fixes: https://trac.ffmpeg.org/ticket/10795 > --- > Changelog | 2 + > doc/filters.texi | 10 +- > libavfilter/vf_scale.c | 130 ++++++++++++----------- > tests/filtergraphs/scale2ref_keep_aspect | 3 +- > 4 files changed, 76 insertions(+), 69 deletions(-) > > diff --git a/Changelog b/Changelog > index 069b8274489..bacda2524ea 100644 > --- a/Changelog > +++ b/Changelog > @@ -32,6 +32,8 @@ version <next>: > - DVD-Video demuxer, powered by libdvdnav and libdvdread > - ffprobe -show_stream_groups option > - ffprobe (with -export_side_data film_grain) now prints film grain metadata > +- scale2ref now only has a single output stream, and supports the framesync > + options is it neccessary to drop compatibility to the old syntax ? thx [...]
On Tue, 19 Mar 2024 22:55:56 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote:
> is it neccessary to drop compatibility to the old syntax ?
Only if we want to use framesync. If we don't care about synchronizing
the streams, then we could drop FS and just use a custom activate
function which forwards output status only to the *corresponding* input.
Maybe also with printing a warning if the refstream parameters do happen
to change, since at that point we know it wouldn't be in sync.
On Wed, Mar 20, 2024 at 03:23:53PM +0100, Niklas Haas wrote: > On Tue, 19 Mar 2024 22:55:56 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > > is it neccessary to drop compatibility to the old syntax ? > > Only if we want to use framesync. If we don't care about synchronizing > the streams, then we could drop FS and just use a custom activate > function which forwards output status only to the *corresponding* input. if the full bugfix cant be done in the same filter then i guess we need a 2nd filter, maybe scale1ref [...] thx
On Wed, 20 Mar 2024 20:55:26 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Mar 20, 2024 at 03:23:53PM +0100, Niklas Haas wrote: > > On Tue, 19 Mar 2024 22:55:56 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > > > is it neccessary to drop compatibility to the old syntax ? > > > > Only if we want to use framesync. If we don't care about synchronizing > > the streams, then we could drop FS and just use a custom activate > > function which forwards output status only to the *corresponding* input. > > if the full bugfix cant be done in the same filter then i guess we need a 2nd > filter, maybe scale1ref I think we can do this by extending vf_scale itself to support taking a second input, that way we don't need a second filter at all. My proposal for this: - Add new variables ref_w, ref_h (and related fields), possibly aliased to 'rw' and 'rh'. - When `ref_*` is referenced in any expression, vf_scale will init() with a second input stream. - Semantics will be equivalent to this patch. - Fully deprecate scale2ref, and eventually remove it and the main_* series of variables. That way, scale2ref just becomes `scale=w=rw:h=rh`, and also allows us to simplify these filters a bit.
diff --git a/Changelog b/Changelog index 069b8274489..bacda2524ea 100644 --- a/Changelog +++ b/Changelog @@ -32,6 +32,8 @@ version <next>: - DVD-Video demuxer, powered by libdvdnav and libdvdread - ffprobe -show_stream_groups option - ffprobe (with -export_side_data film_grain) now prints film grain metadata +- scale2ref now only has a single output stream, and supports the framesync + options version 6.1: diff --git a/doc/filters.texi b/doc/filters.texi index e0436a5755c..07e8136adb3 100644 --- a/doc/filters.texi +++ b/doc/filters.texi @@ -21555,9 +21555,9 @@ Deprecated, do not use. Scale (resize) the input video, based on a reference video. See the scale filter for available options, scale2ref supports the same but -uses the reference video instead of the main input as basis. scale2ref also -supports the following additional constants for the @option{w} and -@option{h} options: +uses the reference video instead of the main input as basis. This filter also +supports the @ref{framesync} options. In addition, scale2ref also supports the +following additional constants for the @option{w} and @option{h} options: @table @var @item main_w @@ -21600,13 +21600,13 @@ Only available with @code{eval=frame}. @item Scale a subtitle stream (b) to match the main video (a) in size before overlaying @example -'scale2ref[b][a];[a][b]overlay' +'[b][a]scale2ref[sub];[a][sub]overlay' @end example @item Scale a logo to 1/10th the height of a video, while preserving its display aspect ratio. @example -[logo-in][video-in]scale2ref=w=oh*mdar:h=ih/10[logo-out][video-out] +[logo-in][video]scale2ref=w=oh*mdar:h=ih/10[logo-out] @end example @end itemize diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c index fc3b5a91e60..d4173b63097 100644 --- a/libavfilter/vf_scale.c +++ b/libavfilter/vf_scale.c @@ -29,6 +29,7 @@ #include "avfilter.h" #include "formats.h" +#include "framesync.h" #include "internal.h" #include "scale_eval.h" #include "video.h" @@ -114,6 +115,7 @@ typedef struct ScaleContext { struct SwsContext *isws[2]; ///< software scaler context for interlaced material // context used for forwarding options to sws struct SwsContext *sws_opts; + FFFrameSync fs; ///< for scale2ref /** * New dimensions. Special values are: @@ -288,6 +290,9 @@ static av_cold int preinit(AVFilterContext *ctx) if (ret < 0) return ret; + if (ctx->filter == &ff_vf_scale2ref) + ff_framesync_preinit(&scale->fs); + return 0; } @@ -303,6 +308,8 @@ static const int sws_colorspaces[] = { -1 }; +static int do_scale2ref(FFFrameSync *fs); + static av_cold int init(AVFilterContext *ctx) { ScaleContext *scale = ctx->priv; @@ -380,6 +387,7 @@ static av_cold int init(AVFilterContext *ctx) if (!threads) av_opt_set_int(scale->sws_opts, "threads", ff_filter_get_nb_threads(ctx), 0); + scale->fs.on_event = do_scale2ref; return 0; } @@ -389,6 +397,7 @@ static av_cold void uninit(AVFilterContext *ctx) av_expr_free(scale->w_pexpr); av_expr_free(scale->h_pexpr); scale->w_pexpr = scale->h_pexpr = NULL; + ff_framesync_uninit(&scale->fs); sws_freeContext(scale->sws_opts); sws_freeContext(scale->sws); sws_freeContext(scale->isws[0]); @@ -678,35 +687,16 @@ static int config_props(AVFilterLink *outlink) flags_val); av_freep(&flags_val); - return 0; - -fail: - return ret; -} - -static int config_props_ref(AVFilterLink *outlink) -{ - AVFilterLink *inlink = outlink->src->inputs[1]; - - outlink->w = inlink->w; - outlink->h = inlink->h; - outlink->sample_aspect_ratio = inlink->sample_aspect_ratio; - outlink->time_base = inlink->time_base; - outlink->frame_rate = inlink->frame_rate; - outlink->colorspace = inlink->colorspace; - outlink->color_range = inlink->color_range; + if (ctx->filter != &ff_vf_scale2ref) + return 0; - return 0; -} + if ((ret = ff_framesync_init_dualinput(&scale->fs, ctx)) < 0) + return ret; -static int request_frame(AVFilterLink *outlink) -{ - return ff_request_frame(outlink->src->inputs[0]); -} + return ff_framesync_configure(&scale->fs); -static int request_frame_ref(AVFilterLink *outlink) -{ - return ff_request_frame(outlink->src->inputs[1]); +fail: + return ret; } static void frame_offset(AVFrame *frame, int dir, int is_pal) @@ -909,43 +899,49 @@ static int filter_frame(AVFilterLink *link, AVFrame *in) return ret; } -static int filter_frame_ref(AVFilterLink *link, AVFrame *in) +static int do_scale2ref(FFFrameSync *fs) { - ScaleContext *scale = link->dst->priv; - AVFilterLink *outlink = link->dst->outputs[1]; - int frame_changed; + AVFilterContext *ctx = fs->parent; + ScaleContext *scale = ctx->priv; + AVFilterLink *reflink = ctx->inputs[1]; + AVFilterLink *outlink = ctx->outputs[0]; + AVFrame *main, *ref, *out; + int ret; - frame_changed = in->width != link->w || - in->height != link->h || - in->format != link->format || - in->sample_aspect_ratio.den != link->sample_aspect_ratio.den || - in->sample_aspect_ratio.num != link->sample_aspect_ratio.num || - in->colorspace != link->colorspace || - in->color_range != link->color_range; + ret = ff_framesync_dualinput_get(fs, &main, &ref); + if (ret < 0) + return ret; - if (frame_changed) { - link->format = in->format; - link->w = in->width; - link->h = in->height; - link->sample_aspect_ratio.num = in->sample_aspect_ratio.num; - link->sample_aspect_ratio.den = in->sample_aspect_ratio.den; - link->colorspace = in->colorspace; - link->color_range = in->color_range; + if (ref) { + reflink->format = ref->format; + reflink->w = ref->width; + reflink->h = ref->height; + reflink->sample_aspect_ratio.num = ref->sample_aspect_ratio.num; + reflink->sample_aspect_ratio.den = ref->sample_aspect_ratio.den; + reflink->colorspace = ref->colorspace; + reflink->color_range = ref->color_range; - config_props_ref(outlink); - } + ret = config_props(outlink); + if (ret < 0) + return ret; - if (scale->eval_mode == EVAL_MODE_FRAME) { - scale->var_values[VAR_N] = link->frame_count_out; - scale->var_values[VAR_T] = TS2T(in->pts, link->time_base); + if (scale->eval_mode == EVAL_MODE_FRAME) { + scale->var_values[VAR_N] = reflink->frame_count_out; + scale->var_values[VAR_T] = TS2T(ref->pts, reflink->time_base); #if FF_API_FRAME_PKT FF_DISABLE_DEPRECATION_WARNINGS - scale->var_values[VAR_POS] = in->pkt_pos == -1 ? NAN : in->pkt_pos; + scale->var_values[VAR_POS] = ref->pkt_pos == -1 ? NAN : ref->pkt_pos; FF_ENABLE_DEPRECATION_WARNINGS #endif + } + } + + ret = scale_frame(ctx->inputs[0], main, &out); + if (out) { + return ff_filter_frame(outlink, out); } - return ff_filter_frame(outlink, in); + return ret; } static int process_command(AVFilterContext *ctx, const char *cmd, const char *args, @@ -973,9 +969,25 @@ static int process_command(AVFilterContext *ctx, const char *cmd, const char *ar return ret; } +static int activate(AVFilterContext *ctx) +{ + ScaleContext *scale = ctx->priv; + return ff_framesync_activate(&scale->fs); +} + static const AVClass *child_class_iterate(void **iter) { - const AVClass *c = *iter ? NULL : sws_get_class(); + void *tmp = NULL; + const AVClass *sws = sws_get_class(); + const AVClass *fs = ff_framesync_child_class_iterate(tmp); + const AVClass *c; + if (!*iter) { + c = sws; + } else if (*iter == (void*)(uintptr_t)sws) { + c = fs; + } else { + c = NULL; + } *iter = (void*)(uintptr_t)c; return c; } @@ -985,6 +997,8 @@ static void *child_next(void *obj, void *prev) ScaleContext *s = obj; if (!prev) return s->sws_opts; + if (prev == s->sws_opts) + return &s->fs; return NULL; } @@ -1082,12 +1096,10 @@ static const AVFilterPad avfilter_vf_scale2ref_inputs[] = { { .name = "default", .type = AVMEDIA_TYPE_VIDEO, - .filter_frame = filter_frame, }, { .name = "ref", .type = AVMEDIA_TYPE_VIDEO, - .filter_frame = filter_frame_ref, }, }; @@ -1096,13 +1108,6 @@ static const AVFilterPad avfilter_vf_scale2ref_outputs[] = { .name = "default", .type = AVMEDIA_TYPE_VIDEO, .config_props = config_props, - .request_frame= request_frame, - }, - { - .name = "ref", - .type = AVMEDIA_TYPE_VIDEO, - .config_props = config_props_ref, - .request_frame= request_frame_ref, }, }; @@ -1117,5 +1122,6 @@ const AVFilter ff_vf_scale2ref = { FILTER_INPUTS(avfilter_vf_scale2ref_inputs), FILTER_OUTPUTS(avfilter_vf_scale2ref_outputs), FILTER_QUERY_FUNC(query_formats), + .activate = activate, .process_command = process_command, }; diff --git a/tests/filtergraphs/scale2ref_keep_aspect b/tests/filtergraphs/scale2ref_keep_aspect index f407460ec7c..d63968666a8 100644 --- a/tests/filtergraphs/scale2ref_keep_aspect +++ b/tests/filtergraphs/scale2ref_keep_aspect @@ -1,5 +1,4 @@ sws_flags=+accurate_rnd+bitexact; testsrc=size=320x240 [main]; testsrc=size=640x360 [ref]; -[main][ref] scale2ref=iw/4:ow/mdar [main][ref]; -[ref] nullsink +[main][ref] scale2ref=iw/4:ow/mdar [main]
From: Niklas Haas <git@haasn.dev> This filter's existing design has a number of issues: - There is no guarantee whatsoever about the order in which frames are pushed onto the main and ref link, due to this being entirely dependent on the order in which downstream filters decide to request frames from their various inputs. As such, there is absolutely no synchronization for ref streams with dynamically changing resolutions (see e.g. fate/h264/reinit-*). - For some (likely historical) reason, this filter outputs its ref stream as a second ref output, which is in principle completely unnecessary (complex filter graph users can just duplicate the input pin), but in practice only required to allow this filter to "eventually" see changes to the ref stream (see first point). In particular, this means that if the user uses the "wrong" pin, this filter may break completely. - The default filter activation function is fundamentally incapable of handling filters with multiple inputs cleanly, because doing so requires both knowledge of how these inputs should be internally ordered, but also how to handle EOF conditions on either input (or downstream). Both of these are best left to the filter specific options. (See #10795 for the consequences of using the default activate with multiple inputs). Switching this filter to framesync fixes all three points: - ff_framesync_activate() correctly handles multiple inputs and EOF conditions (and is configurable with the framesync-specific options) - framesync only supports a single output, so we can (indeed must) drop the redundant ref output stream Update documentation, changelog and tests to correspond to the new usage pattern. Fixes: https://trac.ffmpeg.org/ticket/10795 --- Changelog | 2 + doc/filters.texi | 10 +- libavfilter/vf_scale.c | 130 ++++++++++++----------- tests/filtergraphs/scale2ref_keep_aspect | 3 +- 4 files changed, 76 insertions(+), 69 deletions(-)