diff options
author | dneto <dneto@chromium.org> | 2014-09-15 10:53:16 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-09-15 10:53:16 -0700 |
commit | 327f905d2cb0d37c302d651d8f2b17ea56368467 (patch) | |
tree | d7f8cb2734b01ad2480458315fc39f5c10862d95 | |
parent | 595aa05efcb504e85358b8d328ac4a9fa1c46e2e (diff) |
Fix recording of saveLayout with unusual Xfermodes.
This is the root cause of a Chrome rendering bug when it tiles
layers with masks.
BUG=skia:1291,chromium:401593
R=reed@google.com, mtklein@google.com, junov@chromium.org
Author: dneto@chromium.org
Review URL: https://codereview.chromium.org/568073004
-rw-r--r-- | gyp/tests.gypi | 1 | ||||
-rw-r--r-- | src/core/SkBBoxHierarchyRecord.cpp | 31 | ||||
-rw-r--r-- | src/core/SkRecordDraw.cpp | 39 | ||||
-rw-r--r-- | tests/RecordingXfermodeTest.cpp | 240 |
4 files changed, 307 insertions, 4 deletions
diff --git a/gyp/tests.gypi b/gyp/tests.gypi index cf78e41f63..f33d864f3c 100644 --- a/gyp/tests.gypi +++ b/gyp/tests.gypi @@ -173,6 +173,7 @@ '../tests/RecordTest.cpp', '../tests/RecorderTest.cpp', '../tests/RecordingTest.cpp', + '../tests/RecordingXfermodeTest.cpp', '../tests/RefCntTest.cpp', '../tests/RefDictTest.cpp', '../tests/RegionTest.cpp', diff --git a/src/core/SkBBoxHierarchyRecord.cpp b/src/core/SkBBoxHierarchyRecord.cpp index 4712abb2b2..a9cd05d85f 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) { + // Unusual Xfermodes require us to process a saved layer + // 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: + 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(); } diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp index ac1e429f12..e1975e1fe3 100644 --- a/src/core/SkRecordDraw.cpp +++ b/src/core/SkRecordDraw.cpp @@ -229,8 +229,43 @@ private: } static bool PaintMayAffectTransparentBlack(const SkPaint* paint) { - // FIXME: this is very conservative - return paint && (paint->getImageFilter() || paint->getColorFilter()); + if (paint) { + // FIXME: this is very conservative + if (paint->getImageFilter() || paint->getColorFilter()) { + return true; + } + + // Unusual Xfermodes require us to process a saved layer + // even with operations outisde the clip. + // For example, DstIn is used by masking layers. + // https://code.google.com/p/skia/issues/detail?id=1291 + // https://crbug.com/401593 + SkXfermode* xfermode = paint->getXfermode(); + SkXfermode::Mode mode; + // SrcOver is ok, and is also 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) { + // For each of the following transfer modes, if the source + // alpha is zero (our transparent black), the resulting + // blended alpha is not necessarily equal to the original + // destination alpha. + case SkXfermode::kClear_Mode: + case SkXfermode::kSrc_Mode: + case SkXfermode::kSrcIn_Mode: + case SkXfermode::kDstIn_Mode: + case SkXfermode::kSrcOut_Mode: + case SkXfermode::kDstATop_Mode: + case SkXfermode::kModulate_Mode: + return true; + break; + default: + break; + } + } + } + return false; } Bounds popSaveBlock() { diff --git a/tests/RecordingXfermodeTest.cpp b/tests/RecordingXfermodeTest.cpp new file mode 100644 index 0000000000..8da81b3680 --- /dev/null +++ b/tests/RecordingXfermodeTest.cpp @@ -0,0 +1,240 @@ +/* + * Copyright 2014 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#include "Test.h" + +#include "../include/core/SkCanvas.h" +#include "../include/core/SkPicture.h" +#include "../include/core/SkStream.h" +#include "../include/core/SkString.h" +#include "../include/record/SkRecording.h" +#include "../include/core/SkPictureRecorder.h" +#include <cstring> + +// Verify that replay of a recording into a clipped canvas +// produces the correct bitmap. +// This arose from http://crbug.com/401593 which has +// https://code.google.com/p/skia/issues/detail?id=1291 as its root cause. + + +namespace { + +class Drawer { + public: + explicit Drawer() + : fImageInfo(SkImageInfo::MakeN32Premul(200,100)) + { + fCircleBM.allocPixels( SkImageInfo::MakeN32Premul(100,100) ); + SkCanvas canvas(fCircleBM); + canvas.clear(0xffffffff); + SkPaint circlePaint; + circlePaint.setColor(0xff000000); + canvas.drawCircle(50,50,50,circlePaint); + } + + const SkImageInfo& imageInfo() const { return fImageInfo; } + + void draw(SkCanvas* canvas, const SkRect& clipRect, SkXfermode::Mode mode) const { + SkPaint greenPaint; + greenPaint.setColor(0xff008000); + SkPaint blackPaint; + blackPaint.setColor(0xff000000); + SkPaint whitePaint; + whitePaint.setColor(0xffffffff); + SkPaint layerPaint; + layerPaint.setColor(0xff000000); + layerPaint.setXfermodeMode(mode); + SkRect canvasRect(SkRect::MakeWH(SkIntToScalar(fImageInfo.width()),SkIntToScalar(fImageInfo.height()))); + + canvas->clipRect(clipRect); + canvas->clear(0xff000000); + + canvas->saveLayer(NULL,&blackPaint); + canvas->drawRect(canvasRect,greenPaint); + canvas->saveLayer(NULL,&layerPaint); + canvas->drawBitmapRect(fCircleBM,SkRect::MakeXYWH(20,20,60,60),&blackPaint); + canvas->restore(); + canvas->restore(); + } + + private: + const SkImageInfo fImageInfo; + SkBitmap fCircleBM; +}; + +class RecordingStrategy { + public: + virtual ~RecordingStrategy() {} + virtual void init(const SkImageInfo&) = 0; + virtual const SkBitmap& recordAndReplay(const Drawer& drawer, + const SkRect& intoClip, + SkXfermode::Mode) = 0; +}; + +class BitmapBackedCanvasStrategy : public RecordingStrategy { + // This version just draws into a bitmap-backed canvas. + public: + BitmapBackedCanvasStrategy() {} + + virtual void init(const SkImageInfo& imageInfo) { + fBitmap.allocPixels(imageInfo); + } + + virtual const SkBitmap& recordAndReplay(const Drawer& drawer, + const SkRect& intoClip, + SkXfermode::Mode mode) { + SkCanvas canvas(fBitmap); + canvas.clear(0xffffffff); + // Note that the scene is drawn just into the clipped region! + canvas.clipRect(intoClip); + drawer.draw(&canvas, intoClip, mode); // Shouild be canvas-wide... + return fBitmap; + } + + private: + SkBitmap fBitmap; +}; + +class DeprecatedRecorderStrategy : public RecordingStrategy { + // This version draws the entire scene into an SkPictureRecorder, + // using the deprecated recording backend. + // Then it then replays the scene through a clip rectangle. + // This backend proved to be buggy. + public: + DeprecatedRecorderStrategy() {} + + virtual void init(const SkImageInfo& imageInfo) { + fBitmap.allocPixels(imageInfo); + fWidth = imageInfo.width(); + fHeight= imageInfo.height(); + } + + virtual const SkBitmap& recordAndReplay(const Drawer& drawer, + const SkRect& intoClip, + SkXfermode::Mode mode) { + SkTileGridFactory::TileGridInfo tileGridInfo = { {100,100}, {0,0}, {0,0} }; + SkTileGridFactory factory(tileGridInfo); + SkPictureRecorder recorder; + SkRect canvasRect(SkRect::MakeWH(SkIntToScalar(fWidth),SkIntToScalar(fHeight))); + SkCanvas* canvas = recorder.DEPRECATED_beginRecording( SkIntToScalar(fWidth), SkIntToScalar(fHeight), &factory); + drawer.draw(canvas, canvasRect, mode); + SkAutoTDelete<SkPicture> picture(recorder.endRecording()); + + SkCanvas replayCanvas(fBitmap); + replayCanvas.clear(0xffffffff); + replayCanvas.clipRect(intoClip); + picture->playback(&replayCanvas); + + return fBitmap; + } + + private: + SkBitmap fBitmap; + int fWidth; + int fHeight; +}; + +class NewRecordingStrategy : public RecordingStrategy { + // This version draws the entire scene into an SkPictureRecorder, + // using the new recording backend. + // Then it then replays the scene through a clip rectangle. + // This backend proved to be buggy. + public: + NewRecordingStrategy() {} + + virtual void init(const SkImageInfo& imageInfo) { + fBitmap.allocPixels(imageInfo); + fWidth = imageInfo.width(); + fHeight= imageInfo.height(); + } + + virtual const SkBitmap& recordAndReplay(const Drawer& drawer, + const SkRect& intoClip, + SkXfermode::Mode mode) { + SkTileGridFactory::TileGridInfo tileGridInfo = { {100,100}, {0,0}, {0,0} }; + SkTileGridFactory factory(tileGridInfo); + SkPictureRecorder recorder; + SkRect canvasRect(SkRect::MakeWH(SkIntToScalar(fWidth),SkIntToScalar(fHeight))); + SkCanvas* canvas = recorder.EXPERIMENTAL_beginRecording( SkIntToScalar(fWidth), SkIntToScalar(fHeight), &factory); + + drawer.draw(canvas, canvasRect, mode); + SkAutoTDelete<SkPicture> picture(recorder.endRecording()); + + SkCanvas replayCanvas(fBitmap); + replayCanvas.clear(0xffffffff); + replayCanvas.clipRect(intoClip); + picture->playback(&replayCanvas); + return fBitmap; + } + + private: + SkBitmap fBitmap; + int fWidth; + int fHeight; +}; + +} + + + +DEF_TEST(SkRecordingAccuracyXfermode, reporter) { +#define FINEGRAIN 0 + + const Drawer drawer; + + BitmapBackedCanvasStrategy golden; // This is the expected result. + DeprecatedRecorderStrategy deprecatedRecording; + NewRecordingStrategy newRecording; + + golden.init(drawer.imageInfo()); + deprecatedRecording.init(drawer.imageInfo()); + newRecording.init(drawer.imageInfo()); + +#if !FINEGRAIN + unsigned numErrors = 0; + SkString errors; +#endif + + for (int iMode = 0; iMode < int(SkXfermode::kLastMode) ; iMode++ ) { + const SkRect& clip = SkRect::MakeXYWH(100,0,100,100); + SkXfermode::Mode mode = SkXfermode::Mode(iMode); + + const SkBitmap& goldenBM = golden.recordAndReplay(drawer, clip, mode); + const SkBitmap& deprecatedBM = deprecatedRecording.recordAndReplay(drawer, clip, mode); + const SkBitmap& newRecordingBM = newRecording.recordAndReplay(drawer, clip, mode); + + size_t pixelsSize = goldenBM.getSize(); + REPORTER_ASSERT( reporter, pixelsSize == deprecatedBM.getSize() ); + REPORTER_ASSERT( reporter, pixelsSize == newRecordingBM.getSize() ); + + // The pixel arrays should match. +#if FINEGRAIN + REPORTER_ASSERT_MESSAGE( reporter, + 0==memcmp( goldenBM.getPixels(), deprecatedBM.getPixels(), pixelsSize ), + "Tiled bitmap is wrong"); + REPORTER_ASSERT_MESSAGE( reporter, + 0==memcmp( goldenBM.getPixels(), recordingBM.getPixels(), pixelsSize ), + "SkRecorder bitmap is wrong"); +#else + if ( memcmp( goldenBM.getPixels(), deprecatedBM.getPixels(), pixelsSize ) ) { + numErrors++; + SkString str; + str.printf("For SkXfermode %d %s: Deprecated recorder bitmap is wrong\n", iMode, SkXfermode::ModeName(mode)); + errors.append(str); + } + if ( memcmp( goldenBM.getPixels(), newRecordingBM.getPixels(), pixelsSize ) ) { + numErrors++; + SkString str; + str.printf("For SkXfermode %d %s: SkPictureRecorder bitmap is wrong\n", iMode, SkXfermode::ModeName(mode)); + errors.append(str); + } +#endif + } +#if !FINEGRAIN + REPORTER_ASSERT_MESSAGE( reporter, 0==numErrors, errors.c_str() ); +#endif +} |