[FFmpeg-devel] [PATCH 15/25] avfilter/vf_format: add color_ranges/spaces

[Anton Khirnov]

Quoting Niklas Haas (2023-11-09 13:19:47)
> From: Niklas Haas <git at haasn.dev>
> 
> Needed for fftools/ffmpeg_filter to be able to force specific output
> formats via the filter chain.
> ---
>  doc/filters.texi        |  8 ++++++++
>  libavfilter/vf_format.c | 44 +++++++++++++++++++++++++++++++++++++----
>  2 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 55a333680c..4599d575a6 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -14241,6 +14241,14 @@ It accepts the following parameters:
>  A '|'-separated list of pixel format names, such as
>  "pix_fmts=yuv420p|monow|rgb24".
>  
> + at item color_spaces
> +A '|'-separated list of color space names, such as
> +"color_spaces=bt709|bt470bg|bt2020nc".
> +
> + at item color_ranges
> +A '|'-separated list of color rangee names, such as
                                      ^
extra e

> +"color_spaces=tv|pc".
> +
>  @end table
>  
>  @subsection Examples
> diff --git a/libavfilter/vf_format.c b/libavfilter/vf_format.c
> index b137e3075e..d1bb9daa97 100644
> --- a/libavfilter/vf_format.c
> +++ b/libavfilter/vf_format.c
> @@ -40,18 +40,24 @@
>  typedef struct FormatContext {
>      const AVClass *class;
>      char *pix_fmts;
> +    char *csps;
> +    char *ranges;
>  
>      AVFilterFormats *formats; ///< parsed from `pix_fmts`
> +    AVFilterFormats *color_spaces; ///< parsed from `csps`
> +    AVFilterFormats *color_ranges; ///< parsed from `ranges`
>  } FormatContext;
>  
>  static av_cold void uninit(AVFilterContext *ctx)
>  {
>      FormatContext *s = ctx->priv;
>      ff_formats_unref(&s->formats);
> +    ff_formats_unref(&s->color_spaces);
> +    ff_formats_unref(&s->color_ranges);
>  }
>  
>  static av_cold int invert_formats(AVFilterFormats **fmts,
> -                                   AVFilterFormats *allfmts)
> +                                  AVFilterFormats *allfmts)
>  {
>      if (!allfmts)
>          return AVERROR(ENOMEM);
> @@ -99,13 +105,35 @@ static av_cold int init(AVFilterContext *ctx)
>              return ret;
>      }
>  
> +    for (char *sep, *cur = s->csps; cur; cur = sep) {
> +        sep = strchr(cur, '|');
> +        if (sep && *sep)
> +            *sep++ = 0;
> +        if ((ret = av_color_space_from_name(cur)) < 0 ||
> +            (ret = ff_add_format(&s->color_spaces, ret)) < 0)

I find this style of assigning inside a condition far more error-prone
and harder to read. Especially when you have three of them in a single
if() below.

Otherwise looks ok.

-- 
Anton Khirnov