diff mbox series

[FFmpeg-devel] avfilter: fix YUV colorspace negotiation for YUVJ

Message ID 20240325134044.17030-1-ffmpeg@haasn.xyz
State New
Headers show
Series [FFmpeg-devel] avfilter: fix YUV colorspace negotiation for YUVJ | 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

Niklas Haas March 25, 2024, 1:40 p.m. UTC 
From: Niklas Haas <git@haasn.dev>

Ironically, despite being introduced to make YUVJ unnecessary, the new
YUV negotiation logic failed to actually negotiate YUVJ formats
themselves correctly, leading to errors when passing YUVJ frames into
a filter graph. (They were effectively treated like RGB or Grayscale
formats, rather than as forced-full-range YUV, and hence did not have
their colorspace matrix correctly negotiated)

Fix this by splitting off the YUVJ check from ff_fmt_is_regular_yuv().
Obviously, we can trivially undo this change again once YUVJ is actually
deleted from the codebase.
---
 libavfilter/avfiltergraph.c | 31 +++++++++++++++++++------------
 libavfilter/buffersrc.c     | 15 ++++++++++-----
 libavfilter/internal.h      |  5 +++++
 3 files changed, 34 insertions(+), 17 deletions(-)

Comments

Niklas Haas March 25, 2024, 3:10 p.m. UTC | #1 
On Mon, 25 Mar 2024 14:40:44 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> From: Niklas Haas <git@haasn.dev>
> 
> Ironically, despite being introduced to make YUVJ unnecessary, the new
> YUV negotiation logic failed to actually negotiate YUVJ formats
> themselves correctly, leading to errors when passing YUVJ frames into
> a filter graph. (They were effectively treated like RGB or Grayscale
> formats, rather than as forced-full-range YUV, and hence did not have
> their colorspace matrix correctly negotiated)
> 
> Fix this by splitting off the YUVJ check from ff_fmt_is_regular_yuv().
> Obviously, we can trivially undo this change again once YUVJ is actually
> deleted from the codebase.

Breaks FATE, I will investigate.
Niklas Haas March 28, 2024, 12:55 p.m. UTC | #2 
On Mon, 25 Mar 2024 16:10:55 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> On Mon, 25 Mar 2024 14:40:44 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> > From: Niklas Haas <git@haasn.dev>
> > 
> > Ironically, despite being introduced to make YUVJ unnecessary, the new
> > YUV negotiation logic failed to actually negotiate YUVJ formats
> > themselves correctly, leading to errors when passing YUVJ frames into
> > a filter graph. (They were effectively treated like RGB or Grayscale
> > formats, rather than as forced-full-range YUV, and hence did not have
> > their colorspace matrix correctly negotiated)
> > 
> > Fix this by splitting off the YUVJ check from ff_fmt_is_regular_yuv().
> > Obviously, we can trivially undo this change again once YUVJ is actually
> > deleted from the codebase.
> 
> Breaks FATE, I will investigate.

This breakage was already fixed by another series of commits I had on
a different branch, which were themselves dependent on the new AVCodec
API.

(Specifically, those commits that enable ffmpeg_enc.c to propagate the
desired output color range back up the filter graph, as well as the FATE
fix to properly tag rawvideo tiles)

It may make more sense to just focus on merging that one first, then.
diff mbox series

Patch

diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
index bb5399c55e8..bc63cb88f32 100644
--- a/libavfilter/avfiltergraph.c
+++ b/libavfilter/avfiltergraph.c
@@ -610,19 +610,22 @@  int ff_fmt_is_regular_yuv(enum AVPixelFormat fmt)
     if (desc->nb_components < 3)
         return 0; /* Grayscale is explicitly full-range in swscale */
     av_assert1(!(desc->flags & AV_PIX_FMT_FLAG_HWACCEL));
-    if (desc->flags & (AV_PIX_FMT_FLAG_RGB | AV_PIX_FMT_FLAG_PAL |
-                       AV_PIX_FMT_FLAG_XYZ | AV_PIX_FMT_FLAG_FLOAT))
-        return 0;
+    return !(desc->flags & (AV_PIX_FMT_FLAG_RGB | AV_PIX_FMT_FLAG_PAL |
+                            AV_PIX_FMT_FLAG_XYZ | AV_PIX_FMT_FLAG_FLOAT));
+}
 
+
+int ff_fmt_is_forced_full_range(enum AVPixelFormat fmt)
+{
     switch (fmt) {
     case AV_PIX_FMT_YUVJ420P:
     case AV_PIX_FMT_YUVJ422P:
     case AV_PIX_FMT_YUVJ444P:
     case AV_PIX_FMT_YUVJ440P:
     case AV_PIX_FMT_YUVJ411P:
-        return 0;
-    default:
         return 1;
+    default:
+        return 0;
     }
 }
 
@@ -698,14 +701,18 @@  static int pick_format(AVFilterLink *link, AVFilterLink *ref)
             link->incfg.color_spaces->nb_formats = 1;
             link->colorspace = link->incfg.color_spaces->formats[0];
 
-            if (!link->incfg.color_ranges->nb_formats) {
-                av_log(link->src, AV_LOG_ERROR, "Cannot select color range for"
-                       " the link between filters %s and %s.\n", link->src->name,
-                       link->dst->name);
-                return AVERROR(EINVAL);
+            if (ff_fmt_is_forced_full_range(swfmt)) {
+                link->color_range = AVCOL_RANGE_JPEG;
+            } else {
+                if (!link->incfg.color_ranges->nb_formats) {
+                    av_log(link->src, AV_LOG_ERROR, "Cannot select color range for"
+                           " the link between filters %s and %s.\n", link->src->name,
+                           link->dst->name);
+                    return AVERROR(EINVAL);
+                }
+                link->incfg.color_ranges->nb_formats = 1;
+                link->color_range = link->incfg.color_ranges->formats[0];
             }
-            link->incfg.color_ranges->nb_formats = 1;
-            link->color_range = link->incfg.color_ranges->formats[0];
         }
     } else if (link->type == AVMEDIA_TYPE_AUDIO) {
         int ret;
diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c
index ddcd4037859..4082a93dfd9 100644
--- a/libavfilter/buffersrc.c
+++ b/libavfilter/buffersrc.c
@@ -459,12 +459,17 @@  static int query_formats(AVFilterContext *ctx)
             if ((ret = ff_add_format(&color_spaces, c->color_space)) < 0 ||
                 (ret = ff_set_common_color_spaces(ctx, color_spaces)) < 0)
                 return ret;
-            if ((ret = ff_add_format(&color_ranges, c->color_range)) < 0)
-                return ret;
-            if (c->color_range == AVCOL_RANGE_UNSPECIFIED) {
-                /* allow implicitly promoting unspecified to mpeg */
-                if ((ret = ff_add_format(&color_ranges, AVCOL_RANGE_MPEG)) < 0)
+            if (ff_fmt_is_forced_full_range(swfmt)) {
+                if ((ret = ff_add_format(&color_ranges, AVCOL_RANGE_JPEG)) < 0)
+                    return ret;
+            } else {
+                if ((ret = ff_add_format(&color_ranges, c->color_range)) < 0)
                     return ret;
+                if (c->color_range == AVCOL_RANGE_UNSPECIFIED) {
+                    /* allow implicitly promoting unspecified to mpeg */
+                    if ((ret = ff_add_format(&color_ranges, AVCOL_RANGE_MPEG)) < 0)
+                        return ret;
+                }
             }
             if ((ret = ff_set_common_color_ranges(ctx, color_ranges)) < 0)
                 return ret;
diff --git a/libavfilter/internal.h b/libavfilter/internal.h
index 000f94cb164..294e54d1410 100644
--- a/libavfilter/internal.h
+++ b/libavfilter/internal.h
@@ -199,6 +199,11 @@  int ff_fmt_is_in(int fmt, const int *fmts);
  */
 int ff_fmt_is_regular_yuv(enum AVPixelFormat fmt);
 
+/**
+ * Returns true if a YUV pixel format is forced full range. (i.e. YUVJ).
+ */
+int ff_fmt_is_forced_full_range(enum AVPixelFormat fmt);
+
 /* Functions to parse audio format arguments */
 
 /**