[FFmpeg-devel] [PATCH 0/5] work around broken (apple) ICCv4 profiles

[FFmpeg-devel] [PATCH 0/5] work around broken (apple) ICCv4 profiles

[Niklas Haas]

Concludes the discussion from https://trac.ffmpeg.org/ticket/9673

It was found that at least one widely-used ICCv4 profile found in the
wild contains broken tagging which violates the ICCv4 specification. It
seems that lcms2 gets confused by this wrong tag and returns wrong
values for this profile as a result. Work around it by detecting this
case and printing a warning + overriding the tag by the fixed result.

Since this code is *already* hidden behind -flags2 icc_profile, I
decided not to add a separate "fix broken profiles" flag.

_______________________________________________
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".

[FFmpeg-devel] [PATCH 1/5] avutil/rational: add av_abs_q

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

Seems like an obvious utility function to be missing, and useful in at
least several places in the code.
---
 doc/APIchanges       |  3 +++
 libavutil/rational.h | 12 ++++++++++++
 libavutil/version.h  |  2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index f333ff5b24f..2219e32ddc1 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09
 
 API changes, most recent first:
 
+2023-09-27 - xxxxxxxxxx - lavu 58.26.100 - rational.h
+  Add av_abs_q()
+
 2023-09-19 - xxxxxxxxxx - lavu 58.25.100 - avutil.h
   Make AV_TIME_BASE_Q compatible with C++.
 
diff --git a/libavutil/rational.h b/libavutil/rational.h
index 8cbfc8e0669..7b559ce515d 100644
--- a/libavutil/rational.h
+++ b/libavutil/rational.h
@@ -162,6 +162,18 @@ static av_always_inline AVRational av_inv_q(AVRational q)
     return r;
 }
 
+/**
+ * Take the absolute value of a rational.
+ * @param q value
+ * @return abs(q)
+ */
+static av_always_inline AVRational av_abs_q(AVRational q)
+{
+    if (q.num < 0)
+        q.num = -q.num;
+    return q;
+}
+
 /**
  * Convert a double precision floating point number to a rational.
  *
diff --git a/libavutil/version.h b/libavutil/version.h
index 00f229d2335..2dc1a647b8f 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  58
-#define LIBAVUTIL_VERSION_MINOR  25
+#define LIBAVUTIL_VERSION_MINOR  26
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
-- 
2.42.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/5] avutil/rational: add av_abs_q

[Anton Khirnov]

Quoting Niklas Haas (2023-09-27 12:03:52)
> +static av_always_inline AVRational av_abs_q(AVRational q)
> +{
> +    if (q.num < 0)
> +        q.num = -q.num;
> +    return q;

What if q.num > 0 and q.den < 0.

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/5] avutil/rational: add av_abs_q

[Andreas Rheinhardt]

Niklas Haas:
> From: Niklas Haas <git@haasn.dev>
> 
> Seems like an obvious utility function to be missing, and useful in at
> least several places in the code.
> ---
>  doc/APIchanges       |  3 +++
>  libavutil/rational.h | 12 ++++++++++++
>  libavutil/version.h  |  2 +-
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index f333ff5b24f..2219e32ddc1 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09
>  
>  API changes, most recent first:
>  
> +2023-09-27 - xxxxxxxxxx - lavu 58.26.100 - rational.h
> +  Add av_abs_q()
> +
>  2023-09-19 - xxxxxxxxxx - lavu 58.25.100 - avutil.h
>    Make AV_TIME_BASE_Q compatible with C++.
>  
> diff --git a/libavutil/rational.h b/libavutil/rational.h
> index 8cbfc8e0669..7b559ce515d 100644
> --- a/libavutil/rational.h
> +++ b/libavutil/rational.h
> @@ -162,6 +162,18 @@ static av_always_inline AVRational av_inv_q(AVRational q)
>      return r;
>  }
>  
> +/**
> + * Take the absolute value of a rational.
> + * @param q value
> + * @return abs(q)
> + */
> +static av_always_inline AVRational av_abs_q(AVRational q)
> +{
> +    if (q.num < 0)
> +        q.num = -q.num;
> +    return q;
> +}

I agree with Anton: We can't generally assume that the denominator is >
0 and so a generic and safe av_abs_q() will add unnecessary checks in
the cases where it is known that the denominator is positive. In other
words, I don't think this is a good addition to the API.

- Andreas

_______________________________________________
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".

[FFmpeg-devel] [PATCH 2/5] avutil/csp: re-use av_abs_q

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

---
 libavutil/csp.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/libavutil/csp.c b/libavutil/csp.c
index 7ef822c60bc..6522225a8d6 100644
--- a/libavutil/csp.c
+++ b/libavutil/csp.c
@@ -100,13 +100,6 @@ const AVColorPrimariesDesc *av_csp_primaries_desc_from_id(enum AVColorPrimaries
     return p;
 }
 
-static av_always_inline AVRational abs_sub_q(AVRational r1, AVRational r2)
-{
-    AVRational diff = av_sub_q(r1, r2);
-    /* denominator assumed to be positive */
-    return av_make_q(abs(diff.num), diff.den);
-}
-
 enum AVColorPrimaries av_csp_primaries_id_from_desc(const AVColorPrimariesDesc *prm)
 {
     AVRational delta;
@@ -116,14 +109,14 @@ enum AVColorPrimaries av_csp_primaries_id_from_desc(const AVColorPrimariesDesc *
         if (!ref->prim.r.x.num)
             continue;
 
-        delta = abs_sub_q(prm->prim.r.x, ref->prim.r.x);
-        delta = av_add_q(delta, abs_sub_q(prm->prim.r.y, ref->prim.r.y));
-        delta = av_add_q(delta, abs_sub_q(prm->prim.g.x, ref->prim.g.x));
-        delta = av_add_q(delta, abs_sub_q(prm->prim.g.y, ref->prim.g.y));
-        delta = av_add_q(delta, abs_sub_q(prm->prim.b.x, ref->prim.b.x));
-        delta = av_add_q(delta, abs_sub_q(prm->prim.b.y, ref->prim.b.y));
-        delta = av_add_q(delta, abs_sub_q(prm->wp.x, ref->wp.x));
-        delta = av_add_q(delta, abs_sub_q(prm->wp.y, ref->wp.y));
+        delta = av_abs_q(av_sub_q(prm->prim.r.x, ref->prim.r.x));
+        delta = av_add_q(delta, av_abs_q(av_sub_q(prm->prim.r.y, ref->prim.r.y)));
+        delta = av_add_q(delta, av_abs_q(av_sub_q(prm->prim.g.x, ref->prim.g.x)));
+        delta = av_add_q(delta, av_abs_q(av_sub_q(prm->prim.g.y, ref->prim.g.y)));
+        delta = av_add_q(delta, av_abs_q(av_sub_q(prm->prim.b.x, ref->prim.b.x)));
+        delta = av_add_q(delta, av_abs_q(av_sub_q(prm->prim.b.y, ref->prim.b.y)));
+        delta = av_add_q(delta, av_abs_q(av_sub_q(prm->wp.x, ref->wp.x)));
+        delta = av_add_q(delta, av_abs_q(av_sub_q(prm->wp.y, ref->wp.y)));
 
         if (av_cmp_q(delta, av_make_q(1, 1000)) < 0)
             return p;
-- 
2.42.0

_______________________________________________
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".

[FFmpeg-devel] [PATCH 3/5] avcodec/fflcms2: add ff_icc_profile_sanitize

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

Buggy ICCv4 profiles are unfortunately used in the wild, and it's quite
easy to work around them by just forcing the white point to the correct
value. Display a warning just in case.

See-Also: https://trac.ffmpeg.org/ticket/9673
---
 libavcodec/fflcms2.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
 libavcodec/fflcms2.h |  7 +++++++
 2 files changed, 52 insertions(+)

diff --git a/libavcodec/fflcms2.c b/libavcodec/fflcms2.c
index 5443f178bc9..41732c9c862 100644
--- a/libavcodec/fflcms2.c
+++ b/libavcodec/fflcms2.c
@@ -201,6 +201,51 @@ static av_always_inline void XYZ_xy(cmsCIEXYZ XYZ, AVCIExy *xy)
     xy->y = av_d2q(k * XYZ.Y, 100000);
 }
 
+static const AVCIExy wp_d50 = { {3457, 10000}, {3585, 10000} }; /* CIE D50 */
+
+int ff_icc_profile_sanitize(FFIccContext *s, cmsHPROFILE profile)
+{
+    cmsCIEXYZ *white, fixed;
+    AVCIExy wpxy;
+    AVRational diff, z;
+    if (!profile)
+        return 0;
+
+    if (cmsGetEncodedICCversion(profile) >= 0x4000000) { // ICC v4
+        switch (cmsGetHeaderRenderingIntent(profile)) {
+        case INTENT_RELATIVE_COLORIMETRIC:
+        case INTENT_ABSOLUTE_COLORIMETRIC: ;
+            /* ICC v4 colorimetric profiles are specified to always use D50
+             * media white point, anything else is a violation of the spec.
+             * Sadly, such profiles are incredibly common (Apple...), so make
+             * an effort to fix them. */
+            if (!(white = cmsReadTag(profile, cmsSigMediaWhitePointTag)))
+                return AVERROR_INVALIDDATA;
+            XYZ_xy(*white, &wpxy);
+            diff = av_add_q(av_abs_q(av_sub_q(wpxy.x, wp_d50.x)),
+                            av_abs_q(av_sub_q(wpxy.y, wp_d50.y)));
+            if (av_cmp_q(diff, av_make_q(1, 1000)) > 0) {
+                av_log(s->avctx, AV_LOG_WARNING, "Invalid colorimetric ICCv4 "
+                       "profile media white point tag (expected %.4f %.4f, "
+                       "got %.4f %.4f)\n",
+                       av_q2d(wp_d50.x), av_q2d(wp_d50.y),
+                       av_q2d(wpxy.x), av_q2d(wpxy.y));
+                /* x+y+z = 1 */
+                z = av_sub_q(av_sub_q(av_make_q(1, 1), wp_d50.x), wp_d50.y);
+                fixed.X = av_q2d(av_div_q(wp_d50.x, wp_d50.y)) * white->Y;
+                fixed.Y = white->Y;
+                fixed.Z = av_q2d(av_div_q(z, wp_d50.y)) * white->Y;
+                if (!cmsWriteTag(profile, cmsSigMediaWhitePointTag, &fixed))
+                    return AVERROR_EXTERNAL;
+            }
+            break;
+        default: break;
+        }
+    }
+
+    return 0;
+}
+
 int ff_icc_profile_read_primaries(FFIccContext *s, cmsHPROFILE profile,
                                   AVColorPrimariesDesc *out_primaries)
 {
diff --git a/libavcodec/fflcms2.h b/libavcodec/fflcms2.h
index af63c9a13c8..b54173e50e2 100644
--- a/libavcodec/fflcms2.h
+++ b/libavcodec/fflcms2.h
@@ -65,6 +65,13 @@ int ff_icc_profile_generate(FFIccContext *s,
  */
 int ff_icc_profile_attach(FFIccContext *s, cmsHPROFILE profile, AVFrame *frame);
 
+/**
+ * Sanitize an ICC profile to try and fix badly broken values.
+ *
+ * Returns 0 on success, or a negative error code.
+ */
+int ff_icc_profile_sanitize(FFIccContext *s, cmsHPROFILE profile);
+
 /**
  * Read the color primaries and white point coefficients encoded by an ICC
  * profile, and return the raw values in `out_primaries`.
-- 
2.42.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/fflcms2: add ff_icc_profile_sanitize

[Andreas Rheinhardt]

Niklas Haas:
> From: Niklas Haas <git@haasn.dev>
> 
> Buggy ICCv4 profiles are unfortunately used in the wild, and it's quite
> easy to work around them by just forcing the white point to the correct
> value. Display a warning just in case.
> 
> See-Also: https://trac.ffmpeg.org/ticket/9673
> ---
>  libavcodec/fflcms2.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/fflcms2.h |  7 +++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/libavcodec/fflcms2.c b/libavcodec/fflcms2.c
> index 5443f178bc9..41732c9c862 100644
> --- a/libavcodec/fflcms2.c
> +++ b/libavcodec/fflcms2.c
> @@ -201,6 +201,51 @@ static av_always_inline void XYZ_xy(cmsCIEXYZ XYZ, AVCIExy *xy)
>      xy->y = av_d2q(k * XYZ.Y, 100000);
>  }
>  
> +static const AVCIExy wp_d50 = { {3457, 10000}, {3585, 10000} }; /* CIE D50 */
> +
> +int ff_icc_profile_sanitize(FFIccContext *s, cmsHPROFILE profile)
> +{
> +    cmsCIEXYZ *white, fixed;
> +    AVCIExy wpxy;
> +    AVRational diff, z;
> +    if (!profile)
> +        return 0;
> +
> +    if (cmsGetEncodedICCversion(profile) >= 0x4000000) { // ICC v4
> +        switch (cmsGetHeaderRenderingIntent(profile)) {
> +        case INTENT_RELATIVE_COLORIMETRIC:
> +        case INTENT_ABSOLUTE_COLORIMETRIC: ;
> +            /* ICC v4 colorimetric profiles are specified to always use D50
> +             * media white point, anything else is a violation of the spec.
> +             * Sadly, such profiles are incredibly common (Apple...), so make
> +             * an effort to fix them. */
> +            if (!(white = cmsReadTag(profile, cmsSigMediaWhitePointTag)))
> +                return AVERROR_INVALIDDATA;
> +            XYZ_xy(*white, &wpxy);
> +            diff = av_add_q(av_abs_q(av_sub_q(wpxy.x, wp_d50.x)),
> +                            av_abs_q(av_sub_q(wpxy.y, wp_d50.y)));
> +            if (av_cmp_q(diff, av_make_q(1, 1000)) > 0) {
> +                av_log(s->avctx, AV_LOG_WARNING, "Invalid colorimetric ICCv4 "
> +                       "profile media white point tag (expected %.4f %.4f, "
> +                       "got %.4f %.4f)\n",
> +                       av_q2d(wp_d50.x), av_q2d(wp_d50.y),
> +                       av_q2d(wpxy.x), av_q2d(wpxy.y));

Will this warn on every frame?

> +                /* x+y+z = 1 */
> +                z = av_sub_q(av_sub_q(av_make_q(1, 1), wp_d50.x), wp_d50.y);
> +                fixed.X = av_q2d(av_div_q(wp_d50.x, wp_d50.y)) * white->Y;
> +                fixed.Y = white->Y;
> +                fixed.Z = av_q2d(av_div_q(z, wp_d50.y)) * white->Y;
> +                if (!cmsWriteTag(profile, cmsSigMediaWhitePointTag, &fixed))
> +                    return AVERROR_EXTERNAL;
> +            }
> +            break;
> +        default: break;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  int ff_icc_profile_read_primaries(FFIccContext *s, cmsHPROFILE profile,
>                                    AVColorPrimariesDesc *out_primaries)
>  {
> diff --git a/libavcodec/fflcms2.h b/libavcodec/fflcms2.h
> index af63c9a13c8..b54173e50e2 100644
> --- a/libavcodec/fflcms2.h
> +++ b/libavcodec/fflcms2.h
> @@ -65,6 +65,13 @@ int ff_icc_profile_generate(FFIccContext *s,
>   */
>  int ff_icc_profile_attach(FFIccContext *s, cmsHPROFILE profile, AVFrame *frame);
>  
> +/**
> + * Sanitize an ICC profile to try and fix badly broken values.
> + *
> + * Returns 0 on success, or a negative error code.
> + */
> +int ff_icc_profile_sanitize(FFIccContext *s, cmsHPROFILE profile);

Why is this a separate function and not just a new parameter to
ff_icc_profile_read_primaries() given that these seem to always be used
a pair?

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/fflcms2: add ff_icc_profile_sanitize

[Niklas Haas]

On Wed, 27 Sep 2023 12:20:37 +0200 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> Why is this a separate function and not just a new parameter to
> ff_icc_profile_read_primaries() given that these seem to always be used
> a pair?

Because it mutates the profile. It seems strange to me for a function
named `ff_icc_profile_*read*_primaries` to modify the profile it's
reading from. If it were possible to easily add this sanitization logic
without touching the profile itself I would have merged the two.

Forcing it to be a separate function forces each caller to think about
whether they want to actually modify the profile or not. At present we
only have two callers, and both of them destroy the cmsHPROFILE after
calling this function, so mutating is fine. But is that a given?
_______________________________________________
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".

[FFmpeg-devel] [PATCH 4/5] avfilter/vf_iccdetect: use ff_icc_profile_sanitize

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

---
 libavfilter/vf_iccdetect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavfilter/vf_iccdetect.c b/libavfilter/vf_iccdetect.c
index 5288b0320d2..16eacbbb560 100644
--- a/libavfilter/vf_iccdetect.c
+++ b/libavfilter/vf_iccdetect.c
@@ -93,7 +93,9 @@ static int iccdetect_filter_frame(AVFilterLink *inlink, AVFrame *frame)
     if (!profile)
         return AVERROR_INVALIDDATA;
 
-    ret = ff_icc_profile_read_primaries(&s->icc, profile, &coeffs);
+    ret = ff_icc_profile_sanitize(&s->icc, profile);
+    if (!ret)
+        ret = ff_icc_profile_read_primaries(&s->icc, profile, &coeffs);
     if (!ret)
         ret = ff_icc_profile_detect_transfer(&s->icc, profile, &s->profile_trc);
     cmsCloseProfile(profile);
-- 
2.42.0

_______________________________________________
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".

[FFmpeg-devel] [PATCH 5/5] avcodec/decode: use ff_icc_profile_sanitize

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

Fixes: https://trac.ffmpeg.org/ticket/9673
---
 libavcodec/decode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 466c393c1e2..6f4ef7da0c2 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -536,7 +536,9 @@ static int detect_colorspace(AVCodecContext *avctx, AVFrame *frame)
     if (!profile)
         return AVERROR_INVALIDDATA;
 
-    ret = ff_icc_profile_read_primaries(&avci->icc, profile, &coeffs);
+    ret = ff_icc_profile_sanitize(&avci->icc, profile);
+    if (!ret)
+        ret = ff_icc_profile_read_primaries(&avci->icc, profile, &coeffs);
     if (!ret)
         ret = ff_icc_profile_detect_transfer(&avci->icc, profile, &trc);
     cmsCloseProfile(profile);
-- 
2.42.0

_______________________________________________
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".