Fix saveLayer() with a pixel-moving filter vs SkBBoxHierarchyRecord / SkRecordDraw

In SkBBoxHierarchyRecord:
  Since the bounds we pass to saveLayer are in the pre-filtering
  coordinate space, they aren't correct for determining the actual
  device pixels touched by the saveLayer in this case.

  The easiest fix for now is to pass the clip bounds, since the final
  draw done in restore() will never draw outside the clip.

In SkRecordDraw:
  We do adjust the bounds passed to saveLayer, so we just need to make
  sure that when we're using a paint that may affect transparent black,
  we ignore the calculated bounds of draw ops and use the clip intersected
  with those adjusted bounds.

See originally crrev.com/497773002

BUG=skia:
R=reed@google.com, senorblanco@chromium.org, junov@chromium.org, mtklein@google.com

Author: mtklein@chromium.org

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

3 files changed, 64 insertions, 11 deletions

diff --git a/src/core/SkBBoxHierarchyRecord.cpp b/src/core/SkBBoxHierarchyRecord.cpp
index 1868e65325..8cdd1d9618 100644
--- a/src/core/SkBBoxHierarchyRecord.cpp
+++ b/src/core/SkBBoxHierarchyRecord.cpp
@@ -41,14 +41,9 @@ SkCanvas::SaveLayerStrategy SkBBoxHierarchyRecord::willSaveLayer(const SkRect* b
          (NULL != paint->getColorFilter()));
     SkRect drawBounds;
     if (paintAffectsTransparentBlack) {
-        if (bounds) {
-            drawBounds = *bounds;
-            this->getTotalMatrix().mapRect(&drawBounds);
-        } else {
-            SkIRect deviceBounds;
-            this->getClipDeviceBounds(&deviceBounds);
-            drawBounds.set(deviceBounds);
-        }
+        SkIRect deviceBounds;
+        this->getClipDeviceBounds(&deviceBounds);
+        drawBounds.set(deviceBounds);
     }
     fStateTree->appendSaveLayer(this->writeStream().bytesWritten());
     SkCanvas::SaveLayerStrategy strategy = this->INHERITED::willSaveLayer(bounds, paint, flags);
diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index a14b031695..5dd2ba3296 100644
--- a/src/core/SkRecordDraw.cpp
+++ b/src/core/SkRecordDraw.cpp
@@ -204,19 +204,29 @@ private:
         this->pushControl();
     }
 
+    static bool PaintMayAffectTransparentBlack(const SkPaint* paint) {
+        // FIXME: this is very conservative
+        return paint && (paint->getImageFilter() || paint->getColorFilter());
+    }
+
     SkIRect popSaveBlock() {
         // We're done the Save block.  Apply the block's bounds to all control ops inside it.
         SaveBounds sb;
         fSaveStack.pop(&sb);
+
+        // If the paint affects transparent black, we can't trust any of our calculated bounds.
+        const SkIRect& bounds =
+            PaintMayAffectTransparentBlack(sb.paint) ? fCurrentClipBounds : sb.bounds;
+
         while (sb.controlOps --> 0) {
-            this->popControl(sb.bounds);
+            this->popControl(bounds);
         }
 
         // This whole Save block may be part another Save block.
-        this->updateSaveBounds(sb.bounds);
+        this->updateSaveBounds(bounds);
 
         // If called from a real Restore (not a phony one for balance), it'll need the bounds.
-        return sb.bounds;
+        return bounds;
     }
 
     void pushControl() {
diff --git a/tests/ImageFilterTest.cpp b/tests/ImageFilterTest.cpp
index 214aed950c..f87f99ce98 100644
--- a/tests/ImageFilterTest.cpp
+++ b/tests/ImageFilterTest.cpp
@@ -421,6 +421,54 @@ DEF_TEST(ImageFilterDrawTiled, reporter) {
     }
 }
 
+static void drawSaveLayerPicture(int width, int height, int tileSize, SkBBHFactory* factory, SkBitmap* result) {
+
+    SkMatrix matrix;
+    matrix.setTranslate(SkIntToScalar(50), 0);
+
+    SkAutoTUnref<SkColorFilter> cf(SkColorFilter::CreateModeFilter(SK_ColorWHITE, SkXfermode::kSrc_Mode));
+    SkAutoTUnref<SkImageFilter> cfif(SkColorFilterImageFilter::Create(cf.get()));
+    SkAutoTUnref<SkImageFilter> imageFilter(SkMatrixImageFilter::Create(matrix, SkPaint::kNone_FilterLevel, cfif.get()));
+
+    SkPaint paint;
+    paint.setImageFilter(imageFilter.get());
+    SkPictureRecorder recorder;
+    SkRect bounds = SkRect::Make(SkIRect::MakeXYWH(0, 0, 50, 50));
+    SkCanvas* recordingCanvas = recorder.beginRecording(width, height, factory, 0);
+    recordingCanvas->translate(-55, 0);
+    recordingCanvas->saveLayer(&bounds, &paint);
+    recordingCanvas->restore();
+    SkAutoTUnref<SkPicture> picture1(recorder.endRecording());
+
+    result->allocN32Pixels(width, height);
+    SkCanvas canvas(*result);
+    canvas.clear(0);
+    canvas.clipRect(SkRect::Make(SkIRect::MakeWH(tileSize, tileSize)));
+    canvas.drawPicture(picture1.get());
+}
+
+DEF_TEST(ImageFilterDrawMatrixBBH, reporter) {
+    // Check that matrix filter when drawn tiled with BBH exactly
+    // matches the same thing drawn without BBH.
+    // Tests pass by not asserting.
+
+    const int width = 200, height = 200;
+    const int tileSize = 100;
+    SkBitmap result1, result2;
+    SkRTreeFactory factory;
+
+    drawSaveLayerPicture(width, height, tileSize, &factory, &result1);
+    drawSaveLayerPicture(width, height, tileSize, NULL, &result2);
+
+    for (int y = 0; y < height; y++) {
+        int diffs = memcmp(result1.getAddr32(0, y), result2.getAddr32(0, y), result1.rowBytes());
+        REPORTER_ASSERT(reporter, !diffs);
+        if (diffs) {
+            break;
+        }
+    }
+}
+
 static void drawBlurredRect(SkCanvas* canvas) {
     SkAutoTUnref<SkImageFilter> filter(SkBlurImageFilter::Create(SkIntToScalar(8), 0));
     SkPaint filterPaint;