From eaf7e9a3a24d6a9f5405eb750c3af36d07772aa0 Mon Sep 17 00:00:00 2001 From: Xaver Hugl Date: Fri, 23 Aug 2024 02:14:31 +0200 Subject: [PATCH] core/colorspace: be more robust about edge cases xyz -> xy and xy -> xyz conversions have divisions in them, so we need to handle the edge cases of the divisor being zero. This can happen if an ICC profile is invalid, or if the XYZ color space is used. --- autotests/test_colorspaces.cpp | 18 ++++++++---------- src/core/colorspace.cpp | 25 +++++++++---------------- src/core/colorspace.h | 1 - src/core/iccprofile.cpp | 32 ++++++++++++++++++-------------- 4 files changed, 35 insertions(+), 41 deletions(-) diff --git a/autotests/test_colorspaces.cpp b/autotests/test_colorspaces.cpp index 6e8d93731d..d11d155fbd 100644 --- a/autotests/test_colorspaces.cpp +++ b/autotests/test_colorspaces.cpp @@ -21,7 +21,7 @@ public: private Q_SLOTS: void roundtripConversion_data(); void roundtripConversion(); - void nonNormalizedPrimaries(); + void testXYZ_XYconversions(); void testIdentityTransformation_data(); void testIdentityTransformation(); void testColorPipeline_data(); @@ -82,16 +82,14 @@ void TestColorspaces::roundtripConversion() } } -void TestColorspaces::nonNormalizedPrimaries() +void TestColorspaces::testXYZ_XYconversions() { - // this test ensures that non-normalized primaries don't mess up the transformations between color spaces - const auto &from = ColorDescription::sRGB; - const ColorDescription to(Colorimetry(Colorimetry::xyToXYZ(from.containerColorimetry().red()) * 2, Colorimetry::xyToXYZ(from.containerColorimetry().green()) * 2, Colorimetry::xyToXYZ(from.containerColorimetry().blue()) * 2, Colorimetry::xyToXYZ(from.containerColorimetry().white()) * 2), from.transferFunction(), from.referenceLuminance(), from.minLuminance(), from.maxAverageLuminance(), from.maxHdrLuminance()); - - const auto convertedWhite = from.toOther(to, RenderingIntent::RelativeColorimetric) * QVector3D(1, 1, 1); - QCOMPARE_LE(std::abs(1 - convertedWhite.x()), s_resolution10bit); - QCOMPARE_LE(std::abs(1 - convertedWhite.y()), s_resolution10bit); - QCOMPARE_LE(std::abs(1 - convertedWhite.z()), s_resolution10bit); + // this test ensures that Colorimetry::xyzToXY and Colorimetry::xyToXYZ can handle weird inputs + // and don't cause crashes + QCOMPARE(Colorimetry::xyzToXY(QVector3D(0, 0, 0)), QVector2D(0, 0)); + QCOMPARE_LE(Colorimetry::xyzToXY(QVector3D(100, 100, 100)).y(), 1); + QCOMPARE(Colorimetry::xyToXYZ(QVector2D(0, 0)), QVector3D(0, 1, 0)); + QCOMPARE(Colorimetry::xyToXYZ(QVector2D(1, 0)), QVector3D(1, 1, 0)); } void TestColorspaces::testIdentityTransformation_data() diff --git a/src/core/colorspace.cpp b/src/core/colorspace.cpp index e78ce890eb..a41991db55 100644 --- a/src/core/colorspace.cpp +++ b/src/core/colorspace.cpp @@ -28,12 +28,20 @@ static QMatrix4x4 matrixFromColumns(const QVector3D &first, const QVector3D &sec QVector3D Colorimetry::xyToXYZ(QVector2D xy) { + if (xy.y() == 0) { + // special case for XYZ Colorimetry + // where xy.y == 0 is valid + return QVector3D(xy.x(), 1, 0); + } return QVector3D(xy.x() / xy.y(), 1, (1 - xy.x() - xy.y()) / xy.y()); } QVector2D Colorimetry::xyzToXY(QVector3D xyz) { - xyz /= xyz.y(); + if (xyz.y() == 0) { + // this is nonsense, but at least doesn't crash + return QVector2D(0, 0); + } return QVector2D(xyz.x() / (xyz.x() + xyz.y() + xyz.z()), xyz.y() / (xyz.x() + xyz.y() + xyz.z())); } @@ -76,11 +84,6 @@ QMatrix4x4 Colorimetry::chromaticAdaptationMatrix(QVector2D sourceWhitepoint, QV return inverseBradford * adaptation * bradford; } -static QVector3D normalizeToY1(const QVector3D &vect) -{ - return vect.y() == 0 ? vect : QVector3D(vect.x() / vect.y(), 1, vect.z() / vect.y()); -} - QMatrix4x4 Colorimetry::calculateToXYZMatrix(QVector3D red, QVector3D green, QVector3D blue, QVector3D white) { const auto component_scale = (matrixFromColumns(red, green, blue)).inverted() * white; @@ -107,16 +110,6 @@ Colorimetry::Colorimetry(QVector2D red, QVector2D green, QVector2D blue, QVector { } -Colorimetry::Colorimetry(QVector3D red, QVector3D green, QVector3D blue, QVector3D white) - : m_red(xyzToXY(red)) - , m_green(xyzToXY(green)) - , m_blue(xyzToXY(blue)) - , m_white(xyzToXY(white)) - , m_toXYZ(calculateToXYZMatrix(normalizeToY1(red), normalizeToY1(green), normalizeToY1(blue), normalizeToY1(white))) - , m_fromXYZ(m_toXYZ.inverted()) -{ -} - const QMatrix4x4 &Colorimetry::toXYZ() const { return m_toXYZ; diff --git a/src/core/colorspace.h b/src/core/colorspace.h index ba0e24a49c..2be28c135c 100644 --- a/src/core/colorspace.h +++ b/src/core/colorspace.h @@ -68,7 +68,6 @@ public: static QMatrix4x4 calculateToXYZMatrix(QVector3D red, QVector3D green, QVector3D blue, QVector3D white); explicit Colorimetry(QVector2D red, QVector2D green, QVector2D blue, QVector2D white); - explicit Colorimetry(QVector3D red, QVector3D green, QVector3D blue, QVector3D white); /** * @returns a matrix that transforms from the linear RGB representation of colors in this colorimetry to the XYZ representation diff --git a/src/core/iccprofile.cpp b/src/core/iccprofile.cpp index 34b5d5f900..ea8676e1da 100644 --- a/src/core/iccprofile.cpp +++ b/src/core/iccprofile.cpp @@ -263,11 +263,15 @@ std::unique_ptr IccProfile::load(const QString &path) qCWarning(KWIN_CORE, "profile is missing the wtpt tag"); return nullptr; } + if (whitepoint->Y == 0) { + qCWarning(KWIN_CORE, "profile has a zero luminance whitepoint"); + return nullptr; + } - QVector3D red; - QVector3D green; - QVector3D blue; - QVector3D white(whitepoint->X, whitepoint->Y, whitepoint->Z); + QVector2D red; + QVector2D green; + QVector2D blue; + QVector2D white = Colorimetry::xyzToXY(QVector3D(whitepoint->X, whitepoint->Y, whitepoint->Z)); std::optional chromaticAdaptationMatrix; if (cmsIsTag(handle, cmsSigChromaticAdaptationTag)) { // the chromatic adaptation tag is a 3x3 matrix that converts from the actual whitepoint to D50 @@ -284,12 +288,12 @@ std::unique_ptr IccProfile::load(const QString &path) return nullptr; } const QVector3D D50(0.9642, 1.0, 0.8249); - white = *chromaticAdaptationMatrix * D50; + white = Colorimetry::xyzToXY(*chromaticAdaptationMatrix * D50); } if (cmsCIExyYTRIPLE *chrmTag = static_cast(cmsReadTag(handle, cmsSigChromaticityTag))) { - red = Colorimetry::xyToXYZ(QVector2D(chrmTag->Red.x, chrmTag->Red.y)) * chrmTag->Red.Y; - green = Colorimetry::xyToXYZ(QVector2D(chrmTag->Green.x, chrmTag->Green.y)) * chrmTag->Green.Y; - blue = Colorimetry::xyToXYZ(QVector2D(chrmTag->Blue.x, chrmTag->Blue.y)) * chrmTag->Blue.Y; + red = QVector2D(chrmTag->Red.x, chrmTag->Red.y); + green = QVector2D(chrmTag->Green.x, chrmTag->Green.y); + blue = QVector2D(chrmTag->Blue.x, chrmTag->Blue.y); } else { const cmsCIEXYZ *r = static_cast(cmsReadTag(handle, cmsSigRedColorantTag)); const cmsCIEXYZ *g = static_cast(cmsReadTag(handle, cmsSigGreenColorantTag)); @@ -299,9 +303,9 @@ std::unique_ptr IccProfile::load(const QString &path) return nullptr; } if (chromaticAdaptationMatrix) { - red = *chromaticAdaptationMatrix * QVector3D(r->X, r->Y, r->Z); - green = *chromaticAdaptationMatrix * QVector3D(g->X, g->Y, g->Z); - blue = *chromaticAdaptationMatrix * QVector3D(b->X, b->Y, b->Z); + red = Colorimetry::xyzToXY(*chromaticAdaptationMatrix * QVector3D(r->X, r->Y, r->Z)); + green = Colorimetry::xyzToXY(*chromaticAdaptationMatrix * QVector3D(g->X, g->Y, g->Z)); + blue = Colorimetry::xyzToXY(*chromaticAdaptationMatrix * QVector3D(b->X, b->Y, b->Z)); } else { // if the chromatic adaptation tag isn't available, fall back to using the media whitepoint instead cmsCIEXYZ adaptedR{}; @@ -313,9 +317,9 @@ std::unique_ptr IccProfile::load(const QString &path) if (!success) { return nullptr; } - red = QVector3D(adaptedR.X, adaptedR.Y, adaptedR.Z); - green = QVector3D(adaptedG.X, adaptedG.Y, adaptedG.Z); - blue = QVector3D(adaptedB.X, adaptedB.Y, adaptedB.Z); + red = Colorimetry::xyzToXY(QVector3D(adaptedR.X, adaptedR.Y, adaptedR.Z)); + green = Colorimetry::xyzToXY(QVector3D(adaptedG.X, adaptedG.Y, adaptedG.Z)); + blue = Colorimetry::xyzToXY(QVector3D(adaptedB.X, adaptedB.Y, adaptedB.Z)); } }