From bb461554e4be6e8fffc7b5925d80dcf4f24955be Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Fri, 17 Apr 2020 14:45:01 -0700 Subject: [PATCH 1/5] Replace RasterCache::Get with RasterCache:Draw This avoids the possible matrix mismatch between RasterCache::Get and RasterCacheResult::draw. See https://github.com/flutter/engine/pull/17790 for an example that tries to fix an earlier mismatch. --- flow/layers/image_filter_layer.cc | 11 +++------ flow/layers/opacity_layer.cc | 12 ++++------ flow/layers/picture_layer.cc | 13 ++++------- flow/raster_cache.cc | 29 ++++++++++++++++------- flow/raster_cache.h | 15 ++++++++++-- flow/raster_cache_unittests.cc | 39 ++++++++++++++++++++----------- 6 files changed, 69 insertions(+), 50 deletions(-) diff --git a/flow/layers/image_filter_layer.cc b/flow/layers/image_filter_layer.cc index 212c396b6efbf..bf787a3ea6ff6 100644 --- a/flow/layers/image_filter_layer.cc +++ b/flow/layers/image_filter_layer.cc @@ -48,14 +48,9 @@ void ImageFilterLayer::Paint(PaintContext& context) const { context.leaf_nodes_canvas->getTotalMatrix())); #endif - if (context.raster_cache) { - const SkMatrix& ctm = context.leaf_nodes_canvas->getTotalMatrix(); - RasterCacheResult layer_cache = - context.raster_cache->Get((Layer*)this, ctm); - if (layer_cache.is_valid()) { - layer_cache.draw(*context.leaf_nodes_canvas); - return; - } + if (context.raster_cache && + context.raster_cache->Draw(this, *context.leaf_nodes_canvas)) { + return; } SkPaint paint; diff --git a/flow/layers/opacity_layer.cc b/flow/layers/opacity_layer.cc index 7899c31784b89..1eeb640e03c63 100644 --- a/flow/layers/opacity_layer.cc +++ b/flow/layers/opacity_layer.cc @@ -74,14 +74,10 @@ void OpacityLayer::Paint(PaintContext& context) const { context.leaf_nodes_canvas->getTotalMatrix())); #endif - if (context.raster_cache) { - ContainerLayer* container = GetChildContainer(); - const SkMatrix& ctm = context.leaf_nodes_canvas->getTotalMatrix(); - RasterCacheResult child_cache = context.raster_cache->Get(container, ctm); - if (child_cache.is_valid()) { - child_cache.draw(*context.leaf_nodes_canvas, &paint); - return; - } + if (context.raster_cache && + context.raster_cache->Draw(GetChildContainer(), + *context.leaf_nodes_canvas, &paint)) { + return; } // Skia may clip the content with saveLayerBounds (although it's not a diff --git a/flow/layers/picture_layer.cc b/flow/layers/picture_layer.cc index 08c09cc9e833b..c60b09264f2f1 100644 --- a/flow/layers/picture_layer.cc +++ b/flow/layers/picture_layer.cc @@ -49,15 +49,10 @@ void PictureLayer::Paint(PaintContext& context) const { context.leaf_nodes_canvas->getTotalMatrix())); #endif - if (context.raster_cache) { - const SkMatrix& ctm = context.leaf_nodes_canvas->getTotalMatrix(); - RasterCacheResult result = context.raster_cache->Get(*picture(), ctm); - if (result.is_valid()) { - TRACE_EVENT_INSTANT0("flutter", "raster cache hit"); - - result.draw(*context.leaf_nodes_canvas); - return; - } + if (context.raster_cache && + context.raster_cache->Draw(*picture(), *context.leaf_nodes_canvas)) { + TRACE_EVENT_INSTANT0("flutter", "raster cache hit"); + return; } picture()->playback(context.leaf_nodes_canvas); } diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index e7709b14c7bf0..aeea99ae5563f 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -210,33 +210,44 @@ bool RasterCache::Prepare(GrContext* context, return true; } -RasterCacheResult RasterCache::Get(const SkPicture& picture, - const SkMatrix& ctm) const { - PictureRasterCacheKey cache_key(picture.uniqueID(), ctm); +bool RasterCache::Draw(const SkPicture& picture, SkCanvas& canvas) const { + PictureRasterCacheKey cache_key(picture.uniqueID(), canvas.getTotalMatrix()); auto it = picture_cache_.find(cache_key); if (it == picture_cache_.end()) { - return RasterCacheResult(); + return false; } Entry& entry = it->second; entry.access_count++; entry.used_this_frame = true; - return entry.image; + if (entry.image.is_valid()) { + entry.image.draw(canvas); + return true; + } + + return false; } -RasterCacheResult RasterCache::Get(Layer* layer, const SkMatrix& ctm) const { - LayerRasterCacheKey cache_key(layer->unique_id(), ctm); +bool RasterCache::Draw(const Layer* layer, + SkCanvas& canvas, + SkPaint* paint) const { + LayerRasterCacheKey cache_key(layer->unique_id(), canvas.getTotalMatrix()); auto it = layer_cache_.find(cache_key); if (it == layer_cache_.end()) { - return RasterCacheResult(); + return false; } Entry& entry = it->second; entry.access_count++; entry.used_this_frame = true; - return entry.image; + if (entry.image.is_valid()) { + entry.image.draw(canvas); + return true; + } + + return false; } void RasterCache::SweepAfterFrame() { diff --git a/flow/raster_cache.h b/flow/raster_cache.h index b51382a5c8d2d..3f7f659da0f69 100644 --- a/flow/raster_cache.h +++ b/flow/raster_cache.h @@ -86,9 +86,20 @@ class RasterCache { void Prepare(PrerollContext* context, Layer* layer, const SkMatrix& ctm); - RasterCacheResult Get(const SkPicture& picture, const SkMatrix& ctm) const; + // Find the raster cache for the picture and draw it to the canvas. + // + // Return true if it's found and drawn. + bool Draw(const SkPicture& picture, SkCanvas& canvas) const; - RasterCacheResult Get(Layer* layer, const SkMatrix& ctm) const; + // Find the raster cache for the layer and draw it to the canvas. + // + // Addional paint can be given to change how the raster cache is drawn (e.g., + // draw the raster cache with some opacity). + // + // Return true if the layer raster cache is found and drawn. + bool Draw(const Layer* layer, + SkCanvas& canvas, + SkPaint* paint = nullptr) const; void SweepAfterFrame(); diff --git a/flow/raster_cache_unittests.cc b/flow/raster_cache_unittests.cc index 0c00a47087f40..b9a0d2b293b6d 100644 --- a/flow/raster_cache_unittests.cc +++ b/flow/raster_cache_unittests.cc @@ -39,11 +39,13 @@ TEST(RasterCache, ThresholdIsRespected) { sk_sp image; + SkCanvas dummy_canvas; + sk_sp srgb = SkColorSpace::MakeSRGB(); ASSERT_FALSE( cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false)); // 1st access. - ASSERT_FALSE(cache.Get(*picture, matrix).is_valid()); + ASSERT_FALSE(cache.Draw(*picture, dummy_canvas)); cache.SweepAfterFrame(); @@ -51,14 +53,14 @@ TEST(RasterCache, ThresholdIsRespected) { cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false)); // 2st access. - ASSERT_FALSE(cache.Get(*picture, matrix).is_valid()); + ASSERT_FALSE(cache.Draw(*picture, dummy_canvas)); cache.SweepAfterFrame(); // Now Prepare should cache it. ASSERT_TRUE( cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false)); - ASSERT_TRUE(cache.Get(*picture, matrix).is_valid()); + ASSERT_TRUE(cache.Draw(*picture, dummy_canvas)); } TEST(RasterCache, AccessThresholdOfZeroDisablesCaching) { @@ -71,11 +73,13 @@ TEST(RasterCache, AccessThresholdOfZeroDisablesCaching) { sk_sp image; + SkCanvas dummy_canvas; + sk_sp srgb = SkColorSpace::MakeSRGB(); ASSERT_FALSE( cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false)); - ASSERT_FALSE(cache.Get(*picture, matrix).is_valid()); + ASSERT_FALSE(cache.Draw(*picture, dummy_canvas)); } TEST(RasterCache, PictureCacheLimitPerFrameIsRespectedWhenZero) { @@ -88,11 +92,13 @@ TEST(RasterCache, PictureCacheLimitPerFrameIsRespectedWhenZero) { sk_sp image; + SkCanvas dummy_canvas; + sk_sp srgb = SkColorSpace::MakeSRGB(); ASSERT_FALSE( cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false)); - ASSERT_FALSE(cache.Get(*picture, matrix).is_valid()); + ASSERT_FALSE(cache.Draw(*picture, dummy_canvas)); } TEST(RasterCache, SweepsRemoveUnusedFrames) { @@ -105,21 +111,23 @@ TEST(RasterCache, SweepsRemoveUnusedFrames) { sk_sp image; + SkCanvas dummy_canvas; + sk_sp srgb = SkColorSpace::MakeSRGB(); ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false)); // 1 - ASSERT_FALSE(cache.Get(*picture, matrix).is_valid()); + ASSERT_FALSE(cache.Draw(*picture, dummy_canvas)); cache.SweepAfterFrame(); ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false)); // 2 - ASSERT_TRUE(cache.Get(*picture, matrix).is_valid()); + ASSERT_TRUE(cache.Draw(*picture, dummy_canvas)); cache.SweepAfterFrame(); cache.SweepAfterFrame(); // Extra frame without a Get image access. - ASSERT_FALSE(cache.Get(*picture, matrix).is_valid()); + ASSERT_FALSE(cache.Draw(*picture, dummy_canvas)); } // Construct a cache result whose device target rectangle rounds out to be one @@ -139,19 +147,22 @@ TEST(RasterCache, DeviceRectRoundOut) { SkMatrix ctm = SkMatrix::MakeAll(1.3312, 0, 233, 0, 1.3312, 206, 0, 0, 1); + SkCanvas canvas(100, 100, nullptr); + canvas.setMatrix(ctm); + sk_sp srgb = SkColorSpace::MakeSRGB(); ASSERT_FALSE( cache.Prepare(NULL, picture.get(), ctm, srgb.get(), true, false)); - ASSERT_FALSE(cache.Get(*picture, ctm).is_valid()); + ASSERT_FALSE(cache.Draw(*picture, canvas)); cache.SweepAfterFrame(); ASSERT_TRUE(cache.Prepare(NULL, picture.get(), ctm, srgb.get(), true, false)); - ASSERT_TRUE(cache.Get(*picture, ctm).is_valid()); + ASSERT_TRUE(cache.Draw(*picture, canvas)); - SkCanvas canvas(100, 100, nullptr); - canvas.setMatrix(ctm); canvas.translate(248, 0); - - cache.Get(*picture, ctm).draw(canvas); +#ifndef SUPPORT_FRACTIONAL_TRANSLATION + canvas.setMatrix(RasterCache::GetIntegralTransCTM(canvas.getTotalMatrix())); +#endif + cache.Draw(*picture, canvas); } } // namespace testing From 29467a6fac7e4bfc56a45d89cec2b3c9e93ae403 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Fri, 17 Apr 2020 14:56:53 -0700 Subject: [PATCH 2/5] Tighten the assert --- flow/raster_cache.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index aeea99ae5563f..87e20a9952ec3 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -26,9 +26,8 @@ void RasterCacheResult::draw(SkCanvas& canvas, const SkPaint* paint) const { SkAutoCanvasRestore auto_restore(&canvas, true); SkIRect bounds = RasterCache::GetDeviceBounds(logical_rect_, canvas.getTotalMatrix()); - FML_DCHECK( - std::abs(bounds.size().width() - image_->dimensions().width()) <= 1 && - std::abs(bounds.size().height() - image_->dimensions().height()) <= 1); + FML_DCHECK(bounds.size().width() == image_->dimensions().width() && + bounds.size().height() == image_->dimensions().height()); canvas.resetMatrix(); canvas.drawImage(image_, bounds.fLeft, bounds.fTop, paint); } From 7b04f6b8319efa24266c1966f1b11f9e4c9c1320 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Sat, 18 Apr 2020 11:19:52 -0700 Subject: [PATCH 3/5] Fixes --- flow/raster_cache_unittests.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/flow/raster_cache_unittests.cc b/flow/raster_cache_unittests.cc index b9a0d2b293b6d..3df335f510966 100644 --- a/flow/raster_cache_unittests.cc +++ b/flow/raster_cache_unittests.cc @@ -52,7 +52,7 @@ TEST(RasterCache, ThresholdIsRespected) { ASSERT_FALSE( cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false)); - // 2st access. + // 2nd access. ASSERT_FALSE(cache.Draw(*picture, dummy_canvas)); cache.SweepAfterFrame(); @@ -162,7 +162,7 @@ TEST(RasterCache, DeviceRectRoundOut) { #ifndef SUPPORT_FRACTIONAL_TRANSLATION canvas.setMatrix(RasterCache::GetIntegralTransCTM(canvas.getTotalMatrix())); #endif - cache.Draw(*picture, canvas); + ASSERT_TRUE(cache.Draw(*picture, canvas)); } } // namespace testing From b22c40b4379407dc790cd9ed7f57ff5b1b775471 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Mon, 20 Apr 2020 15:22:25 -0700 Subject: [PATCH 4/5] Respect paint --- flow/raster_cache.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index 87e20a9952ec3..e2c7992dc9bc0 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -242,7 +242,7 @@ bool RasterCache::Draw(const Layer* layer, entry.used_this_frame = true; if (entry.image.is_valid()) { - entry.image.draw(canvas); + entry.image.draw(canvas, paint); return true; } From 5cf9804c61fd0111f57bc23fe19f1efd2b7bd5af Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Tue, 21 Apr 2020 16:32:39 -0700 Subject: [PATCH 5/5] Revert "Tighten the assert" This reverts commit b428ca150abe1316c7d1807a0bc34e18b5e2b991. The rare floating point error may cause the 1-off difference. Although in most cases, they should exactly match. --- flow/raster_cache.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index e2c7992dc9bc0..d41408beb5ab6 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -26,8 +26,9 @@ void RasterCacheResult::draw(SkCanvas& canvas, const SkPaint* paint) const { SkAutoCanvasRestore auto_restore(&canvas, true); SkIRect bounds = RasterCache::GetDeviceBounds(logical_rect_, canvas.getTotalMatrix()); - FML_DCHECK(bounds.size().width() == image_->dimensions().width() && - bounds.size().height() == image_->dimensions().height()); + FML_DCHECK( + std::abs(bounds.size().width() - image_->dimensions().width()) <= 1 && + std::abs(bounds.size().height() - image_->dimensions().height()) <= 1); canvas.resetMatrix(); canvas.drawImage(image_, bounds.fLeft, bounds.fTop, paint); }