diff mbox series

[FFmpeg-devel,4/8] avfilter/vf_scale: properly respect to input colorimetry

Message ID 20231027170446.63684-4-ffmpeg@haasn.xyz
State New
Headers show
Series [FFmpeg-devel,1/8] swscale: fix sws_setColorspaceDetails after sws_init_context | expand

Checks

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

Commit Message

Niklas Haas Oct. 27, 2023, 5:04 p.m. UTC 
From: Niklas Haas <git@haasn.dev>

This code, as written, skips sws_init_context if scale->in_range was not
set, even if scale->in_frame_range is set to something. So this would
hit the 'no sws context' fast path in scale_frame and skip color range
conversion even where technically required. This had the effect of, in
practice, effectively applying limited/full range conversion if and only
if you set e.g. a nonzero yuv color matrix, which is obviously
undesirable behavior.

Second, the assignment of out->color_range inside the branch would fail
to properly propagate tags for any actually applied conversion, for
example on conversion to a forced full range format.

Solve both of these problems and make the code much easier to understand
and follow by doing the following:

1. Always initialize sws context on get_props
2. Always use sws_getColorspaceDetails + sws_setColorspaceDetails to
   fix the conversion matrices per-frame.
3. Rather than testing if the context exists, do the no-op test after
   settling the above values and deciding what conversion to actually
   perform.
---
 libavfilter/vf_scale.c | 186 +++++++++++++++++------------------------
 1 file changed, 76 insertions(+), 110 deletions(-)

Comments

Michael Niedermayer Oct. 27, 2023, 9:01 p.m. UTC | #1 
On Fri, Oct 27, 2023 at 07:04:42PM +0200, Niklas Haas wrote:
> From: Niklas Haas <git@haasn.dev>
> 
> This code, as written, skips sws_init_context if scale->in_range was not
> set, even if scale->in_frame_range is set to something. So this would
> hit the 'no sws context' fast path in scale_frame and skip color range
> conversion even where technically required. This had the effect of, in
> practice, effectively applying limited/full range conversion if and only
> if you set e.g. a nonzero yuv color matrix, which is obviously
> undesirable behavior.
> 
> Second, the assignment of out->color_range inside the branch would fail
> to properly propagate tags for any actually applied conversion, for
> example on conversion to a forced full range format.
> 
> Solve both of these problems and make the code much easier to understand
> and follow by doing the following:
> 
> 1. Always initialize sws context on get_props
> 2. Always use sws_getColorspaceDetails + sws_setColorspaceDetails to
>    fix the conversion matrices per-frame.
> 3. Rather than testing if the context exists, do the no-op test after
>    settling the above values and deciding what conversion to actually
>    perform.
> ---
>  libavfilter/vf_scale.c | 186 +++++++++++++++++------------------------
>  1 file changed, 76 insertions(+), 110 deletions(-)

This breaks
tickets/2939/lena.jpg

(color looks slightly wrong compared to reference lena image)



[...]
Niklas Haas Oct. 28, 2023, 10:50 a.m. UTC | #2 
On Fri, 27 Oct 2023 23:01:25 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Fri, Oct 27, 2023 at 07:04:42PM +0200, Niklas Haas wrote:
> > From: Niklas Haas <git@haasn.dev>
> > 
> > This code, as written, skips sws_init_context if scale->in_range was not
> > set, even if scale->in_frame_range is set to something. So this would
> > hit the 'no sws context' fast path in scale_frame and skip color range
> > conversion even where technically required. This had the effect of, in
> > practice, effectively applying limited/full range conversion if and only
> > if you set e.g. a nonzero yuv color matrix, which is obviously
> > undesirable behavior.
> > 
> > Second, the assignment of out->color_range inside the branch would fail
> > to properly propagate tags for any actually applied conversion, for
> > example on conversion to a forced full range format.
> > 
> > Solve both of these problems and make the code much easier to understand
> > and follow by doing the following:
> > 
> > 1. Always initialize sws context on get_props
> > 2. Always use sws_getColorspaceDetails + sws_setColorspaceDetails to
> >    fix the conversion matrices per-frame.
> > 3. Rather than testing if the context exists, do the no-op test after
> >    settling the above values and deciding what conversion to actually
> >    perform.
> > ---
> >  libavfilter/vf_scale.c | 186 +++++++++++++++++------------------------
> >  1 file changed, 76 insertions(+), 110 deletions(-)
> 
> This breaks
> tickets/2939/lena.jpg
> 
> (color looks slightly wrong compared to reference lena image)

How do I reproduce this? I tested with:

  ffmpeg -i lena.jpg lena.png

But the resulting checksum was identical before and after this commit.
Niklas Haas Oct. 28, 2023, 11:18 a.m. UTC | #3 
On Sat, 28 Oct 2023 12:50:14 +0200 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> On Fri, 27 Oct 2023 23:01:25 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Fri, Oct 27, 2023 at 07:04:42PM +0200, Niklas Haas wrote:
> > > From: Niklas Haas <git@haasn.dev>
> > > 
> > > This code, as written, skips sws_init_context if scale->in_range was not
> > > set, even if scale->in_frame_range is set to something. So this would
> > > hit the 'no sws context' fast path in scale_frame and skip color range
> > > conversion even where technically required. This had the effect of, in
> > > practice, effectively applying limited/full range conversion if and only
> > > if you set e.g. a nonzero yuv color matrix, which is obviously
> > > undesirable behavior.
> > > 
> > > Second, the assignment of out->color_range inside the branch would fail
> > > to properly propagate tags for any actually applied conversion, for
> > > example on conversion to a forced full range format.
> > > 
> > > Solve both of these problems and make the code much easier to understand
> > > and follow by doing the following:
> > > 
> > > 1. Always initialize sws context on get_props
> > > 2. Always use sws_getColorspaceDetails + sws_setColorspaceDetails to
> > >    fix the conversion matrices per-frame.
> > > 3. Rather than testing if the context exists, do the no-op test after
> > >    settling the above values and deciding what conversion to actually
> > >    perform.
> > > ---
> > >  libavfilter/vf_scale.c | 186 +++++++++++++++++------------------------
> > >  1 file changed, 76 insertions(+), 110 deletions(-)
> > 
> > This breaks
> > tickets/2939/lena.jpg
> > 
> > (color looks slightly wrong compared to reference lena image)
> 
> How do I reproduce this? I tested with:
> 
>   ffmpeg -i lena.jpg lena.png
> 
> But the resulting checksum was identical before and after this commit.

Nvm, I see the issue. Actually, the bug is in patch 1 - the logic is
insufficient to cover all cases, since ff_sws_init_range_convert is
never re-called after changing range.

I will rewrite that commit, which should fix this one also.
diff mbox series

Patch

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index 23335cef4b..19c91cb86e 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -143,7 +143,6 @@  typedef struct ScaleContext {
     char *out_color_matrix;
 
     int in_range;
-    int in_frame_range;
     int out_range;
 
     int out_h_chr_pos;
@@ -356,8 +355,6 @@  static av_cold int init(AVFilterContext *ctx)
     if (!threads)
         av_opt_set_int(scale->sws_opts, "threads", ff_filter_get_nb_threads(ctx), 0);
 
-    scale->in_frame_range = AVCOL_RANGE_UNSPECIFIED;
-
     return 0;
 }
 
@@ -520,6 +517,7 @@  static int config_props(AVFilterLink *outlink)
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
     const AVPixFmtDescriptor *outdesc = av_pix_fmt_desc_get(outfmt);
     ScaleContext *scale = ctx->priv;
+    struct SwsContext **swscs[3] = {&scale->sws, &scale->isws[0], &scale->isws[1]};
     uint8_t *flags_val = NULL;
     int ret;
 
@@ -552,65 +550,46 @@  static int config_props(AVFilterLink *outlink)
     if (scale->isws[1])
         sws_freeContext(scale->isws[1]);
     scale->isws[0] = scale->isws[1] = scale->sws = NULL;
-    if (inlink0->w == outlink->w &&
-        inlink0->h == outlink->h &&
-        !scale->out_color_matrix &&
-        scale->in_range == scale->out_range &&
-        inlink0->format == outlink->format)
-        ;
-    else {
-        struct SwsContext **swscs[3] = {&scale->sws, &scale->isws[0], &scale->isws[1]};
-        int i;
-
-        for (i = 0; i < 3; i++) {
-            int in_v_chr_pos = scale->in_v_chr_pos, out_v_chr_pos = scale->out_v_chr_pos;
-            struct SwsContext *const s = sws_alloc_context();
-            if (!s)
-                return AVERROR(ENOMEM);
-            *swscs[i] = s;
-
-            ret = av_opt_copy(s, scale->sws_opts);
-            if (ret < 0)
-                return ret;
 
-            av_opt_set_int(s, "srcw", inlink0 ->w, 0);
-            av_opt_set_int(s, "srch", inlink0 ->h >> !!i, 0);
-            av_opt_set_int(s, "src_format", inlink0->format, 0);
-            av_opt_set_int(s, "dstw", outlink->w, 0);
-            av_opt_set_int(s, "dsth", outlink->h >> !!i, 0);
-            av_opt_set_int(s, "dst_format", outfmt, 0);
-            if (scale->in_range != AVCOL_RANGE_UNSPECIFIED)
-                av_opt_set_int(s, "src_range",
-                               scale->in_range == AVCOL_RANGE_JPEG, 0);
-            else if (scale->in_frame_range != AVCOL_RANGE_UNSPECIFIED)
-                av_opt_set_int(s, "src_range",
-                               scale->in_frame_range == AVCOL_RANGE_JPEG, 0);
-            if (scale->out_range != AVCOL_RANGE_UNSPECIFIED)
-                av_opt_set_int(s, "dst_range",
-                               scale->out_range == AVCOL_RANGE_JPEG, 0);
-
-            /* Override chroma location default settings to have the correct
-             * chroma positions. MPEG chroma positions are used by convention.
-             * Note that this works for both MPEG-1/JPEG and MPEG-2/4 chroma
-             * locations, since they share a vertical alignment */
-            if (desc->log2_chroma_h == 1 && scale->in_v_chr_pos == -513) {
-                in_v_chr_pos = (i == 0) ? 128 : (i == 1) ? 64 : 192;
-            }
-
-            if (outdesc->log2_chroma_h == 1 && scale->out_v_chr_pos == -513) {
-                out_v_chr_pos = (i == 0) ? 128 : (i == 1) ? 64 : 192;
-            }
-
-            av_opt_set_int(s, "src_h_chr_pos", scale->in_h_chr_pos, 0);
-            av_opt_set_int(s, "src_v_chr_pos", in_v_chr_pos, 0);
-            av_opt_set_int(s, "dst_h_chr_pos", scale->out_h_chr_pos, 0);
-            av_opt_set_int(s, "dst_v_chr_pos", out_v_chr_pos, 0);
-
-            if ((ret = sws_init_context(s, NULL, NULL)) < 0)
-                return ret;
-            if (!scale->interlaced)
-                break;
+    for (int i = 0; i < 3; i++) {
+        int in_v_chr_pos = scale->in_v_chr_pos, out_v_chr_pos = scale->out_v_chr_pos;
+        struct SwsContext *const s = sws_alloc_context();
+        if (!s)
+            return AVERROR(ENOMEM);
+        *swscs[i] = s;
+
+        ret = av_opt_copy(s, scale->sws_opts);
+        if (ret < 0)
+            return ret;
+
+        av_opt_set_int(s, "srcw", inlink0 ->w, 0);
+        av_opt_set_int(s, "srch", inlink0 ->h >> !!i, 0);
+        av_opt_set_int(s, "src_format", inlink0->format, 0);
+        av_opt_set_int(s, "dstw", outlink->w, 0);
+        av_opt_set_int(s, "dsth", outlink->h >> !!i, 0);
+        av_opt_set_int(s, "dst_format", outfmt, 0);
+
+        /* Override chroma location default settings to have the correct
+         * chroma positions. MPEG chroma positions are used by convention.
+         * Note that this works for both MPEG-1/JPEG and MPEG-2/4 chroma
+         * locations, since they share a vertical alignment */
+        if (desc->log2_chroma_h == 1 && scale->in_v_chr_pos == -513) {
+            in_v_chr_pos = (i == 0) ? 128 : (i == 1) ? 64 : 192;
+        }
+
+        if (outdesc->log2_chroma_h == 1 && scale->out_v_chr_pos == -513) {
+            out_v_chr_pos = (i == 0) ? 128 : (i == 1) ? 64 : 192;
         }
+
+        av_opt_set_int(s, "src_h_chr_pos", scale->in_h_chr_pos, 0);
+        av_opt_set_int(s, "src_v_chr_pos", in_v_chr_pos, 0);
+        av_opt_set_int(s, "dst_h_chr_pos", scale->out_h_chr_pos, 0);
+        av_opt_set_int(s, "dst_v_chr_pos", out_v_chr_pos, 0);
+
+        if ((ret = sws_init_context(s, NULL, NULL)) < 0)
+            return ret;
+        if (!scale->interlaced)
+            break;
     }
 
     if (inlink0->sample_aspect_ratio.num){
@@ -716,9 +695,10 @@  static int scale_frame(AVFilterLink *link, AVFrame *in, AVFrame **frame_out)
     AVFrame *out;
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(link->format);
     char buf[32];
-    int ret;
-    int in_range;
+    int in_full, out_full, brightness, contrast, saturation;
+    const int *inv_table, *table;
     int frame_changed;
+    int ret;
 
     *frame_out = NULL;
     if (in->colorspace == AVCOL_SPC_YCGCO)
@@ -730,13 +710,6 @@  static int scale_frame(AVFilterLink *link, AVFrame *in, AVFrame **frame_out)
                     in->sample_aspect_ratio.den != link->sample_aspect_ratio.den ||
                     in->sample_aspect_ratio.num != link->sample_aspect_ratio.num;
 
-    if (in->color_range != AVCOL_RANGE_UNSPECIFIED &&
-        scale->in_range == AVCOL_RANGE_UNSPECIFIED &&
-        in->color_range != scale->in_frame_range) {
-        scale->in_frame_range = in->color_range;
-        frame_changed = 1;
-    }
-
     if (scale->eval_mode == EVAL_MODE_FRAME || frame_changed) {
         unsigned vars_w[VARS_NB] = { 0 }, vars_h[VARS_NB] = { 0 };
 
@@ -804,7 +777,30 @@  FF_ENABLE_DEPRECATION_WARNINGS
     }
 
 scale:
-    if (!scale->sws) {
+    sws_getColorspaceDetails(scale->sws, (int **)&inv_table, &in_full,
+                             (int **)&table, &out_full,
+                             &brightness, &contrast, &saturation);
+
+    if (scale->in_color_matrix)
+        inv_table = parse_yuv_type(scale->in_color_matrix, in->colorspace);
+    if (scale->out_color_matrix)
+        table     = parse_yuv_type(scale->out_color_matrix, AVCOL_SPC_UNSPECIFIED);
+    else if (scale->in_color_matrix)
+        table = inv_table;
+
+    if (scale->in_range != AVCOL_RANGE_UNSPECIFIED)
+        in_full = scale->in_range == AVCOL_RANGE_JPEG;
+    else if (in->color_range != AVCOL_RANGE_UNSPECIFIED)
+        in_full = in->color_range == AVCOL_RANGE_JPEG;
+    if (scale->out_range != AVCOL_RANGE_UNSPECIFIED)
+        out_full = scale->out_range == AVCOL_RANGE_JPEG;
+
+    if (in->width == outlink->w &&
+        in->height == outlink->h &&
+        in->format == outlink->format &&
+        inv_table == table &&
+        in_full == out_full) {
+        /* no conversion needed */
         *frame_out = in;
         return 0;
     }
@@ -822,6 +818,7 @@  scale:
     av_frame_copy_props(out, in);
     out->width  = outlink->w;
     out->height = outlink->h;
+    out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
 
     // Sanity checks:
     //   1. If the output is RGB, set the matrix coefficients to RGB.
@@ -838,48 +835,17 @@  scale:
     if (scale->output_is_pal)
         avpriv_set_systematic_pal2((uint32_t*)out->data[1], outlink->format == AV_PIX_FMT_PAL8 ? AV_PIX_FMT_BGR8 : outlink->format);
 
-    in_range = in->color_range;
-
-    if (   scale->in_color_matrix
-        || scale->out_color_matrix
-        || scale-> in_range != AVCOL_RANGE_UNSPECIFIED
-        || in_range != AVCOL_RANGE_UNSPECIFIED
-        || scale->out_range != AVCOL_RANGE_UNSPECIFIED) {
-        int in_full, out_full, brightness, contrast, saturation;
-        const int *inv_table, *table;
-
-        sws_getColorspaceDetails(scale->sws, (int **)&inv_table, &in_full,
-                                 (int **)&table, &out_full,
-                                 &brightness, &contrast, &saturation);
-
-        if (scale->in_color_matrix)
-            inv_table = parse_yuv_type(scale->in_color_matrix, in->colorspace);
-        if (scale->out_color_matrix)
-            table     = parse_yuv_type(scale->out_color_matrix, AVCOL_SPC_UNSPECIFIED);
-        else if (scale->in_color_matrix)
-            table = inv_table;
-
-        if (scale-> in_range != AVCOL_RANGE_UNSPECIFIED)
-            in_full  = (scale-> in_range == AVCOL_RANGE_JPEG);
-        else if (in_range != AVCOL_RANGE_UNSPECIFIED)
-            in_full  = (in_range == AVCOL_RANGE_JPEG);
-        if (scale->out_range != AVCOL_RANGE_UNSPECIFIED)
-            out_full = (scale->out_range == AVCOL_RANGE_JPEG);
-
-        sws_setColorspaceDetails(scale->sws, inv_table, in_full,
+    sws_setColorspaceDetails(scale->sws, inv_table, in_full,
+                             table, out_full,
+                             brightness, contrast, saturation);
+    if (scale->isws[0])
+        sws_setColorspaceDetails(scale->isws[0], inv_table, in_full,
+                                 table, out_full,
+                                 brightness, contrast, saturation);
+    if (scale->isws[1])
+        sws_setColorspaceDetails(scale->isws[1], inv_table, in_full,
                                  table, out_full,
                                  brightness, contrast, saturation);
-        if (scale->isws[0])
-            sws_setColorspaceDetails(scale->isws[0], inv_table, in_full,
-                                     table, out_full,
-                                     brightness, contrast, saturation);
-        if (scale->isws[1])
-            sws_setColorspaceDetails(scale->isws[1], inv_table, in_full,
-                                     table, out_full,
-                                     brightness, contrast, saturation);
-
-        out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
-    }
 
     av_reduce(&out->sample_aspect_ratio.num, &out->sample_aspect_ratio.den,
               (int64_t)in->sample_aspect_ratio.num * outlink->h * link->w,