add impl limit for number of leaf-nodes in composecolorfilter

BUG=skia:

Review URL: https://codereview.chromium.org/972153010

diff --git a/include/core/SkColorFilter.h b/include/core/SkColorFilter.h
index 5a25814..fe151a2 100644 (file)
--- a/include/core/SkColorFilter.h
+++ b/include/core/SkColorFilter.h
@@ -134,6 +134,9 @@ public:
     /** Construct a colorfilter whose effect is to first apply the inner filter and then apply
      *  the outer filter to the result of the inner's.
      *  The reference counts for outer and inner are incremented.
+     *
+     *  Due to internal limits, it is possible that this will return NULL, so the caller must
+     *  always check.
      */
     static SkColorFilter* CreateComposeFilter(SkColorFilter* outer, SkColorFilter* inner);
 
@@ -162,6 +165,16 @@ protected:
     SkColorFilter() {}
 
 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; }
+    friend class SkComposeColorFilter;
+
     typedef SkFlattenable INHERITED;
 };
 

diff --git a/src/core/SkColorFilter.cpp b/src/core/SkColorFilter.cpp
index fd76dd1..0a9cd93 100644 (file)
--- a/src/core/SkColorFilter.cpp
+++ b/src/core/SkColorFilter.cpp
@@ -39,13 +39,18 @@ SkColor SkColorFilter::filterColor(SkColor c) const {
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
+/*
+ *  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:
-    SkComposeColorFilter(SkColorFilter* outer, SkColorFilter* inner)
-        : fOuter(SkRef(outer))
-        , fInner(SkRef(inner))
-    {}
-    
     uint32_t getFlags() const SK_OVERRIDE {
         // Can only claim alphaunchanged and 16bit support if both our proxys do.
         return fOuter->getFlags() & fInner->getFlags();
@@ -89,8 +94,22 @@ protected:
     }
     
 private:
+    SkComposeColorFilter(SkColorFilter* outer, SkColorFilter* inner, int composedFilterCount)
+        : fOuter(SkRef(outer))
+        , fInner(SkRef(inner))
+        , fComposedFilterCount(composedFilterCount)
+    {
+        SkASSERT(composedFilterCount >= 2);
+        SkASSERT(composedFilterCount <= SK_MAX_COMPOSE_COLORFILTER_COUNT);
+    }
+
+    int privateComposedFilterCount() const SK_OVERRIDE {
+        return fComposedFilterCount;
+    }
+
     SkAutoTUnref<SkColorFilter> fOuter;
     SkAutoTUnref<SkColorFilter> fInner;
+    const int                   fComposedFilterCount;
 
     friend class SkColorFilter;
 
@@ -115,10 +134,15 @@ SkColorFilter* SkColorFilter::CreateComposeFilter(SkColorFilter* outer, SkColorF
 
     // Give the subclass a shot at a more optimal composition...
     SkColorFilter* composition = outer->newComposed(inner);
-    if (NULL == composition) {
-        composition = SkNEW_ARGS(SkComposeColorFilter, (outer, inner));
+    if (composition) {
+        return composition;
+    }
+
+    int count = inner->privateComposedFilterCount() + outer->privateComposedFilterCount();
+    if (count > SK_MAX_COMPOSE_COLORFILTER_COUNT) {
+        return NULL;
     }
-    return composition;
+    return SkNEW_ARGS(SkComposeColorFilter, (outer, inner, count));
 }
 
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_START(SkColorFilter)

diff --git a/tests/ColorFilterTest.cpp b/tests/ColorFilterTest.cpp
index f3f6a0a..b2e3718 100644 (file)
--- a/tests/ColorFilterTest.cpp
+++ b/tests/ColorFilterTest.cpp
@@ -30,6 +30,26 @@ static SkColorFilter* reincarnate_colorfilter(SkFlattenable* obj) {
 
 ///////////////////////////////////////////////////////////////////////////////
 
+static SkColorFilter* make_filter() {
+    // pick a filter that cannot compose with itself via newComposed()
+    return SkColorFilter::CreateModeFilter(SK_ColorRED, SkXfermode::kColorBurn_Mode);
+}
+
+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;
+    SkAutoTUnref<SkColorFilter> parent(make_filter());
+    for (int i = 2; i < way_too_many; ++i) {
+        SkAutoTUnref<SkColorFilter> filter(make_filter());
+        parent.reset(SkColorFilter::CreateComposeFilter(parent, filter));
+        if (NULL == parent) {
+            REPORTER_ASSERT(reporter, i > 2); // we need to have succeeded at least once!
+            return;
+        }
+    }
+    REPORTER_ASSERT(reporter, false); // we never saw a NULL :(
+}
+
 #define ILLEGAL_MODE    ((SkXfermode::Mode)-1)
 
 DEF_TEST(ColorFilter, reporter) {
@@ -89,6 +109,8 @@ DEF_TEST(ColorFilter, reporter) {
             REPORTER_ASSERT(reporter, m2 == expectedMode);
         }
     }
+
+    test_composecolorfilter_limit(reporter);
 }
 
 ///////////////////////////////////////////////////////////////////////////////