From 3f22e8c44a16d93377c0a3881f936e692b5b3320 Mon Sep 17 00:00:00 2001 From: dneto Date: Wed, 30 Jul 2014 15:42:22 -0700 Subject: [PATCH] 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 | 1 - tests/PictureTest.cpp | 48 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) 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 @@ static bool match(SkWriter32* writer, uint32_t offset, return false; } - curOffset += curSize; if (curOffset < writer->bytesWritten()) { // Something else between the last command and the end of the stream return false; 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 @@ DEF_TEST(Canvas_EmptyBitmap, r) { 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 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); +} -- 2.7.4