[FFmpeg-devel] [PATCH v2] avfilter: propagate colorspace and color_range from buffer filter and between AVFilterLink.

[Damiano Galassi]

On Sun, Mar 24, 2024 at 1:23 PM Niklas Haas <ffmpeg at haasn.xyz> wrote:

> On Sun, 24 Mar 2024 10:26:35 +0100 Damiano Galassi <damiog at gmail.com>
> wrote:
> > There two new fields were never sent down the filter chain,
> > and no filter after the first had colorspace and color_range set,
> > causing breakage in zscale and possible other filters.
> > ---
> >  libavfilter/avfilter.c  | 4 ++++
> >  libavfilter/buffersrc.c | 2 ++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > index 831871de90..66733f5ecf 100644
> > --- a/libavfilter/avfilter.c
> > +++ b/libavfilter/avfilter.c
> > @@ -391,6 +391,10 @@ int ff_filter_config_links(AVFilterContext *filter)
> >                          link->w = inlink->w;
> >                      if (!link->h)
> >                          link->h = inlink->h;
> > +                    if (link->colorspace == AVCOL_SPC_UNSPECIFIED)
> > +                        link->colorspace = inlink->colorspace;
> > +                    if (link->color_range == AVCOL_RANGE_UNSPECIFIED)
> > +                        link->color_range = inlink->color_range;
> >                  } else if (!link->w || !link->h) {
> >                      av_log(link->src, AV_LOG_ERROR,
> >                             "Video source filters must set their output
> link's "
> > diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c
> > index ddcd403785..2760097edf 100644
> > --- a/libavfilter/buffersrc.c
> > +++ b/libavfilter/buffersrc.c
> > @@ -499,6 +499,8 @@ static int config_props(AVFilterLink *link)
> >          link->w = c->w;
> >          link->h = c->h;
> >          link->sample_aspect_ratio = c->pixel_aspect;
> > +        link->colorspace = c->color_space;
> > +        link->color_range = c->color_range;
> >
> >          if (c->hw_frames_ctx) {
> >              link->hw_frames_ctx = av_buffer_ref(c->hw_frames_ctx);
>
> This still breaks FATE.
>
> I think the problem is that AVCOL_SPC_UNSPECIFIED is a valid,
> configurable possibility for this field. So we need to set it to -1
> initially to distinguish "not set" from "unspecified".
>

AVFilterLink colorspace and color_range are first set in
avfiltergraph.c pick_format(),
so in ff_filter_config_links() they will never be AVCOL_SPC_NONE or
AVCOL_SPC_NONE.


> This diff resolves the FATE breakage:
>
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index 153fb700d30..2f22c9143df 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -186,7 +186,8 @@ int avfilter_link(AVFilterContext *src, unsigned
> srcpad,
>      link->type    = src->output_pads[srcpad].type;
>      av_assert0(AV_PIX_FMT_NONE == -1 && AV_SAMPLE_FMT_NONE == -1);
>      link->format  = -1;
> -    link->colorspace = AVCOL_SPC_UNSPECIFIED;
> +    link->colorspace = AVCOL_SPC_NONE;
> +    link->color_range = AVCOL_RANGE_NONE;
>      ff_framequeue_init(&li->fifo,
> &fffiltergraph(src->graph)->frame_queues);
>
>      return 0;
> @@ -391,9 +392,9 @@ int ff_filter_config_links(AVFilterContext *filter)
>                          link->w = inlink->w;
>                      if (!link->h)
>                          link->h = inlink->h;
> -                    if (link->colorspace == AVCOL_SPC_UNSPECIFIED)
> +                    if (link->colorspace < 0)
>                          link->colorspace = inlink->color_range;
> -                    if (link->color_range == AVCOL_RANGE_UNSPECIFIED)
> +                    if (link->color_range < 0)
>                          link->color_range = inlink->color_range;
>                  } else if (!link->w || !link->h) {
>                      av_log(link->src, AV_LOG_ERROR,
> diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> index 4aa20e4e585..605881a43de 100644
> --- a/libavutil/pixfmt.h
> +++ b/libavutil/pixfmt.h
> @@ -607,6 +607,7 @@ enum AVColorTransferCharacteristic {
>   * These values match the ones defined by ISO/IEC 23091-2_2019 subclause
> 8.3.
>   */
>  enum AVColorSpace {
> +    AVCOL_SPC_NONE        = -1, ///< invalid (reserved for internal use)
>      AVCOL_SPC_RGB         = 0,  ///< order of coefficients is actually
> GBR, also IEC 61966-2-1 (sRGB), YZX and ST 428-1
>      AVCOL_SPC_BT709       = 1,  ///< also ITU-R BT1361 / IEC 61966-2-4
> xvYCC709 / derived in SMPTE RP 177 Annex B
>      AVCOL_SPC_UNSPECIFIED = 2,
> @@ -646,6 +647,7 @@ enum AVColorSpace {
>   *     bit unsigned integer range, please refer to BT.2100 (Table 9).
>   */
>  enum AVColorRange {
> +    AVCOL_RANGE_NONE = -1, ///< invalid (reserved for internal use)
>      AVCOL_RANGE_UNSPECIFIED = 0,
>
>      /**
> diff --git a/tests/ref/fate/rgb24-mkv b/tests/ref/fate/rgb24-mkv
> index 1cbed136dde..99234f10523 100644
> --- a/tests/ref/fate/rgb24-mkv
> +++ b/tests/ref/fate/rgb24-mkv
> @@ -1,5 +1,5 @@
> -7d767e8238c674ecfa80458cb281c09e *tests/data/fate/rgb24-mkv.matroska
> -58236 tests/data/fate/rgb24-mkv.matroska
> +e181dc84058c3584598333dabd110123 *tests/data/fate/rgb24-mkv.matroska
> +58225 tests/data/fate/rgb24-mkv.matroska
>  #tb 0: 1/10
>  #media_type 0: video
>  #codec_id 0: rawvideo
>