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
diff --git a/libs/hwui/BakedOpRenderer.cpp b/libs/hwui/BakedOpRenderer.cpp
index b9c13e6..c1f19a3 100644
--- a/libs/hwui/BakedOpRenderer.cpp
+++ b/libs/hwui/BakedOpRenderer.cpp
@@ -201,6 +201,7 @@
}
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::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;
diff --git a/libs/hwui/BakedOpState.cpp b/libs/hwui/BakedOpState.cpp
index f1cc846..f0406fa 100644
--- a/libs/hwui/BakedOpState.cpp
+++ b/libs/hwui/BakedOpState.cpp
@@ -48,10 +48,10 @@
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);
- clipState = nullptr;
- }
+
+ // 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
diff --git a/libs/hwui/BakedOpState.h b/libs/hwui/BakedOpState.h
index 5c7b43f..3db28c9 100644
--- a/libs/hwui/BakedOpState.h
+++ b/libs/hwui/BakedOpState.h
@@ -99,6 +99,7 @@
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 @@
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 @@
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 @@
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);
diff --git a/libs/hwui/tests/unit/BakedOpStateTests.cpp b/libs/hwui/tests/unit/BakedOpStateTests.cpp
index 3fd822d..b350686 100644
--- a/libs/hwui/tests/unit/BakedOpStateTests.cpp
+++ b/libs/hwui/tests/unit/BakedOpStateTests.cpp
@@ -176,29 +176,26 @@
}
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
- }
- {
- 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);
+ LinearAllocator allocator;
+ RectOp successOp(Rect(30, 40, 100, 200), Matrix4::identity(), &clip, &paint);
+ auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));
+ 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 @@
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";
}
}