From b8ae2f24afbf130bf3dcae2f3d43f338f9de438c Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 9 Sep 2024 11:17:59 -0700 Subject: [PATCH] [Impeller] add herustic for ignoring coverage limit w/ image filters. (flutter/engine#55030) Switching to save_layer_utils regressed perf/memory usage on several devicelab benchmarks, see https://flutter-flutter-perf.skia.org/e/?queries=device_type%3DPixel_7_Pro%26sub_result%3D90th_percentile_gpu_memory_mb%26sub_result%3Daverage_gpu_memory_mb%26test%3Dnew_gallery_impeller_old_zoom__transition_perf&selected=commit%3D42418%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DPixel_7_Pro%252Cdevice_version%253Dnone%252Chost_type%253Dlinux%252Csub_result%253D90th_percentile_gpu_memory_mb%252Ctest%253Dnew_gallery_impeller_old_zoom__transition_perf%252C The reason is that the source_coverage_limit [intersection](https://github.com/flutter/engine/blob/main/impeller/entity/save_layer_utils.cc#L67-L68) is highly unstable from frame to frame if there is an animated blur and/or image filter. This change mostly matches the old entity pass behavior. . In cases where the difference between the intersected coverage result is small (< 20% in both dimensions), we just use the transformed content coverage. The old behavior would _always_ use the transformed content coverage even if it was substantially large than the coverage limit. This posed problems if folks drew content larger than the max texture size. This herustic attempts to prevent that by allowing the source coverage limit to kick in once the difference is larger than 20% in either direction --- .../impeller/entity/save_layer_utils.cc | 30 +++++++++- .../entity/save_layer_utils_unittests.cc | 56 +++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/engine/src/flutter/impeller/entity/save_layer_utils.cc b/engine/src/flutter/impeller/entity/save_layer_utils.cc index 68abbf7a9c..fa8d7d7121 100644 --- a/engine/src/flutter/impeller/entity/save_layer_utils.cc +++ b/engine/src/flutter/impeller/entity/save_layer_utils.cc @@ -6,6 +6,13 @@ namespace impeller { +namespace { +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; +} +} // namespace + std::optional ComputeSaveLayerCoverage( const Rect& content_coverage, const Matrix& effect_transform, @@ -64,8 +71,27 @@ std::optional ComputeSaveLayerCoverage( return source_coverage_limit; } - return coverage.TransformBounds(effect_transform) - .Intersection(source_coverage_limit.value()); + // Trimming the content coverage by the coverage limit can reduce memory + // coverage. limit can reduce memory bandwith. But in cases where there are + // animated matrix filters, such as in the framework's zoom transition, the + // changing scale values continually change the source_coverage_limit. + // Intersecting the source_coverage_limit with the coverage may result in + // slightly different texture sizes each frame of the animation. This leads + // to non-optimal allocation patterns as differently sized textures cannot + // be reused. Hence the following herustic: If the coverage is within a + // semi-arbitrary percentage of the intersected coverage, then just use the + // transformed coverage. In other cases, use the intersection. + auto transformed_coverage = coverage.TransformBounds(effect_transform); + auto intersected_coverage = + transformed_coverage.Intersection(source_coverage_limit.value()); + if (intersected_coverage.has_value() && + SizeDifferenceUnderThreshold(transformed_coverage.GetSize(), + intersected_coverage->GetSize(), 0.2)) { + // Returning the transformed coverage is always correct, it just may + // be larger than the clip area or onscreen texture. + return transformed_coverage; + } + return intersected_coverage; } // If the input coverage is maximum, just return the coverage limit that diff --git a/engine/src/flutter/impeller/entity/save_layer_utils_unittests.cc b/engine/src/flutter/impeller/entity/save_layer_utils_unittests.cc index aa5dfd92ca..b72ac33ae7 100644 --- a/engine/src/flutter/impeller/entity/save_layer_utils_unittests.cc +++ b/engine/src/flutter/impeller/entity/save_layer_utils_unittests.cc @@ -224,6 +224,62 @@ TEST(SaveLayerUtilsTest, ASSERT_FALSE(coverage.has_value()); } +TEST( + SaveLayerUtilsTest, + CoverageLimitIgnoredIfIntersectedValueIsCloseToActualCoverageSmallerWithImageFilter) { + // Create an image filter that slightly shrinks the coverage limit + auto image_filter = FilterContents::MakeMatrixFilter( + FilterInput::Make(Rect()), Matrix::MakeScale({1.1, 1.1, 1}), {}); + + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*effect_transform=*/{}, // + /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*image_filter=*/image_filter // + ); + + ASSERT_TRUE(coverage.has_value()); + // The transfomed coverage limit is ((0, 0), (90.9091, 90.9091)). + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 100, 100)); +} + +TEST( + SaveLayerUtilsTest, + CoverageLimitIgnoredIfIntersectedValueIsCloseToActualCoverageLargerWithImageFilter) { + // Create an image filter that slightly stretches the coverage limit. Even + // without the special logic for using the original content coverage, we + // verify that we don't introduce any artifacts from the intersection. + auto image_filter = FilterContents::MakeMatrixFilter( + FilterInput::Make(Rect()), Matrix::MakeScale({0.9, 0.9, 1}), {}); + + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*effect_transform=*/{}, // + /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*image_filter=*/image_filter // + ); + + ASSERT_TRUE(coverage.has_value()); + // The transfomed coverage limit is ((0, 0), (111.111, 111.111)). + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 100, 100)); +} + +TEST(SaveLayerUtilsTest, + CoverageLimitRespectedIfSubstantiallyDifferentFromContentCoverge) { + auto image_filter = FilterContents::MakeMatrixFilter( + FilterInput::Make(Rect()), Matrix::MakeScale({2, 2, 1}), {}); + + auto coverage = ComputeSaveLayerCoverage( + /*content_coverage=*/Rect::MakeLTRB(0, 0, 1000, 1000), // + /*effect_transform=*/{}, // + /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), // + /*image_filter=*/image_filter // + ); + + ASSERT_TRUE(coverage.has_value()); + EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 50, 50)); +} + } // namespace testing } // namespace impeller