commit 3f22e8c44a16d93377c0a3881f936e692b5b3320
author dneto <dneto@chromium.org> Wed Jul 30 15:42:22 2014 -0700
committer Commit bot <commit-bot@chromium.org> Wed Jul 30 15:42:22 2014 -0700
tree f47787b30555eee25078d6b411f3fbb54e0f2eaf
parent 3d822c29d4272a6749e59801b202b1ed6d611be8

Fix end-of-pattern matching for Skia recording optimization.

The recorder optimizer's pattern matcher was accepting command sequences
when it shouldn't have.

In the submitted case, and the pattern matcher was looking for:
	saveLayer, drawBitmap, restore
and in the rendering for the submitted case, the sequence of commands
were:
	saveLayer, drawBitmap, drawBitmap, restore

This sequence was improperly accepted by the matcher, and the optimizer
reduced the sequence to:
	drawBitmap, drawBitmap
where the opacity from the saveLayer paint argument was applied
to the first drawBitmap only.

The user-visible effect in Chrome was a flashing effect on an image
caused by incorrect (too-high) opacity.

The patch adds a Skia test to check for pixel colour values in
a similarly structured recording.  All other Skia tests pass.
Blink layout tests also pass with this change.

BUG=chromium:344987
R=robertphillips@google.com, reed@google.com, mtklein@google.com

Author: dneto@chromium.org

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

src/core/SkPictureRecord.cpp

diff --git a/src/core/SkPictureRecord.cpp b/src/core/SkPictureRecord.cpp
index 566cb19..ab4c3fa 100644
--- a/src/core/SkPictureRecord.cpp
+++ b/src/core/SkPictureRecord.cpp
@@ -319,7 +319,6 @@
         return false;
     }
 
-    curOffset += curSize;
     if (curOffset < writer->bytesWritten()) {
         // Something else between the last command and the end of the stream
         return false;

tests/PictureTest.cpp

diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp
index 95ceb05..7abef0e 100644
--- a/tests/PictureTest.cpp
+++ b/tests/PictureTest.cpp
@@ -1610,3 +1610,51 @@
 
     test_draw_bitmaps(&canvas);
 }
+
+DEF_TEST(DontOptimizeSaveLayerDrawDrawRestore, reporter) {
+    // This test is from crbug.com/344987.
+    // The commands are:
+    //   saveLayer with paint that modifies alpha
+    //     drawBitmapRectToRect
+    //     drawBitmapRectToRect
+    //   restore
+    // The bug was that this structure was modified so that:
+    //  - The saveLayer and restore were eliminated
+    //  - The alpha was only applied to the first drawBitmapRectToRect
+
+    // This test draws blue and red squares inside a 50% transparent
+    // layer.  Both colours should show up muted.
+    // When the bug is present, the red square (the second bitmap)
+    // shows upwith full opacity.
+
+    SkBitmap blueBM;
+    make_bm(&blueBM, 100, 100, SkColorSetARGB(255, 0, 0, 255), true);
+    SkBitmap redBM;
+    make_bm(&redBM, 100, 100, SkColorSetARGB(255, 255, 0, 0), true);
+    SkPaint semiTransparent;
+    semiTransparent.setAlpha(0x80);
+
+    SkPictureRecorder recorder;
+    SkCanvas* canvas = recorder.beginRecording(100, 100);
+    canvas->drawARGB(0, 0, 0, 0);
+
+    canvas->saveLayer(0, &semiTransparent);
+    canvas->drawBitmap(blueBM, 25, 25);
+    canvas->drawBitmap(redBM, 50, 50);
+    canvas->restore();
+
+    SkAutoTUnref<SkPicture> picture(recorder.endRecording());
+
+    // Now replay the picture back on another canvas
+    // and check a couple of its pixels.
+    SkBitmap replayBM;
+    make_bm(&replayBM, 100, 100, SK_ColorBLACK, false);
+    SkCanvas replayCanvas(replayBM);
+    picture->draw(&replayCanvas);
+    replayCanvas.flush();
+
+    // With the bug present, at (55, 55) we would get a fully opaque red
+    // intead of a dark red.
+    REPORTER_ASSERT(reporter, replayBM.getColor(30, 30) == 0xff000080);
+    REPORTER_ASSERT(reporter, replayBM.getColor(55, 55) == 0xff800000);
+}