diff mbox series

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

Message ID 20240324092635.17850-1-damiog@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avfilter: propagate colorspace and color_range from buffer filter and between AVFilterLink. | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 fail Make fate failed
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed

Commit Message

Damiano Galassi March 24, 2024, 9:26 a.m. UTC 
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(+)

Comments

Niklas Haas March 24, 2024, 12:23 p.m. UTC | #1 
On Sun, 24 Mar 2024 10:26:35 +0100 Damiano Galassi <damiog@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".

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
Niklas Haas March 24, 2024, 12:25 p.m. UTC | #2 
On Sun, 24 Mar 2024 13:23:28 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> 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

This is actually a reversal of previous commit 2303bf32327b94c25, so
probably this value is more correct.
Damiano Galassi March 24, 2024, 12:49 p.m. UTC | #3 
On Sun, Mar 24, 2024 at 1:23 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:

> On Sun, 24 Mar 2024 10:26:35 +0100 Damiano Galassi <damiog@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
>
Niklas Haas March 24, 2024, 1:14 p.m. UTC | #4 
On Sun, 24 Mar 2024 13:49:04 +0100 Damiano Galassi <damiog@gmail.com> wrote:
> 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.

Wait, now I'm confused what this patch even accomplishes then. If it's
already set, what else is there to do?
Damiano Galassi March 24, 2024, 1:47 p.m. UTC | #5 
On Sun, Mar 24, 2024 at 2:14 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:

> On Sun, 24 Mar 2024 13:49:04 +0100 Damiano Galassi <damiog@gmail.com>
> wrote:
> > 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.
>
> Wait, now I'm confused what this patch even accomplishes then. If it's
> already set, what else is there to do?


Because pick_format() doesn't set the right values, it doesn't know anything
outside the AVFilterLink it's working on.
So it sets a value that works for the link incfg and outcfg, but it doesn't
propagate
values between different links, and 99% of the times sets an unspecified
value.

What I meant was that your patch didn't make any difference from the
existing behavior
because even if it sets the default AVFilterLink values to AVCOL_SPC_NONE
and AVCOL_SPC_NONE,
when it's the time to call ff_filter_config_links(), those two values have
already been reset
to a default unspecified value, so you still get a wrongly configure graph
with something like:

Buffer
    Link:  AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
Scale
    Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
Whateverfilter
    Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
Buffersink

instead of

Buffer
    Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
Scale
    Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
Whateverfilter
    Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
Buffersink

Sorry if I'm not clear enough.
Niklas Haas March 24, 2024, 5:53 p.m. UTC | #6 
On Sun, 24 Mar 2024 14:47:27 +0100 Damiano Galassi <damiog@gmail.com> wrote:
> On Sun, Mar 24, 2024 at 2:14 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:
> 
> > On Sun, 24 Mar 2024 13:49:04 +0100 Damiano Galassi <damiog@gmail.com>
> > wrote:
> > > 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.
> >
> > Wait, now I'm confused what this patch even accomplishes then. If it's
> > already set, what else is there to do?
> 
> 
> Because pick_format() doesn't set the right values, it doesn't know anything
> outside the AVFilterLink it's working on.
> So it sets a value that works for the link incfg and outcfg, but it doesn't
> propagate
> values between different links, and 99% of the times sets an unspecified
> value.

But wait - aren't all filter's lists set to the same reference? Isn't that the
point of the design? If they share the same format list, they will all
inherit the correct setting (via pick_format). Except for filters like
vf_scale which deliberately use separate format lists, since they can
perform conversion. But even in this case,
swap_color_spaces_on_filters() should ensure that both lists are set to
the same value IFF they support the same color spaces.

Upon further inspection, it actually appears like the propagation you
propose in this patch is only needed for things which *aren't* subject
to format negotiation, which is why width, height and sample aspect
ratio are propagated here, but things like format or sample rates are not.

So this patch as written should not be applied, I think.

> What I meant was that your patch didn't make any difference from the
> existing behavior
> because even if it sets the default AVFilterLink values to AVCOL_SPC_NONE
> and AVCOL_SPC_NONE,
> when it's the time to call ff_filter_config_links(), those two values have
> already been reset
> to a default unspecified value, so you still get a wrongly configure graph
> with something like:
> 
> Buffer
>     Link:  AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> Scale
>     Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
> Whateverfilter
>     Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
> Buffersink
> 
> instead of
> 
> Buffer
>     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> Scale
>     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> Whateverfilter
>     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> Buffersink

Perhaps you can describe this scenario more thoroughly? What filters are
you using downstream of 'scale' here, and does it advertise BT709 as
supported?

> 
> Sorry if I'm not clear enough.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Damiano Galassi March 24, 2024, 7:03 p.m. UTC | #7 
On Sun, Mar 24, 2024 at 6:53 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:

> But wait - aren't all filter's lists set to the same reference? Isn't that
> the
> point of the design? If they share the same format list, they will all
> inherit the correct setting (via pick_format). Except for filters like
> vf_scale which deliberately use separate format lists, since they can
> perform conversion. But even in this case,
> swap_color_spaces_on_filters() should ensure that both lists are set to
> the same value IFF they support the same color spaces.
>
> Upon further inspection, it actually appears like the propagation you
> propose in this patch is only needed for things which *aren't* subject
> to format negotiation, which is why width, height and sample aspect
> ratio are propagated here, but things like format or sample rates are not.
>
> So this patch as written should not be applied, I think.
>
> > What I meant was that your patch didn't make any difference from the
> > existing behavior
> > because even if it sets the default AVFilterLink values to AVCOL_SPC_NONE
> > and AVCOL_SPC_NONE,
> > when it's the time to call ff_filter_config_links(), those two values
> have
> > already been reset
> > to a default unspecified value, so you still get a wrongly configure
> graph
> > with something like:
> >
> > Buffer
> >     Link:  AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > Scale
> >     Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
> > Whateverfilter
> >     Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
> > Buffersink
> >
> > instead of
> >
> > Buffer
> >     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > Scale
> >     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > Whateverfilter
> >     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > Buffersink
>
> Perhaps you can describe this scenario more thoroughly? What filters are
> you using downstream of 'scale' here, and does it advertise BT709 as
> supported?
>

It's entirely possible that I am completely wrong, I took a look at
avfilter code
for the first time just a few days ago.

Here's two way to reproduce with the cli something similar to what I am
seeing
when using the API directly:

ffmpeg -i in.mp4 -vf
"scale='width=1920:height=1080',zscale='width=1920:height=1080'" out.mp4
ffmpeg -i in.jpg -vf "zscale='width=1920:height=1080'" out.jpg

it doesn't need a specific mp4 or jpg file.
Niklas Haas March 25, 2024, 1:25 p.m. UTC | #8 
On Sun, 24 Mar 2024 20:03:03 +0100 Damiano Galassi <damiog@gmail.com> wrote:
> On Sun, Mar 24, 2024 at 6:53 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:
> 
> > But wait - aren't all filter's lists set to the same reference? Isn't that
> > the
> > point of the design? If they share the same format list, they will all
> > inherit the correct setting (via pick_format). Except for filters like
> > vf_scale which deliberately use separate format lists, since they can
> > perform conversion. But even in this case,
> > swap_color_spaces_on_filters() should ensure that both lists are set to
> > the same value IFF they support the same color spaces.
> >
> > Upon further inspection, it actually appears like the propagation you
> > propose in this patch is only needed for things which *aren't* subject
> > to format negotiation, which is why width, height and sample aspect
> > ratio are propagated here, but things like format or sample rates are not.
> >
> > So this patch as written should not be applied, I think.
> >
> > > What I meant was that your patch didn't make any difference from the
> > > existing behavior
> > > because even if it sets the default AVFilterLink values to AVCOL_SPC_NONE
> > > and AVCOL_SPC_NONE,
> > > when it's the time to call ff_filter_config_links(), those two values
> > have
> > > already been reset
> > > to a default unspecified value, so you still get a wrongly configure
> > graph
> > > with something like:
> > >
> > > Buffer
> > >     Link:  AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > > Scale
> > >     Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
> > > Whateverfilter
> > >     Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
> > > Buffersink
> > >
> > > instead of
> > >
> > > Buffer
> > >     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > > Scale
> > >     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > > Whateverfilter
> > >     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > > Buffersink
> >
> > Perhaps you can describe this scenario more thoroughly? What filters are
> > you using downstream of 'scale' here, and does it advertise BT709 as
> > supported?
> >
> 
> It's entirely possible that I am completely wrong, I took a look at
> avfilter code
> for the first time just a few days ago.
> 
> Here's two way to reproduce with the cli something similar to what I am
> seeing
> when using the API directly:
> 
> ffmpeg -i in.mp4 -vf
> "scale='width=1920:height=1080',zscale='width=1920:height=1080'" out.mp4
> ffmpeg -i in.jpg -vf "zscale='width=1920:height=1080'" out.jpg
> 
> it doesn't need a specific mp4 or jpg file.

So, in the second case, the bug is actually that buffersrc.c has a check
ff_fmt_is_regular_yuv() in order to set the YUV colorspace/matrix
fields, but YUVJ formats return 0 for this function. So the colorspace
never actually gets set to anything, it is left as UNSPECIFIED, which
then propagates through the filter chain, eventually leading to zimg
trying to convert from the real colorspace to unspec, which fails.

In retrospect, this logic is wrong - since YUV matrix still applies to
irregular YUV formats. We should split the check into is_yuv() and
is_yuv_forced_full_range(), and handle the overlapping cases
accordingly.

I will fix it.
Niklas Haas March 25, 2024, 1:40 p.m. UTC | #9 
On Sun, 24 Mar 2024 20:03:03 +0100 Damiano Galassi <damiog@gmail.com> wrote:
> On Sun, Mar 24, 2024 at 6:53 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:
> 
> > But wait - aren't all filter's lists set to the same reference? Isn't that
> > the
> > point of the design? If they share the same format list, they will all
> > inherit the correct setting (via pick_format). Except for filters like
> > vf_scale which deliberately use separate format lists, since they can
> > perform conversion. But even in this case,
> > swap_color_spaces_on_filters() should ensure that both lists are set to
> > the same value IFF they support the same color spaces.
> >
> > Upon further inspection, it actually appears like the propagation you
> > propose in this patch is only needed for things which *aren't* subject
> > to format negotiation, which is why width, height and sample aspect
> > ratio are propagated here, but things like format or sample rates are not.
> >
> > So this patch as written should not be applied, I think.
> >
> > > What I meant was that your patch didn't make any difference from the
> > > existing behavior
> > > because even if it sets the default AVFilterLink values to AVCOL_SPC_NONE
> > > and AVCOL_SPC_NONE,
> > > when it's the time to call ff_filter_config_links(), those two values
> > have
> > > already been reset
> > > to a default unspecified value, so you still get a wrongly configure
> > graph
> > > with something like:
> > >
> > > Buffer
> > >     Link:  AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > > Scale
> > >     Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
> > > Whateverfilter
> > >     Link: AVCOL_SPC_UNSPECIFIED AVCOL_RANGE_UNSPECIFIED
> > > Buffersink
> > >
> > > instead of
> > >
> > > Buffer
> > >     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > > Scale
> > >     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > > Whateverfilter
> > >     Link: AVCOL_SPC_BT709 AVCOL_RANGE_MPEG
> > > Buffersink
> >
> > Perhaps you can describe this scenario more thoroughly? What filters are
> > you using downstream of 'scale' here, and does it advertise BT709 as
> > supported?
> >
> 
> It's entirely possible that I am completely wrong, I took a look at
> avfilter code
> for the first time just a few days ago.
> 
> Here's two way to reproduce with the cli something similar to what I am
> seeing
> when using the API directly:
> 
> ffmpeg -i in.mp4 -vf
> "scale='width=1920:height=1080',zscale='width=1920:height=1080'" out.mp4
> ffmpeg -i in.jpg -vf "zscale='width=1920:height=1080'" out.jpg
> 
> it doesn't need a specific mp4 or jpg file.

I cannot reproduce any error with your first command line.
Damiano Galassi March 25, 2024, 2:36 p.m. UTC | #10 
On Mon, Mar 25, 2024 at 2:40 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:

> > ffmpeg -i in.mp4 -vf
> > "scale='width=1920:height=1080',zscale='width=1920:height=1080'" out.mp4
> > ffmpeg -i in.jpg -vf "zscale='width=1920:height=1080'" out.jpg
> >
> > it doesn't need a specific mp4 or jpg file.
>
> I cannot reproduce any error with your first command line.


 here's a sample file, and the output I get:

https://subler.org/downloads/test.mp4
https://gist.github.com/galad87/a6ddc3318cf20cd2f1ac7d053e1a0786
Niklas Haas March 25, 2024, 3:02 p.m. UTC | #11 
On Mon, 25 Mar 2024 15:36:26 +0100 Damiano Galassi <damiog@gmail.com> wrote:
> On Mon, Mar 25, 2024 at 2:40 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:
> 
> > > ffmpeg -i in.mp4 -vf
> > > "scale='width=1920:height=1080',zscale='width=1920:height=1080'" out.mp4
> > > ffmpeg -i in.jpg -vf "zscale='width=1920:height=1080'" out.jpg
> > >
> > > it doesn't need a specific mp4 or jpg file.
> >
> > I cannot reproduce any error with your first command line.
> 
> 
>  here's a sample file, and the output I get:
> 
> https://subler.org/downloads/test.mp4
> https://gist.github.com/galad87/a6ddc3318cf20cd2f1ac7d053e1a0786

Okay, I see the problem. Basically, swap_color_spaces() doesn't re-run
after pick_formats() settles a filter's format. So swap_color_spaces()
never actually sets zscale's preferred output format, because at that
point there are still multiple options for swscale's output format.

This patch appears to help, but I'm honestly not sure what exactly the
difference is between this REDUCE_FORMATS() and swap_color_spaces(), or
actually, for what case the latter even exists.

diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
index bb5399c55e8..3b8f7464889 100644
--- a/libavfilter/avfiltergraph.c
+++ b/libavfilter/avfiltergraph.c
@@ -794,6 +794,10 @@ static int reduce_formats_on_filter(AVFilterContext *filter)
                    nb_formats, ff_add_format);
     REDUCE_FORMATS(int,      AVFilterFormats,        samplerates,     formats,
                    nb_formats, ff_add_format);
+    REDUCE_FORMATS(int,      AVFilterFormats,        color_spaces,    formats,
+                   nb_formats, ff_add_format);
+    REDUCE_FORMATS(int,      AVFilterFormats,        color_ranges,    formats,
+                   nb_formats, ff_add_format);
 
     /* reduce channel layouts */
     for (i = 0; i < filter->nb_inputs; i++) {
Niklas Haas March 25, 2024, 3:07 p.m. UTC | #12 
On Mon, 25 Mar 2024 16:02:23 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> On Mon, 25 Mar 2024 15:36:26 +0100 Damiano Galassi <damiog@gmail.com> wrote:
> > On Mon, Mar 25, 2024 at 2:40 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:
> > 
> > > > ffmpeg -i in.mp4 -vf
> > > > "scale='width=1920:height=1080',zscale='width=1920:height=1080'" out.mp4
> > > > ffmpeg -i in.jpg -vf "zscale='width=1920:height=1080'" out.jpg
> > > >
> > > > it doesn't need a specific mp4 or jpg file.
> > >
> > > I cannot reproduce any error with your first command line.
> > 
> > 
> >  here's a sample file, and the output I get:
> > 
> > https://subler.org/downloads/test.mp4
> > https://gist.github.com/galad87/a6ddc3318cf20cd2f1ac7d053e1a0786
> 
> Okay, I see the problem. Basically, swap_color_spaces() doesn't re-run
> after pick_formats() settles a filter's format. So swap_color_spaces()
> never actually sets zscale's preferred output format, because at that
> point there are still multiple options for swscale's output format.
> 
> This patch appears to help, but I'm honestly not sure what exactly the
> difference is between this REDUCE_FORMATS() and swap_color_spaces(), or
> actually, for what case the latter even exists.

I *think* that the former is intended for picking an exact match,
whereas the latter is intended for picking the *best* remaining match
(where bestness can be meaningfully measured by some score)

So based on this, given that csp/range is only concerned with exact
matches, after the suggest patch we can just delete swap_color_spaces()
and swap_color_ranges(). Good riddance, too.

> 
> diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
> index bb5399c55e8..3b8f7464889 100644
> --- a/libavfilter/avfiltergraph.c
> +++ b/libavfilter/avfiltergraph.c
> @@ -794,6 +794,10 @@ static int reduce_formats_on_filter(AVFilterContext *filter)
>                     nb_formats, ff_add_format);
>      REDUCE_FORMATS(int,      AVFilterFormats,        samplerates,     formats,
>                     nb_formats, ff_add_format);
> +    REDUCE_FORMATS(int,      AVFilterFormats,        color_spaces,    formats,
> +                   nb_formats, ff_add_format);
> +    REDUCE_FORMATS(int,      AVFilterFormats,        color_ranges,    formats,
> +                   nb_formats, ff_add_format);
>  
>      /* reduce channel layouts */
>      for (i = 0; i < filter->nb_inputs; i++) {
diff mbox series

Patch

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);