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.

Committed: http://code.google.com/p/skia/source/detail?r=14650

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@14659 2bbb7eff-a529-9590-31e7-b0007b416f81

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,17 +912,32 @@ 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);
 
-                    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);

src/core/SkPicturePlayback.h

@@ -273,7 +273,7 @@ private:
         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 @@ private:
 
         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

src/core/SkPictureStateTree.cpp

@@ -114,35 +114,6 @@ 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<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();

src/core/SkPictureStateTree.h

@@ -82,21 +82,6 @@ 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; }

src/gpu/GrPictureUtils.cpp

@@ -137,16 +137,7 @@ protected:
         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)

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.
-        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;

src/gpu/SkGpuDevice.cpp

@@ -1997,12 +1997,19 @@ 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
-        // 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 ||

tests/PictureTest.cpp

@@ -889,7 +889,8 @@ static void test_gpu_picture_optimization(skiatest::Reporter* reporter,
         REPORTER_ASSERT(reporter, kWidth/2 == info2.fSize.fWidth &&
                                   kHeight/2 == info2.fSize.fHeight); // bound reduces size
         REPORTER_ASSERT(reporter, info2.fCTM.isIdentity());         // translated
-        REPORTER_ASSERT(reporter, 0 == info2.fOffset.fX && 0 == info2.fOffset.fY);
+        REPORTER_ASSERT(reporter, kWidth/2 == info2.fOffset.fX && 
+                                  kHeight/2 == info2.fOffset.fY);
         REPORTER_ASSERT(reporter, NULL != info1.fPaint);
         REPORTER_ASSERT(reporter, info2.fIsNested && !info2.fHasNestedLayers); // is nested