Fix recording of saveLayout with unusual Xfermodes.

src/core/SkBBoxHierarchyRecord.cpp

 
 /*
  * Copyright 2012 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBBoxHierarchyRecord.h"
 #include "SkPictureStateTree.h"
@@ skipping 19 matching lines @@
 }
 
 SkCanvas::SaveLayerStrategy SkBBoxHierarchyRecord::willSaveLayer(const SkRect* bounds,
                                                                  const SkPaint* paint,
                                                                  SaveFlags flags) {
     // For now, assume all filters affect transparent black.
     // FIXME: This could be made less conservative as an optimization.
     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();
     }
     return strategy;
 }
 
 void SkBBoxHierarchyRecord::willRestore() {
     fStateTree->appendRestore();
     this->INHERITED::willRestore();
 }
@@ skipping 37 matching lines @@
 
 bool SkBBoxHierarchyRecord::shouldRewind(void* data) {
     // SkBBoxHierarchy::rewindInserts is called by SkPicture after the
     // SkPicture has rewound its command stream.  To match that rewind in the
     // BBH, we rewind all draws that reference commands that were recorded
     // past the point to which the SkPicture has rewound, which is given by
     // writeStream().bytesWritten().
     SkPictureStateTree::Draw* draw = static_cast<SkPictureStateTree::Draw*>(data);
     return draw->fOffset >= writeStream().bytesWritten();
 }