From 3d41c4add81e95784faa81f9a59b4a32b7cff082 Mon Sep 17 00:00:00 2001 From: "robertphillips@google.com" Date: Thu, 8 May 2014 18:45:20 +0000 Subject: [PATCH] Revert r14650 (Fix rendering artifacts in pull-saveLayers-forward mode - https://codereview.chromium.org/267293007) due to unit test failures git-svn-id: http://skia.googlecode.com/svn/trunk@14654 2bbb7eff-a529-9590-31e7-b0007b416f81 --- src/core/SkPicturePlayback.cpp | 33 +++++++++------------------------ src/core/SkPicturePlayback.h | 7 ++++--- src/core/SkPictureStateTree.cpp | 29 +++++++++++++++++++++++++++++ src/core/SkPictureStateTree.h | 15 +++++++++++++++ src/gpu/GrPictureUtils.cpp | 11 ++++++++++- src/gpu/GrPictureUtils.h | 4 ++-- src/gpu/SkGpuDevice.cpp | 13 +++---------- 7 files changed, 72 insertions(+), 40 deletions(-) diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp index f2356bdef..28ca689d0 100644 --- a/src/core/SkPicturePlayback.cpp +++ b/src/core/SkPicturePlayback.cpp @@ -896,15 +896,15 @@ void SkPicturePlayback::draw(SkCanvas& canvas, SkDrawPictureCallback* callback) 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,32 +912,17 @@ void SkPicturePlayback::draw(SkCanvas& canvas, SkDrawPictureCallback* callback) // 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); - uint32_t skipTo; - do { - skipTo = it.nextDraw(); - if (kDrawComplete == skipTo) { - break; - } + while (kDrawComplete != it.peekDraw() && it.peekDraw() <= temp->fStop) { + it.skipDraw(); + } - 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) { + if (kDrawComplete == it.peekDraw()) { 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 d6f0cf191..37caa8a43 100644 --- a/src/core/SkPicturePlayback.h +++ b/src/core/SkPicturePlayback.h @@ -273,7 +273,7 @@ private: struct ReplacementInfo { size_t fStart; size_t fStop; - SkIPoint fPos; + SkPoint fPos; SkBitmap* fBM; const SkPaint* fPaint; // Note: this object doesn't own the paint }; @@ -293,13 +293,14 @@ private: void freeAll(); -#ifdef SK_DEBUG + #ifdef SK_DEBUG void validate() const; -#endif + #endif SkTDArray 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 fdd86464a..8b5bc0aa2 100644 --- a/src/core/SkPictureStateTree.cpp +++ b/src/core/SkPictureStateTree.cpp @@ -114,6 +114,35 @@ void SkPictureStateTree::Iterator::setCurrentMatrix(const SkMatrix* matrix) { fCurrentMatrix = matrix; } +uint32_t SkPictureStateTree::Iterator::peekDraw() { + SkASSERT(this->isValid()); + if (fPlaybackIndex >= fDraws->count()) { + return kDrawComplete; + } + + Draw* draw = static_cast((*fDraws)[fPlaybackIndex]); + return draw->fOffset; +} + +uint32_t SkPictureStateTree::Iterator::skipDraw() { + SkASSERT(this->isValid()); + if (fPlaybackIndex >= fDraws->count()) { + return this->finish(); + } + + Draw* draw = static_cast((*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 da51a5b95..f9e187a0c 100644 --- a/src/core/SkPictureStateTree.h +++ b/src/core/SkPictureStateTree.h @@ -82,6 +82,21 @@ public: 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 84a13be46..a66f34c0a 100644 --- a/src/gpu/GrPictureUtils.cpp +++ b/src/gpu/GrPictureUtils.cpp @@ -137,7 +137,16 @@ protected: device->fInfo.fCTM.postTranslate(SkIntToScalar(-device->getOrigin().fX), SkIntToScalar(-device->getOrigin().fY)); - device->fInfo.fOffset = device->getOrigin(); + // 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; + } 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 a280a16ce..b8a7814cc 100644 --- a/src/gpu/GrPictureUtils.h +++ b/src/gpu/GrPictureUtils.h @@ -27,8 +27,8 @@ public: // 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. It is in device space. - SkIPoint fOffset; + // position the pre-rendered layer. + SkPoint 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 483a1496c..b279c43c1 100644 --- a/src/gpu/SkGpuDevice.cpp +++ b/src/gpu/SkGpuDevice.cpp @@ -1997,19 +1997,12 @@ bool SkGpuDevice::EXPERIMENTAL_drawPicture(SkCanvas* canvas, SkPicture* picture) } } else { // In this case there is no BBH associated with the picture. Pre-render - // all the layers that intersect the drawn region + // all the layers + // TODO: intersect the bounds of each layer with the clip region to + // reduce the number of pre-rendered layers 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 ||