From f9e6ecd298d5bbc0668aa498160c7687f935fc29 Mon Sep 17 00:00:00 2001 From: Xaver Hugl Date: Mon, 19 Aug 2024 02:50:05 +0200 Subject: [PATCH] core/colorpipeline: improve optimization with differing reference luminances This is done by - fixing isFuzzyScalingOnly to not check the [3, 3] component, which is always 1 - making the comparison between transfer functions fuzzy, so small floating point errors don't prevent two practically identical functions to be optimized out - switching manual optimizations to use addMatrix instead, which removes the matrix or replaces it with a multiplier and the autotest is expanded to test transformations between color descriptions with transfer functions and reference luminances that are just scaled versions of each other --- autotests/test_colorspaces.cpp | 6 ++++-- src/core/colorpipeline.cpp | 38 ++++++++++++++-------------------- src/core/colorpipeline.h | 7 +++++++ src/core/colorspace.cpp | 12 ++++++++++- src/core/colorspace.h | 2 +- 5 files changed, 39 insertions(+), 26 deletions(-) diff --git a/autotests/test_colorspaces.cpp b/autotests/test_colorspaces.cpp index c52dd8c88f..6e8d93731d 100644 --- a/autotests/test_colorspaces.cpp +++ b/autotests/test_colorspaces.cpp @@ -114,7 +114,9 @@ void TestColorspaces::testIdentityTransformation() QFETCH(NamedColorimetry, colorimetry); QFETCH(TransferFunction::Type, transferFunction); const TransferFunction tf(transferFunction); - const ColorDescription color(colorimetry, tf, 100, tf.minLuminance, tf.maxLuminance, tf.maxLuminance); + const ColorDescription src(colorimetry, tf, 100, tf.minLuminance, tf.maxLuminance, tf.maxLuminance); + const TransferFunction tf2(transferFunction, tf.minLuminance * 1.1, tf.maxLuminance * 1.1); + const ColorDescription dst(colorimetry, tf2, 110, tf2.minLuminance, tf2.maxLuminance, tf2.maxLuminance); constexpr std::array renderingIntents = { RenderingIntent::Perceptual, @@ -123,7 +125,7 @@ void TestColorspaces::testIdentityTransformation() RenderingIntent::RelativeColorimetricWithBPC, }; for (const RenderingIntent intent : renderingIntents) { - const auto pipeline = ColorPipeline::create(color, color, intent); + const auto pipeline = ColorPipeline::create(src, dst, intent); if (!pipeline.isIdentity()) { qWarning() << pipeline; } diff --git a/src/core/colorpipeline.cpp b/src/core/colorpipeline.cpp index af78412fbd..ff261e1cac 100644 --- a/src/core/colorpipeline.cpp +++ b/src/core/colorpipeline.cpp @@ -71,24 +71,26 @@ void ColorPipeline::addMultiplier(const QVector3D &factors) if (!ops.empty()) { auto *lastOp = &ops.back().operation; if (const auto mat = std::get_if(lastOp)) { - mat->mat.scale(factors); - ops.back().output = output; + auto newMat = mat->mat; + newMat.scale(factors); + ops.erase(ops.end() - 1); + addMatrix(newMat, output); return; } else if (const auto mult = std::get_if(lastOp)) { mult->factors *= factors; - if (mult->factors == QVector3D(1, 1, 1)) { + if ((mult->factors - QVector3D(1, 1, 1)).lengthSquared() < s_maxResolution * s_maxResolution) { ops.erase(ops.end() - 1); } else { ops.back().output = output; } return; - } else if (factors.x() == factors.y() && factors.y() == factors.z()) { - if (const auto tf = std::get_if(lastOp); tf && tf->tf.isRelative()) { + } else if (std::abs(factors.x() - factors.y()) < s_maxResolution && std::abs(factors.x() - factors.z()) < s_maxResolution) { + if (const auto tf = std::get_if(lastOp)) { tf->tf.minLuminance *= factors.x(); tf->tf.maxLuminance *= factors.x(); ops.back().output = output; return; - } else if (const auto tf = std::get_if(lastOp); tf && tf->tf.isRelative()) { + } else if (const auto tf = std::get_if(lastOp)) { tf->tf.minLuminance /= factors.x(); tf->tf.maxLuminance /= factors.x(); ops.back().output = output; @@ -165,13 +167,10 @@ void ColorPipeline::addInverseTransferFunction(TransferFunction tf) static bool isFuzzyIdentity(const QMatrix4x4 &mat) { - // matrix calculations with floating point numbers can result in very small errors - // -> ignore them, as that just causes inefficiencies and more rounding errors - constexpr float maxResolution = 0.00001; for (int i = 0; i < 4; i++) { for (int j = 0; j < 4; j++) { const float targetValue = i == j ? 1 : 0; - if (std::abs(mat(i, j) - targetValue) > maxResolution) { + if (std::abs(mat(i, j) - targetValue) > ColorPipeline::s_maxResolution) { return false; } } @@ -181,15 +180,12 @@ static bool isFuzzyIdentity(const QMatrix4x4 &mat) static bool isFuzzyScalingOnly(const QMatrix4x4 &mat) { - // matrix calculations with floating point numbers can result in very small errors - // -> ignore them, as that just causes inefficiencies and more rounding errors - constexpr float maxResolution = 0.00001; for (int i = 0; i < 4; i++) { for (int j = 0; j < 4; j++) { - if (i < 3 && i == j) { + if (i == j) { continue; } - if (std::abs(mat(i, j)) > maxResolution) { + if (std::abs(mat(i, j)) > ColorPipeline::s_maxResolution) { return false; } } @@ -205,17 +201,15 @@ void ColorPipeline::addMatrix(const QMatrix4x4 &mat, const ValueRange &output) if (!ops.empty()) { auto *lastOp = &ops.back().operation; if (const auto otherMat = std::get_if(lastOp)) { - otherMat->mat = mat * otherMat->mat; - ops.back().output = output; + const auto newMat = mat * otherMat->mat; + ops.erase(ops.end() - 1); + addMatrix(newMat, output); return; } else if (const auto mult = std::get_if(lastOp)) { QMatrix4x4 scaled = mat; scaled.scale(mult->factors); - ops.back() = ColorOp{ - .input = currentOutputRange(), - .operation = ColorMatrix(scaled), - .output = output, - }; + ops.erase(ops.end() - 1); + addMatrix(scaled, output); return; } } diff --git a/src/core/colorpipeline.h b/src/core/colorpipeline.h index bb55321912..c3ffb2d3fd 100644 --- a/src/core/colorpipeline.h +++ b/src/core/colorpipeline.h @@ -77,6 +77,13 @@ public: class KWIN_EXPORT ColorPipeline { public: + /** + * matrix calculations with floating point numbers can result in very small errors + * this value is the minimum difference we actually care about; everything below + * can and should be optimized out + */ + static constexpr float s_maxResolution = 0.00001; + explicit ColorPipeline(); explicit ColorPipeline(const ValueRange &inputRange); diff --git a/src/core/colorspace.cpp b/src/core/colorspace.cpp index d9f7da1387..886eb24302 100644 --- a/src/core/colorspace.cpp +++ b/src/core/colorspace.cpp @@ -4,8 +4,9 @@ SPDX-License-Identifier: GPL-2.0-or-later */ #include "colorspace.h" +#include "colorpipeline.h" -#include +#include namespace KWin { @@ -434,6 +435,15 @@ double TransferFunction::defaultReferenceLuminanceFor(Type type) Q_UNREACHABLE(); } +bool TransferFunction::operator==(const TransferFunction &other) const +{ + // allow for a greater error with large max. luminance, as floating point errors get larger there + // and the effect of errors is smaller too + return type == other.type + && std::abs(other.minLuminance - minLuminance) < ColorPipeline::s_maxResolution + && std::abs(other.maxLuminance - maxLuminance) < ColorPipeline::s_maxResolution * maxLuminance; +} + TransferFunction::TransferFunction(Type tf) : TransferFunction(tf, defaultMinLuminanceFor(tf), defaultMaxLuminanceFor(tf)) { diff --git a/src/core/colorspace.h b/src/core/colorspace.h index efc32e0bd0..e02de1fb7a 100644 --- a/src/core/colorspace.h +++ b/src/core/colorspace.h @@ -120,7 +120,7 @@ public: explicit TransferFunction(Type tf); explicit TransferFunction(Type tf, double minLuminance, double maxLuminance); - auto operator<=>(const TransferFunction &) const = default; + bool operator==(const TransferFunction &) const; bool isRelative() const; TransferFunction relativeScaledTo(double referenceLuminance) const;