From ca03f8caabbaaf9d1c050b4d152928e3f79fb6b4 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 7 Dec 2024 11:55:48 -0800 Subject: [PATCH 1/4] simplify rounding up heuristics. --- impeller/entity/save_layer_utils.cc | 40 +++++++++-------------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/impeller/entity/save_layer_utils.cc b/impeller/entity/save_layer_utils.cc index 8d10181198b1d..6b6cc72e2d247 100644 --- a/impeller/entity/save_layer_utils.cc +++ b/impeller/entity/save_layer_utils.cc @@ -12,6 +12,8 @@ bool SizeDifferenceUnderThreshold(Size a, Size b, Scalar threshold) { return (std::abs(a.width - b.width) / b.width) < threshold && (std::abs(a.height - b.height) / b.height) < threshold; } + +static constexpr Scalar kDefaultSizeThreshold = 0.3; } // namespace std::optional ComputeSaveLayerCoverage( @@ -84,7 +86,8 @@ std::optional ComputeSaveLayerCoverage( transformed_coverage.Intersection(source_coverage_limit.value()); if (intersected_coverage.has_value() && SizeDifferenceUnderThreshold(transformed_coverage.GetSize(), - intersected_coverage->GetSize(), 0.2)) { + intersected_coverage->GetSize(), + kDefaultSizeThreshold)) { // Returning the transformed coverage is always correct, it just may // be larger than the clip area or onscreen texture. return transformed_coverage; @@ -113,35 +116,16 @@ std::optional ComputeSaveLayerCoverage( if (!intersection.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 - // 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 < kThresholdLimit) { - 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); - } - } + // The the resulting coverage rect is nearly the same as the coverage_limit, + // round up to the coverage_limit. + Rect intersect_rect = intersection.value(); + if (SizeDifferenceUnderThreshold(intersect_rect.GetSize(), + coverage_limit.GetSize(), + kDefaultSizeThreshold)) { + return coverage_limit; } - return intersection; + return intersection.value(); } } // namespace impeller From b667d595e4dd349d30c8c232d34f44af5d9802a8 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 7 Dec 2024 13:26:13 -0800 Subject: [PATCH 2/4] update testing. --- impeller/entity/save_layer_utils_unittests.cc | 71 ++----------------- 1 file changed, 6 insertions(+), 65 deletions(-) diff --git a/impeller/entity/save_layer_utils_unittests.cc b/impeller/entity/save_layer_utils_unittests.cc index d86c8209ab6e3..cb398857bcc7c 100644 --- a/impeller/entity/save_layer_utils_unittests.cc +++ b/impeller/entity/save_layer_utils_unittests.cc @@ -280,77 +280,18 @@ TEST(SaveLayerUtilsTest, EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 50, 50)); } -TEST(SaveLayerUtilsTest, - PartiallyIntersectingCoverageIgnoresOriginWithSlideSemanticsX) { +TEST(SaveLayerUtilsTest, RoundUpCoverageWhenCloseToCoverageLimit) { // X varies, translation is performed on coverage. auto coverage = ComputeSaveLayerCoverage( - /*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 // - ); - - ASSERT_TRUE(coverage.has_value()); - // Size that matches coverage limit - 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)); -} - -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 // + /*content_coverage=*/Rect::MakeLTRB(0, 0, 90, 90), // + /*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)); + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 100, 100)); } } // namespace testing From 745ab801fe95e68b20baa73858309897ded79869 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 9 Dec 2024 15:51:17 -0800 Subject: [PATCH 3/4] flar feedback. --- impeller/entity/save_layer_utils.cc | 2 +- impeller/entity/save_layer_utils_unittests.cc | 43 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/impeller/entity/save_layer_utils.cc b/impeller/entity/save_layer_utils.cc index 6b6cc72e2d247..f6ad180f8cfba 100644 --- a/impeller/entity/save_layer_utils.cc +++ b/impeller/entity/save_layer_utils.cc @@ -125,7 +125,7 @@ std::optional ComputeSaveLayerCoverage( return coverage_limit; } - return intersection.value(); + return intersect_rect; } } // namespace impeller diff --git a/impeller/entity/save_layer_utils_unittests.cc b/impeller/entity/save_layer_utils_unittests.cc index cb398857bcc7c..5ac61584f91fd 100644 --- a/impeller/entity/save_layer_utils_unittests.cc +++ b/impeller/entity/save_layer_utils_unittests.cc @@ -294,6 +294,49 @@ TEST(SaveLayerUtilsTest, RoundUpCoverageWhenCloseToCoverageLimit) { EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 100, 100)); } +TEST(SaveLayerUtilsTest, DontRoundUpCoverageWhenNotCloseToCoverageLimitWidth) { + // X varies, translation is performed on coverage. + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(0, 0, 50, 90), // + /*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, 0, 90, 50)); +} + +TEST(SaveLayerUtilsTest, DontRoundUpCoverageWhenNotCloseToCoverageLimitHeight) { + // X varies, translation is performed on coverage. + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(0, 0, 90, 50), // + /*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, 0, 90, 50)); +} + +TEST(SaveLayerUtilsTest, + DontRoundUpCoverageWhenNotCloseToCoverageLimitWidthHeight) { + // X varies, translation is performed on coverage. + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(0, 0, 50, 50), // + /*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, 0, 50, 50)); +} + } // namespace testing } // namespace impeller From 951bc022985c18b06a0bf3e5845d73213edd547e Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 9 Dec 2024 15:57:07 -0800 Subject: [PATCH 4/4] add more test cases. --- impeller/entity/save_layer_utils_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/entity/save_layer_utils_unittests.cc b/impeller/entity/save_layer_utils_unittests.cc index 5ac61584f91fd..5139204d7c2af 100644 --- a/impeller/entity/save_layer_utils_unittests.cc +++ b/impeller/entity/save_layer_utils_unittests.cc @@ -305,7 +305,7 @@ TEST(SaveLayerUtilsTest, DontRoundUpCoverageWhenNotCloseToCoverageLimitWidth) { ASSERT_TRUE(coverage.has_value()); // Size that matches coverage limit - EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 90, 50)); + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 50, 90)); } TEST(SaveLayerUtilsTest, DontRoundUpCoverageWhenNotCloseToCoverageLimitHeight) {