Fix recording of saveLayout with unusual Xfermodes.

src/core/SkBBoxHierarchyRecord.cpp

Index: src/core/SkBBoxHierarchyRecord.cpp
diff --git a/src/core/SkBBoxHierarchyRecord.cpp b/src/core/SkBBoxHierarchyRecord.cpp
index 4712abb2b260369233afa1a39c99ade2dfea6c39..a9cd05d85ff1dc3cf1028ac29982025e44b50d53 100644
--- a/src/core/SkBBoxHierarchyRecord.cpp
+++ b/src/core/SkBBoxHierarchyRecord.cpp
@@ -37,15 +37,42 @@ SkCanvas::SaveLayerStrategy SkBBoxHierarchyRecord::willSaveLayer(const SkRect* b
     bool paintAffectsTransparentBlack = paint &&
         ((paint->getImageFilter()) ||
          (paint->getColorFilter()));
+    bool needToHandleBBox = paintAffectsTransparentBlack;
+    if (!needToHandleBBox && paint) {

[dneto, 2014/09/12 18:23:03]

The obvious thing would be to commonize this Xfermode checking logic in one
place.  But I believe we're removing this version of the recording logic, so I
didn't want to overgeneralize at this moment.
This deprecated codepath is still used by Chromium.

The exact list of unsafe xfermodes was determined by the test I added.

[mtklein, 2014/09/12 19:20:08]

SGTM.  In fact, at ToT you should now see the new code path active.

+      // Unusual Xfermodes require us to process a saved layer

[mtklein, 2014/09/12 19:20:08]

Can you copy (or move) this comment over to SkRecordDraw.cpp?  It'd be a shame
to lose it when we kill this code off.

[dneto, 2014/09/12 20:41:42]

Done.

+      // even with operations outisde the clip.
+      // For example, DstIn is used by masking layers.
+      // https://code.google.com/p/skia/issues/detail?id=1291
+      SkXfermode* xfermode = paint->getXfermode();
+      SkXfermode::Mode mode;
+      // SrcOver is the common case with a NULL xfermode, so we should
+      // make that the fast path and bypass the mode extraction and test.
+      if (xfermode && xfermode->asMode(&mode)) {
+        switch (mode) {
+          case SkXfermode::kClear_Mode:

[Justin Novosad, 2014/09/12 20:17:06]

I feel this logic belongs in a more central location because it is susceptible
of being reused.  Actually, you even duplicated it inside this CL. I would even
suggest exposing it in the public API. Perhaps something like bool
SkXfermode::transparentSourceAffectsDestination().

[mtklein, 2014/09/12 20:23:07]

Let's hold off on that for another CL.  This duplication is ephemeral.  I don't
want to slow down this bug fix with the bikeshedding that comes along with
making things public API.

[dneto, 2014/09/12 20:41:42]

Acknowledged.

+          case SkXfermode::kSrc_Mode:
+          case SkXfermode::kSrcIn_Mode:
+          case SkXfermode::kDstIn_Mode:
+          case SkXfermode::kSrcOut_Mode:
+          case SkXfermode::kDstATop_Mode:
+          case SkXfermode::kModulate_Mode:
+            needToHandleBBox = true;
+            break;
+          default:
+            break;
+        }
+      }
+    }
+
     SkRect drawBounds;
-    if (paintAffectsTransparentBlack) {
+    if (needToHandleBBox) {
         SkIRect deviceBounds;
         this->getClipDeviceBounds(&deviceBounds);
         drawBounds.set(deviceBounds);
     }
     fStateTree->appendSaveLayer(this->writeStream().bytesWritten());
     SkCanvas::SaveLayerStrategy strategy = this->INHERITED::willSaveLayer(bounds, paint, flags);
-    if (paintAffectsTransparentBlack) {
+    if (needToHandleBBox) {
         this->handleBBox(drawBounds);
         this->addNoOp();
     }