Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 49b403dc authored by Chris Craik's avatar Chris Craik
Browse files

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
parent 1e92e7fb

libs/hwui/BakedOpState.cpp

+9 −7
Original line number Diff line number Diff line
@@ -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

libs/hwui/BakedOpState.h

+5 −4
Original line number Diff line number Diff line
@@ -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) {}

libs/hwui/FrameBuilder.cpp

+8 −4
Original line number Diff line number Diff line
@@ -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));

libs/hwui/FrameBuilder.h

+2 −1
Original line number Diff line number Diff line
@@ -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.

libs/hwui/tests/unit/BakedOpStateTests.cpp

+8 −8
Original line number Diff line number Diff line
@@ -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);