From d1bff720d2ee9677dce59f50d35445a7121b9aff Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 22 Nov 2024 12:36:29 -0800 Subject: [PATCH 1/5] [Impeller] better handle allocation herustics of Android slide in page transition. --- impeller/entity/save_layer_utils.cc | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/impeller/entity/save_layer_utils.cc b/impeller/entity/save_layer_utils.cc index 304990240ec84..0fcb3fc098a58 100644 --- a/impeller/entity/save_layer_utils.cc +++ b/impeller/entity/save_layer_utils.cc @@ -106,8 +106,25 @@ std::optional ComputeSaveLayerCoverage( // Transform the input coverage into the global coordinate space before // computing the bounds limit intersection. - return coverage.TransformBounds(effect_transform) - .Intersection(coverage_limit); + Rect transformed_coverage = coverage.TransformBounds(effect_transform); + if (!transformed_coverage.Intersection(coverage_limit).has_value()) { + return std::nullopt; + } + + // Sometimes a saveLayer is only slightly shifted outside of the cull rect, + // but is being animated in. This is common for the Android slide in page + // transitions. In these cases, computing a cull that is too tight can cause + // thrasing of the texture cache. Instead, we try to determine the + // intersection using only the sizing by shifting the coverage rect into the + // cull rect origin. + Point delta = coverage_limit.GetOrigin() - transformed_coverage.GetOrigin(); + std::optional shifted_intersected_value = + transformed_coverage.Shift(delta).Intersection(coverage_limit); + + if (shifted_intersected_value.has_value()) { + return shifted_intersected_value.value().Shift(-delta); + } + return std::nullopt; } } // namespace impeller From 71fd81a964b50ca4ed2e1b5965bede1fe2f11f7c Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 22 Nov 2024 13:07:18 -0800 Subject: [PATCH 2/5] add unit test. --- impeller/entity/save_layer_utils_unittests.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/impeller/entity/save_layer_utils_unittests.cc b/impeller/entity/save_layer_utils_unittests.cc index b72ac33ae768b..7ddf74efae291 100644 --- a/impeller/entity/save_layer_utils_unittests.cc +++ b/impeller/entity/save_layer_utils_unittests.cc @@ -280,6 +280,20 @@ TEST(SaveLayerUtilsTest, EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 50, 50)); } +TEST(SaveLayerUtilsTest, PartiallyIntersectingCoverageIgnoresOrigin) { + // No intersection in coverage + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(5, 5, 210, 210), // + /*effect_transform=*/{}, // + /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*image_filter=*/nullptr // + ); + + ASSERT_TRUE(coverage.has_value()); + // Size that matches coverage limit + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(5, 5, 105, 105)); +} + } // namespace testing } // namespace impeller From dbde61e62fc66030420709be404603d15e4262c7 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 22 Nov 2024 13:08:14 -0800 Subject: [PATCH 3/5] ++ --- impeller/entity/save_layer_utils_unittests.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/impeller/entity/save_layer_utils_unittests.cc b/impeller/entity/save_layer_utils_unittests.cc index 7ddf74efae291..4f4ad0f638681 100644 --- a/impeller/entity/save_layer_utils_unittests.cc +++ b/impeller/entity/save_layer_utils_unittests.cc @@ -284,9 +284,9 @@ TEST(SaveLayerUtilsTest, PartiallyIntersectingCoverageIgnoresOrigin) { // No intersection in coverage auto coverage = ComputeSaveLayerCoverage( /*content_coverage=*/Rect::MakeLTRB(5, 5, 210, 210), // - /*effect_transform=*/{}, // - /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // - /*image_filter=*/nullptr // + /*effect_transform=*/{}, // + /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*image_filter=*/nullptr // ); ASSERT_TRUE(coverage.has_value()); From b0bdeaae2143afcf3cca134d1a5e7e19e10b3f92 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 25 Nov 2024 11:19:57 -0800 Subject: [PATCH 4/5] adjust heurstics to limit transation to 25% and single direction. --- impeller/entity/save_layer_utils.cc | 22 ++++++++--- impeller/entity/save_layer_utils_unittests.cc | 37 +++++++++++++++++-- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/impeller/entity/save_layer_utils.cc b/impeller/entity/save_layer_utils.cc index 0fcb3fc098a58..41b756835c419 100644 --- a/impeller/entity/save_layer_utils.cc +++ b/impeller/entity/save_layer_utils.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "impeller/entity/save_layer_utils.h" +#include "impeller/geometry/scalar.h" namespace impeller { @@ -107,7 +108,9 @@ std::optional ComputeSaveLayerCoverage( // Transform the input coverage into the global coordinate space before // computing the bounds limit intersection. Rect transformed_coverage = coverage.TransformBounds(effect_transform); - if (!transformed_coverage.Intersection(coverage_limit).has_value()) { + std::optional intersection = + transformed_coverage.Intersection(coverage_limit); + if (!intersection.has_value()) { return std::nullopt; } @@ -118,13 +121,20 @@ std::optional ComputeSaveLayerCoverage( // intersection using only the sizing by shifting the coverage rect into the // cull rect origin. Point delta = coverage_limit.GetOrigin() - transformed_coverage.GetOrigin(); - std::optional shifted_intersected_value = - transformed_coverage.Shift(delta).Intersection(coverage_limit); + if (ScalarNearlyEqual(delta.y, 0) || ScalarNearlyEqual(delta.x, 0)) { + Scalar threshold = std::max(std::abs(delta.x / coverage_limit.GetWidth()), + std::abs(delta.y / coverage_limit.GetHeight())); + if (threshold < 0.3) { + std::optional shifted_intersected_value = + transformed_coverage.Shift(delta).Intersection(coverage_limit); - if (shifted_intersected_value.has_value()) { - return shifted_intersected_value.value().Shift(-delta); + if (shifted_intersected_value.has_value()) { + return shifted_intersected_value.value().Shift(-delta); + } + } } - return std::nullopt; + + return intersection; } } // namespace impeller diff --git a/impeller/entity/save_layer_utils_unittests.cc b/impeller/entity/save_layer_utils_unittests.cc index 4f4ad0f638681..3653ec7587528 100644 --- a/impeller/entity/save_layer_utils_unittests.cc +++ b/impeller/entity/save_layer_utils_unittests.cc @@ -280,10 +280,26 @@ TEST(SaveLayerUtilsTest, EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 50, 50)); } -TEST(SaveLayerUtilsTest, PartiallyIntersectingCoverageIgnoresOrigin) { - // No intersection in coverage +TEST(SaveLayerUtilsTest, + PartiallyIntersectingCoverageIgnoresOriginWithSlideSemanticsX) { + // X varies, translation is performed on coverage. auto coverage = ComputeSaveLayerCoverage( - /*content_coverage=*/Rect::MakeLTRB(5, 5, 210, 210), // + /*content_coverage=*/Rect::MakeLTRB(5, 0, 210, 210), // + /*effect_transform=*/{}, // + /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*image_filter=*/nullptr // + ); + + ASSERT_TRUE(coverage.has_value()); + // Size that matches coverage limit + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(5, 0, 105, 100)); +} + +TEST(SaveLayerUtilsTest, + PartiallyIntersectingCoverageIgnoresOriginWithSlideSemanticsY) { + // Y varies, translation is performed on coverage. + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(0, 5, 210, 210), // /*effect_transform=*/{}, // /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // /*image_filter=*/nullptr // @@ -291,7 +307,20 @@ TEST(SaveLayerUtilsTest, PartiallyIntersectingCoverageIgnoresOrigin) { ASSERT_TRUE(coverage.has_value()); // Size that matches coverage limit - EXPECT_EQ(coverage.value(), Rect::MakeLTRB(5, 5, 105, 105)); + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 5, 100, 105)); +} + +TEST(SaveLayerUtilsTest, PartiallyIntersectingCoverageIgnoresOriginBothXAndY) { + // Both X and Y vary, no transation is performed. + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(5, 5, 210, 210), // + /*effect_transform=*/{}, // + /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*image_filter=*/nullptr // + ); + + ASSERT_TRUE(coverage.has_value()); + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(5, 5, 100, 100)); } } // namespace testing From ca684e4197c8de7176f0670ce6a3179a1af70278 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 25 Nov 2024 14:57:06 -0800 Subject: [PATCH 5/5] add more doc comment and additional test cases. --- impeller/entity/save_layer_utils.cc | 11 +++++-- impeller/entity/save_layer_utils_unittests.cc | 30 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/impeller/entity/save_layer_utils.cc b/impeller/entity/save_layer_utils.cc index 41b756835c419..8d10181198b1d 100644 --- a/impeller/entity/save_layer_utils.cc +++ b/impeller/entity/save_layer_utils.cc @@ -117,14 +117,21 @@ std::optional ComputeSaveLayerCoverage( // Sometimes a saveLayer is only slightly shifted outside of the cull rect, // but is being animated in. This is common for the Android slide in page // transitions. In these cases, computing a cull that is too tight can cause - // thrasing of the texture cache. Instead, we try to determine the + // thrashing of the texture cache. Instead, we try to determine the // intersection using only the sizing by shifting the coverage rect into the // cull rect origin. Point delta = coverage_limit.GetOrigin() - transformed_coverage.GetOrigin(); + + // This herustic is limited to perfectly vertical or horizontal transitions + // that slide in, limited to a fixed threshold of ~30%. This value is based on + // the Android slide in page transition which experimental has threshold + // values of up to 28%. + static constexpr Scalar kThresholdLimit = 0.3; + if (ScalarNearlyEqual(delta.y, 0) || ScalarNearlyEqual(delta.x, 0)) { Scalar threshold = std::max(std::abs(delta.x / coverage_limit.GetWidth()), std::abs(delta.y / coverage_limit.GetHeight())); - if (threshold < 0.3) { + if (threshold < kThresholdLimit) { std::optional shifted_intersected_value = transformed_coverage.Shift(delta).Intersection(coverage_limit); diff --git a/impeller/entity/save_layer_utils_unittests.cc b/impeller/entity/save_layer_utils_unittests.cc index 3653ec7587528..d86c8209ab6e3 100644 --- a/impeller/entity/save_layer_utils_unittests.cc +++ b/impeller/entity/save_layer_utils_unittests.cc @@ -323,6 +323,36 @@ TEST(SaveLayerUtilsTest, PartiallyIntersectingCoverageIgnoresOriginBothXAndY) { EXPECT_EQ(coverage.value(), Rect::MakeLTRB(5, 5, 100, 100)); } +TEST(SaveLayerUtilsTest, PartiallyIntersectingCoverageWithOveriszedThresholdY) { + // Y varies, translation is not performed on coverage because it is too far + // offscreen. + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(0, 80, 200, 200), // + /*effect_transform=*/{}, // + /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*image_filter=*/nullptr // + ); + + ASSERT_TRUE(coverage.has_value()); + // Size that matches coverage limit + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 80, 100, 100)); +} + +TEST(SaveLayerUtilsTest, PartiallyIntersectingCoverageWithOveriszedThresholdX) { + // X varies, translation is not performed on coverage because it is too far + // offscreen. + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(80, 0, 200, 200), // + /*effect_transform=*/{}, // + /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*image_filter=*/nullptr // + ); + + ASSERT_TRUE(coverage.has_value()); + // Size that matches coverage limit + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(80, 0, 100, 100)); +} + } // namespace testing } // namespace impeller