Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 04191a1

Browse files
author
Jonah Williams
authored
[Impeller] fix unbalanced restores. (#51648)
Fixes flutter/flutter#145528 by checking if the clip coverage stack is empty for restoring. Adds a protective empty check in clip replay. I have a plan to pull the clip stack into something more testable to make this better, but opening this as a simpler fix first.
1 parent ef896e9 commit 04191a1

File tree

4 files changed

+60
-2
lines changed

4 files changed

+60
-2
lines changed

ci/licenses_golden/excluded_files

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@
152152
../../../flutter/impeller/entity/contents/tiled_texture_contents_unittests.cc
153153
../../../flutter/impeller/entity/contents/vertices_contents_unittests.cc
154154
../../../flutter/impeller/entity/entity_pass_target_unittests.cc
155+
../../../flutter/impeller/entity/entity_pass_unittests.cc
155156
../../../flutter/impeller/entity/entity_unittests.cc
156157
../../../flutter/impeller/entity/geometry/geometry_unittests.cc
157158
../../../flutter/impeller/entity/render_target_cache_unittests.cc

impeller/entity/BUILD.gn

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,7 @@ impeller_component("entity_unittests") {
272272
"contents/tiled_texture_contents_unittests.cc",
273273
"contents/vertices_contents_unittests.cc",
274274
"entity_pass_target_unittests.cc",
275+
"entity_pass_unittests.cc",
275276
"entity_playground.cc",
276277
"entity_playground.h",
277278
"entity_unittests.cc",

impeller/entity/entity_pass.cc

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -869,7 +869,9 @@ bool EntityPass::RenderElement(Entity& element_entity,
869869

870870
if constexpr (ContentContext::kEnableStencilThenCover) {
871871
// Skip all clip restores when stencil-then-cover is enabled.
872-
clip_replay_->RecordEntity(element_entity, clip_coverage.type);
872+
if (clip_coverage_stack.back().coverage.has_value()) {
873+
clip_replay_->RecordEntity(element_entity, clip_coverage.type);
874+
}
873875
return true;
874876
}
875877

@@ -1269,7 +1271,9 @@ void EntityPassClipRecorder::RecordEntity(const Entity& entity,
12691271
rendered_clip_entities_.push_back(entity.Clone());
12701272
break;
12711273
case Contents::ClipCoverage::Type::kRestore:
1272-
rendered_clip_entities_.pop_back();
1274+
if (!rendered_clip_entities_.empty()) {
1275+
rendered_clip_entities_.pop_back();
1276+
}
12731277
break;
12741278
}
12751279
}
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include "flutter/testing/testing.h"
6+
#include "gtest/gtest.h"
7+
#include "impeller/entity/entity_pass.h"
8+
9+
namespace impeller {
10+
namespace testing {
11+
12+
TEST(EntityPassClipRecorderTest, CanPushAndPopEntities) {
13+
EntityPassClipRecorder recorder = EntityPassClipRecorder();
14+
15+
EXPECT_TRUE(recorder.GetReplayEntities().empty());
16+
17+
Entity entity;
18+
recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend);
19+
EXPECT_EQ(recorder.GetReplayEntities().size(), 1u);
20+
21+
recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend);
22+
EXPECT_EQ(recorder.GetReplayEntities().size(), 2u);
23+
24+
recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore);
25+
EXPECT_EQ(recorder.GetReplayEntities().size(), 1u);
26+
27+
recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore);
28+
EXPECT_TRUE(recorder.GetReplayEntities().empty());
29+
}
30+
31+
TEST(EntityPassClipRecorderTest, CanPopEntitiesSafely) {
32+
EntityPassClipRecorder recorder = EntityPassClipRecorder();
33+
34+
EXPECT_TRUE(recorder.GetReplayEntities().empty());
35+
36+
Entity entity;
37+
recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore);
38+
EXPECT_TRUE(recorder.GetReplayEntities().empty());
39+
}
40+
41+
TEST(EntityPassClipRecorderTest, CanAppendNoChange) {
42+
EntityPassClipRecorder recorder = EntityPassClipRecorder();
43+
44+
EXPECT_TRUE(recorder.GetReplayEntities().empty());
45+
46+
Entity entity;
47+
recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kNoChange);
48+
EXPECT_TRUE(recorder.GetReplayEntities().empty());
49+
}
50+
51+
} // namespace testing
52+
} // namespace impeller

0 commit comments

Comments
 (0)