Prevent EndLayerOps when Begin was rejected

bug:30537130

BeginLayerOps were being rejected in a way that allowed the associated
EndLayerOps to still be recorded. This was a violation of DisplayList
content expectations, and caused crashes in FrameBuilder when trying to
play these DisplayLists back.

Change-Id: I531b840aa5c4ffb1ee458da3f4b366978eaeafbe

diff --git a/libs/hwui/RecordingCanvas.cpp b/libs/hwui/RecordingCanvas.cpp
index 1802fd4..0c552ba 100644 (file)
--- a/libs/hwui/RecordingCanvas.cpp
+++ b/libs/hwui/RecordingCanvas.cpp
@@ -149,50 +149,53 @@ int RecordingCanvas::saveLayer(float left, float top, float right, float bottom,
 
     // Map visible bounds back to layer space, and intersect with parameter bounds
     Rect layerBounds = visibleBounds;
-    Matrix4 inverse;
-    inverse.loadInverse(*previous.transform);
-    inverse.mapRect(layerBounds);
-    layerBounds.doIntersect(unmappedBounds);
+    if (CC_LIKELY(!layerBounds.isEmpty())) {
+        // if non-empty, can safely map by the inverse transform
+        Matrix4 inverse;
+        inverse.loadInverse(*previous.transform);
+        inverse.mapRect(layerBounds);
+        layerBounds.doIntersect(unmappedBounds);
+    }
 
     int saveValue = mState.save((int) flags);
     Snapshot& snapshot = *mState.writableSnapshot();
 
     // layerBounds is in original bounds space, but clipped by current recording clip
-    if (layerBounds.isEmpty() || unmappedBounds.isEmpty()) {
-        // Don't bother recording layer, since it's been rejected
+    if (!layerBounds.isEmpty() && !unmappedBounds.isEmpty()) {
         if (CC_LIKELY(clippedLayer)) {
-            snapshot.resetClip(0, 0, 0, 0);
+            auto previousClip = getRecordedClip(); // capture before new snapshot clip has changed
+            if (addOp(alloc().create_trivial<BeginLayerOp>(
+                    unmappedBounds,
+                    *previous.transform, // transform to *draw* with
+                    previousClip, // clip to *draw* with
+                    refPaint(paint))) >= 0) {
+                snapshot.flags |= Snapshot::kFlagIsLayer | Snapshot::kFlagIsFboLayer;
+                snapshot.initializeViewport(unmappedBounds.getWidth(), unmappedBounds.getHeight());
+                snapshot.transform->loadTranslate(-unmappedBounds.left, -unmappedBounds.top, 0.0f);
+
+                Rect clip = layerBounds;
+                clip.translate(-unmappedBounds.left, -unmappedBounds.top);
+                snapshot.resetClip(clip.left, clip.top, clip.right, clip.bottom);
+                snapshot.roundRectClipState = nullptr;
+                return saveValue;
+            }
+        } else {
+            if (addOp(alloc().create_trivial<BeginUnclippedLayerOp>(
+                    unmappedBounds,
+                    *mState.currentSnapshot()->transform,
+                    getRecordedClip(),
+                    refPaint(paint))) >= 0) {
+                snapshot.flags |= Snapshot::kFlagIsLayer;
+                return saveValue;
+            }
         }
-        return saveValue;
     }
 
+    // Layer not needed, so skip recording it...
     if (CC_LIKELY(clippedLayer)) {
-        auto previousClip = getRecordedClip(); // note: done before new snapshot's clip has changed
-
-        snapshot.flags |= Snapshot::kFlagIsLayer | Snapshot::kFlagIsFboLayer;
-        snapshot.initializeViewport(unmappedBounds.getWidth(), unmappedBounds.getHeight());
-        snapshot.transform->loadTranslate(-unmappedBounds.left, -unmappedBounds.top, 0.0f);
-
-        Rect clip = layerBounds;
-        clip.translate(-unmappedBounds.left, -unmappedBounds.top);
-        snapshot.resetClip(clip.left, clip.top, clip.right, clip.bottom);
-        snapshot.roundRectClipState = nullptr;
-
-        addOp(alloc().create_trivial<BeginLayerOp>(
-                unmappedBounds,
-                *previous.transform, // transform to *draw* with
-                previousClip, // clip to *draw* with
-                refPaint(paint)));
-    } else {
-        snapshot.flags |= Snapshot::kFlagIsLayer;
-
-        addOp(alloc().create_trivial<BeginUnclippedLayerOp>(
-                unmappedBounds,
-                *mState.currentSnapshot()->transform,
-                getRecordedClip(),
-                refPaint(paint)));
+        // ... and set empty clip to reject inner content, if possible
+        snapshot.resetClip(0, 0, 0, 0);
     }
-
     return saveValue;
 }
 
@@ -619,7 +622,7 @@ void RecordingCanvas::callDrawGLFunction(Functor* functor,
             functor));
 }
 
-size_t RecordingCanvas::addOp(RecordedOp* op) {
+int RecordingCanvas::addOp(RecordedOp* op) {
     // skip op with empty clip
     if (op->localClip && op->localClip->rect.isEmpty()) {
         // NOTE: this rejection happens after op construction/content ref-ing, so content ref'd

diff --git a/libs/hwui/RecordingCanvas.h b/libs/hwui/RecordingCanvas.h
index 372be24..337e97b 100644 (file)
--- a/libs/hwui/RecordingCanvas.h
+++ b/libs/hwui/RecordingCanvas.h
@@ -208,7 +208,7 @@ private:
     void drawSimpleRects(const float* rects, int vertexCount, const SkPaint* paint);
 
 
-    size_t addOp(RecordedOp* op);
+    int addOp(RecordedOp* op);
 // ----------------------------------------------------------------------------
 // lazy object copy
 // ----------------------------------------------------------------------------

diff --git a/libs/hwui/tests/unit/RecordingCanvasTests.cpp b/libs/hwui/tests/unit/RecordingCanvasTests.cpp
index 28b375f..c072d0b 100644 (file)
--- a/libs/hwui/tests/unit/RecordingCanvasTests.cpp
+++ b/libs/hwui/tests/unit/RecordingCanvasTests.cpp
@@ -545,6 +545,21 @@ TEST(RecordingCanvas, saveLayer_rotateClipped) {
     EXPECT_EQ(3, count);
 }
 
+TEST(RecordingCanvas, saveLayer_rejectBegin) {
+    auto dl = TestUtils::createDisplayList<RecordingCanvas>(200, 200, [](RecordingCanvas& canvas) {
+        canvas.save(SaveFlags::MatrixClip);
+        canvas.translate(0, -20); // avoid identity case
+        // empty clip rect should force layer + contents to be rejected
+        canvas.clipRect(0, -20, 200, -20, SkRegion::kIntersect_Op);
+        canvas.saveLayerAlpha(0, 0, 200, 200, 128, SaveFlags::ClipToLayer);
+        canvas.drawRect(0, 0, 200, 200, SkPaint());
+        canvas.restore();
+        canvas.restore();
+    });
+
+    ASSERT_EQ(0u, dl->getOps().size()) << "Begin/Rect/End should all be rejected.";
+}
+
 TEST(RecordingCanvas, drawRenderNode_rejection) {
     auto child = TestUtils::createNode(50, 50, 150, 150,
             [](RenderProperties& props, RecordingCanvas& canvas) {