From dadac55c7f2f6b3783f7d5f03e519f0794014b36 Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Tue, 12 Mar 2019 10:01:28 -0500 Subject: [PATCH] simplify matrix color filter clamps We must keep alpha sanely [0,1]. This makes the existing clamps unconditional, which seems the most compatible with our clients' expectations, and which makes the CPU backend agree with the GPU backend. I'm not sure I've got a more principled reason to clamp than those. Clamping means we can't really concatenate these into a new matrix filter ever, deleting a bunch of code supporting that and a few unit tests that thought we could. (Good candidate for a user-defined effect with user-defined clamping.) Cq-Include-Trybots: luci.chromium.try:linux-blink-rel Change-Id: Ib92a4c882320e65ae705833bf623e4e961bc6651 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/200394 Reviewed-by: Brian Osman Commit-Queue: Mike Klein --- src/core/SkColorMatrixFilterRowMajor255.cpp | 134 +++----------------- src/core/SkColorMatrixFilterRowMajor255.h | 1 - tests/ImageFilterTest.cpp | 16 +-- 3 files changed, 22 insertions(+), 129 deletions(-) diff --git a/src/core/SkColorMatrixFilterRowMajor255.cpp b/src/core/SkColorMatrixFilterRowMajor255.cpp index f013f1e0d5..8bf185fd2d 100644 --- a/src/core/SkColorMatrixFilterRowMajor255.cpp +++ b/src/core/SkColorMatrixFilterRowMajor255.cpp @@ -15,43 +15,28 @@ #include "SkUnPreMultiply.h" #include "SkWriteBuffer.h" -static void transpose_and_scale01(float dst[20], const float src[20]) { - const float* srcR = src + 0; - const float* srcG = src + 5; - const float* srcB = src + 10; - const float* srcA = src + 15; +void SkColorMatrixFilterRowMajor255::initState() { + const float* srcR = fMatrix + 0; + const float* srcG = fMatrix + 5; + const float* srcB = fMatrix + 10; + const float* srcA = fMatrix + 15; + + fFlags = (srcA[0] == 0 && srcA[1] == 0 && srcA[2] == 0 && srcA[3] == 1 && srcA[4] == 0) + ? kAlphaUnchanged_Flag : 0; for (int i = 0; i < 16; i += 4) { - dst[i + 0] = *srcR++; - dst[i + 1] = *srcG++; - dst[i + 2] = *srcB++; - dst[i + 3] = *srcA++; + fTranspose[i + 0] = *srcR++; + fTranspose[i + 1] = *srcG++; + fTranspose[i + 2] = *srcB++; + fTranspose[i + 3] = *srcA++; } // Might as well scale these translates down to [0,1] here instead of every filter call. - dst[16] = *srcR * (1/255.0f); - dst[17] = *srcG * (1/255.0f); - dst[18] = *srcB * (1/255.0f); - dst[19] = *srcA * (1/255.0f); + fTranspose[16] = *srcR * (1/255.0f); + fTranspose[17] = *srcG * (1/255.0f); + fTranspose[18] = *srcB * (1/255.0f); + fTranspose[19] = *srcA * (1/255.0f); } -void SkColorMatrixFilterRowMajor255::initState() { - transpose_and_scale01(fTranspose, fMatrix); - - const float* array = fMatrix; - - // check if we have to munge Alpha - bool changesAlpha = (array[15] || array[16] || array[17] || (array[18] - 1) || array[19]); - bool usesAlpha = (array[3] || array[8] || array[13]); - - if (changesAlpha || usesAlpha) { - fFlags = changesAlpha ? 0 : kAlphaUnchanged_Flag; - } else { - fFlags = kAlphaUnchanged_Flag; - } -} - -/////////////////////////////////////////////////////////////////////////////// - SkColorMatrixFilterRowMajor255::SkColorMatrixFilterRowMajor255(const SkScalar array[20]) { memcpy(fMatrix, array, 20 * sizeof(SkScalar)); this->initState(); @@ -61,8 +46,6 @@ uint32_t SkColorMatrixFilterRowMajor255::getFlags() const { return this->INHERITED::getFlags() | fFlags; } -/////////////////////////////////////////////////////////////////////////////// - void SkColorMatrixFilterRowMajor255::flatten(SkWriteBuffer& buffer) const { SkASSERT(sizeof(fMatrix)/sizeof(SkScalar) == 20); buffer.writeScalarArray(fMatrix, 20); @@ -83,96 +66,19 @@ bool SkColorMatrixFilterRowMajor255::asColorMatrix(SkScalar matrix[20]) const { return true; } -/////////////////////////////////////////////////////////////////////////////// -// This code was duplicated from src/effects/SkColorMatrix.cpp in order to be used in core. -////// - -// To detect if we need to apply clamping after applying a matrix, we check if -// any output component might go outside of [0, 255] for any combination of -// input components in [0..255]. -// Each output component is an affine transformation of the input component, so -// the minimum and maximum values are for any combination of minimum or maximum -// values of input components (i.e. 0 or 255). -// E.g. if R' = x*R + y*G + z*B + w*A + t -// Then the maximum value will be for R=255 if x>0 or R=0 if x<0, and the -// minimum value will be for R=0 if x>0 or R=255 if x<0. -// Same goes for all components. -static bool component_needs_clamping(const SkScalar row[5]) { - SkScalar maxValue = row[4] / 255; - SkScalar minValue = row[4] / 255; - for (int i = 0; i < 4; ++i) { - if (row[i] > 0) - maxValue += row[i]; - else - minValue += row[i]; - } - return (maxValue > 1) || (minValue < 0); -} - -static bool needs_clamping(const SkScalar matrix[20]) { - return component_needs_clamping(matrix) - || component_needs_clamping(matrix+5) - || component_needs_clamping(matrix+10) - || component_needs_clamping(matrix+15); -} - -static void set_concat(SkScalar result[20], const SkScalar outer[20], const SkScalar inner[20]) { - int index = 0; - for (int j = 0; j < 20; j += 5) { - for (int i = 0; i < 4; i++) { - result[index++] = outer[j + 0] * inner[i + 0] + - outer[j + 1] * inner[i + 5] + - outer[j + 2] * inner[i + 10] + - outer[j + 3] * inner[i + 15]; - } - result[index++] = outer[j + 0] * inner[4] + - outer[j + 1] * inner[9] + - outer[j + 2] * inner[14] + - outer[j + 3] * inner[19] + - outer[j + 4]; - } -} - -/////////////////////////////////////////////////////////////////////////////// -// End duplication -////// - void SkColorMatrixFilterRowMajor255::onAppendStages(SkRasterPipeline* p, SkColorSpace* dst, SkArenaAlloc* scratch, - bool shaderIsOpaque) const { - bool willStayOpaque = shaderIsOpaque && (fFlags & kAlphaUnchanged_Flag); - bool needsClamp0 = false, - needsClamp1 = false; - for (int i = 0; i < 4; i++) { - SkScalar min = fTranspose[i+16], - max = fTranspose[i+16]; - (fTranspose[i+ 0] < 0 ? min : max) += fTranspose[i+ 0]; - (fTranspose[i+ 4] < 0 ? min : max) += fTranspose[i+ 4]; - (fTranspose[i+ 8] < 0 ? min : max) += fTranspose[i+ 8]; - (fTranspose[i+12] < 0 ? min : max) += fTranspose[i+12]; - needsClamp0 = needsClamp0 || min < 0; - needsClamp1 = needsClamp1 || max > 1; - } + const bool shaderIsOpaque) const { + const bool willStayOpaque = shaderIsOpaque && (fFlags & kAlphaUnchanged_Flag); if (!shaderIsOpaque) { p->append(SkRasterPipeline::unpremul); } if ( true) { p->append(SkRasterPipeline::matrix_4x5, fTranspose); } - if ( needsClamp0) { p->append(SkRasterPipeline::clamp_0); } - if ( needsClamp1) { p->append(SkRasterPipeline::clamp_1); } + if ( true) { p->append(SkRasterPipeline::clamp_0); } + if ( true) { p->append(SkRasterPipeline::clamp_1); } if (!willStayOpaque) { p->append(SkRasterPipeline::premul); } } -sk_sp -SkColorMatrixFilterRowMajor255::onMakeComposed(sk_sp innerFilter) const { - SkScalar innerMatrix[20]; - if (innerFilter->asColorMatrix(innerMatrix) && !needs_clamping(innerMatrix)) { - SkScalar concat[20]; - set_concat(concat, fMatrix, innerMatrix); - return sk_make_sp(concat); - } - return nullptr; -} - #if SK_SUPPORT_GPU #include "GrFragmentProcessor.h" #include "glsl/GrGLSLFragmentProcessor.h" diff --git a/src/core/SkColorMatrixFilterRowMajor255.h b/src/core/SkColorMatrixFilterRowMajor255.h index e4adf972ea..3c7a4df30c 100644 --- a/src/core/SkColorMatrixFilterRowMajor255.h +++ b/src/core/SkColorMatrixFilterRowMajor255.h @@ -21,7 +21,6 @@ public: uint32_t getFlags() const override; bool asColorMatrix(SkScalar matrix[20]) const override; - sk_sp onMakeComposed(sk_sp) const override; #if SK_SUPPORT_GPU std::unique_ptr asFragmentProcessor( diff --git a/tests/ImageFilterTest.cpp b/tests/ImageFilterTest.cpp index 3d5306ac73..26be9eef19 100644 --- a/tests/ImageFilterTest.cpp +++ b/tests/ImageFilterTest.cpp @@ -414,25 +414,13 @@ static sk_sp create_empty_special_image(GrContext* context, int DEF_TEST(ImageFilter, reporter) { { - // Check that two non-clipping color-matrice-filters concatenate into a single filter. - sk_sp halfBrightness(make_scale(0.5f, nullptr)); - sk_sp quarterBrightness(make_scale(0.5f, std::move(halfBrightness))); - REPORTER_ASSERT(reporter, nullptr == quarterBrightness->getInput(0)); - SkColorFilter* cf; - REPORTER_ASSERT(reporter, quarterBrightness->asColorFilter(&cf)); - REPORTER_ASSERT(reporter, cf->asColorMatrix(nullptr)); - cf->unref(); - } - - { - // Check that a clipping color-matrice-filter followed by a color-matrice-filters - // concatenates into a single filter, but not a matrixfilter (due to clamping). + // Check that a color matrix filter followed by a color matrix filter + // concatenates into a single filter. sk_sp doubleBrightness(make_scale(2.0f, nullptr)); sk_sp halfBrightness(make_scale(0.5f, std::move(doubleBrightness))); REPORTER_ASSERT(reporter, nullptr == halfBrightness->getInput(0)); SkColorFilter* cf; REPORTER_ASSERT(reporter, halfBrightness->asColorFilter(&cf)); - REPORTER_ASSERT(reporter, !cf->asColorMatrix(nullptr)); cf->unref(); }