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

Commit 15f04686 authored by Chris Craik's avatar Chris Craik
Browse files

Fix clip serialization crash 

Can't safely rewind clip allocations, since those pointers are cached by
ClipArea. Instead add early rejection to handle most cases, and update
tests.

Change-Id: Ic32f95cf95602f427f25761a8da1583c4495f36d
parent 5ea1724b

libs/hwui/BakedOpRenderer.cpp

+2 −0
Original line number Diff line number Diff line
@@ -201,6 +201,7 @@ void BakedOpRenderer::setupStencilQuads(std::vector<Vertex>& quadVertices, 
}



void BakedOpRenderer::setupStencilRectList(const ClipBase* clip) {

    LOG_ALWAYS_FATAL_IF(clip->mode != ClipMode::RectangleList, "can't rectlist clip without rectlist");

    auto&& rectList = reinterpret_cast<const ClipRectList*>(clip)->rectList;

    int quadCount = rectList.getTransformedRectanglesCount();

    std::vector<Vertex> rectangleVertices;
@@ -234,6 +235,7 @@ void BakedOpRenderer::setupStencilRectList(const ClipBase* clip) { 
}



void BakedOpRenderer::setupStencilRegion(const ClipBase* clip) {

    LOG_ALWAYS_FATAL_IF(clip->mode != ClipMode::Region, "can't region clip without region");

    auto&& region = reinterpret_cast<const ClipRegion*>(clip)->region;



    std::vector<Vertex> regionVertices;

libs/hwui/BakedOpState.cpp

+4 −4
Original line number Diff line number Diff line
@@ -48,10 +48,10 @@ ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& s 
    const Rect& clipRect = clipState->rect;

    if (CC_UNLIKELY(clipRect.isEmpty() || !clippedBounds.intersects(clipRect))) {

        // Rejected based on either empty clip, or bounds not intersecting with clip

        if (clipState) {

            allocator.rewindIfLastAlloc(clipState);



        // Note: we could rewind the clipState object in situations where the clipRect is empty,

        // but *only* if the caching logic within ClipArea was aware of the rewind.

        clipState = nullptr;

        }

        clippedBounds.setEmpty();

    } else {

        // Not rejected! compute true clippedBounds and clipSideFlags

libs/hwui/BakedOpState.h

+4 −1
Original line number Diff line number Diff line
@@ -99,6 +99,7 @@ class BakedOpState { 
public:

    static BakedOpState* tryConstruct(LinearAllocator& allocator,

            Snapshot& snapshot, const RecordedOp& recordedOp) {

        if (CC_UNLIKELY(snapshot.getRenderTargetClip().isEmpty())) return nullptr;

        BakedOpState* bakedState = new (allocator) BakedOpState(

                allocator, snapshot, recordedOp, false);

        if (bakedState->computedState.clippedBounds.isEmpty()) {
@@ -118,6 +119,7 @@ public: 


    static BakedOpState* tryStrokeableOpConstruct(LinearAllocator& allocator,

            Snapshot& snapshot, const RecordedOp& recordedOp, StrokeBehavior strokeBehavior) {

        if (CC_UNLIKELY(snapshot.getRenderTargetClip().isEmpty())) return nullptr;

        bool expandForStroke = (strokeBehavior == StrokeBehavior::StyleDefined)

                ? (recordedOp.paint && recordedOp.paint->getStyle() != SkPaint::kFill_Style)

                : true;
@@ -126,6 +128,7 @@ public: 
                allocator, snapshot, recordedOp, expandForStroke);

        if (bakedState->computedState.clippedBounds.isEmpty()) {

            // bounds are empty, so op is rejected

            // NOTE: this won't succeed if a clip was allocated

            allocator.rewindIfLastAlloc(bakedState);

            return nullptr;

        }
@@ -134,7 +137,7 @@ public: 


    static BakedOpState* tryShadowOpConstruct(LinearAllocator& allocator,

            Snapshot& snapshot, const ShadowOp* shadowOpPtr) {

        if (snapshot.getRenderTargetClip().isEmpty()) return nullptr;

        if (CC_UNLIKELY(snapshot.getRenderTargetClip().isEmpty())) return nullptr;



        // clip isn't empty, so construct the op

        return new (allocator) BakedOpState(allocator, snapshot, shadowOpPtr);

libs/hwui/tests/unit/BakedOpStateTests.cpp

+19 −21
Original line number Diff line number Diff line
@@ -176,29 +176,26 @@ TEST(ResolvedRenderState, construct_expandForStroke) { 
}



TEST(BakedOpState, tryConstruct) {

    LinearAllocator allocator;



    Matrix4 translate100x0;

    translate100x0.loadTranslate(100, 0, 0);



    SkPaint paint;

    ClipRect clip(Rect(100, 200));

    {

        RectOp rejectOp(Rect(30, 40, 100, 200), translate100x0, &clip, &paint);

        auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));

        BakedOpState* bakedState = BakedOpState::tryConstruct(allocator, *snapshot, rejectOp);



        EXPECT_EQ(nullptr, bakedState); // rejected by clip, so not constructed

        EXPECT_GT(8u, allocator.usedSize()); // no significant allocation space used for rejected op

    }

    {

    LinearAllocator allocator;

    RectOp successOp(Rect(30, 40, 100, 200), Matrix4::identity(), &clip, &paint);

    auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));

        BakedOpState* bakedState = BakedOpState::tryConstruct(allocator, *snapshot, successOp);

    EXPECT_NE(nullptr, BakedOpState::tryConstruct(allocator, *snapshot, successOp))

            << "successOp NOT rejected by clip, so should be constructed";

    size_t successAllocSize = allocator.usedSize();

    EXPECT_LE(64u, successAllocSize) << "relatively large alloc for non-rejected op";



        EXPECT_NE(nullptr, bakedState); // NOT rejected by clip, so will be constructed

        EXPECT_LE(64u, allocator.usedSize()); // relatively large alloc for non-rejected op

    }

    RectOp rejectOp(Rect(30, 40, 100, 200), translate100x0, &clip, &paint);

    EXPECT_EQ(nullptr, BakedOpState::tryConstruct(allocator, *snapshot, rejectOp))

            << "rejectOp rejected by clip, so should not be constructed";



    // NOTE: this relies on the clip having already been serialized by the op above

    EXPECT_EQ(successAllocSize, allocator.usedSize()) << "no extra allocation used for rejected op";

}



TEST(BakedOpState, tryShadowOpConstruct) {
@@ -207,15 +204,16 @@ TEST(BakedOpState, tryShadowOpConstruct) { 
        auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect()); // Note: empty clip

        BakedOpState* bakedState = BakedOpState::tryShadowOpConstruct(allocator, *snapshot, (ShadowOp*)0x1234);



        EXPECT_EQ(nullptr, bakedState); // rejected by clip, so not constructed

        EXPECT_GT(8u, allocator.usedSize()); // no significant allocation space used for rejected op

        EXPECT_EQ(nullptr, bakedState) << "op should be rejected by clip, so not constructed";

        EXPECT_EQ(0u, allocator.usedSize()) << "no serialization, even for clip,"

                "since op is quick rejected based on snapshot clip";

    }

    {

        auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));

        BakedOpState* bakedState = BakedOpState::tryShadowOpConstruct(allocator, *snapshot, (ShadowOp*)0x1234);



        ASSERT_NE(nullptr, bakedState); // NOT rejected by clip, so will be constructed

        EXPECT_LE(64u, allocator.usedSize()); // relatively large alloc for non-rejected op

        ASSERT_NE(nullptr, bakedState) << "NOT rejected by clip, so op should be constructed";

        EXPECT_LE(64u, allocator.usedSize()) << "relatively large alloc for non-rejected op";

    }

}