Fix premature DisplayList caching

flow/layers/display_list_layer_unittests.cc

@@ -238,9 +238,9 @@ TEST_F(DisplayListLayerTest, CachedIncompatibleDisplayListOpacityInheritance) {
   EXPECT_FALSE(context->subtree_can_inherit_opacity);
 
   // Pump the DisplayListLayer until it is ready to cache its DL
-  display_list_layer->Paint(paint_context());
-  display_list_layer->Paint(paint_context());
-  display_list_layer->Paint(paint_context());
+  display_list_layer->Preroll(preroll_context(), SkMatrix());
+  display_list_layer->Preroll(preroll_context(), SkMatrix());
+  display_list_layer->Preroll(preroll_context(), SkMatrix());
 
   int opacity_alpha = 0x7F;
   SkPoint opacity_offset = SkPoint::Make(10, 10);
@@ -398,5 +398,97 @@ TEST_F(DisplayListLayerTest, NoLayerTreeSnapshotsWhenDisabledByDefault) {
   EXPECT_EQ(0u, snapshot_store.Size());
 }
 
+TEST_F(DisplayListLayerTest, DisplayListAccessCountDependsOnVisibility) {
+  const SkPoint layer_offset = SkPoint::Make(1.5f, -0.5f);
+  const SkRect picture_bounds = SkRect::MakeLTRB(5.0f, 6.0f, 20.5f, 21.5f);
+  const SkRect missed_cull_rect = SkRect::MakeLTRB(100, 100, 200, 200);
+  const SkRect hit_cull_rect = SkRect::MakeLTRB(0, 0, 200, 200);
+  DisplayListBuilder builder;
+  builder.drawRect(picture_bounds);
+  auto display_list = builder.Build();
+  auto layer = std::make_shared<DisplayListLayer>(
+      layer_offset, SkiaGPUObject(display_list, unref_queue()), true, false);
+
+  auto raster_cache_item = layer->raster_cache_item();
+  use_mock_raster_cache();
+
+  // First Preroll the DisplayListLayer a few times where it does not intersect
+  // the cull rect. No caching progress should occur during this time, the
+  // access_count should remain 0 because the DisplayList was never "visible".
+  preroll_context()->cull_rect = missed_cull_rect;
+  for (int i = 0; i < 10; i++) {
+    preroll_context()->raster_cached_entries->clear();
+    layer->Preroll(preroll_context(), SkMatrix::I());
+    ASSERT_EQ(raster_cache_item->cache_state(), RasterCacheItem::kNone);
+    ASSERT_TRUE(raster_cache_item->GetId().has_value());
+    ASSERT_EQ(preroll_context()->raster_cache->GetAccessCount(
+                  raster_cache_item->GetId().value(), SkMatrix::I()),
+              0);
+    ASSERT_EQ(preroll_context()->raster_cached_entries->size(), size_t(1));
+    ASSERT_EQ(preroll_context()->raster_cache->EstimatePictureCacheByteSize(),
+              size_t(0));
+    ASSERT_FALSE(raster_cache_item->TryToPrepareRasterCache(paint_context()));
+    ASSERT_FALSE(raster_cache_item->Draw(paint_context(), nullptr));
+  }
+
+  // Next Preroll the DisplayListLayer once where it does intersect
+  // the cull rect. No caching progress should occur during this time
+  // since this is the first frame in which it was visible, but the
+  // count should start incrementing.
+  preroll_context()->cull_rect = hit_cull_rect;
+  preroll_context()->raster_cached_entries->clear();
+  layer->Preroll(preroll_context(), SkMatrix());
+  ASSERT_EQ(raster_cache_item->cache_state(), RasterCacheItem::kNone);
+  ASSERT_TRUE(raster_cache_item->GetId().has_value());
+  ASSERT_EQ(preroll_context()->raster_cache->GetAccessCount(
+                raster_cache_item->GetId().value(), SkMatrix::I()),
+            1);
+  ASSERT_EQ(preroll_context()->raster_cached_entries->size(), size_t(1));
+  ASSERT_EQ(preroll_context()->raster_cache->EstimatePictureCacheByteSize(),
+            size_t(0));
+  ASSERT_FALSE(raster_cache_item->TryToPrepareRasterCache(paint_context()));
+  ASSERT_FALSE(raster_cache_item->Draw(paint_context(), nullptr));
+
+  // Now we can Preroll the DisplayListLayer again with a cull rect that
+  // it does not intersect and it should continue to count these operations
+  // even though it is not visible. No actual caching should occur yet,
+  // even though we will surpass its threshold.
+  preroll_context()->cull_rect = missed_cull_rect;
+  for (int i = 0; i < 10; i++) {
+    preroll_context()->raster_cached_entries->clear();
+    layer->Preroll(preroll_context(), SkMatrix());
+    ASSERT_EQ(raster_cache_item->cache_state(), RasterCacheItem::kNone);
+    ASSERT_TRUE(raster_cache_item->GetId().has_value());
+    ASSERT_EQ(preroll_context()->raster_cache->GetAccessCount(
+                  raster_cache_item->GetId().value(), SkMatrix::I()),
+              i + 2);
+    ASSERT_EQ(preroll_context()->raster_cached_entries->size(), size_t(1));
+    ASSERT_EQ(preroll_context()->raster_cache->EstimatePictureCacheByteSize(),
+              size_t(0));
+    ASSERT_FALSE(raster_cache_item->TryToPrepareRasterCache(paint_context()));
+    ASSERT_FALSE(raster_cache_item->Draw(paint_context(), nullptr));
+  }
+
+  // Finally Preroll the DisplayListLayer again where it does intersect
+  // the cull rect. Since we should have exhausted our access count
+  // threshold in the loop above, these operations should result in the
+  // DisplayList being cached.
+  preroll_context()->cull_rect = hit_cull_rect;
+  preroll_context()->raster_cached_entries->clear();
+  layer->Preroll(preroll_context(), SkMatrix());
+  ASSERT_EQ(raster_cache_item->cache_state(), RasterCacheItem::kCurrent);
+  ASSERT_TRUE(raster_cache_item->GetId().has_value());
+  ASSERT_EQ(preroll_context()->raster_cache->GetAccessCount(
+                raster_cache_item->GetId().value(), SkMatrix::I()),
+            12);
+  ASSERT_EQ(preroll_context()->raster_cached_entries->size(), size_t(1));
+  ASSERT_EQ(preroll_context()->raster_cache->EstimatePictureCacheByteSize(),
+            size_t(0));
+  ASSERT_TRUE(raster_cache_item->TryToPrepareRasterCache(paint_context()));
+  ASSERT_GT(preroll_context()->raster_cache->EstimatePictureCacheByteSize(),
+            size_t(0));
+  ASSERT_TRUE(raster_cache_item->Draw(paint_context(), nullptr));
+}
+
 }  // namespace testing
 }  // namespace flutter

flow/layers/display_list_raster_cache_item.cc

@@ -108,16 +108,13 @@ void DisplayListRasterCacheItem::PrerollFinalize(PrerollContext* context,
   // if the rect is intersect we will get the entry access_count to confirm if
   // it great than the threshold. Otherwise we only increase the entry
   // access_count.
-  if (context->cull_rect.intersect(bounds)) {
-    if (raster_cache->MarkSeen(key_id_, transformation_matrix_) <
-        raster_cache->access_threshold()) {
-      cache_state_ = CacheState::kNone;
-      return;
-    }
-    context->subtree_can_inherit_opacity = true;
-    cache_state_ = CacheState::kCurrent;
+  bool visible = context->cull_rect.intersect(bounds);
+  int accesses = raster_cache->MarkSeen(key_id_, matrix, visible);
+  if (!visible || accesses <= raster_cache->access_threshold()) {
+    cache_state_ = kNone;
   } else {
-    raster_cache->Touch(key_id_, matrix);
+    context->subtree_can_inherit_opacity = true;
+    cache_state_ = kCurrent;
   }
   return;
 }
@@ -136,9 +133,6 @@ bool DisplayListRasterCacheItem::Draw(const PaintContext& context,
   if (cache_state_ == CacheState::kCurrent) {
     return context.raster_cache->Draw(key_id_, *canvas, paint);
   }
-  // This display_list doesn't cache itself, this only increase the entry
-  // access_count;
-  context.raster_cache->Touch(key_id_, canvas->getTotalMatrix());
   return false;
 }
 

flow/layers/layer_raster_cache_item.cc

@@ -53,7 +53,7 @@ void LayerRasterCacheItem::PrerollFinalize(PrerollContext* context,
   if (num_cache_attempts_ >= layer_cached_threshold_) {
     // the layer can be cached
     cache_state_ = CacheState::kCurrent;
-    context->raster_cache->MarkSeen(key_id_, matrix_);
+    context->raster_cache->MarkSeen(key_id_, matrix_, true);
   } else {
     num_cache_attempts_++;
     // access current layer
@@ -67,7 +67,8 @@ void LayerRasterCacheItem::PrerollFinalize(PrerollContext* context,
                                    RasterCacheKeyType::kLayerChildren);
       }
       cache_state_ = CacheState::kChildren;
-      context->raster_cache->MarkSeen(layer_children_id_.value(), matrix_);
+      context->raster_cache->MarkSeen(layer_children_id_.value(), matrix_,
+                                      true);
     }
   }
 }

flow/raster_cache.cc

@@ -92,7 +92,6 @@ bool RasterCache::UpdateCacheEntry(
     const std::function<void(SkCanvas*)>& render_function) const {
   RasterCacheKey key = RasterCacheKey(id, raster_cache_context.matrix);
   Entry& entry = cache_[key];
-  entry.used_this_frame = true;
   if (!entry.image) {
     entry.image = Rasterize(raster_cache_context, render_function);
     if (entry.image != nullptr) {
@@ -110,24 +109,27 @@ bool RasterCache::UpdateCacheEntry(
   return entry.image != nullptr;
 }
 
-bool RasterCache::Touch(const RasterCacheKeyID& id,
-                        const SkMatrix& matrix) const {
-  RasterCacheKey cache_key = RasterCacheKey(id, matrix);
-  auto it = cache_.find(cache_key);
-  if (it != cache_.end()) {
-    it->second.access_count++;
-    it->second.used_this_frame = true;
-    return true;
+int 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;
+  entry.visible_this_frame = visible;
+  if (visible || entry.accesses_since_visible > 0) {
+    entry.accesses_since_visible++;
   }
-  return false;
+  return entry.accesses_since_visible;
 }
 
-int RasterCache::MarkSeen(const RasterCacheKeyID& id,
-                          const SkMatrix& matrix) const {
+int RasterCache::GetAccessCount(const RasterCacheKeyID& id,
+                                const SkMatrix& matrix) const {
   RasterCacheKey key = RasterCacheKey(id, matrix);
-  Entry& entry = cache_[key];
-  entry.used_this_frame = true;
-  return entry.access_count;
+  auto entry = cache_.find(key);
+  if (entry != cache_.cend()) {
+    return entry->second.accesses_since_visible;
+  }
+  return -1;
 }
 
 bool RasterCache::HasEntry(const RasterCacheKeyID& id,
@@ -148,8 +150,6 @@ bool RasterCache::Draw(const RasterCacheKeyID& id,
   }
 
   Entry& entry = it->second;
-  entry.access_count++;
-  entry.used_this_frame = true;
 
   if (entry.image) {
     entry.image->draw(canvas, paint);
@@ -171,7 +171,7 @@ void RasterCache::SweepOneCacheAfterFrame(RasterCacheKey::Map<Entry>& cache,
   for (auto it = cache.begin(); it != cache.end(); ++it) {
     Entry& entry = it->second;
 
-    if (!entry.used_this_frame) {
+    if (!entry.encountered_this_frame) {
       dead.push_back(it);
     } else if (entry.image) {
       RasterCacheKeyKind kind = it->first.kind();
@@ -186,7 +186,7 @@ void RasterCache::SweepOneCacheAfterFrame(RasterCacheKey::Map<Entry>& cache,
           break;
       }
     }
-    entry.used_this_frame = false;
+    entry.encountered_this_frame = false;
   }
 
   for (auto it : dead) {

flow/raster_cache.h

@@ -123,8 +123,6 @@ class RasterCache {
             SkCanvas& canvas,
             const SkPaint* paint) const;
 
-  bool Touch(const RasterCacheKeyID& id, const SkMatrix& matrix) const;
-
   bool HasEntry(const RasterCacheKeyID& id, const SkMatrix&) const;
 
   void PrepareNewFrame();
@@ -191,13 +189,24 @@ class RasterCache {
   }
 
   /**
-   * @brief The entry which RasterCacheId is generated by RasterCacheKeyID and
-   * matrix, that will be used by the current frame. If current frame doesn't
-   * have this entry we will create a new entry.
-   * @return the number of times the entry has been hit since it was created. If
-   * a new entry that will be 1.
+   * @brief The entry whose RasterCacheKey is generated by RasterCacheKeyID
+   * and matrix is marked as encountered by the current frame. The entry
+   * will be created if it does not exist. Optionally the entry will be marked
+   * as visible in the current frame if the caller determines that it
+   * intersects the cull rect. The access_count of the entry will be
+   * increased if it is visible, or if it was ever visible.
+   * @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) const;
+
+  /**
+   * Returns the access count (i.e. accesses_since_visible) for the given
+   * entry in the cache, or -1 if no such entry exists.
    */
-  int MarkSeen(const RasterCacheKeyID& id, const SkMatrix& matrix) const;
+  int GetAccessCount(const RasterCacheKeyID& id, const SkMatrix& matrix) const;
 
   bool UpdateCacheEntry(
       const RasterCacheKeyID& id,
@@ -206,8 +215,9 @@ class RasterCache {
 
  private:
   struct Entry {
-    bool used_this_frame = false;
-    size_t access_count = 0;
+    bool encountered_this_frame = false;
+    bool visible_this_frame = false;
+    size_t accesses_since_visible = 0;
     std::unique_ptr<RasterCacheResult> image;
   };
 

flow/testing/mock_raster_cache.cc

@@ -61,10 +61,10 @@ 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++) {
-    MarkSeen(display_list_item.GetId().value(), ctm);
+    MarkSeen(display_list_item.GetId().value(), ctm, true);
     Draw(display_list_item.GetId().value(), mock_canvas_, nullptr);
   }
-  MarkSeen(display_list_item.GetId().value(), ctm);
+  MarkSeen(display_list_item.GetId().value(), ctm, true);
   RasterCache::Context r_context = {
       // clang-format off
       .gr_context         = preroll_context_.gr_context,