Properly reject empty unclipped savelayers

bug:27225580
bug:27281241

Empty unclipped savelayers (clipped at defer time, often by dirty rect)
were resulting in invalid layer clear rectangles. Simplify by just
rejecting these unclipped savelayers entirely at defer.

Also, use repaint rect as base clip for constructed ops within
LayerBuilder.

Change-Id: I5c466199e85201a2f68f5cdc60b29187c849961b

7 files changed, 111 insertions, 40 deletions

diff --git a/libs/hwui/BakedOpRenderer.cpp b/libs/hwui/BakedOpRenderer.cpp
index 35c8f6b52d38..5f689b48a844 100644
--- a/libs/hwui/BakedOpRenderer.cpp
+++ b/libs/hwui/BakedOpRenderer.cpp
@@ -347,7 +347,7 @@ void BakedOpRenderer::dirtyRenderTarget(const Rect& uiDirty) {
         if (valid) {
             dirty = android::Rect(uiDirty.left, uiDirty.top, uiDirty.right, uiDirty.bottom);
         } else {
-            dirty = android::Rect(0, 0,
+            dirty = android::Rect(
                     mRenderTarget.offscreenBuffer->viewportWidth,
                     mRenderTarget.offscreenBuffer->viewportHeight);
         }
diff --git a/libs/hwui/BakedOpState.cpp b/libs/hwui/BakedOpState.cpp
index a542c26b41b8..682bd045098d 100644
--- a/libs/hwui/BakedOpState.cpp
+++ b/libs/hwui/BakedOpState.cpp
@@ -21,6 +21,15 @@
 namespace android {
 namespace uirenderer {
 
+static int computeClipSideFlags(const Rect& clip, const Rect& bounds) {
+    int clipSideFlags = 0;
+    if (clip.left > bounds.left) clipSideFlags |= OpClipSideFlags::Left;
+    if (clip.top > bounds.top) clipSideFlags |= OpClipSideFlags::Top;
+    if (clip.right < bounds.right) clipSideFlags |= OpClipSideFlags::Right;
+    if (clip.bottom < bounds.bottom) clipSideFlags |= OpClipSideFlags::Bottom;
+    return clipSideFlags;
+}
+
 ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& snapshot,
         const RecordedOp& recordedOp, bool expandForStroke) {
     // resolvedMatrix = parentMatrix * localMatrix
@@ -55,10 +64,7 @@ ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& s
         clippedBounds.setEmpty();
     } else {
         // Not rejected! compute true clippedBounds and clipSideFlags
-        if (clipRect.left > clippedBounds.left) clipSideFlags |= OpClipSideFlags::Left;
-        if (clipRect.top > clippedBounds.top) clipSideFlags |= OpClipSideFlags::Top;
-        if (clipRect.right < clippedBounds.right) clipSideFlags |= OpClipSideFlags::Right;
-        if (clipRect.bottom < clippedBounds.bottom) clipSideFlags |= OpClipSideFlags::Bottom;
+        clipSideFlags = computeClipSideFlags(clipRect, clippedBounds);
         clippedBounds.doIntersect(clipRect);
     }
 }
@@ -69,11 +75,13 @@ ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& s
         , clippedBounds(clipState->rect)
         , clipSideFlags(OpClipSideFlags::Full) {}
 
-ResolvedRenderState::ResolvedRenderState(const ClipRect* viewportRect, const Rect& dstRect)
+ResolvedRenderState::ResolvedRenderState(const ClipRect* clipRect, const Rect& dstRect)
         : transform(Matrix4::identity())
-        , clipState(viewportRect)
+        , clipState(clipRect)
         , clippedBounds(dstRect)
-        , clipSideFlags(OpClipSideFlags::None) {}
+        , clipSideFlags(computeClipSideFlags(clipRect->rect, dstRect)) {
+    clippedBounds.doIntersect(clipRect->rect);
+}
 
 } // namespace uirenderer
 } // namespace android
diff --git a/libs/hwui/BakedOpState.h b/libs/hwui/BakedOpState.h
index 5a5845af81b9..4365ef870dda 100644
--- a/libs/hwui/BakedOpState.h
+++ b/libs/hwui/BakedOpState.h
@@ -175,8 +175,8 @@ private:
             , projectionPathMask(snapshot.projectionPathMask)
             , op(shadowOpPtr) {}
 
-    BakedOpState(const ClipRect* viewportRect, const Rect& dstRect, const RecordedOp& recordedOp)
-            : computedState(viewportRect, dstRect)
+    BakedOpState(const ClipRect* clipRect, const Rect& dstRect, const RecordedOp& recordedOp)
+            : computedState(clipRect, dstRect)
             , alpha(1.0f)
             , roundRectClipState(nullptr)
             , projectionPathMask(nullptr)
diff --git a/libs/hwui/FrameBuilder.cpp b/libs/hwui/FrameBuilder.cpp
index 4f51036b336e..04de98afe85c 100644
--- a/libs/hwui/FrameBuilder.cpp
+++ b/libs/hwui/FrameBuilder.cpp
@@ -782,39 +782,46 @@ void FrameBuilder::deferBeginUnclippedLayerOp(const BeginUnclippedLayerOp& op) {
     boundsTransform.mapRect(dstRect);
     dstRect.doIntersect(mCanvasState.currentSnapshot()->getRenderTargetClip());
 
-    // Allocate a holding position for the layer object (copyTo will produce, copyFrom will consume)
-    OffscreenBuffer** layerHandle = mAllocator.create<OffscreenBuffer*>(nullptr);
-
-    /**
-     * First, defer an operation to copy out the content from the rendertarget into a layer.
-     */
-    auto copyToOp = mAllocator.create_trivial<CopyToLayerOp>(op, layerHandle);
-    BakedOpState* bakedState = BakedOpState::directConstruct(mAllocator,
-            &(currentLayer().viewportClip), dstRect, *copyToOp);
-    currentLayer().deferUnmergeableOp(mAllocator, bakedState, OpBatchType::CopyToLayer);
-
-    /**
-     * Defer a clear rect, so that clears from multiple unclipped layers can be drawn
-     * both 1) simultaneously, and 2) as long after the copyToLayer executes as possible
-     */
-    currentLayer().deferLayerClear(dstRect);
-
-    /**
-     * And stash an operation to copy that layer back under the rendertarget until
-     * a balanced EndUnclippedLayerOp is seen
-     */
-    auto copyFromOp = mAllocator.create_trivial<CopyFromLayerOp>(op, layerHandle);
-    bakedState = BakedOpState::directConstruct(mAllocator,
-            &(currentLayer().viewportClip), dstRect, *copyFromOp);
-    currentLayer().activeUnclippedSaveLayers.push_back(bakedState);
+    if (dstRect.isEmpty()) {
+        // Unclipped layer rejected - push a null op, so next EndUnclippedLayerOp is ignored
+        currentLayer().activeUnclippedSaveLayers.push_back(nullptr);
+    } else {
+        // Allocate a holding position for the layer object (copyTo will produce, copyFrom will consume)
+        OffscreenBuffer** layerHandle = mAllocator.create<OffscreenBuffer*>(nullptr);
+
+        /**
+         * First, defer an operation to copy out the content from the rendertarget into a layer.
+         */
+        auto copyToOp = mAllocator.create_trivial<CopyToLayerOp>(op, layerHandle);
+        BakedOpState* bakedState = BakedOpState::directConstruct(mAllocator,
+                &(currentLayer().repaintClip), dstRect, *copyToOp);
+        currentLayer().deferUnmergeableOp(mAllocator, bakedState, OpBatchType::CopyToLayer);
+
+        /**
+         * Defer a clear rect, so that clears from multiple unclipped layers can be drawn
+         * both 1) simultaneously, and 2) as long after the copyToLayer executes as possible
+         */
+        currentLayer().deferLayerClear(dstRect);
+
+        /**
+         * And stash an operation to copy that layer back under the rendertarget until
+         * a balanced EndUnclippedLayerOp is seen
+         */
+        auto copyFromOp = mAllocator.create_trivial<CopyFromLayerOp>(op, layerHandle);
+        bakedState = BakedOpState::directConstruct(mAllocator,
+                &(currentLayer().repaintClip), dstRect, *copyFromOp);
+        currentLayer().activeUnclippedSaveLayers.push_back(bakedState);
+    }
 }
 
 void FrameBuilder::deferEndUnclippedLayerOp(const EndUnclippedLayerOp& /* ignored */) {
     LOG_ALWAYS_FATAL_IF(currentLayer().activeUnclippedSaveLayers.empty(), "no layer to end!");
 
     BakedOpState* copyFromLayerOp = currentLayer().activeUnclippedSaveLayers.back();
-    currentLayer().deferUnmergeableOp(mAllocator, copyFromLayerOp, OpBatchType::CopyFromLayer);
     currentLayer().activeUnclippedSaveLayers.pop_back();
+    if (copyFromLayerOp) {
+        currentLayer().deferUnmergeableOp(mAllocator, copyFromLayerOp, OpBatchType::CopyFromLayer);
+    }
 }
 
 } // namespace uirenderer
diff --git a/libs/hwui/LayerBuilder.cpp b/libs/hwui/LayerBuilder.cpp
index c5d7191f1b62..bc39621f2cb2 100644
--- a/libs/hwui/LayerBuilder.cpp
+++ b/libs/hwui/LayerBuilder.cpp
@@ -199,10 +199,10 @@ LayerBuilder::LayerBuilder(uint32_t width, uint32_t height,
         : width(width)
         , height(height)
         , repaintRect(repaintRect)
+        , repaintClip(repaintRect)
         , offscreenBuffer(renderNode ? renderNode->getLayer() : nullptr)
         , beginLayerOp(beginLayerOp)
-        , renderNode(renderNode)
-        , viewportClip(Rect(width, height)) {}
+        , renderNode(renderNode) {}
 
 // iterate back toward target to see if anything drawn since should overlap the new op
 // if no target, merging ops still iterate to find similar batch to insert after
@@ -260,7 +260,7 @@ void LayerBuilder::flushLayerClears(LinearAllocator& allocator) {
                 Matrix4::identity(), nullptr, paint,
                 verts, vertCount);
         BakedOpState* bakedState = BakedOpState::directConstruct(allocator,
-                &viewportClip, bounds, *op);
+                &repaintClip, bounds, *op);
         deferUnmergeableOp(allocator, bakedState, OpBatchType::Vertices);
     }
 }
diff --git a/libs/hwui/LayerBuilder.h b/libs/hwui/LayerBuilder.h
index 99968e1750c8..4a7ca2de9b1b 100644
--- a/libs/hwui/LayerBuilder.h
+++ b/libs/hwui/LayerBuilder.h
@@ -109,10 +109,10 @@ public:
     const uint32_t width;
     const uint32_t height;
     const Rect repaintRect;
+    const ClipRect repaintClip;
     OffscreenBuffer* offscreenBuffer;
     const BeginLayerOp* beginLayerOp;
     const RenderNode* renderNode;
-    const ClipRect viewportClip;
 
     // list of deferred CopyFromLayer ops, to be deferred upon encountering EndUnclippedLayerOps
     std::vector<BakedOpState*> activeUnclippedSaveLayers;
diff --git a/libs/hwui/tests/unit/FrameBuilderTests.cpp b/libs/hwui/tests/unit/FrameBuilderTests.cpp
index f49dd3f6bc81..f86898fd669a 100644
--- a/libs/hwui/tests/unit/FrameBuilderTests.cpp
+++ b/libs/hwui/tests/unit/FrameBuilderTests.cpp
@@ -651,6 +651,62 @@ TEST(FrameBuilder, saveLayerUnclipped_mergedClears) {
             << "Expect 4 copyTos, 4 copyFroms, 1 clear SimpleRects, and 1 rect.";
 }
 
+TEST(FrameBuilder, saveLayerUnclipped_clearClip) {
+    class SaveLayerUnclippedClearClipTestRenderer : public TestRendererBase {
+    public:
+        void onCopyToLayerOp(const CopyToLayerOp& op, const BakedOpState& state) override {
+            EXPECT_EQ(0, mIndex++);
+        }
+        void onSimpleRectsOp(const SimpleRectsOp& op, const BakedOpState& state) override {
+            EXPECT_EQ(1, mIndex++);
+            ASSERT_NE(nullptr, op.paint);
+            EXPECT_EQ(SkXfermode::kClear_Mode, PaintUtils::getXfermodeDirect(op.paint));
+            EXPECT_EQ(Rect(50, 50, 150, 150), state.computedState.clippedBounds)
+                    << "Expect dirty rect as clip";
+            ASSERT_NE(nullptr, state.computedState.clipState);
+            EXPECT_EQ(Rect(50, 50, 150, 150), state.computedState.clipState->rect);
+            EXPECT_EQ(ClipMode::Rectangle, state.computedState.clipState->mode);
+        }
+        void onRectOp(const RectOp& op, const BakedOpState& state) override {
+            EXPECT_EQ(2, mIndex++);
+        }
+        void onCopyFromLayerOp(const CopyFromLayerOp& op, const BakedOpState& state) override {
+            EXPECT_EQ(3, mIndex++);
+        }
+    };
+
+    auto node = TestUtils::createNode(0, 0, 200, 200,
+            [](RenderProperties& props, RecordingCanvas& canvas) {
+        // save smaller than clip, so we get unclipped behavior
+        canvas.saveLayerAlpha(10, 10, 190, 190, 128, (SaveFlags::Flags)(0));
+        canvas.drawRect(0, 0, 200, 200, SkPaint());
+        canvas.restore();
+    });
+
+    // draw with partial screen dirty, and assert we see that rect later
+    FrameBuilder frameBuilder(sEmptyLayerUpdateQueue, SkRect::MakeLTRB(50, 50, 150, 150), 200, 200,
+            TestUtils::createSyncedNodeList(node), sLightGeometry, nullptr);
+    SaveLayerUnclippedClearClipTestRenderer renderer;
+    frameBuilder.replayBakedOps<TestDispatcher>(renderer);
+    EXPECT_EQ(4, renderer.getIndex());
+}
+
+TEST(FrameBuilder, saveLayerUnclipped_reject) {
+    auto node = TestUtils::createNode(0, 0, 200, 200,
+            [](RenderProperties& props, RecordingCanvas& canvas) {
+        // unclipped savelayer + rect both in area that won't intersect with dirty
+        canvas.saveLayerAlpha(100, 100, 200, 200, 128, (SaveFlags::Flags)(0));
+        canvas.drawRect(100, 100, 200, 200, SkPaint());
+        canvas.restore();
+    });
+
+    // draw with partial screen dirty that doesn't intersect with savelayer
+    FrameBuilder frameBuilder(sEmptyLayerUpdateQueue, SkRect::MakeWH(100, 100), 200, 200,
+            TestUtils::createSyncedNodeList(node), sLightGeometry, nullptr);
+    FailRenderer renderer;
+    frameBuilder.replayBakedOps<TestDispatcher>(renderer);
+}
+
 /* saveLayerUnclipped { saveLayer { saveLayerUnclipped { rect } } } will play back as:
  * - startTemporaryLayer, onCopyToLayer, onSimpleRects, onRect, onCopyFromLayer, endLayer
  * - startFrame, onCopyToLayer, onSimpleRects, drawLayer, onCopyFromLayer, endframe