Harmonize FilterEffect::mapRect and mapPaintRect

third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp

 /*
  * Copyright (C) 2004, 2005, 2006, 2007 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005 Rob Buis <buis@kde.org>
  * Copyright (C) 2005 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2009 Dirk Schulze <krit@webkit.org>
  * Copyright (C) Research In Motion Limited 2010. All rights reserved.
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 95 matching lines @@
 }
 
 bool FEComposite::setK4(float k4)
 {
     if (m_k4 == k4)
         return false;
     m_k4 = k4;
     return true;
 }
 
-FloatRect FEComposite::determineAbsolutePaintRect(const FloatRect& originalRequestedRect) const
+bool FEComposite::affectsTransparentPixels() const
 {
-    FloatRect requestedRect = originalRequestedRect;
-    if (clipsToBounds())
-        requestedRect.intersect(absoluteBounds());
+    return k4() > 0;

[pdr., 2016/09/16 18:29:18]

Can you add a comment explaining this line?

There's this comment in mapInputs but it doesn't really explain why. The spec's
equation is "result = k1*i1*i2 + k2*i1 + k3*i2 + k4" and it's not clear how k4
different from k1-k3 in terms of affecting transparent pixels.

[fs, 2016/09/16 18:53:28]

Sure.
i1=i2=0, k1=k2=k3=x, k4>0 => result > 0 (not a transparent pixel)

More generally "(transparent <op> transparent) != transparent".  

[fs, 2016/09/16 20:16:21]

Noticed that I should probably have checked m_type here as well to not pessimize
unnecessarily, so thank for prodding! =)

+}
 
-    // No mapPaintRect required for FEComposite.
-    FloatRect input1Rect = inputEffect(1)->determineAbsolutePaintRect(requestedRect);
-    FloatRect affectedRect;
+FloatRect FEComposite::mapInputs(const FloatRect& rect) const
+{
+    FloatRect input1Rect = inputEffect(1)->mapRect(rect);
     switch (m_type) {
     case FECOMPOSITE_OPERATOR_IN:
         // 'in' has output only in the intersection of both inputs.
-        affectedRect = intersection(input1Rect, inputEffect(0)->determineAbsolutePaintRect(input1Rect));
-        break;
+        return intersection(input1Rect, inputEffect(0)->mapRect(input1Rect));
     case FECOMPOSITE_OPERATOR_ATOP:
         // 'atop' has output only in the extents of the second input.
-        // Make sure first input knows where it needs to produce output.
-        inputEffect(0)->determineAbsolutePaintRect(input1Rect);
-        affectedRect = input1Rect;
-        break;
+        return input1Rect;
     case FECOMPOSITE_OPERATOR_ARITHMETIC:
-        if (k4() > 0) {
-            // Make sure first input knows where it needs to produce output.
-            inputEffect(0)->determineAbsolutePaintRect(requestedRect);
-            // Arithmetic with non-zero k4 may influnce the complete filter primitive
-            // region. So we can't optimize the paint region here.
-            affectedRect = requestedRect;
-            break;
-        }
+        // Arithmetic with non-zero k4 may influnce the complete filter primitive

[pdr., 2016/09/16 18:29:18]

Nit: "influence"

If possible, can you add comments above each of these conditionals explaining
why?

[fs, 2016/09/16 18:53:28]

I'll see if can express this in a not too verbose way.

[fs, 2016/09/16 20:16:21]

Comments added.

+        // region.
+        if (k4() > 0)
+            return rect;
         if (k2() <= 0) {
             // Input 0 does not appear where input 1 is not present.
-            FloatRect input0Rect = inputEffect(0)->determineAbsolutePaintRect(input1Rect);
-            if (k3() > 0) {
-                affectedRect = input1Rect;
-            } else {
-                // Just k1 is positive. Use intersection.
-                affectedRect = intersection(input1Rect, input0Rect);
-            }
-            break;
+            if (k3() > 0)
+                return input1Rect;
+            // Just k1 is positive. Use intersection.
+            return intersection(input1Rect, inputEffect(0)->mapRect(input1Rect));
         }
         // else fall through to use union
     default:
-        // Take the union of both input effects.
-        affectedRect = unionRect(input1Rect, inputEffect(0)->determineAbsolutePaintRect(requestedRect));
         break;
     }
-
-    affectedRect.intersect(requestedRect);
-    return affectedRect;
+    // Take the union of both input effects.
+    return unionRect(input1Rect, inputEffect(0)->mapRect(rect));
 }
 
 SkXfermode::Mode toXfermode(CompositeOperationType mode)
 {
     switch (mode) {
     case FECOMPOSITE_OPERATOR_OVER:
         return SkXfermode::kSrcOver_Mode;
     case FECOMPOSITE_OPERATOR_IN:
         return SkXfermode::kSrcIn_Mode;
     case FECOMPOSITE_OPERATOR_OUT:
@@ skipping 72 matching lines @@
     ts << " operation=\"" << m_type << "\"";
     if (m_type == FECOMPOSITE_OPERATOR_ARITHMETIC)
         ts << " k1=\"" << m_k1 << "\" k2=\"" << m_k2 << "\" k3=\"" << m_k3 << "\" k4=\"" << m_k4 << "\"";
     ts << "]\n";
     inputEffect(0)->externalRepresentation(ts, indent + 1);
     inputEffect(1)->externalRepresentation(ts, indent + 1);
     return ts;
 }
 
 } // namespace blink