From 371a1fcbd0cae71e6a083d8b0e500b21bf7eef2a Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Thu, 18 Apr 2019 12:54:01 -0700 Subject: [PATCH 1/4] Generate layer unique id for raster cache key The raw pointer isn't a reliable id as the allocator can reuse an address that's just been released for another layer. This will fix a Fuchsia retained rendering bug. This problem was not affecting non-Fuchsia Flutter probably because non-Fuchsia Flutter purges the raster cache key much more frequently so we won't see a key collision. In Fuchsia, as the key has to wait for the Vulkan surface to render asynchronously, this suddenly becomes an issue. --- flow/layers/layer.cc | 12 +++++++++++- flow/layers/layer.h | 5 +++++ flow/layers/physical_shape_layer.cc | 2 +- flow/raster_cache.cc | 4 ++-- flow/raster_cache_key.h | 3 ++- flow/scene_update_context.cc | 7 ++++++- 6 files changed, 27 insertions(+), 6 deletions(-) diff --git a/flow/layers/layer.cc b/flow/layers/layer.cc index 45f1a08bcd094..e04e7bd612559 100644 --- a/flow/layers/layer.cc +++ b/flow/layers/layer.cc @@ -12,10 +12,20 @@ namespace flutter { Layer::Layer() : parent_(nullptr), needs_system_composite_(false), - paint_bounds_(SkRect::MakeEmpty()) {} + paint_bounds_(SkRect::MakeEmpty()), + unique_id_(NextUniqueID()) {} Layer::~Layer() = default; +uint32_t Layer::NextUniqueID() { + static std::atomic nextID(1); + uint32_t id; + do { + id = nextID.fetch_add(1); + } while (id == 0); // 0 is reserved for an invalid id. + return id; +} + void Layer::Preroll(PrerollContext* context, const SkMatrix& matrix) {} #if defined(OS_FUCHSIA) diff --git a/flow/layers/layer.h b/flow/layers/layer.h index 1880ff064e7ca..18c7f614f7d55 100644 --- a/flow/layers/layer.h +++ b/flow/layers/layer.h @@ -143,10 +143,15 @@ class Layer { bool needs_painting() const { return !paint_bounds_.isEmpty(); } + uint32_t unique_id() const { return unique_id_; } + private: ContainerLayer* parent_; bool needs_system_composite_; SkRect paint_bounds_; + uint32_t unique_id_; + + static uint32_t NextUniqueID(); FML_DISALLOW_COPY_AND_ASSIGN(Layer); }; diff --git a/flow/layers/physical_shape_layer.cc b/flow/layers/physical_shape_layer.cc index 2bf45ea763052..f6b0d20c62789 100644 --- a/flow/layers/physical_shape_layer.cc +++ b/flow/layers/physical_shape_layer.cc @@ -109,7 +109,7 @@ void PhysicalShapeLayer::UpdateScene(SceneUpdateContext& context) { // Retained rendering: speedup by reusing a retained entity node if possible. // When an entity node is reused, no paint layer is added to the frame so we // won't call PhysicalShapeLayer::Paint. - LayerRasterCacheKey key(this, context.Matrix()); + LayerRasterCacheKey key(unique_id(), context.Matrix()); if (context.HasRetainedNode(key)) { const scenic::EntityNode& retained_node = context.GetRetainedNode(key); FML_DCHECK(context.top_entity()); diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index 2cd03473f0d74..4689c21cf711a 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -151,7 +151,7 @@ static inline size_t ClampSize(size_t value, size_t min, size_t max) { void RasterCache::Prepare(PrerollContext* context, Layer* layer, const SkMatrix& ctm) { - LayerRasterCacheKey cache_key(layer, ctm); + LayerRasterCacheKey cache_key(layer->unique_id(), ctm); Entry& entry = layer_cache_[cache_key]; entry.access_count = ClampSize(entry.access_count + 1, 0, access_threshold_); entry.used_this_frame = true; @@ -230,7 +230,7 @@ RasterCacheResult RasterCache::Get(const SkPicture& picture, } RasterCacheResult RasterCache::Get(Layer* layer, const SkMatrix& ctm) const { - LayerRasterCacheKey cache_key(layer, ctm); + LayerRasterCacheKey cache_key(layer->unique_id(), ctm); auto it = layer_cache_.find(cache_key); return it == layer_cache_.end() ? RasterCacheResult() : it->second.image; } diff --git a/flow/raster_cache_key.h b/flow/raster_cache_key.h index a64cabe182076..48497abfb226d 100644 --- a/flow/raster_cache_key.h +++ b/flow/raster_cache_key.h @@ -56,7 +56,8 @@ using PictureRasterCacheKey = RasterCacheKey; class Layer; -using LayerRasterCacheKey = RasterCacheKey; +// The ID is the uint32_t layer unique_id +using LayerRasterCacheKey = RasterCacheKey; } // namespace flutter diff --git a/flow/scene_update_context.cc b/flow/scene_update_context.cc index d1d33450772e7..8d1a494d41d32 100644 --- a/flow/scene_update_context.cc +++ b/flow/scene_update_context.cc @@ -163,9 +163,14 @@ scenic::Image* SceneUpdateContext::GenerateImageIfNeeded( return nullptr; // Acquire a surface from the surface producer and register the paint tasks. + std::unique_ptr surface = surface_producer_->ProduceSurface(physical_size, - LayerRasterCacheKey(layer, Matrix()), + LayerRasterCacheKey( + // Root frame has a nullptr layer + layer ? layer->unique_id() : 0, + Matrix() + ), std::move(entity_node)); if (!surface) { From b1f7cf4edbaf89ef59c7ed88a993a0fea2951089 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Thu, 18 Apr 2019 13:03:43 -0700 Subject: [PATCH 2/4] Clang-format --- flow/scene_update_context.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/flow/scene_update_context.cc b/flow/scene_update_context.cc index 8d1a494d41d32..ca206da206f0f 100644 --- a/flow/scene_update_context.cc +++ b/flow/scene_update_context.cc @@ -165,13 +165,12 @@ scenic::Image* SceneUpdateContext::GenerateImageIfNeeded( // Acquire a surface from the surface producer and register the paint tasks. std::unique_ptr surface = - surface_producer_->ProduceSurface(physical_size, - LayerRasterCacheKey( - // Root frame has a nullptr layer - layer ? layer->unique_id() : 0, - Matrix() - ), - std::move(entity_node)); + surface_producer_->ProduceSurface( + physical_size, + LayerRasterCacheKey( + // Root frame has a nullptr layer + layer ? layer->unique_id() : 0, Matrix()), + std::move(entity_node)); if (!surface) { FML_LOG(ERROR) << "Could not acquire a surface from the surface producer " From c2cabfaeea0be0006d4ff5f6445d9c994a62cbe1 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Thu, 18 Apr 2019 13:44:58 -0700 Subject: [PATCH 3/4] Change to uint64_t --- flow/layers/layer.cc | 6 +++--- flow/layers/layer.h | 6 +++--- flow/raster_cache_key.h | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/flow/layers/layer.cc b/flow/layers/layer.cc index e04e7bd612559..b729f582a0a9a 100644 --- a/flow/layers/layer.cc +++ b/flow/layers/layer.cc @@ -17,9 +17,9 @@ Layer::Layer() Layer::~Layer() = default; -uint32_t Layer::NextUniqueID() { - static std::atomic nextID(1); - uint32_t id; +uint64_t Layer::NextUniqueID() { + static std::atomic nextID(1); + uint64_t id; do { id = nextID.fetch_add(1); } while (id == 0); // 0 is reserved for an invalid id. diff --git a/flow/layers/layer.h b/flow/layers/layer.h index 18c7f614f7d55..095251608f7ad 100644 --- a/flow/layers/layer.h +++ b/flow/layers/layer.h @@ -143,15 +143,15 @@ class Layer { bool needs_painting() const { return !paint_bounds_.isEmpty(); } - uint32_t unique_id() const { return unique_id_; } + uint64_t unique_id() const { return unique_id_; } private: ContainerLayer* parent_; bool needs_system_composite_; SkRect paint_bounds_; - uint32_t unique_id_; + uint64_t unique_id_; - static uint32_t NextUniqueID(); + static uint64_t NextUniqueID(); FML_DISALLOW_COPY_AND_ASSIGN(Layer); }; diff --git a/flow/raster_cache_key.h b/flow/raster_cache_key.h index 48497abfb226d..5eae4d3dccacb 100644 --- a/flow/raster_cache_key.h +++ b/flow/raster_cache_key.h @@ -56,8 +56,8 @@ using PictureRasterCacheKey = RasterCacheKey; class Layer; -// The ID is the uint32_t layer unique_id -using LayerRasterCacheKey = RasterCacheKey; +// The ID is the uint64_t layer unique_id +using LayerRasterCacheKey = RasterCacheKey; } // namespace flutter From 7d502365ea6cd3092173bfa7f49204a9c0846def Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Thu, 18 Apr 2019 13:57:13 -0700 Subject: [PATCH 4/4] Remove extra blank line --- flow/scene_update_context.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/flow/scene_update_context.cc b/flow/scene_update_context.cc index ca206da206f0f..d351ad5c0e3c1 100644 --- a/flow/scene_update_context.cc +++ b/flow/scene_update_context.cc @@ -163,7 +163,6 @@ scenic::Image* SceneUpdateContext::GenerateImageIfNeeded( return nullptr; // Acquire a surface from the surface producer and register the paint tasks. - std::unique_ptr surface = surface_producer_->ProduceSurface( physical_size,