From 5c32db01e938d2ed3203e22f1d48b3cf11e2dfc6 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 16 Feb 2023 12:08:46 -0800 Subject: [PATCH 1/3] only indicate opacity inheritance when DL is actually cached --- display_list/display_list.cc | 5 ++--- display_list/display_list.h | 3 +-- flow/layers/display_list_raster_cache_item.cc | 8 ++++++-- flow/raster_cache.cc | 6 +++++- flow/raster_cache.h | 3 ++- 5 files changed, 16 insertions(+), 9 deletions(-) diff --git a/display_list/display_list.cc b/display_list/display_list.cc index 751e16158ae58..f5bdcd533ef1c 100644 --- a/display_list/display_list.cc +++ b/display_list/display_list.cc @@ -294,9 +294,7 @@ static bool CompareOps(uint8_t* ptrA, return true; } -void DisplayList::RenderTo(DisplayListBuilder* builder, - SkScalar opacity) const { - // TODO(100983): Opacity is not respected and attributes are not reset. +void DisplayList::RenderTo(DisplayListBuilder* builder) const { if (!builder) { return; } @@ -308,6 +306,7 @@ void DisplayList::RenderTo(DisplayListBuilder* builder, } void DisplayList::RenderTo(SkCanvas* canvas, SkScalar opacity) const { + FML_DCHECK(can_apply_group_opacity() || opacity >= SK_Scalar1); DisplayListCanvasDispatcher dispatcher(canvas, opacity); if (has_rtree()) { Dispatch(dispatcher, canvas->getLocalClipBounds()); diff --git a/display_list/display_list.h b/display_list/display_list.h index 1ee8645960737..78b739d6fe914 100644 --- a/display_list/display_list.h +++ b/display_list/display_list.h @@ -254,8 +254,7 @@ class DisplayList : public SkRefCnt { void Dispatch(Dispatcher& ctx) const; void Dispatch(Dispatcher& ctx, const SkRect& cull_rect) const; - void RenderTo(DisplayListBuilder* builder, - SkScalar opacity = SK_Scalar1) const; + void RenderTo(DisplayListBuilder* builder) const; void RenderTo(SkCanvas* canvas, SkScalar opacity = SK_Scalar1) const; diff --git a/flow/layers/display_list_raster_cache_item.cc b/flow/layers/display_list_raster_cache_item.cc index 37a1a6bf1a3c5..99f220a26b85d 100644 --- a/flow/layers/display_list_raster_cache_item.cc +++ b/flow/layers/display_list_raster_cache_item.cc @@ -105,11 +105,15 @@ void DisplayListRasterCacheItem::PrerollFinalize(PrerollContext* context, auto* raster_cache = context->raster_cache; SkRect bounds = display_list_->bounds().makeOffset(offset_.x(), offset_.y()); bool visible = !context->state_stack.content_culled(bounds); - int accesses = raster_cache->MarkSeen(key_id_, matrix, visible); + bool has_image = false; + int accesses = raster_cache->MarkSeen(key_id_, matrix, visible, &has_image); if (!visible || accesses <= raster_cache->access_threshold()) { cache_state_ = kNone; } else { - context->renderable_state_flags |= LayerStateStack::kCallerCanApplyOpacity; + if (has_image) { + context->renderable_state_flags |= + LayerStateStack::kCallerCanApplyOpacity; + } cache_state_ = kCurrent; } return; diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index 9eb420f3b7a94..3ea180c7129fa 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -112,7 +112,8 @@ bool RasterCache::UpdateCacheEntry( int RasterCache::MarkSeen(const RasterCacheKeyID& id, const SkMatrix& matrix, - bool visible) const { + bool visible, + bool* has_image) const { RasterCacheKey key = RasterCacheKey(id, matrix); Entry& entry = cache_[key]; entry.encountered_this_frame = true; @@ -120,6 +121,9 @@ int RasterCache::MarkSeen(const RasterCacheKeyID& id, if (visible || entry.accesses_since_visible > 0) { entry.accesses_since_visible++; } + if (has_image) { + *has_image = (entry.image != nullptr); + } return entry.accesses_since_visible; } diff --git a/flow/raster_cache.h b/flow/raster_cache.h index 331f57349f11c..b39574f952990 100644 --- a/flow/raster_cache.h +++ b/flow/raster_cache.h @@ -223,7 +223,8 @@ class RasterCache { */ int MarkSeen(const RasterCacheKeyID& id, const SkMatrix& matrix, - bool visible) const; + bool visible, + bool* has_image = nullptr) const; /** * Returns the access count (i.e. accesses_since_visible) for the given From 5bc861e90d158eff70cccde2a1746736c2b08e3d Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 16 Feb 2023 13:00:57 -0800 Subject: [PATCH 2/3] unit test --- flow/layers/display_list_layer_unittests.cc | 67 +++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/flow/layers/display_list_layer_unittests.cc b/flow/layers/display_list_layer_unittests.cc index 8c7fec7c548c2..a771e9c375b03 100644 --- a/flow/layers/display_list_layer_unittests.cc +++ b/flow/layers/display_list_layer_unittests.cc @@ -7,6 +7,7 @@ #include "flutter/flow/layers/display_list_layer.h" #include "flutter/display_list/display_list_builder.h" +#include "flutter/flow/layers/layer_tree.h" #include "flutter/flow/testing/diff_context_test.h" #include "flutter/flow/testing/skia_gpu_object_layer_test.h" #include "flutter/fml/macros.h" @@ -249,6 +250,8 @@ TEST_F(DisplayListLayerTest, CachedIncompatibleDisplayListOpacityInheritance) { display_list_layer->Preroll(preroll_context()); display_list_layer->Preroll(preroll_context()); display_list_layer->Preroll(preroll_context()); + LayerTree::TryToRasterCache(*preroll_context()->raster_cached_entries, + &paint_context(), false); int opacity_alpha = 0x7F; SkPoint opacity_offset = SkPoint::Make(10, 10); @@ -516,5 +519,69 @@ TEST_F(DisplayListLayerTest, DisplayListAccessCountDependsOnVisibility) { ASSERT_TRUE(raster_cache_item->Draw(paint_context(), nullptr)); } +TEST_F(DisplayListLayerTest, OverflowCachedDisplayListOpacityInheritance) { + use_mock_raster_cache(); + PrerollContext* context = preroll_context(); + int per_frame = + RasterCacheUtil::kDefaultPictureAndDispLayListCacheLimitPerFrame; + int layer_count = per_frame + 1; + SkPoint opacity_offset = {10, 10}; + auto opacity_layer = std::make_shared(0.5f, opacity_offset); + std::shared_ptr layers[layer_count]; + for (int i = 0; i < layer_count; i++) { + DisplayListBuilder builder(false); + builder.drawRect({0, 0, 100, 100}); + builder.drawRect({50, 50, 100, 100}); + auto display_list = builder.Build(); + ASSERT_FALSE(display_list->can_apply_group_opacity()); + SkPoint offset = {i * 200.0f, 0}; + + layers[i] = std::make_shared( + offset, SkiaGPUObject(display_list, unref_queue()), true, false); + opacity_layer->Add(layers[i]); + } + for (int j = 0; j < context->raster_cache->access_threshold(); j++) { + context->raster_cache->BeginFrame(); + for (int i = 0; i < layer_count; i++) { + context->renderable_state_flags = 0; + layers[i]->Preroll(context); + ASSERT_EQ(context->renderable_state_flags, 0) << "pass " << (j + 1); + } + } + opacity_layer->Preroll(context); + ASSERT_FALSE(opacity_layer->children_can_accept_opacity()); + LayerTree::TryToRasterCache(*context->raster_cached_entries, &paint_context(), + false); + context->raster_cached_entries->clear(); + context->raster_cache->BeginFrame(); + for (int i = 0; i < per_frame; i++) { + context->renderable_state_flags = 0; + layers[i]->Preroll(context); + ASSERT_EQ(context->renderable_state_flags, + LayerStateStack::kCallerCanApplyOpacity) + << "layer " << (i + 1); + } + for (int i = per_frame; i < layer_count; i++) { + context->renderable_state_flags = 0; + layers[i]->Preroll(context); + ASSERT_EQ(context->renderable_state_flags, 0) << "layer " << (i + 1); + } + opacity_layer->Preroll(context); + ASSERT_FALSE(opacity_layer->children_can_accept_opacity()); + LayerTree::TryToRasterCache(*context->raster_cached_entries, &paint_context(), + false); + context->raster_cached_entries->clear(); + context->raster_cache->BeginFrame(); + for (int i = 0; i < layer_count; i++) { + context->renderable_state_flags = 0; + layers[i]->Preroll(context); + ASSERT_EQ(context->renderable_state_flags, + LayerStateStack::kCallerCanApplyOpacity) + << "layer " << (i + 1); + } + opacity_layer->Preroll(context); + ASSERT_TRUE(opacity_layer->children_can_accept_opacity()); +} + } // namespace testing } // namespace flutter From a224d6f8a63966c288909a1c35085b599fb63c56 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 16 Feb 2023 14:04:09 -0800 Subject: [PATCH 3/3] use CacheInfo struct to simplify return values --- flow/layers/display_list_layer_unittests.cc | 2 +- flow/layers/display_list_raster_cache_item.cc | 9 +++++---- flow/raster_cache.cc | 12 ++++-------- flow/raster_cache.h | 13 ++++++++----- flow/testing/mock_raster_cache.cc | 2 +- 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/flow/layers/display_list_layer_unittests.cc b/flow/layers/display_list_layer_unittests.cc index a771e9c375b03..c4852a42b006e 100644 --- a/flow/layers/display_list_layer_unittests.cc +++ b/flow/layers/display_list_layer_unittests.cc @@ -540,7 +540,7 @@ TEST_F(DisplayListLayerTest, OverflowCachedDisplayListOpacityInheritance) { offset, SkiaGPUObject(display_list, unref_queue()), true, false); opacity_layer->Add(layers[i]); } - for (int j = 0; j < context->raster_cache->access_threshold(); j++) { + for (size_t j = 0; j < context->raster_cache->access_threshold(); j++) { context->raster_cache->BeginFrame(); for (int i = 0; i < layer_count; i++) { context->renderable_state_flags = 0; diff --git a/flow/layers/display_list_raster_cache_item.cc b/flow/layers/display_list_raster_cache_item.cc index 99f220a26b85d..42f6d1884bed2 100644 --- a/flow/layers/display_list_raster_cache_item.cc +++ b/flow/layers/display_list_raster_cache_item.cc @@ -105,12 +105,13 @@ void DisplayListRasterCacheItem::PrerollFinalize(PrerollContext* context, auto* raster_cache = context->raster_cache; SkRect bounds = display_list_->bounds().makeOffset(offset_.x(), offset_.y()); bool visible = !context->state_stack.content_culled(bounds); - bool has_image = false; - int accesses = raster_cache->MarkSeen(key_id_, matrix, visible, &has_image); - if (!visible || accesses <= raster_cache->access_threshold()) { + RasterCache::CacheInfo cache_info = + raster_cache->MarkSeen(key_id_, matrix, visible); + if (!visible || + cache_info.accesses_since_visible <= raster_cache->access_threshold()) { cache_state_ = kNone; } else { - if (has_image) { + if (cache_info.has_image) { context->renderable_state_flags |= LayerStateStack::kCallerCanApplyOpacity; } diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index 3ea180c7129fa..f6bc2b0156dfa 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -110,10 +110,9 @@ bool RasterCache::UpdateCacheEntry( return entry.image != nullptr; } -int RasterCache::MarkSeen(const RasterCacheKeyID& id, - const SkMatrix& matrix, - bool visible, - bool* has_image) const { +RasterCache::CacheInfo RasterCache::MarkSeen(const RasterCacheKeyID& id, + const SkMatrix& matrix, + bool visible) const { RasterCacheKey key = RasterCacheKey(id, matrix); Entry& entry = cache_[key]; entry.encountered_this_frame = true; @@ -121,10 +120,7 @@ int RasterCache::MarkSeen(const RasterCacheKeyID& id, if (visible || entry.accesses_since_visible > 0) { entry.accesses_since_visible++; } - if (has_image) { - *has_image = (entry.image != nullptr); - } - return entry.accesses_since_visible; + return {entry.accesses_since_visible, entry.image != nullptr}; } int RasterCache::GetAccessCount(const RasterCacheKeyID& id, diff --git a/flow/raster_cache.h b/flow/raster_cache.h index b39574f952990..8f51b3f52ae89 100644 --- a/flow/raster_cache.h +++ b/flow/raster_cache.h @@ -121,6 +121,10 @@ class RasterCache { const SkRect& logical_rect; const char* flow_type; }; + struct CacheInfo { + const size_t accesses_since_visible; + const bool has_image; + }; std::unique_ptr Rasterize( const RasterCache::Context& context, @@ -203,7 +207,7 @@ class RasterCache { * If the number is one, then it must be prepared and drawn on 1 frame * and it will then be cached on the next frame if it is prepared. */ - int access_threshold() const { return access_threshold_; } + size_t access_threshold() const { return access_threshold_; } bool GenerateNewCacheInThisFrame() const { // Disabling caching when access_threshold is zero is historic behavior. @@ -221,10 +225,9 @@ class RasterCache { * @return the number of times the entry has been hit since it was created. * For a new entry that will be 1 if it is visible, or zero if non-visible. */ - int MarkSeen(const RasterCacheKeyID& id, - const SkMatrix& matrix, - bool visible, - bool* has_image = nullptr) const; + CacheInfo MarkSeen(const RasterCacheKeyID& id, + const SkMatrix& matrix, + bool visible) const; /** * Returns the access count (i.e. accesses_since_visible) for the given diff --git a/flow/testing/mock_raster_cache.cc b/flow/testing/mock_raster_cache.cc index b08b0f6d829f4..ca8cb71ccf089 100644 --- a/flow/testing/mock_raster_cache.cc +++ b/flow/testing/mock_raster_cache.cc @@ -65,7 +65,7 @@ void MockRasterCache::AddMockPicture(int width, int height) { DisplayListRasterCacheItem display_list_item(display_list.get(), SkPoint(), true, false); - for (int i = 0; i < access_threshold(); i++) { + for (size_t i = 0; i < access_threshold(); i++) { AutoCache(&display_list_item, &preroll_context_, ctm); } RasterCache::Context r_context = {