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
diff --git a/libs/hwui/BakedOpState.cpp b/libs/hwui/BakedOpState.cpp
index 9f98241..9823a02 100644
--- a/libs/hwui/BakedOpState.cpp
+++ b/libs/hwui/BakedOpState.cpp
@@ -31,7 +31,7 @@
}
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 @@
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 @@
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::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 e1441fc..7b0b34f 100644
--- a/libs/hwui/BakedOpState.h
+++ b/libs/hwui/BakedOpState.h
@@ -53,7 +53,7 @@
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 @@
};
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 @@
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 1b57e29..86f9a5d 100644
--- a/libs/hwui/FrameBuilder.cpp
+++ b/libs/hwui/FrameBuilder.cpp
@@ -562,10 +562,11 @@
* 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 @@
}
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 @@
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 b915443..46048f7 100644
--- a/libs/hwui/FrameBuilder.h
+++ b/libs/hwui/FrameBuilder.h
@@ -211,7 +211,8 @@
}
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 0f8e047..d51db2e 100644
--- a/libs/hwui/tests/unit/BakedOpStateTests.cpp
+++ b/libs/hwui/tests/unit/BakedOpStateTests.cpp
@@ -35,7 +35,7 @@
{
// 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 @@
{
// 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 @@
{
// 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 @@
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 @@
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 @@
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 @@
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 95d9459..a329980 100644
--- a/libs/hwui/tests/unit/FrameBuilderTests.cpp
+++ b/libs/hwui/tests/unit/FrameBuilderTests.cpp
@@ -171,6 +171,35 @@
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) {