diff mbox series

[FFmpeg-devel,v3,1/2] lavfi/vf_scale: use default swscale flags for simple and complex filter graph

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

Checks

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

Commit Message

Linjie Fu Aug. 3, 2021, 4:13 p.m. UTC 
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(-)

Comments

Nicolas George Aug. 4, 2021, 11:03 a.m. UTC | #1 
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,
Linjie Fu Aug. 4, 2021, 4:57 p.m. UTC | #2 
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
Nicolas George Aug. 4, 2021, 6:22 p.m. UTC | #3 
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,
Linjie Fu Aug. 5, 2021, 12:21 a.m. UTC | #4 
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 mbox series

Patch

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