Workaround arc textures drawing outside of bounds

Fixes: 34077513
Test: hwui unit tests passing

This fixes an issue where drawArc operations would cause artifacts by
drawing outside of the clip / screen damage area. We now more
conservatively clip drawArc operations specifically, as they tend to
draw into the outer parts of their path textures more than other
operations.

A more long term fix would involve alignment between draw operation
sizing (in terms of what's resolved in a BakedOpState), and
PathTexture sizing (which currently conservatively expands beyond
stroked op bounds).

Change-Id: I5aff39cc04382323b457b159974032f5f371251a

6 files changed, 61 insertions, 24 deletions

diff --git a/libs/hwui/BakedOpState.cpp b/libs/hwui/BakedOpState.cpp
index 9f98241c6caa..9823a02dc847 100644
--- a/libs/hwui/BakedOpState.cpp
+++ b/libs/hwui/BakedOpState.cpp
@@ -31,7 +31,7 @@ static int computeClipSideFlags(const Rect& clip, const Rect& bounds) {
 }
 
 ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& snapshot,
-        const RecordedOp& recordedOp, bool expandForStroke) {
+        const RecordedOp& recordedOp, bool expandForStroke, bool expandForPathTexture) {
     // resolvedMatrix = parentMatrix * localMatrix
     transform.loadMultiply(*snapshot.transform, recordedOp.localMatrix);
 
@@ -40,6 +40,8 @@ ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& s
     if (CC_UNLIKELY(expandForStroke)) {
         // account for non-hairline stroke
         clippedBounds.outset(recordedOp.paint->getStrokeWidth() * 0.5f);
+    } else if (CC_UNLIKELY(expandForPathTexture)) {
+        clippedBounds.outset(1);
     }
     transform.mapRect(clippedBounds);
     if (CC_UNLIKELY(expandForStroke
@@ -111,7 +113,7 @@ BakedOpState* BakedOpState::tryConstruct(LinearAllocator& allocator,
         Snapshot& snapshot, const RecordedOp& recordedOp) {
     if (CC_UNLIKELY(snapshot.getRenderTargetClip().isEmpty())) return nullptr;
     BakedOpState* bakedState = allocator.create_trivial<BakedOpState>(
-            allocator, snapshot, recordedOp, false);
+            allocator, snapshot, recordedOp, false, false);
     if (bakedState->computedState.clippedBounds.isEmpty()) {
         // bounds are empty, so op is rejected
         allocator.rewindIfLastAlloc(bakedState);
@@ -127,14 +129,14 @@ BakedOpState* BakedOpState::tryConstructUnbounded(LinearAllocator& allocator,
 }
 
 BakedOpState* BakedOpState::tryStrokeableOpConstruct(LinearAllocator& allocator,
-        Snapshot& snapshot, const RecordedOp& recordedOp, StrokeBehavior strokeBehavior) {
+        Snapshot& snapshot, const RecordedOp& recordedOp, StrokeBehavior strokeBehavior,
+        bool expandForPathTexture) {
     if (CC_UNLIKELY(snapshot.getRenderTargetClip().isEmpty())) return nullptr;
-    bool expandForStroke = (strokeBehavior == StrokeBehavior::StyleDefined)
-            ? (recordedOp.paint && recordedOp.paint->getStyle() != SkPaint::kFill_Style)
-            : true;
+    bool expandForStroke = (strokeBehavior == StrokeBehavior::Forced
+            || (recordedOp.paint && recordedOp.paint->getStyle() != SkPaint::kFill_Style));
 
     BakedOpState* bakedState = allocator.create_trivial<BakedOpState>(
-           allocator, snapshot, recordedOp, expandForStroke);
+           allocator, snapshot, recordedOp, expandForStroke, expandForPathTexture);
     if (bakedState->computedState.clippedBounds.isEmpty()) {
         // bounds are empty, so op is rejected
         // NOTE: this won't succeed if a clip was allocated
diff --git a/libs/hwui/BakedOpState.h b/libs/hwui/BakedOpState.h
index e1441fca5ee2..7b0b34f3d9c0 100644
--- a/libs/hwui/BakedOpState.h
+++ b/libs/hwui/BakedOpState.h
@@ -53,7 +53,7 @@ struct MergedBakedOpList {
 class ResolvedRenderState {
 public:
     ResolvedRenderState(LinearAllocator& allocator, Snapshot& snapshot,
-            const RecordedOp& recordedOp, bool expandForStroke);
+            const RecordedOp& recordedOp, bool expandForStroke, bool expandForPathTexture);
 
     // Constructor for unbounded ops *with* transform/clip
     ResolvedRenderState(LinearAllocator& allocator, Snapshot& snapshot,
@@ -117,7 +117,8 @@ public:
     };
 
     static BakedOpState* tryStrokeableOpConstruct(LinearAllocator& allocator,
-            Snapshot& snapshot, const RecordedOp& recordedOp, StrokeBehavior strokeBehavior);
+            Snapshot& snapshot, const RecordedOp& recordedOp, StrokeBehavior strokeBehavior,
+            bool expandForPathTexture);
 
     static BakedOpState* tryShadowOpConstruct(LinearAllocator& allocator,
             Snapshot& snapshot, const ShadowOp* shadowOpPtr);
@@ -140,8 +141,8 @@ private:
     friend class LinearAllocator;
 
     BakedOpState(LinearAllocator& allocator, Snapshot& snapshot,
-            const RecordedOp& recordedOp, bool expandForStroke)
-            : computedState(allocator, snapshot, recordedOp, expandForStroke)
+            const RecordedOp& recordedOp, bool expandForStroke, bool expandForPathTexture)
+            : computedState(allocator, snapshot, recordedOp, expandForStroke, expandForPathTexture)
             , alpha(snapshot.alpha)
             , roundRectClipState(snapshot.roundRectClipState)
             , op(&recordedOp) {}
diff --git a/libs/hwui/FrameBuilder.cpp b/libs/hwui/FrameBuilder.cpp
index 1b57e290c198..86f9a5d73fd1 100644
--- a/libs/hwui/FrameBuilder.cpp
+++ b/libs/hwui/FrameBuilder.cpp
@@ -562,10 +562,11 @@ void FrameBuilder::deferRenderNodeOp(const RenderNodeOp& op) {
  * for paint's style on the bounds being computed.
  */
 BakedOpState* FrameBuilder::deferStrokeableOp(const RecordedOp& op, batchid_t batchId,
-        BakedOpState::StrokeBehavior strokeBehavior) {
+        BakedOpState::StrokeBehavior strokeBehavior, bool expandForPathTexture) {
     // Note: here we account for stroke when baking the op
     BakedOpState* bakedState = BakedOpState::tryStrokeableOpConstruct(
-            mAllocator, *mCanvasState.writableSnapshot(), op, strokeBehavior);
+            mAllocator, *mCanvasState.writableSnapshot(), op,
+            strokeBehavior, expandForPathTexture);
     if (!bakedState) return nullptr; // quick rejected
 
     if (op.opId == RecordedOpId::RectOp && op.paint->getStyle() != SkPaint::kStroke_Style) {
@@ -590,7 +591,10 @@ static batchid_t tessBatchId(const RecordedOp& op) {
 }
 
 void FrameBuilder::deferArcOp(const ArcOp& op) {
-    deferStrokeableOp(op, tessBatchId(op));
+    // Pass true below since arcs have a tendency to draw outside their expected bounds within
+    // their path textures. Passing true makes it more likely that we'll scissor, instead of
+    // corrupting the frame by drawing outside of clip bounds.
+    deferStrokeableOp(op, tessBatchId(op), BakedOpState::StrokeBehavior::StyleDefined, true);
 }
 
 static bool hasMergeableClip(const BakedOpState& state) {
@@ -748,7 +752,7 @@ static batchid_t textBatchId(const SkPaint& paint) {
 void FrameBuilder::deferTextOp(const TextOp& op) {
     BakedOpState* bakedState = BakedOpState::tryStrokeableOpConstruct(
             mAllocator, *mCanvasState.writableSnapshot(), op,
-            BakedOpState::StrokeBehavior::StyleDefined);
+            BakedOpState::StrokeBehavior::StyleDefined, false);
     if (!bakedState) return; // quick rejected
 
     batchid_t batchId = textBatchId(*(op.paint));
diff --git a/libs/hwui/FrameBuilder.h b/libs/hwui/FrameBuilder.h
index b9154435c1e5..46048f7125ce 100644
--- a/libs/hwui/FrameBuilder.h
+++ b/libs/hwui/FrameBuilder.h
@@ -211,7 +211,8 @@ private:
     }
 
     BakedOpState* deferStrokeableOp(const RecordedOp& op, batchid_t batchId,
-            BakedOpState::StrokeBehavior strokeBehavior = BakedOpState::StrokeBehavior::StyleDefined);
+            BakedOpState::StrokeBehavior strokeBehavior = BakedOpState::StrokeBehavior::StyleDefined,
+            bool expandForPathTexture = false);
 
     /**
      * Declares all FrameBuilder::deferXXXXOp() methods for every RecordedOp type.
diff --git a/libs/hwui/tests/unit/BakedOpStateTests.cpp b/libs/hwui/tests/unit/BakedOpStateTests.cpp
index 0f8e0471556f..d51db2ebb169 100644
--- a/libs/hwui/tests/unit/BakedOpStateTests.cpp
+++ b/libs/hwui/tests/unit/BakedOpStateTests.cpp
@@ -35,7 +35,7 @@ TEST(ResolvedRenderState, construct) {
     {
         // recorded with transform, no parent transform
         auto parentSnapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));
-        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, false);
+        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, false, false);
         EXPECT_MATRIX_APPROX_EQ(state.transform, translate10x20);
         EXPECT_EQ(Rect(100, 200), state.clipRect());
         EXPECT_EQ(Rect(40, 60, 100, 200), state.clippedBounds); // translated and also clipped
@@ -44,7 +44,7 @@ TEST(ResolvedRenderState, construct) {
     {
         // recorded with transform and parent transform
         auto parentSnapshot = TestUtils::makeSnapshot(translate10x20, Rect(100, 200));
-        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, false);
+        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, false, false);
 
         Matrix4 expectedTranslate;
         expectedTranslate.loadTranslate(20, 40, 0);
@@ -70,14 +70,14 @@ TEST(ResolvedRenderState, computeLocalSpaceClip) {
     {
         // recorded with transform, no parent transform
         auto parentSnapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));
-        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, false);
+        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, false, false);
         EXPECT_EQ(Rect(-10, -20, 90, 180), state.computeLocalSpaceClip())
             << "Local clip rect should be 100x200, offset by -10,-20";
     }
     {
         // recorded with transform + parent transform
         auto parentSnapshot = TestUtils::makeSnapshot(translate10x20, Rect(100, 200));
-        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, false);
+        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, false, false);
         EXPECT_EQ(Rect(-10, -20, 80, 160), state.computeLocalSpaceClip())
             << "Local clip rect should be 90x190, offset by -10,-20";
     }
@@ -170,7 +170,7 @@ TEST(ResolvedRenderState, construct_expandForStroke) {
         snapshotMatrix.loadScale(testCase.scale, testCase.scale, 1);
         auto parentSnapshot = TestUtils::makeSnapshot(snapshotMatrix, Rect(200, 200));
 
-        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, true);
+        ResolvedRenderState state(allocator, *parentSnapshot, recordedOp, true, false);
         testCase.validator(state);
     }
 }
@@ -234,7 +234,7 @@ TEST(BakedOpState, tryStrokeableOpConstruct) {
         RectOp rejectOp(Rect(100, 200), Matrix4::identity(), &clip, &paint);
         auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect()); // Note: empty clip
         auto bakedState = BakedOpState::tryStrokeableOpConstruct(allocator, *snapshot, rejectOp,
-                BakedOpState::StrokeBehavior::StyleDefined);
+                BakedOpState::StrokeBehavior::StyleDefined, false);
 
         EXPECT_EQ(nullptr, bakedState);
         EXPECT_GT(8u, allocator.usedSize()); // no significant allocation space used for rejected op
@@ -248,7 +248,7 @@ TEST(BakedOpState, tryStrokeableOpConstruct) {
         RectOp rejectOp(Rect(50, 50, 150, 150), Matrix4::identity(), &clip, &paint);
         auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(200, 200));
         auto bakedState = BakedOpState::tryStrokeableOpConstruct(allocator, *snapshot, rejectOp,
-                BakedOpState::StrokeBehavior::StyleDefined);
+                BakedOpState::StrokeBehavior::StyleDefined, false);
 
         ASSERT_NE(nullptr, bakedState);
         EXPECT_EQ(Rect(45, 45, 155, 155), bakedState->computedState.clippedBounds);
@@ -263,7 +263,7 @@ TEST(BakedOpState, tryStrokeableOpConstruct) {
         RectOp rejectOp(Rect(50, 50, 150, 150), Matrix4::identity(), &clip, &paint);
         auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(200, 200));
         auto bakedState = BakedOpState::tryStrokeableOpConstruct(allocator, *snapshot, rejectOp,
-                BakedOpState::StrokeBehavior::Forced);
+                BakedOpState::StrokeBehavior::Forced, false);
 
         ASSERT_NE(nullptr, bakedState);
         EXPECT_EQ(Rect(45, 45, 155, 155), bakedState->computedState.clippedBounds);
diff --git a/libs/hwui/tests/unit/FrameBuilderTests.cpp b/libs/hwui/tests/unit/FrameBuilderTests.cpp
index 95d9459e898c..a329980b7609 100644
--- a/libs/hwui/tests/unit/FrameBuilderTests.cpp
+++ b/libs/hwui/tests/unit/FrameBuilderTests.cpp
@@ -171,6 +171,35 @@ RENDERTHREAD_OPENGL_PIPELINE_TEST(FrameBuilder, simpleStroke) {
     EXPECT_EQ(1, renderer.getIndex());
 }
 
+
+RENDERTHREAD_OPENGL_PIPELINE_TEST(FrameBuilder, arcStrokeClip) {
+    class ArcStrokeClipTestRenderer : public TestRendererBase {
+    public:
+        void onArcOp(const ArcOp& op, const BakedOpState& state) override {
+            EXPECT_EQ(0, mIndex++);
+            EXPECT_EQ(Rect(25, 25, 175, 175), op.unmappedBounds);
+            EXPECT_EQ(Rect(25, 25, 175, 175), state.computedState.clippedBounds);
+            EXPECT_EQ(OpClipSideFlags::Full, state.computedState.clipSideFlags)
+                    << "Arc op clipped conservatively, since path texture may be expanded";
+        }
+    };
+
+    auto node = TestUtils::createNode<RecordingCanvas>(0, 0, 200, 200,
+            [](RenderProperties& props, RecordingCanvas& canvas) {
+        canvas.clipRect(25, 25, 175, 175, SkClipOp::kIntersect);
+        SkPaint aaPaint;
+        aaPaint.setAntiAlias(true);
+        canvas.drawArc(25, 25, 175, 175, 40, 180, true, aaPaint);
+    });
+    FrameBuilder frameBuilder(SkRect::MakeWH(200, 200), 200, 200,
+            sLightGeometry, Caches::getInstance());
+    frameBuilder.deferRenderNode(*TestUtils::getSyncedNode(node));
+
+    ArcStrokeClipTestRenderer renderer;
+    frameBuilder.replayBakedOps<TestDispatcher>(renderer);
+    EXPECT_EQ(1, renderer.getIndex());
+}
+
 RENDERTHREAD_OPENGL_PIPELINE_TEST(FrameBuilder, simpleRejection) {
     auto node = TestUtils::createNode<RecordingCanvas>(0, 0, 200, 200,
             [](RenderProperties& props, RecordingCanvas& canvas) {