Message ID | 20210803161309.83767-1-fulinjie@zju.edu.cn |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,v3,1/2] lavfi/vf_scale: use default swscale flags for simple and complex filter graph | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
Linjie Fu (12021-08-04): > From: Linjie Fu <linjie.justin.fu@gmail.com> > > Currently the default swscale flags for simple filter graph is bicubic, > however for complex filter graph it uses bilinear as decleared in scale > filter. > > $ffmpeg -v verbose -i input.mp4 -vf format=yuv420p,scale=800x600 -an -f null - > [Parsed_scale_1 @ 0x7f86d2c160c0] w:1920 h:1080 fmt:yuv420p sar:0/1 -> w:800 h:600 fmt:yuv420p sar:0/1 flags:0x4 > > $ffmpeg -v verbose -i input.mp4 -filter_complex format=yuv420p,scale=800x600 -an -f null - > [Parsed_scale_1 @ 0x7f8779e046c0] w:1920 h:1080 fmt:yuv420p sar:0/1 -> w:800 h:600 fmt:yuv420p sar:0/1 flags:0x2 > > Use default swscale flags (bicubic currently) for scale filter. > - Remove flags="bilinear" from vf_scale > - Remove setting defaults in ffmpeg/ffprobe/ffplay > - Update the FATE refs > > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com> > --- > fftools/cmdutils.c | 8 -------- > fftools/ffplay.c | 2 -- > fftools/ffprobe.c | 1 - > libavfilter/vf_scale.c | 4 ++-- > tests/ref/fate/filter-scale2ref_keep_aspect | 10 +++++----- > 5 files changed, 7 insertions(+), 18 deletions(-) Why are patches for fftools squashed with this patch? They should be separate, as they were. > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index 0b1ef03a25..912e881174 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -81,11 +81,6 @@ enum show_muxdemuxers { > SHOW_MUXERS, > }; > > -void init_opts(void) > -{ > - av_dict_set(&sws_dict, "flags", "bicubic", 0); > -} > - > void uninit_opts(void) > { > av_dict_free(&swr_opts); > @@ -670,7 +665,6 @@ static void finish_group(OptionParseContext *octx, int group_idx, > resample_opts = NULL; > sws_dict = NULL; > swr_opts = NULL; > - init_opts(); > > memset(&octx->cur_group, 0, sizeof(octx->cur_group)); > } > @@ -708,8 +702,6 @@ static void init_parse_context(OptionParseContext *octx, > > octx->global_opts.group_def = &global_group; > octx->global_opts.arg = ""; > - > - init_opts(); > } > > void uninit_parse_context(OptionParseContext *octx) > diff --git a/fftools/ffplay.c b/fftools/ffplay.c > index 6b19574eae..46758b9f55 100644 > --- a/fftools/ffplay.c > +++ b/fftools/ffplay.c > @@ -3695,8 +3695,6 @@ int main(int argc, char **argv) > #endif > avformat_network_init(); > > - init_opts(); > - > signal(SIGINT , sigterm_handler); /* Interrupt (ANSI). */ > signal(SIGTERM, sigterm_handler); /* Termination (ANSI). */ > > diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c > index f411ba35b5..95263e1e6f 100644 > --- a/fftools/ffprobe.c > +++ b/fftools/ffprobe.c > @@ -3721,7 +3721,6 @@ int main(int argc, char **argv) > options = real_options; > parse_loglevel(argc, argv, options); > avformat_network_init(); > - init_opts(); > #if CONFIG_AVDEVICE > avdevice_register_all(); > #endif > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > index f07e01bf90..683c4caa37 100644 > --- a/libavfilter/vf_scale.c > +++ b/libavfilter/vf_scale.c > @@ -313,7 +313,7 @@ static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts) > > scale->flags = 0; > > - if (scale->flags_str) { > + if (*scale->flags_str) { It could still be NULL, IIRC. > const AVClass *class = sws_get_class(); > const AVOption *o = av_opt_find(&class, "sws_flags", NULL, 0, > AV_OPT_SEARCH_FAKE_OBJ); > @@ -900,7 +900,7 @@ static const AVOption scale_options[] = { > { "width", "Output video width", OFFSET(w_expr), AV_OPT_TYPE_STRING, .flags = TFLAGS }, > { "h", "Output video height", OFFSET(h_expr), AV_OPT_TYPE_STRING, .flags = TFLAGS }, > { "height","Output video height", OFFSET(h_expr), AV_OPT_TYPE_STRING, .flags = TFLAGS }, > - { "flags", "Flags to pass to libswscale", OFFSET(flags_str), AV_OPT_TYPE_STRING, { .str = "bilinear" }, .flags = FLAGS }, > + { "flags", "Flags to pass to libswscale", OFFSET(flags_str), AV_OPT_TYPE_STRING, { .str = "" }, .flags = FLAGS }, This is now correct. > { "interl", "set interlacing", OFFSET(interlaced), AV_OPT_TYPE_BOOL, {.i64 = 0 }, -1, 1, FLAGS }, > { "size", "set video size", OFFSET(size_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, FLAGS }, > { "s", "set video size", OFFSET(size_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, FLAGS }, > diff --git a/tests/ref/fate/filter-scale2ref_keep_aspect b/tests/ref/fate/filter-scale2ref_keep_aspect > index 8dd0dbb13b..53c6fc14ec 100644 > --- a/tests/ref/fate/filter-scale2ref_keep_aspect > +++ b/tests/ref/fate/filter-scale2ref_keep_aspect > @@ -7,8 +7,8 @@ > #dimensions 0: 160x120 > #sar 0: 1/1 > #stream#, dts, pts, duration, size, hash > -0, 0, 0, 1, 57600, 9a19c23dc3a557786840d0098606d5f1 > -0, 1, 1, 1, 57600, e6fbdabaf1bb0d28afc648ed4d27e9f0 > -0, 2, 2, 1, 57600, 52924ed0a751214c50fb2e7a626c8cc5 > -0, 3, 3, 1, 57600, 67d5fd6ee71793f1cf8794d1c27afdce > -0, 4, 4, 1, 57600, 85f7775f7b01afd369fc8919dc759d30 > +0, 0, 0, 1, 57600, 65fe9892ad710cc5763b04b390327d40 > +0, 1, 1, 1, 57600, 5e8d4524bc8889afa8769e851e998bc0 > +0, 2, 2, 1, 57600, 8f5e0e58d1f4c2104b82ef7a16850f1e > +0, 3, 3, 1, 57600, cfe4142845e1445d33748493faa63cda > +0, 4, 4, 1, 57600, bb491f3b01788773fb6129aef0f0abd2 Regards,
On Wed, Aug 4, 2021 at 7:03 PM Nicolas George <george@nsup.org> wrote: > Linjie Fu (12021-08-04): > > From: Linjie Fu <linjie.justin.fu@gmail.com> > > > > Currently the default swscale flags for simple filter graph is bicubic, > > however for complex filter graph it uses bilinear as decleared in scale > > filter. > > > > $ffmpeg -v verbose -i input.mp4 -vf format=yuv420p,scale=800x600 -an -f > null - > > [Parsed_scale_1 @ 0x7f86d2c160c0] w:1920 h:1080 fmt:yuv420p sar:0/1 -> > w:800 h:600 fmt:yuv420p sar:0/1 flags:0x4 > > > > $ffmpeg -v verbose -i input.mp4 -filter_complex > format=yuv420p,scale=800x600 -an -f null - > > [Parsed_scale_1 @ 0x7f8779e046c0] w:1920 h:1080 fmt:yuv420p sar:0/1 -> > w:800 h:600 fmt:yuv420p sar:0/1 flags:0x2 > > > > Use default swscale flags (bicubic currently) for scale filter. > > - Remove flags="bilinear" from vf_scale > > - Remove setting defaults in ffmpeg/ffprobe/ffplay > > - Update the FATE refs > > > > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com> > > --- > > fftools/cmdutils.c | 8 -------- > > fftools/ffplay.c | 2 -- > > fftools/ffprobe.c | 1 - > > libavfilter/vf_scale.c | 4 ++-- > > tests/ref/fate/filter-scale2ref_keep_aspect | 10 +++++----- > > 5 files changed, 7 insertions(+), 18 deletions(-) > > Why are patches for fftools squashed with this patch? They should be > separate, as they were. > You're right, splitted locally. > > > > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > > index 0b1ef03a25..912e881174 100644 > > --- a/fftools/cmdutils.c > > +++ b/fftools/cmdutils.c > > @@ -81,11 +81,6 @@ enum show_muxdemuxers { > > SHOW_MUXERS, > > }; > > > > -void init_opts(void) > > -{ > > - av_dict_set(&sws_dict, "flags", "bicubic", 0); > > -} > > - > > void uninit_opts(void) > > { > > av_dict_free(&swr_opts); > > @@ -670,7 +665,6 @@ static void finish_group(OptionParseContext *octx, > int group_idx, > > resample_opts = NULL; > > sws_dict = NULL; > > swr_opts = NULL; > > - init_opts(); > > > > memset(&octx->cur_group, 0, sizeof(octx->cur_group)); > > } > > @@ -708,8 +702,6 @@ static void init_parse_context(OptionParseContext > *octx, > > > > octx->global_opts.group_def = &global_group; > > octx->global_opts.arg = ""; > > - > > - init_opts(); > > } > > > > void uninit_parse_context(OptionParseContext *octx) > > diff --git a/fftools/ffplay.c b/fftools/ffplay.c > > index 6b19574eae..46758b9f55 100644 > > --- a/fftools/ffplay.c > > +++ b/fftools/ffplay.c > > @@ -3695,8 +3695,6 @@ int main(int argc, char **argv) > > #endif > > avformat_network_init(); > > > > - init_opts(); > > - > > signal(SIGINT , sigterm_handler); /* Interrupt (ANSI). */ > > signal(SIGTERM, sigterm_handler); /* Termination (ANSI). */ > > > > diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c > > index f411ba35b5..95263e1e6f 100644 > > --- a/fftools/ffprobe.c > > +++ b/fftools/ffprobe.c > > @@ -3721,7 +3721,6 @@ int main(int argc, char **argv) > > options = real_options; > > parse_loglevel(argc, argv, options); > > avformat_network_init(); > > - init_opts(); > > #if CONFIG_AVDEVICE > > avdevice_register_all(); > > #endif > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > > index f07e01bf90..683c4caa37 100644 > > --- a/libavfilter/vf_scale.c > > +++ b/libavfilter/vf_scale.c > > @@ -313,7 +313,7 @@ static av_cold int init_dict(AVFilterContext *ctx, > AVDictionary **opts) > > > > scale->flags = 0; > > > > > - if (scale->flags_str) { > > + if (*scale->flags_str) { > > It could still be NULL, IIRC. > sws_flags doesn't have a candidate for "" (NULL) [1]. Hence NULL input for flags leads to a parsing issue: $ffmpeg -v verbose -i input.mp4 -vf format=yuv420p,scale=800x600 -an -vframes 10 -f md5 - [Parsed_scale_1 @ 0x7fc03f44e6c0] w:800 h:600 flags:'' interl:0 [swscaler @ 0x7ffee24cb830] [Eval @ 0x7ffee24cb130] Undefined constant or missing '(' in '' [swscaler @ 0x7ffee24cb830] Unable to parse option value "" [AVFilterGraph @ 0x7fc03f44e5c0] Error initializing filter 'scale' with args '800x600' Error reinitializing filters! Failed to inject frame into filter network: Invalid argument > > > const AVClass *class = sws_get_class(); > > const AVOption *o = av_opt_find(&class, "sws_flags", NULL, 0, > > AV_OPT_SEARCH_FAKE_OBJ); > > @@ -900,7 +900,7 @@ static const AVOption scale_options[] = { > > { "width", "Output video width", OFFSET(w_expr), > AV_OPT_TYPE_STRING, .flags = TFLAGS }, > > { "h", "Output video height", OFFSET(h_expr), > AV_OPT_TYPE_STRING, .flags = TFLAGS }, > > { "height","Output video height", OFFSET(h_expr), > AV_OPT_TYPE_STRING, .flags = TFLAGS }, > > > - { "flags", "Flags to pass to libswscale", OFFSET(flags_str), > AV_OPT_TYPE_STRING, { .str = "bilinear" }, .flags = FLAGS }, > > + { "flags", "Flags to pass to libswscale", OFFSET(flags_str), > AV_OPT_TYPE_STRING, { .str = "" }, .flags = FLAGS }, > > This is now correct. > Thanks. - linjie [1] https://github.com/FFmpeg/FFmpeg/blob/master/libswscale/options.c#L37
Linjie Fu (12021-08-05): > > > - if (scale->flags_str) { > > > + if (*scale->flags_str) { > > It could still be NULL, IIRC. > sws_flags doesn't have a candidate for "" (NULL) [1]. > Hence NULL input for flags leads to a parsing issue: It is not what I am talking about. The application can set scale->flags to NULL. If it does, *scale->flags_str crashes. Regards,
On Thu, Aug 5, 2021 at 2:22 AM Nicolas George <george@nsup.org> wrote: > Linjie Fu (12021-08-05): > > > > - if (scale->flags_str) { > > > > + if (*scale->flags_str) { > > > It could still be NULL, IIRC. > > sws_flags doesn't have a candidate for "" (NULL) [1]. > > Hence NULL input for flags leads to a parsing issue: > > It is not what I am talking about. > > The application can set scale->flags to NULL. If it does, > *scale->flags_str crashes. > Got your point, change it locally into: - if (scale->flags_str) { + if (scale->flags_str && *scale->flags_str) { Thanks for the review. Plan to apply soon if no more comments. - linjie
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 0b1ef03a25..912e881174 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -81,11 +81,6 @@ enum show_muxdemuxers { SHOW_MUXERS, }; -void init_opts(void) -{ - av_dict_set(&sws_dict, "flags", "bicubic", 0); -} - void uninit_opts(void) { av_dict_free(&swr_opts); @@ -670,7 +665,6 @@ static void finish_group(OptionParseContext *octx, int group_idx, resample_opts = NULL; sws_dict = NULL; swr_opts = NULL; - init_opts(); memset(&octx->cur_group, 0, sizeof(octx->cur_group)); } @@ -708,8 +702,6 @@ static void init_parse_context(OptionParseContext *octx, octx->global_opts.group_def = &global_group; octx->global_opts.arg = ""; - - init_opts(); } void uninit_parse_context(OptionParseContext *octx) diff --git a/fftools/ffplay.c b/fftools/ffplay.c index 6b19574eae..46758b9f55 100644 --- a/fftools/ffplay.c +++ b/fftools/ffplay.c @@ -3695,8 +3695,6 @@ int main(int argc, char **argv) #endif avformat_network_init(); - init_opts(); - signal(SIGINT , sigterm_handler); /* Interrupt (ANSI). */ signal(SIGTERM, sigterm_handler); /* Termination (ANSI). */ diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c index f411ba35b5..95263e1e6f 100644 --- a/fftools/ffprobe.c +++ b/fftools/ffprobe.c @@ -3721,7 +3721,6 @@ int main(int argc, char **argv) options = real_options; parse_loglevel(argc, argv, options); avformat_network_init(); - init_opts(); #if CONFIG_AVDEVICE avdevice_register_all(); #endif diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c index f07e01bf90..683c4caa37 100644 --- a/libavfilter/vf_scale.c +++ b/libavfilter/vf_scale.c @@ -313,7 +313,7 @@ static av_cold int init_dict(AVFilterContext *ctx, AVDictionary **opts) scale->flags = 0; - if (scale->flags_str) { + if (*scale->flags_str) { const AVClass *class = sws_get_class(); const AVOption *o = av_opt_find(&class, "sws_flags", NULL, 0, AV_OPT_SEARCH_FAKE_OBJ); @@ -900,7 +900,7 @@ static const AVOption scale_options[] = { { "width", "Output video width", OFFSET(w_expr), AV_OPT_TYPE_STRING, .flags = TFLAGS }, { "h", "Output video height", OFFSET(h_expr), AV_OPT_TYPE_STRING, .flags = TFLAGS }, { "height","Output video height", OFFSET(h_expr), AV_OPT_TYPE_STRING, .flags = TFLAGS }, - { "flags", "Flags to pass to libswscale", OFFSET(flags_str), AV_OPT_TYPE_STRING, { .str = "bilinear" }, .flags = FLAGS }, + { "flags", "Flags to pass to libswscale", OFFSET(flags_str), AV_OPT_TYPE_STRING, { .str = "" }, .flags = FLAGS }, { "interl", "set interlacing", OFFSET(interlaced), AV_OPT_TYPE_BOOL, {.i64 = 0 }, -1, 1, FLAGS }, { "size", "set video size", OFFSET(size_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, FLAGS }, { "s", "set video size", OFFSET(size_str), AV_OPT_TYPE_STRING, {.str = NULL}, 0, FLAGS }, diff --git a/tests/ref/fate/filter-scale2ref_keep_aspect b/tests/ref/fate/filter-scale2ref_keep_aspect index 8dd0dbb13b..53c6fc14ec 100644 --- a/tests/ref/fate/filter-scale2ref_keep_aspect +++ b/tests/ref/fate/filter-scale2ref_keep_aspect @@ -7,8 +7,8 @@ #dimensions 0: 160x120 #sar 0: 1/1 #stream#, dts, pts, duration, size, hash -0, 0, 0, 1, 57600, 9a19c23dc3a557786840d0098606d5f1 -0, 1, 1, 1, 57600, e6fbdabaf1bb0d28afc648ed4d27e9f0 -0, 2, 2, 1, 57600, 52924ed0a751214c50fb2e7a626c8cc5 -0, 3, 3, 1, 57600, 67d5fd6ee71793f1cf8794d1c27afdce -0, 4, 4, 1, 57600, 85f7775f7b01afd369fc8919dc759d30 +0, 0, 0, 1, 57600, 65fe9892ad710cc5763b04b390327d40 +0, 1, 1, 1, 57600, 5e8d4524bc8889afa8769e851e998bc0 +0, 2, 2, 1, 57600, 8f5e0e58d1f4c2104b82ef7a16850f1e +0, 3, 3, 1, 57600, cfe4142845e1445d33748493faa63cda +0, 4, 4, 1, 57600, bb491f3b01788773fb6129aef0f0abd2