| Message ID | 20230927100630.50510-4-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | work around broken (apple) ICCv4 profiles | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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?
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`.
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(+)