From 5e27140f48a1ec0ae3890dca84bfa91bd32ecb3b Mon Sep 17 00:00:00 2001
From: Florin Malita <fmalita@google.com>
Date: Wed, 4 Nov 2015 14:36:02 -0500
Subject: [PATCH] Fix persistent Canvas clip handling

Partial Canvas save semantics (clip or matrix persisting after restore)
are currently emulated in the native canvas wrapper (SkiaCanvas.cpp).

Persistent clips (save w/ MATRIX_SAVE_FLAG only) in particular are
emulated using the SkCanvas clip stack.  There are two problems with
the current implementation:

1) The canvas save count is used to identify the clip stack topmost
frame, on the assumption that it is the same as the actual clip stack
save count.  But with the introduction of lazy SkCanvas saves in Skia,
the two values can diverge: the clip stack save count only reflects
*committed* saves, while the canvas save count includes both committed
and pending saves.  This means that we can no longer compare canvas and
clip stack save counts directly.

While we're looking at addressing the save count discrepancy in Skia
proper, we can also refactor the partial save emulation to no longer
rely on the two values being synchronized: instead of using the canvas
save count to locate the top clip stack frame, simply use the clip
stack save count for the same purpose - getClipStack()->getSaveCount()
always points to the correct top clip stack frame.

With this patch:

  * we use SkCanvas::getSaveCount() to track *canvas* save frames which
      require persistent matrix/clip handling (mSaveRecStack)
  * we use SkClipStack::getSaveCount() to extract the clips from the
    top clip stack frame

Also, since we're no longer mixing/comparing the two save counts, we
don't have to decrement the canvas value anymore (to make it zero-based
like its clip stack counterpart).

2) When iterating over clip stack elements, we currently start at
kTopIterStart and advance using next().  This is incorrect as next()
moves up the stack, so we only iterate over the topmost element =>
if there are multiple (non-consolidated) clip elements in the top
frame, we only get to see one.

We need to iterate using prev() instead.

Change-Id: Ic2d8cad684018925e749b9172fbf7c6202d9fb62
---
 libs/hwui/SkiaCanvas.cpp | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/libs/hwui/SkiaCanvas.cpp b/libs/hwui/SkiaCanvas.cpp
index a8f8134170d3d..6d3dfac4ba9c0 100644
--- a/libs/hwui/SkiaCanvas.cpp
+++ b/libs/hwui/SkiaCanvas.cpp
@@ -240,11 +240,15 @@ int SkiaCanvas::save(SkCanvas::SaveFlags flags) {
     return count;
 }
 
+// The SkiaCanvas::restore operation layers on the capability to preserve
+// either (or both) the matrix and/or clip state after a SkCanvas::restore
+// operation. It does this by explicitly saving off the clip & matrix state
+// when requested and playing it back after the SkCanvas::restore.
 void SkiaCanvas::restore() {
     const SaveRec* rec = (NULL == mSaveStack.get())
             ? NULL
             : static_cast<SaveRec*>(mSaveStack->back());
-    int currentSaveCount = mCanvas->getSaveCount() - 1;
+    int currentSaveCount = mCanvas->getSaveCount();
     SkASSERT(NULL == rec || currentSaveCount >= rec->saveCount);
 
     if (NULL == rec || rec->saveCount != currentSaveCount) {
@@ -262,8 +266,9 @@ void SkiaCanvas::restore() {
     }
 
     SkTArray<SkClipStack::Element> savedClips;
+    int topClipStackFrame = mCanvas->getClipStack()->getSaveCount();
     if (preserveClip) {
-        saveClipsForFrame(savedClips, currentSaveCount);
+        saveClipsForFrame(savedClips, topClipStackFrame);
     }
 
     mCanvas->restore();
@@ -272,7 +277,11 @@ void SkiaCanvas::restore() {
         mCanvas->setMatrix(savedMatrix);
     }
 
-    if (preserveClip && !savedClips.empty()) {
+    if (preserveClip && !savedClips.empty() &&
+        topClipStackFrame != mCanvas->getClipStack()->getSaveCount()) {
+        // Only reapply the saved clips if the top clip stack frame was actually
+        // popped by restore().  If it wasn't, it means it doesn't belong to the
+        // restored canvas frame (SkCanvas lazy save/restore kicked in).
         applyClips(savedClips);
     }
 
@@ -322,21 +331,23 @@ void SkiaCanvas::recordPartialSave(SkCanvas::SaveFlags flags) {
     }
 
     SaveRec* rec = static_cast<SaveRec*>(mSaveStack->push_back());
-    // Store the save counter in the SkClipStack domain.
-    // (0-based, equal to the number of save ops on the stack).
-    rec->saveCount = mCanvas->getSaveCount() - 1;
+    rec->saveCount = mCanvas->getSaveCount();
     rec->saveFlags = flags;
 }
 
-void SkiaCanvas::saveClipsForFrame(SkTArray<SkClipStack::Element>& clips, int frameSaveCount) {
+void SkiaCanvas::saveClipsForFrame(SkTArray<SkClipStack::Element>& clips,
+                                   int saveCountToBackup) {
+    // Each SkClipStack::Element stores the index of the canvas save
+    // with which it is associated. Backup only those Elements that
+    // are associated with 'saveCountToBackup'
     SkClipStack::Iter clipIterator(*mCanvas->getClipStack(),
                                    SkClipStack::Iter::kTop_IterStart);
-    while (const SkClipStack::Element* elem = clipIterator.next()) {
-        if (elem->getSaveCount() < frameSaveCount) {
-            // done with the current frame.
+    while (const SkClipStack::Element* elem = clipIterator.prev()) {
+        if (elem->getSaveCount() < saveCountToBackup) {
+            // done with the target save count.
             break;
         }
-        SkASSERT(elem->getSaveCount() == frameSaveCount);
+        SkASSERT(elem->getSaveCount() == saveCountToBackup);
         clips.push_back(*elem);
     }
 }