Fix rendering artifacts in pull-saveLayers-forward mode
This CL fixes 2 bugs in the pre-rendering of saveLayers:
1) The drawBitmapRect call wasn't occurring in device space so could sometimes be double transformed
2) The BBH op skipping in SkPicturePlayback could sometimes mess up the SkPictureStateTree's state
It also reduces the number of layers that are pre-rendered when a BBH is not in use.
R=bsalomon@google.com, reed@google.com
Author: robertphillips@google.com
Review URL: https://codereview.chromium.org/267293007
git-svn-id: http://skia.googlecode.com/svn/trunk@14650 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp
index 28ca689..f2356bd 100644
--- a/src/core/SkPicturePlayback.cpp
+++ b/src/core/SkPicturePlayback.cpp
@@ -896,15 +896,15 @@
if (NULL != temp) {
SkASSERT(NULL != temp->fBM);
SkASSERT(NULL != temp->fPaint);
+ canvas.save();
+ canvas.setMatrix(initialMatrix);
canvas.drawBitmap(*temp->fBM, temp->fPos.fX, temp->fPos.fY, temp->fPaint);
+ canvas.restore();
if (it.isValid()) {
// This save is needed since the BBH will automatically issue
// a restore to balanced the saveLayer we're skipping
canvas.save();
- // Note: This skipping only works if the client only issues
- // well behaved saveLayer calls (i.e., doesn't use
- // kMatrix_SaveFlag or kClip_SaveFlag in isolation)
// At this point we know that the PictureStateTree was aiming
// for some draw op within temp's saveLayer (although potentially
@@ -912,17 +912,32 @@
// We need to skip all the operations inside temp's range
// along with all the associated state changes but update
// the state tree to the first operation outside temp's range.
- SkASSERT(it.peekDraw() >= temp->fStart && it.peekDraw() <= temp->fStop);
- while (kDrawComplete != it.peekDraw() && it.peekDraw() <= temp->fStop) {
- it.skipDraw();
- }
+ uint32_t skipTo;
+ do {
+ skipTo = it.nextDraw();
+ if (kDrawComplete == skipTo) {
+ break;
+ }
- if (kDrawComplete == it.peekDraw()) {
+ if (skipTo <= temp->fStop) {
+ reader.setOffset(skipTo);
+ uint32_t size;
+ DrawType op = read_op_and_size(&reader, &size);
+ // Since we are relying on the normal SkPictureStateTree
+ // playback we need to convert any nested saveLayer calls
+ // it may issue into saves (so that all its internal
+ // restores will be balanced).
+ if (SAVE_LAYER == op) {
+ canvas.save();
+ }
+ }
+ } while (skipTo <= temp->fStop);
+
+ if (kDrawComplete == skipTo) {
break;
}
- uint32_t skipTo = it.nextDraw();
reader.setOffset(skipTo);
} else {
reader.setOffset(temp->fStop);
diff --git a/src/core/SkPicturePlayback.h b/src/core/SkPicturePlayback.h
index 37caa8a..d6f0cf1 100644
--- a/src/core/SkPicturePlayback.h
+++ b/src/core/SkPicturePlayback.h
@@ -273,7 +273,7 @@
struct ReplacementInfo {
size_t fStart;
size_t fStop;
- SkPoint fPos;
+ SkIPoint fPos;
SkBitmap* fBM;
const SkPaint* fPaint; // Note: this object doesn't own the paint
};
@@ -293,14 +293,13 @@
void freeAll();
- #ifdef SK_DEBUG
+#ifdef SK_DEBUG
void validate() const;
- #endif
+#endif
SkTDArray<ReplacementInfo> fReplacements;
};
-
// Replace all the draw ops in the replacement ranges in 'replacements' with
// the associated drawBitmap call
// Draw replacing cannot be enabled at the same time as draw limiting
diff --git a/src/core/SkPictureStateTree.cpp b/src/core/SkPictureStateTree.cpp
index 8b5bc0a..fdd8646 100644
--- a/src/core/SkPictureStateTree.cpp
+++ b/src/core/SkPictureStateTree.cpp
@@ -114,35 +114,6 @@
fCurrentMatrix = matrix;
}
-uint32_t SkPictureStateTree::Iterator::peekDraw() {
- SkASSERT(this->isValid());
- if (fPlaybackIndex >= fDraws->count()) {
- return kDrawComplete;
- }
-
- Draw* draw = static_cast<Draw*>((*fDraws)[fPlaybackIndex]);
- return draw->fOffset;
-}
-
-uint32_t SkPictureStateTree::Iterator::skipDraw() {
- SkASSERT(this->isValid());
- if (fPlaybackIndex >= fDraws->count()) {
- return this->finish();
- }
-
- Draw* draw = static_cast<Draw*>((*fDraws)[fPlaybackIndex]);
-
- if (fSave) {
- fCanvas->save();
- fSave = false;
- }
-
- fNodes.rewind();
-
- ++fPlaybackIndex;
- return draw->fOffset;
-}
-
uint32_t SkPictureStateTree::Iterator::finish() {
if (fCurrentNode->fFlags & Node::kSaveLayer_Flag) {
fCanvas->restore();
diff --git a/src/core/SkPictureStateTree.h b/src/core/SkPictureStateTree.h
index f9e187a..da51a5b 100644
--- a/src/core/SkPictureStateTree.h
+++ b/src/core/SkPictureStateTree.h
@@ -82,21 +82,6 @@
TODO: this might be better named nextOp
*/
uint32_t nextDraw();
- /** Peek at the currently queued up draw op's offset. Note that this can
- be different then what 'nextDraw' would return b.c. it is
- the offset of the next _draw_ op while 'nextDraw' can return
- the offsets to saveLayer and clip ops while it is creating the proper
- drawing context for the queued up draw op.
- */
- uint32_t peekDraw();
- /** Stop trying to create the drawing context for the currently queued
- up _draw_ operation and queue up the next one. This call returns
- the offset of the skipped _draw_ operation. Obviously (since the
- correct drawing context has not been established), the skipped
- _draw_ operation should not be issued. Returns kDrawComplete if
- the end of the draw operations is reached.
- */
- uint32_t skipDraw();
static const uint32_t kDrawComplete = SK_MaxU32;
Iterator() : fPlaybackMatrix(), fValid(false) { }
bool isValid() const { return fValid; }
diff --git a/src/gpu/GrPictureUtils.cpp b/src/gpu/GrPictureUtils.cpp
index a66f34c..84a13be 100644
--- a/src/gpu/GrPictureUtils.cpp
+++ b/src/gpu/GrPictureUtils.cpp
@@ -137,16 +137,7 @@
device->fInfo.fCTM.postTranslate(SkIntToScalar(-device->getOrigin().fX),
SkIntToScalar(-device->getOrigin().fY));
- // We need the x & y values that will yield 'getOrigin' when transformed
- // by 'draw.fMatrix'.
- device->fInfo.fOffset.iset(device->getOrigin());
-
- SkMatrix invMatrix;
- if (draw.fMatrix->invert(&invMatrix)) {
- invMatrix.mapPoints(&device->fInfo.fOffset, 1);
- } else {
- device->fInfo.fValid = false;
- }
+ device->fInfo.fOffset = device->getOrigin();
if (NeedsDeepCopy(paint)) {
// This NULL acts as a signal that the paint was uncopyable (for now)
diff --git a/src/gpu/GrPictureUtils.h b/src/gpu/GrPictureUtils.h
index b8a7814..a280a16 100644
--- a/src/gpu/GrPictureUtils.h
+++ b/src/gpu/GrPictureUtils.h
@@ -27,8 +27,8 @@
// the translation needed to map the layer's top-left point to the origin.
SkMatrix fCTM;
// The offset that needs to be passed to drawBitmap to correctly
- // position the pre-rendered layer.
- SkPoint fOffset;
+ // position the pre-rendered layer. It is in device space.
+ SkIPoint fOffset;
// The paint to use on restore. NULL if the paint was not copyable (and
// thus that this layer should not be pulled forward).
const SkPaint* fPaint;
diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
index b279c43..483a149 100644
--- a/src/gpu/SkGpuDevice.cpp
+++ b/src/gpu/SkGpuDevice.cpp
@@ -1997,12 +1997,19 @@
}
} else {
// In this case there is no BBH associated with the picture. Pre-render
- // all the layers
- // TODO: intersect the bounds of each layer with the clip region to
- // reduce the number of pre-rendered layers
+ // all the layers that intersect the drawn region
for (int j = 0; j < gpuData->numSaveLayers(); ++j) {
const GPUAccelData::SaveLayerInfo& info = gpuData->saveLayerInfo(j);
+ SkIRect layerRect = SkIRect::MakeXYWH(info.fOffset.fX,
+ info.fOffset.fY,
+ info.fSize.fWidth,
+ info.fSize.fHeight);
+
+ if (!SkIRect::Intersects(query, layerRect)) {
+ continue;
+ }
+
// TODO: once this code is more stable unsuitable layers can
// just be omitted during the optimization stage
if (!info.fValid ||