From 9a3ee1913acb1652f10e1063a8bf55a9a9143405 Mon Sep 17 00:00:00 2001 From: Brian Osman Date: Fri, 27 Mar 2020 10:06:54 -0400 Subject: [PATCH] Remove arbitrary limit on number of composed color filters This effectively reverts https://codereview.chromium.org/972153010/ Note that we have no such hard limit on SkShaders, and it's never been a problem. Complex SkPaint objects can generate too many FPs for us to handle, but those are rare, and better managed by just reporting the shader compile error (which we do). Change-Id: Iee5dc3d65ec130f2ce0a29e55fbe3c25b00dc828 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/279836 Reviewed-by: Mike Reed Commit-Queue: Brian Osman --- include/core/SkColorFilter.h | 14 -------------- src/core/SkColorFilter.cpp | 32 +++----------------------------- tests/ColorFilterTest.cpp | 22 ---------------------- 3 files changed, 3 insertions(+), 65 deletions(-) diff --git a/include/core/SkColorFilter.h b/include/core/SkColorFilter.h index 58518656cf..30b9fe770e 100644 --- a/include/core/SkColorFilter.h +++ b/include/core/SkColorFilter.h @@ -89,9 +89,6 @@ public: * this filter, applied to the output of the inner filter. * * result = this(inner(...)) - * - * Due to internal limits, it is possible that this will return NULL, so the caller must - * always check. */ sk_sp makeComposed(sk_sp inner) const; @@ -137,22 +134,11 @@ protected: virtual bool onAsAColorMode(SkColor* color, SkBlendMode* bmode) const; private: - /* - * Returns 1 if this is a single filter (not a composition of other filters), otherwise it - * reutrns the number of leaf-node filters in a composition. This should be the same value - * as the number of GrFragmentProcessors returned by asFragmentProcessors's array parameter. - * - * e.g. compose(filter, compose(compose(filter, filter), filter)) --> 4 - */ - virtual int privateComposedFilterCount() const { return 1; } - virtual bool onAppendStages(const SkStageRec& rec, bool shaderIsOpaque) const = 0; virtual skvm::Color onProgram(skvm::Builder*, skvm::Color, SkColorSpace* dstCS, skvm::Uniforms*, SkArenaAlloc*) const; - friend class SkComposeColorFilter; - typedef SkFlattenable INHERITED; }; diff --git a/src/core/SkColorFilter.cpp b/src/core/SkColorFilter.cpp index b94d33522b..91552c5c60 100644 --- a/src/core/SkColorFilter.cpp +++ b/src/core/SkColorFilter.cpp @@ -99,16 +99,6 @@ SkColor4f SkColorFilter::filterColor4f(const SkColor4f& origSrcColor, SkColorSpa /////////////////////////////////////////////////////////////////////////////////////////////////// -/* - * Since colorfilters may be used on the GPU backend, and in that case we may string together - * many GrFragmentProcessors, we might exceed some internal instruction/resource limit. - * - * Since we don't yet know *what* those limits might be when we construct the final shader, - * we just set an arbitrary limit during construction. If later we find smarter ways to know what - * the limnits are, we can change this constant (or remove it). - */ -#define SK_MAX_COMPOSE_COLORFILTER_COUNT 4 - class SkComposeColorFilter : public SkColorFilter { public: uint32_t getFlags() const override { @@ -154,23 +144,12 @@ protected: private: SK_FLATTENABLE_HOOKS(SkComposeColorFilter) - SkComposeColorFilter(sk_sp outer, sk_sp inner, - int composedFilterCount) + SkComposeColorFilter(sk_sp outer, sk_sp inner) : fOuter(std::move(outer)) - , fInner(std::move(inner)) - , fComposedFilterCount(composedFilterCount) - { - SkASSERT(composedFilterCount >= 2); - SkASSERT(composedFilterCount <= SK_MAX_COMPOSE_COLORFILTER_COUNT); - } - - int privateComposedFilterCount() const override { - return fComposedFilterCount; - } + , fInner(std::move(inner)) {} sk_sp fOuter; sk_sp fInner; - const int fComposedFilterCount; friend class SkColorFilter; @@ -183,17 +162,12 @@ sk_sp SkComposeColorFilter::CreateProc(SkReadBuffer& buffer) { return outer ? outer->makeComposed(std::move(inner)) : inner; } - sk_sp SkColorFilter::makeComposed(sk_sp inner) const { if (!inner) { return sk_ref_sp(this); } - int count = inner->privateComposedFilterCount() + this->privateComposedFilterCount(); - if (count > SK_MAX_COMPOSE_COLORFILTER_COUNT) { - return nullptr; - } - return sk_sp(new SkComposeColorFilter(sk_ref_sp(this), std::move(inner), count)); + return sk_sp(new SkComposeColorFilter(sk_ref_sp(this), std::move(inner))); } /////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/tests/ColorFilterTest.cpp b/tests/ColorFilterTest.cpp index 78eda7f30a..c0d0dd88ec 100644 --- a/tests/ColorFilterTest.cpp +++ b/tests/ColorFilterTest.cpp @@ -33,26 +33,6 @@ static sk_sp reincarnate_colorfilter(SkFlattenable* obj) { /////////////////////////////////////////////////////////////////////////////// -static sk_sp make_filter() { - // pick a filter that cannot compose with itself via newComposed() - return SkColorFilters::Blend(SK_ColorRED, SkBlendMode::kColorBurn); -} - -static void test_composecolorfilter_limit(skiatest::Reporter* reporter) { - // Test that CreateComposeFilter() has some finite limit (i.e. that the factory can return null) - const int way_too_many = 100; - auto parent(make_filter()); - for (int i = 2; i < way_too_many; ++i) { - auto filter(make_filter()); - parent = parent->makeComposed(filter); - if (nullptr == parent) { - REPORTER_ASSERT(reporter, i > 2); // we need to have succeeded at least once! - return; - } - } - REPORTER_ASSERT(reporter, false); // we never saw a nullptr :( -} - #define ILLEGAL_MODE ((SkBlendMode)-1) DEF_TEST(ColorFilter, reporter) { @@ -109,6 +89,4 @@ DEF_TEST(ColorFilter, reporter) { REPORTER_ASSERT(reporter, m2 == expectedMode); } } - - test_composecolorfilter_limit(reporter); }