From 01c87693a3ad70da55932c67bb2db3040b56b220 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Fri, 14 Apr 2023 23:18:15 -0700 Subject: [PATCH 1/3] focus SkiaGPUObject wrappers on only those objects that need the protection --- display_list/display_list.cc | 5 +- display_list/display_list.h | 3 + display_list/dl_builder.cc | 13 ++- display_list/dl_builder.h | 2 + display_list/effects/dl_color_source.h | 35 ++++++++ display_list/image/dl_image.h | 10 +++ display_list/image/dl_image_skia.cc | 11 +++ display_list/image/dl_image_skia.h | 3 + flow/layers/display_list_layer.cc | 20 +++-- flow/layers/display_list_layer.h | 9 +-- flow/layers/display_list_layer_unittests.cc | 50 ++++++------ flow/raster_cache_unittests.cc | 7 +- flow/testing/diff_context_test.cc | 6 +- flow/testing/diff_context_test.h | 2 +- .../display_list_image_impeller.cc | 6 ++ .../display_list_image_impeller.h | 3 + lib/ui/compositing/scene_builder.cc | 5 +- ...isplay_list_deferred_image_gpu_impeller.cc | 5 ++ ...display_list_deferred_image_gpu_impeller.h | 3 + .../display_list_deferred_image_gpu_skia.cc | 5 ++ .../display_list_deferred_image_gpu_skia.h | 3 + lib/ui/painting/display_list_image_gpu.cc | 15 ++++ lib/ui/painting/display_list_image_gpu.h | 4 + lib/ui/painting/fragment_shader.cc | 15 +++- lib/ui/painting/gradient.cc | 8 ++ lib/ui/painting/image.h | 5 +- lib/ui/painting/image_encoding_unittests.cc | 1 + lib/ui/painting/image_shader.cc | 13 +-- lib/ui/painting/image_shader.h | 2 +- lib/ui/painting/picture.cc | 22 +++--- lib/ui/painting/picture.h | 13 ++- lib/ui/painting/picture_recorder.cc | 3 +- lib/ui/painting/scene/scene_shader.cc | 11 ++- shell/common/shell_unittests.cc | 79 +++---------------- shell/common/snapshot_controller_skia.cc | 3 +- .../android/android_external_texture_gl.cc | 3 +- shell/platform/darwin/graphics/BUILD.gn | 1 + .../FlutterDarwinExternalTextureMetal.mm | 5 +- .../embedder/embedder_external_texture_gl.cc | 3 +- .../embedder_external_texture_metal.mm | 3 +- .../tests/embedder_metal_unittests.mm | 4 + 41 files changed, 252 insertions(+), 167 deletions(-) diff --git a/display_list/display_list.cc b/display_list/display_list.cc index 146eef1296b69..2425f699a16c0 100644 --- a/display_list/display_list.cc +++ b/display_list/display_list.cc @@ -21,7 +21,8 @@ DisplayList::DisplayList() nested_op_count_(0), unique_id_(0), bounds_({0, 0, 0, 0}), - can_apply_group_opacity_(true) {} + can_apply_group_opacity_(true), + is_ui_thread_safe_(true) {} DisplayList::DisplayList(DisplayListStorage&& storage, size_t byte_count, @@ -30,6 +31,7 @@ DisplayList::DisplayList(DisplayListStorage&& storage, unsigned int nested_op_count, const SkRect& bounds, bool can_apply_group_opacity, + bool is_ui_thread_safe, sk_sp rtree) : storage_(std::move(storage)), byte_count_(byte_count), @@ -39,6 +41,7 @@ DisplayList::DisplayList(DisplayListStorage&& storage, unique_id_(next_unique_id()), bounds_(bounds), can_apply_group_opacity_(can_apply_group_opacity), + is_ui_thread_safe_(is_ui_thread_safe), rtree_(std::move(rtree)) {} DisplayList::~DisplayList() { diff --git a/display_list/display_list.h b/display_list/display_list.h index 42a7de0ee9390..cbc9eb7925d8f 100644 --- a/display_list/display_list.h +++ b/display_list/display_list.h @@ -262,6 +262,7 @@ class DisplayList : public SkRefCnt { } bool can_apply_group_opacity() const { return can_apply_group_opacity_; } + bool isUIThreadSafe() const { return is_ui_thread_safe_; } static void DisposeOps(uint8_t* ptr, uint8_t* end); @@ -273,6 +274,7 @@ class DisplayList : public SkRefCnt { unsigned int nested_op_count, const SkRect& bounds, bool can_apply_group_opacity, + bool is_ui_thread_safe, sk_sp rtree); static uint32_t next_unique_id(); @@ -288,6 +290,7 @@ class DisplayList : public SkRefCnt { const SkRect bounds_; const bool can_apply_group_opacity_; + const bool is_ui_thread_safe_; const sk_sp rtree_; void Dispatch(DlOpReceiver& ctx, diff --git a/display_list/dl_builder.cc b/display_list/dl_builder.cc index 68f790c5c9ca2..d48659426add3 100644 --- a/display_list/dl_builder.cc +++ b/display_list/dl_builder.cc @@ -73,9 +73,10 @@ sk_sp DisplayListBuilder::Build() { nested_bytes_ = nested_op_count_ = 0; storage_.realloc(bytes); bool compatible = layer_stack_.back().is_group_opacity_compatible(); - return sk_sp(new DisplayList(std::move(storage_), bytes, count, - nested_bytes, nested_count, - bounds(), compatible, rtree())); + bool is_safe = is_ui_thread_safe_; + return sk_sp( + new DisplayList(std::move(storage_), bytes, count, nested_bytes, + nested_count, bounds(), compatible, is_safe, rtree())); } DisplayListBuilder::DisplayListBuilder(const SkRect& cull_rect, @@ -156,6 +157,7 @@ void DisplayListBuilder::onSetColorSource(const DlColorSource* source) { Push(0, 0); } else { current_.setColorSource(source->shared()); + is_ui_thread_safe_ = is_ui_thread_safe_ && source->isUIThreadSafe(); switch (source->type()) { case DlColorSourceType::kColor: { const DlColorColorSource* color_source = source->asColor(); @@ -923,6 +925,7 @@ void DisplayListBuilder::drawImage(const sk_sp image, ? Push(0, 1, image, point, sampling) : Push(0, 1, image, point, sampling); CheckLayerOpacityCompatibility(render_with_attributes); + is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); SkRect bounds = SkRect::MakeXYWH(point.fX, point.fY, // image->width(), image->height()); DisplayListAttributeFlags flags = render_with_attributes // @@ -951,6 +954,7 @@ void DisplayListBuilder::drawImageRect(const sk_sp image, Push(0, 1, image, src, dst, sampling, render_with_attributes, constraint); CheckLayerOpacityCompatibility(render_with_attributes); + is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); DisplayListAttributeFlags flags = render_with_attributes ? kDrawImageRectWithPaintFlags : kDrawImageRectFlags; @@ -979,6 +983,7 @@ void DisplayListBuilder::drawImageNine(const sk_sp image, ? Push(0, 1, image, center, dst, filter) : Push(0, 1, image, center, dst, filter); CheckLayerOpacityCompatibility(render_with_attributes); + is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); DisplayListAttributeFlags flags = render_with_attributes ? kDrawImageNineWithPaintFlags : kDrawImageNineFlags; @@ -1034,6 +1039,7 @@ void DisplayListBuilder::drawAtlas(const sk_sp atlas, // on it to distribute the opacity without overlap without checking all // of the transforms and texture rectangles. UpdateLayerOpacityCompatibility(false); + is_ui_thread_safe_ = is_ui_thread_safe_ && atlas->isUIThreadSafe(); SkPoint quad[4]; RectBoundsAccumulator atlasBounds; @@ -1075,6 +1081,7 @@ void DisplayListBuilder::DrawDisplayList(const sk_sp display_list, SkScalar opacity) { DlPaint current_paint = current_; Push(0, 1, display_list, opacity); + is_ui_thread_safe_ = is_ui_thread_safe_ && display_list->isUIThreadSafe(); // Not really necessary if the developer is interacting with us via // our attribute-state-less DlCanvas methods, but this avoids surprises // for those who may have been using the stateful Dispatcher methods. diff --git a/display_list/dl_builder.h b/display_list/dl_builder.h index 4917783e263c2..e35e160e704dd 100644 --- a/display_list/dl_builder.h +++ b/display_list/dl_builder.h @@ -480,6 +480,8 @@ class DisplayListBuilder final : public virtual DlCanvas, size_t nested_bytes_ = 0; int nested_op_count_ = 0; + bool is_ui_thread_safe_ = true; + template void* Push(size_t extra, int op_inc, Args&&... args); diff --git a/display_list/effects/dl_color_source.h b/display_list/effects/dl_color_source.h index b34e8eed87069..394fe092af0fe 100644 --- a/display_list/effects/dl_color_source.h +++ b/display_list/effects/dl_color_source.h @@ -109,6 +109,16 @@ class DlColorSource : public DlAttribute { virtual bool is_opaque() const = 0; + //---------------------------------------------------------------------------- + /// @brief If the underlying platform data held by this object is + /// held in a way that it can be stored and potentially + /// released from the UI thread, this method returns true. + /// + /// @return True if the class has no GPU related resources or if any + /// that it holds are held in a thread-safe manner. + /// + virtual bool isUIThreadSafe() const = 0; + // Return a DlColorColorSource pointer to this object iff it is an Color // type of ColorSource, otherwise return nullptr. virtual const DlColorColorSource* asColor() const { return nullptr; } @@ -160,6 +170,8 @@ class DlColorColorSource final : public DlColorSource { public: DlColorColorSource(DlColor color) : color_(color) {} + bool isUIThreadSafe() const override { return true; } + std::shared_ptr shared() const override { return std::make_shared(color_); } @@ -215,6 +227,10 @@ class DlImageColorSource final : public SkRefCnt, vertical_tile_mode_(vertical_tile_mode), sampling_(sampling) {} + bool isUIThreadSafe() const override { + return image_ ? image_->isUIThreadSafe() : true; + } + const DlImageColorSource* asImage() const override { return this; } std::shared_ptr shared() const override { @@ -338,6 +354,8 @@ class DlLinearGradientColorSource final : public DlGradientColorSourceBase { return this; } + bool isUIThreadSafe() const override { return true; } + DlColorSourceType type() const override { return DlColorSourceType::kLinearGradient; } @@ -399,6 +417,8 @@ class DlRadialGradientColorSource final : public DlGradientColorSourceBase { return this; } + bool isUIThreadSafe() const override { return true; } + std::shared_ptr shared() const override { return MakeRadial(center_, radius_, stop_count(), colors(), stops(), tile_mode(), matrix_ptr()); @@ -460,6 +480,8 @@ class DlConicalGradientColorSource final : public DlGradientColorSourceBase { return this; } + bool isUIThreadSafe() const override { return true; } + std::shared_ptr shared() const override { return MakeConical(start_center_, start_radius_, end_center_, end_radius_, stop_count(), colors(), stops(), tile_mode(), @@ -534,6 +556,8 @@ class DlSweepGradientColorSource final : public DlGradientColorSourceBase { return this; } + bool isUIThreadSafe() const override { return true; } + std::shared_ptr shared() const override { return MakeSweep(center_, start_, end_, stop_count(), colors(), stops(), tile_mode(), matrix_ptr()); @@ -604,6 +628,15 @@ class DlRuntimeEffectColorSource final : public DlColorSource { samplers_(std::move(samplers)), uniform_data_(std::move(uniform_data)) {} + bool isUIThreadSafe() const override { + for (auto sampler : samplers_) { + if (!sampler->isUIThreadSafe()) { + return false; + } + } + return true; + } + const DlRuntimeEffectColorSource* asRuntimeEffect() const override { return this; } @@ -666,6 +699,8 @@ class DlSceneColorSource final : public DlColorSource { impeller::Matrix camera_matrix) : node_(std::move(node)), camera_matrix_(camera_matrix) {} + bool isUIThreadSafe() const override { return true; } + const DlSceneColorSource* asScene() const override { return this; } std::shared_ptr shared() const override { diff --git a/display_list/image/dl_image.h b/display_list/image/dl_image.h index 19ae1d935bf6d..0b78b55a4c934 100644 --- a/display_list/image/dl_image.h +++ b/display_list/image/dl_image.h @@ -67,6 +67,16 @@ class DlImage : public SkRefCnt { virtual bool isTextureBacked() const = 0; + //---------------------------------------------------------------------------- + /// @brief If the underlying platform image held by this object has no + /// threading requirements for the release of that image (or if + /// arrangements have already been made to forward that image to + /// the correct thread upon deletion), this method returns true. + /// + /// @return True if the underlying image is held in a thread-safe manner. + /// + virtual bool isUIThreadSafe() const = 0; + //---------------------------------------------------------------------------- /// @return The dimensions of the pixel grid. /// diff --git a/display_list/image/dl_image_skia.cc b/display_list/image/dl_image_skia.cc index da6398ebd3603..8c896e5c6f6f8 100644 --- a/display_list/image/dl_image_skia.cc +++ b/display_list/image/dl_image_skia.cc @@ -31,6 +31,17 @@ bool DlImageSkia::isTextureBacked() const { return image_ ? image_->isTextureBacked() : false; } +// |DlImage| +bool DlImageSkia::isUIThreadSafe() const { + // Technically if the image is null then we are thread-safe, and possibly + // if the image is constructed from a heap raster as well, but there + // should never be a leak of an instance of this class into any data that + // is shared with the UI thread, regardless of value. + // All images intended to be shared with the UI thread should be constructed + // via one of the DlImage subclasses designed for that purpose. + return false; +} + // |DlImage| SkISize DlImageSkia::dimensions() const { return image_ ? image_->dimensions() : SkISize::MakeEmpty(); diff --git a/display_list/image/dl_image_skia.h b/display_list/image/dl_image_skia.h index c277cb2d04ed7..a29985d095929 100644 --- a/display_list/image/dl_image_skia.h +++ b/display_list/image/dl_image_skia.h @@ -29,6 +29,9 @@ class DlImageSkia final : public DlImage { // |DlImage| bool isTextureBacked() const override; + // |DlImage| + bool isUIThreadSafe() const override; + // |DlImage| SkISize dimensions() const override; diff --git a/flow/layers/display_list_layer.cc b/flow/layers/display_list_layer.cc index 7a36358cc240f..543af526be64d 100644 --- a/flow/layers/display_list_layer.cc +++ b/flow/layers/display_list_layer.cc @@ -16,15 +16,14 @@ namespace flutter { DisplayListLayer::DisplayListLayer(const SkPoint& offset, - SkiaGPUObject display_list, + sk_sp display_list, bool is_complex, bool will_change) : offset_(offset), display_list_(std::move(display_list)) { - if (display_list_.skia_object() != nullptr) { - bounds_ = display_list_.skia_object()->bounds().makeOffset(offset_.x(), - offset_.y()); + if (display_list_) { + bounds_ = display_list_->bounds().makeOffset(offset_.x(), offset_.y()); display_list_raster_cache_item_ = DisplayListRasterCacheItem::Make( - display_list_.skia_object(), offset_, is_complex, will_change); + display_list_, offset_, is_complex, will_change); } } @@ -61,8 +60,8 @@ void DisplayListLayer::Diff(DiffContext* context, const Layer* old_layer) { bool DisplayListLayer::Compare(DiffContext::Statistics& statistics, const DisplayListLayer* l1, const DisplayListLayer* l2) { - const auto& dl1 = l1->display_list_.skia_object(); - const auto& dl2 = l2->display_list_.skia_object(); + const auto& dl1 = l1->display_list_; + const auto& dl2 = l2->display_list_; if (dl1.get() == dl2.get()) { statistics.AddSameInstancePicture(); return true; @@ -105,7 +104,7 @@ void DisplayListLayer::Preroll(PrerollContext* context) { } void DisplayListLayer::Paint(PaintContext& context) const { - FML_DCHECK(display_list_.skia_object()); + FML_DCHECK(display_list_); FML_DCHECK(needs_painting(context)); auto mutator = context.state_stack.save(); @@ -143,7 +142,7 @@ void DisplayListLayer::Paint(PaintContext& context) const { DlAutoCanvasRestore save(canvas, true); canvas->Clear(DlColor::kTransparent()); canvas->SetTransform(ctm); - canvas->DrawDisplayList(display_list_.skia_object(), opacity); + canvas->DrawDisplayList(display_list_, opacity); } canvas->Flush(); } @@ -158,8 +157,7 @@ void DisplayListLayer::Paint(PaintContext& context) const { context.layer_snapshot_store->Add(snapshot_data); } - auto display_list = display_list_.skia_object(); - context.canvas->DrawDisplayList(display_list, opacity); + context.canvas->DrawDisplayList(display_list_, opacity); } } // namespace flutter diff --git a/flow/layers/display_list_layer.h b/flow/layers/display_list_layer.h index fd981e167a556..d7efe4e8ee0b9 100644 --- a/flow/layers/display_list_layer.h +++ b/flow/layers/display_list_layer.h @@ -11,7 +11,6 @@ #include "flutter/flow/layers/display_list_raster_cache_item.h" #include "flutter/flow/layers/layer.h" #include "flutter/flow/raster_cache_item.h" -#include "flutter/flow/skia_gpu_object.h" namespace flutter { @@ -20,13 +19,11 @@ class DisplayListLayer : public Layer { static constexpr size_t kMaxBytesToCompare = 10000; DisplayListLayer(const SkPoint& offset, - SkiaGPUObject display_list, + sk_sp display_list, bool is_complex, bool will_change); - DisplayList* display_list() const { - return display_list_.skia_object().get(); - } + DisplayList* display_list() const { return display_list_.get(); } bool IsReplacing(DiffContext* context, const Layer* layer) const override; @@ -55,7 +52,7 @@ class DisplayListLayer : public Layer { SkPoint offset_; SkRect bounds_; - flutter::SkiaGPUObject display_list_; + sk_sp display_list_; static bool Compare(DiffContext::Statistics& statistics, const DisplayListLayer* l1, diff --git a/flow/layers/display_list_layer_unittests.cc b/flow/layers/display_list_layer_unittests.cc index 1f0ef185a7967..eba4be82fbf78 100644 --- a/flow/layers/display_list_layer_unittests.cc +++ b/flow/layers/display_list_layer_unittests.cc @@ -9,23 +9,21 @@ #include "flutter/display_list/dl_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" #include "flutter/testing/mock_canvas.h" namespace flutter { namespace testing { -using DisplayListLayerTest = SkiaGPUObjectLayerTest; +using DisplayListLayerTest = LayerTest; #ifndef NDEBUG TEST_F(DisplayListLayerTest, PaintBeforePrerollInvalidDisplayListDies) { const SkPoint layer_offset = SkPoint::Make(0.0f, 0.0f); auto layer = std::make_shared( - layer_offset, SkiaGPUObject(), false, false); + layer_offset, sk_sp(), false, false); - EXPECT_DEATH_IF_SUPPORTED(layer->Paint(paint_context()), - "display_list_\\.skia_object\\(\\)"); + EXPECT_DEATH_IF_SUPPORTED(layer->Paint(paint_context()), "display_list_"); } TEST_F(DisplayListLayerTest, PaintBeforePrerollDies) { @@ -34,9 +32,8 @@ TEST_F(DisplayListLayerTest, PaintBeforePrerollDies) { DisplayListBuilder builder; builder.DrawRect(picture_bounds, DlPaint()); auto display_list = builder.Build(); - auto layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), - false, false); + auto layer = std::make_shared(layer_offset, display_list, + false, false); EXPECT_EQ(layer->paint_bounds(), SkRect::MakeEmpty()); EXPECT_DEATH_IF_SUPPORTED(layer->Paint(paint_context()), @@ -49,9 +46,8 @@ TEST_F(DisplayListLayerTest, PaintingEmptyLayerDies) { DisplayListBuilder builder; builder.DrawRect(picture_bounds, DlPaint()); auto display_list = builder.Build(); - auto layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), - false, false); + auto layer = std::make_shared(layer_offset, display_list, + false, false); layer->Preroll(preroll_context()); EXPECT_EQ(layer->paint_bounds(), SkRect::MakeEmpty()); @@ -64,7 +60,7 @@ TEST_F(DisplayListLayerTest, PaintingEmptyLayerDies) { TEST_F(DisplayListLayerTest, InvalidDisplayListDies) { const SkPoint layer_offset = SkPoint::Make(0.0f, 0.0f); auto layer = std::make_shared( - layer_offset, SkiaGPUObject(), false, false); + layer_offset, sk_sp(), false, false); // Crashes reading a nullptr. EXPECT_DEATH_IF_SUPPORTED(layer->Preroll(preroll_context()), ""); @@ -79,8 +75,8 @@ TEST_F(DisplayListLayerTest, SimpleDisplayList) { DisplayListBuilder builder; builder.DrawRect(picture_bounds, DlPaint()); auto display_list = builder.Build(); - auto layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), false, false); + auto layer = std::make_shared(layer_offset, display_list, + false, false); layer->Preroll(preroll_context()); EXPECT_EQ(layer->paint_bounds(), @@ -104,8 +100,8 @@ TEST_F(DisplayListLayerTest, CachingDoesNotChangeCullRect) { DisplayListBuilder builder; builder.DrawRect({10, 10, 20, 20}, DlPaint()); auto display_list = builder.Build(); - auto layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), true, false); + auto layer = std::make_shared(layer_offset, display_list, + true, false); SkRect original_cull_rect = preroll_context()->state_stack.device_cull_rect(); use_mock_raster_cache(); @@ -121,7 +117,7 @@ TEST_F(DisplayListLayerTest, SimpleDisplayListOpacityInheritance) { builder.DrawRect(picture_bounds, DlPaint()); auto display_list = builder.Build(); auto display_list_layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), false, false); + layer_offset, display_list, false, false); EXPECT_TRUE(display_list->can_apply_group_opacity()); auto context = preroll_context(); @@ -173,7 +169,7 @@ TEST_F(DisplayListLayerTest, IncompatibleDisplayListOpacityInheritance) { builder.DrawRect(picture2_bounds, DlPaint()); auto display_list = builder.Build(); auto display_list_layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), false, false); + layer_offset, display_list, false, false); EXPECT_FALSE(display_list->can_apply_group_opacity()); auto context = preroll_context(); @@ -233,7 +229,7 @@ TEST_F(DisplayListLayerTest, CachedIncompatibleDisplayListOpacityInheritance) { builder.DrawRect(picture2_bounds, DlPaint()); auto display_list = builder.Build(); auto display_list_layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), true, false); + layer_offset, display_list, true, false); EXPECT_FALSE(display_list->can_apply_group_opacity()); use_skia_raster_cache(); @@ -383,8 +379,8 @@ TEST_F(DisplayListLayerTest, LayerTreeSnapshotsWhenEnabled) { DisplayListBuilder builder; builder.DrawRect(picture_bounds, DlPaint()); auto display_list = builder.Build(); - auto layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), false, false); + auto layer = std::make_shared(layer_offset, display_list, + false, false); layer->Preroll(preroll_context()); @@ -402,8 +398,8 @@ TEST_F(DisplayListLayerTest, NoLayerTreeSnapshotsWhenDisabledByDefault) { DisplayListBuilder builder; builder.DrawRect(picture_bounds, DlPaint()); auto display_list = builder.Build(); - auto layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), false, false); + auto layer = std::make_shared(layer_offset, display_list, + false, false); layer->Preroll(preroll_context()); layer->Paint(paint_context()); @@ -420,8 +416,8 @@ TEST_F(DisplayListLayerTest, DisplayListAccessCountDependsOnVisibility) { DisplayListBuilder builder; builder.DrawRect(picture_bounds, DlPaint()); auto display_list = builder.Build(); - auto layer = std::make_shared( - layer_offset, SkiaGPUObject(display_list, unref_queue()), true, false); + auto layer = std::make_shared(layer_offset, display_list, + true, false); auto raster_cache_item = layer->raster_cache_item(); use_mock_raster_cache(); @@ -525,8 +521,8 @@ TEST_F(DisplayListLayerTest, OverflowCachedDisplayListOpacityInheritance) { 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); + layers[i] = + std::make_shared(offset, display_list, true, false); opacity_layer->Add(layers[i]); } for (size_t j = 0; j < context->raster_cache->access_threshold(); j++) { diff --git a/flow/raster_cache_unittests.cc b/flow/raster_cache_unittests.cc index 1484c58f68a78..00b99d05fd524 100644 --- a/flow/raster_cache_unittests.cc +++ b/flow/raster_cache_unittests.cc @@ -13,8 +13,8 @@ #include "flutter/flow/layers/transform_layer.h" #include "flutter/flow/raster_cache.h" #include "flutter/flow/raster_cache_item.h" +#include "flutter/flow/testing/layer_test.h" #include "flutter/flow/testing/mock_raster_cache.h" -#include "flutter/flow/testing/skia_gpu_object_layer_test.h" #include "flutter/testing/assertions_skia.h" #include "gtest/gtest.h" #include "include/core/SkMatrix.h" @@ -858,7 +858,7 @@ TEST(RasterCache, RasterCacheKeyIDHashCode) { ASSERT_EQ(fourth_hash, fourth.GetHash()); } -using RasterCacheTest = SkiaGPUObjectLayerTest; +using RasterCacheTest = LayerTest; TEST_F(RasterCacheTest, RasterCacheKeyIDLayerChildrenIds) { auto layer = std::make_shared(); @@ -869,8 +869,7 @@ TEST_F(RasterCacheTest, RasterCacheKeyIDLayerChildrenIds) { auto display_list = GetSampleDisplayList(); auto display_list_layer = std::make_shared( - SkPoint::Make(0.0f, 0.0f), - SkiaGPUObject(display_list, unref_queue()), false, false); + SkPoint::Make(0.0f, 0.0f), display_list, false, false); layer->Add(display_list_layer); auto ids = RasterCacheKeyID::LayerChildrenIds(layer.get()).value(); diff --git a/flow/testing/diff_context_test.cc b/flow/testing/diff_context_test.cc index 6bf66b212561a..b704be110a223 100644 --- a/flow/testing/diff_context_test.cc +++ b/flow/testing/diff_context_test.cc @@ -40,11 +40,9 @@ sk_sp DiffContextTest::CreateDisplayList(const SkRect& bounds, } std::shared_ptr DiffContextTest::CreateDisplayListLayer( - sk_sp display_list, + const sk_sp& display_list, const SkPoint& offset) { - return std::make_shared( - offset, SkiaGPUObject(std::move(display_list), unref_queue()), false, - false); + return std::make_shared(offset, display_list, false, false); } std::shared_ptr DiffContextTest::CreateContainerLayer( diff --git a/flow/testing/diff_context_test.h b/flow/testing/diff_context_test.h index 0401b3232028d..0c4223cfce52b 100644 --- a/flow/testing/diff_context_test.h +++ b/flow/testing/diff_context_test.h @@ -47,7 +47,7 @@ class DiffContextTest : public ThreadTest { sk_sp CreateDisplayList(const SkRect& bounds, uint32_t color); std::shared_ptr CreateDisplayListLayer( - sk_sp display_list, + const sk_sp& display_list, const SkPoint& offset = SkPoint::Make(0, 0)); std::shared_ptr CreateContainerLayer( diff --git a/impeller/display_list/display_list_image_impeller.cc b/impeller/display_list/display_list_image_impeller.cc index 01339088649ae..6cc69188b342a 100644 --- a/impeller/display_list/display_list_image_impeller.cc +++ b/impeller/display_list/display_list_image_impeller.cc @@ -64,6 +64,12 @@ bool DlImageImpeller::isTextureBacked() const { return true; } +// |DlImage| +bool DlImageImpeller::isUIThreadSafe() const { + // Impeller textures are always ... thread-safe :/ + return true; +} + // |DlImage| SkISize DlImageImpeller::dimensions() const { const auto size = texture_ ? texture_->GetSize() : ISize{}; diff --git a/impeller/display_list/display_list_image_impeller.h b/impeller/display_list/display_list_image_impeller.h index d76aaba5d3957..72e3b68171e65 100644 --- a/impeller/display_list/display_list_image_impeller.h +++ b/impeller/display_list/display_list_image_impeller.h @@ -39,6 +39,9 @@ class DlImageImpeller final : public flutter::DlImage { // |DlImage| bool isTextureBacked() const override; + // |DlImage| + bool isUIThreadSafe() const override; + // |DlImage| SkISize dimensions() const override; diff --git a/lib/ui/compositing/scene_builder.cc b/lib/ui/compositing/scene_builder.cc index 6c3e83ba76116..63ac03c92c872 100644 --- a/lib/ui/compositing/scene_builder.cc +++ b/lib/ui/compositing/scene_builder.cc @@ -243,9 +243,8 @@ void SceneBuilder::addPicture(double dx, // been disposed but not collected yet, but the display list is null. if (picture->display_list()) { auto layer = std::make_unique( - SkPoint::Make(SafeNarrow(dx), SafeNarrow(dy)), - UIDartState::CreateGPUObject(picture->display_list()), !!(hints & 1), - !!(hints & 2)); + SkPoint::Make(SafeNarrow(dx), SafeNarrow(dy)), picture->display_list(), + !!(hints & 1), !!(hints & 2)); AddLayer(std::move(layer)); } } diff --git a/lib/ui/painting/display_list_deferred_image_gpu_impeller.cc b/lib/ui/painting/display_list_deferred_image_gpu_impeller.cc index 49fb3e214d777..40d09b8ff00c6 100644 --- a/lib/ui/painting/display_list_deferred_image_gpu_impeller.cc +++ b/lib/ui/painting/display_list_deferred_image_gpu_impeller.cc @@ -63,6 +63,11 @@ bool DlDeferredImageGPUImpeller::isTextureBacked() const { return wrapper_ && wrapper_->isTextureBacked(); } +// |DlImage| +bool DlDeferredImageGPUImpeller::isUIThreadSafe() const { + return true; +} + // |DlImage| SkISize DlDeferredImageGPUImpeller::dimensions() const { if (!wrapper_) { diff --git a/lib/ui/painting/display_list_deferred_image_gpu_impeller.h b/lib/ui/painting/display_list_deferred_image_gpu_impeller.h index 08692ff2b4bd1..664818da8a0fc 100644 --- a/lib/ui/painting/display_list_deferred_image_gpu_impeller.h +++ b/lib/ui/painting/display_list_deferred_image_gpu_impeller.h @@ -44,6 +44,9 @@ class DlDeferredImageGPUImpeller final : public DlImage { // |DlImage| bool isTextureBacked() const override; + // |DlImage| + bool isUIThreadSafe() const override; + // |DlImage| SkISize dimensions() const override; diff --git a/lib/ui/painting/display_list_deferred_image_gpu_skia.cc b/lib/ui/painting/display_list_deferred_image_gpu_skia.cc index fa4441fb86e18..a2bb4c0b94cda 100644 --- a/lib/ui/painting/display_list_deferred_image_gpu_skia.cc +++ b/lib/ui/painting/display_list_deferred_image_gpu_skia.cc @@ -75,6 +75,11 @@ bool DlDeferredImageGPUSkia::isTextureBacked() const { return image_wrapper_ ? image_wrapper_->isTextureBacked() : false; } +// |DlImage| +bool DlDeferredImageGPUSkia::isUIThreadSafe() const { + return true; +} + // |DlImage| SkISize DlDeferredImageGPUSkia::dimensions() const { return image_wrapper_ ? image_wrapper_->image_info().dimensions() diff --git a/lib/ui/painting/display_list_deferred_image_gpu_skia.h b/lib/ui/painting/display_list_deferred_image_gpu_skia.h index 6abbaeacbe13a..44c9d360f86d7 100644 --- a/lib/ui/painting/display_list_deferred_image_gpu_skia.h +++ b/lib/ui/painting/display_list_deferred_image_gpu_skia.h @@ -55,6 +55,9 @@ class DlDeferredImageGPUSkia final : public DlImage { // |DlImage| bool isTextureBacked() const override; + // |DlImage| + bool isUIThreadSafe() const override; + // |DlImage| SkISize dimensions() const override; diff --git a/lib/ui/painting/display_list_image_gpu.cc b/lib/ui/painting/display_list_image_gpu.cc index ce69f06a15007..b06ce691c7998 100644 --- a/lib/ui/painting/display_list_image_gpu.cc +++ b/lib/ui/painting/display_list_image_gpu.cc @@ -4,6 +4,8 @@ #include "flutter/lib/ui/painting/display_list_image_gpu.h" +#include "flutter/lib/ui/ui_dart_state.h" + namespace flutter { sk_sp DlImageGPU::Make(SkiaGPUObject image) { @@ -13,6 +15,14 @@ sk_sp DlImageGPU::Make(SkiaGPUObject image) { return sk_sp(new DlImageGPU(std::move(image))); } +sk_sp DlImageGPU::Make(sk_sp image) { + if (!image) { + return nullptr; + } + return sk_sp( + new DlImageGPU(UIDartState::CreateGPUObject(std::move(image)))); +} + DlImageGPU::DlImageGPU(SkiaGPUObject image) : image_(std::move(image)) {} @@ -45,6 +55,11 @@ bool DlImageGPU::isTextureBacked() const { return false; } +// |DlImage| +bool DlImageGPU::isUIThreadSafe() const { + return true; +} + // |DlImage| SkISize DlImageGPU::dimensions() const { const auto image = skia_image(); diff --git a/lib/ui/painting/display_list_image_gpu.h b/lib/ui/painting/display_list_image_gpu.h index 9122ae63060fe..47d2acde39b53 100644 --- a/lib/ui/painting/display_list_image_gpu.h +++ b/lib/ui/painting/display_list_image_gpu.h @@ -14,6 +14,7 @@ namespace flutter { class DlImageGPU final : public DlImage { public: static sk_sp Make(SkiaGPUObject image); + static sk_sp Make(sk_sp image); // |DlImage| ~DlImageGPU() override; @@ -30,6 +31,9 @@ class DlImageGPU final : public DlImage { // |DlImage| bool isTextureBacked() const override; + // |DlImage| + bool isUIThreadSafe() const override; + // |DlImage| SkISize dimensions() const override; diff --git a/lib/ui/painting/fragment_shader.cc b/lib/ui/painting/fragment_shader.cc index 5a90c585e1718..491d312e7e6c0 100644 --- a/lib/ui/painting/fragment_shader.cc +++ b/lib/ui/painting/fragment_shader.cc @@ -59,6 +59,9 @@ bool ReusableFragmentShader::ValidateSamplers() { if (samplers_[i] == nullptr) { return false; } + // The samplers should have been checked as they were added, this + // is a double-sanity-check. + FML_DCHECK(samplers_[i]->isUIThreadSafe()); } return true; } @@ -71,12 +74,18 @@ void ReusableFragmentShader::SetImageSampler(Dart_Handle index_handle, if (index >= samplers_.size()) { Dart_ThrowException(tonic::ToDart("Sampler index out of bounds")); } + if (!image->image()->isUIThreadSafe()) { + Dart_ThrowException(tonic::ToDart("Image is not thread-safe")); + } // TODO(115794): Once the DlImageSampling enum is replaced, expose the // sampling options as a new default parameter for users. samplers_[index] = std::make_shared( image->image(), DlTileMode::kClamp, DlTileMode::kClamp, DlImageSampling::kNearestNeighbor, nullptr); + // This should be true since we already checked the image above, but + // we check again for sanity. + FML_DCHECK(samplers_[index]->isUIThreadSafe()); auto* uniform_floats = reinterpret_cast(uniform_data_->writable_data()); @@ -95,7 +104,11 @@ std::shared_ptr ReusableFragmentShader::shader( uniform_data->resize(uniform_data_->size()); memcpy(uniform_data->data(), uniform_data_->bytes(), uniform_data->size()); - return program_->MakeDlColorSource(std::move(uniform_data), samplers_); + auto source = program_->MakeDlColorSource(std::move(uniform_data), samplers_); + // The samplers should have been checked as they were added, this + // is a double-sanity-check. + FML_DCHECK(source->isUIThreadSafe()); + return source; } void ReusableFragmentShader::Dispose() { diff --git a/lib/ui/painting/gradient.cc b/lib/ui/painting/gradient.cc index 6f692add01776..e54f39f41fadd 100644 --- a/lib/ui/painting/gradient.cc +++ b/lib/ui/painting/gradient.cc @@ -50,6 +50,8 @@ void CanvasGradient::initLinear(const tonic::Float32List& end_points, dl_shader_ = DlColorSource::MakeLinear( p0, p1, colors.num_elements(), colors_array, color_stops.data(), tile_mode, has_matrix ? &sk_matrix : nullptr); + // Just a sanity check, all gradient shaders should be thread-safe + FML_DCHECK(dl_shader_->isUIThreadSafe()); } void CanvasGradient::initRadial(double center_x, @@ -77,6 +79,8 @@ void CanvasGradient::initRadial(double center_x, SkPoint::Make(SafeNarrow(center_x), SafeNarrow(center_y)), SafeNarrow(radius), colors.num_elements(), colors_array, color_stops.data(), tile_mode, has_matrix ? &sk_matrix : nullptr); + // Just a sanity check, all gradient shaders should be thread-safe + FML_DCHECK(dl_shader_->isUIThreadSafe()); } void CanvasGradient::initSweep(double center_x, @@ -107,6 +111,8 @@ void CanvasGradient::initSweep(double center_x, SafeNarrow(end_angle) * 180.0f / static_cast(M_PI), colors.num_elements(), colors_array, color_stops.data(), tile_mode, has_matrix ? &sk_matrix : nullptr); + // Just a sanity check, all gradient shaders should be thread-safe + FML_DCHECK(dl_shader_->isUIThreadSafe()); } void CanvasGradient::initTwoPointConical(double start_x, @@ -139,6 +145,8 @@ void CanvasGradient::initTwoPointConical(double start_x, SkPoint::Make(SafeNarrow(end_x), SafeNarrow(end_y)), SafeNarrow(end_radius), colors.num_elements(), colors_array, color_stops.data(), tile_mode, has_matrix ? &sk_matrix : nullptr); + // Just a sanity check, all gradient shaders should be thread-safe + FML_DCHECK(dl_shader_->isUIThreadSafe()); } CanvasGradient::CanvasGradient() = default; diff --git a/lib/ui/painting/image.h b/lib/ui/painting/image.h index 0ffd4ec4f5255..2352180b742de 100644 --- a/lib/ui/painting/image.h +++ b/lib/ui/painting/image.h @@ -41,7 +41,10 @@ class CanvasImage final : public RefCountedDartWrappable { sk_sp image() const { return image_; } - void set_image(sk_sp image) { image_ = image; } + void set_image(sk_sp image) { + FML_DCHECK(image->isUIThreadSafe()); + image_ = image; + } int colorSpace(); diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc index 40e587c523c87..8a111167fc64d 100644 --- a/lib/ui/painting/image_encoding_unittests.cc +++ b/lib/ui/painting/image_encoding_unittests.cc @@ -35,6 +35,7 @@ class MockDlImage : public DlImage { MOCK_CONST_METHOD0(impeller_texture, std::shared_ptr()); MOCK_CONST_METHOD0(isOpaque, bool()); MOCK_CONST_METHOD0(isTextureBacked, bool()); + MOCK_CONST_METHOD0(isUIThreadSafe, bool()); MOCK_CONST_METHOD0(dimensions, SkISize()); MOCK_CONST_METHOD0(GetApproximateByteSize, size_t()); }; diff --git a/lib/ui/painting/image_shader.cc b/lib/ui/painting/image_shader.cc index e2cdae7dfb308..0135f481e1393 100644 --- a/lib/ui/painting/image_shader.cc +++ b/lib/ui/painting/image_shader.cc @@ -28,7 +28,8 @@ Dart_Handle ImageShader::initWithImage(CanvasImage* image, DlTileMode tmy, int filter_quality_index, Dart_Handle matrix_handle) { - if (!image) { + // CanvasImage should have already checked for a UI thread safe image. + if (!image || !image->image()->isUIThreadSafe()) { return ToDart("ImageShader constructor called with non-genuine Image."); } @@ -40,15 +41,15 @@ Dart_Handle ImageShader::initWithImage(CanvasImage* image, DlImageSampling sampling = sampling_is_locked_ ? ImageFilter::SamplingFromIndex(filter_quality_index) : DlImageSampling::kLinear; - cached_shader_ = UIDartState::CreateGPUObject(sk_make_sp( - image_, tmx, tmy, sampling, &local_matrix)); + cached_shader_ = + sk_make_sp(image_, tmx, tmy, sampling, &local_matrix); + FML_DCHECK(cached_shader_->isUIThreadSafe()); return Dart_Null(); } std::shared_ptr ImageShader::shader(DlImageSampling sampling) { if (sampling_is_locked_) { - return cached_shader_.skia_object()->with_sampling( - cached_shader_.skia_object()->sampling()); + return cached_shader_->with_sampling(cached_shader_->sampling()); } // It might seem that if the sampling is locked we can just return the // cached version, but since we need to hold the cached shader in a @@ -58,7 +59,7 @@ std::shared_ptr ImageShader::shader(DlImageSampling sampling) { // our copy. // If we can get rid of the need for the GPU unref queue, then this can all // be simplified down to just a shared_ptr. - return cached_shader_.skia_object()->with_sampling(sampling); + return cached_shader_->with_sampling(sampling); } int ImageShader::width() { diff --git a/lib/ui/painting/image_shader.h b/lib/ui/painting/image_shader.h index 7c69d1389bd89..c5e8fc2c3435f 100644 --- a/lib/ui/painting/image_shader.h +++ b/lib/ui/painting/image_shader.h @@ -43,7 +43,7 @@ class ImageShader : public Shader { sk_sp image_; bool sampling_is_locked_; - flutter::SkiaGPUObject cached_shader_; + sk_sp cached_shader_; }; } // namespace flutter diff --git a/lib/ui/painting/picture.cc b/lib/ui/painting/picture.cc index 0121c85c7c662..3703c270a0a79 100644 --- a/lib/ui/painting/picture.cc +++ b/lib/ui/painting/picture.cc @@ -25,16 +25,16 @@ namespace flutter { IMPLEMENT_WRAPPERTYPEINFO(ui, Picture); -fml::RefPtr Picture::Create( - Dart_Handle dart_handle, - flutter::SkiaGPUObject display_list) { +fml::RefPtr Picture::Create(Dart_Handle dart_handle, + sk_sp display_list) { + FML_DCHECK(display_list->isUIThreadSafe()); auto canvas_picture = fml::MakeRefCounted(std::move(display_list)); canvas_picture->AssociateWithDartWrapper(dart_handle); return canvas_picture; } -Picture::Picture(flutter::SkiaGPUObject display_list) +Picture::Picture(sk_sp display_list) : display_list_(std::move(display_list)) {} Picture::~Picture() = default; @@ -42,19 +42,17 @@ Picture::~Picture() = default; Dart_Handle Picture::toImage(uint32_t width, uint32_t height, Dart_Handle raw_image_callback) { - if (!display_list_.skia_object()) { + if (!display_list_) { return tonic::ToDart("Picture is null"); } - return RasterizeToImage(display_list_.skia_object(), width, height, - raw_image_callback); + return RasterizeToImage(display_list_, width, height, raw_image_callback); } void Picture::toImageSync(uint32_t width, uint32_t height, Dart_Handle raw_image_handle) { - FML_DCHECK(display_list_.skia_object()); - RasterizeToImageSync(display_list_.skia_object(), width, height, - raw_image_handle); + FML_DCHECK(display_list_); + RasterizeToImageSync(display_list_, width, height, raw_image_handle); } static sk_sp CreateDeferredImage( @@ -108,8 +106,8 @@ void Picture::dispose() { } size_t Picture::GetAllocationSize() const { - if (auto display_list = display_list_.skia_object()) { - return display_list->bytes() + sizeof(Picture); + if (display_list_) { + return display_list_->bytes() + sizeof(Picture); } else { return sizeof(Picture); } diff --git a/lib/ui/painting/picture.h b/lib/ui/painting/picture.h index 33bee1994f9ec..edffcaacf4d1b 100644 --- a/lib/ui/painting/picture.h +++ b/lib/ui/painting/picture.h @@ -22,13 +22,10 @@ class Picture : public RefCountedDartWrappable { public: ~Picture() override; - static fml::RefPtr Create( - Dart_Handle dart_handle, - flutter::SkiaGPUObject display_list); + static fml::RefPtr Create(Dart_Handle dart_handle, + sk_sp display_list); - sk_sp display_list() const { - return display_list_.skia_object(); - } + sk_sp display_list() const { return display_list_; } Dart_Handle toImage(uint32_t width, uint32_t height, @@ -68,9 +65,9 @@ class Picture : public RefCountedDartWrappable { Dart_Handle raw_image_callback); private: - explicit Picture(flutter::SkiaGPUObject display_list); + explicit Picture(sk_sp display_list); - flutter::SkiaGPUObject display_list_; + sk_sp display_list_; }; } // namespace flutter diff --git a/lib/ui/painting/picture_recorder.cc b/lib/ui/painting/picture_recorder.cc index effbb196dce25..a99b5e5855e40 100644 --- a/lib/ui/painting/picture_recorder.cc +++ b/lib/ui/painting/picture_recorder.cc @@ -38,8 +38,7 @@ fml::RefPtr PictureRecorder::endRecording(Dart_Handle dart_picture) { fml::RefPtr picture; - picture = Picture::Create(dart_picture, UIDartState::CreateGPUObject( - display_list_builder_->Build())); + picture = Picture::Create(dart_picture, display_list_builder_->Build()); display_list_builder_ = nullptr; canvas_->Invalidate(); diff --git a/lib/ui/painting/scene/scene_shader.cc b/lib/ui/painting/scene/scene_shader.cc index 05382d628f779..fdfc0a1763461 100644 --- a/lib/ui/painting/scene/scene_shader.cc +++ b/lib/ui/painting/scene/scene_shader.cc @@ -78,10 +78,13 @@ std::shared_ptr SceneShader::shader(DlImageSampling sampling) { // TODO(bdero): Gather the mutation log and include it in the scene color // source. - return std::make_shared(scene_node_->node_, - camera_transform_.IsIdentity() - ? DefaultCameraTransform() - : camera_transform_); + auto source = std::make_shared( + scene_node_->node_, camera_transform_.IsIdentity() + ? DefaultCameraTransform() + : camera_transform_); + // Just a sanity check, all scene shaders should be thread-safe + FML_DCHECK(source->isUIThreadSafe()); + return source; } void SceneShader::Dispose() { diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 82dc5b3682457..293767a25ef55 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -804,13 +804,8 @@ TEST_F(ShellTest, ExternalEmbedderNoThreadMerger) { RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; @@ -935,13 +930,8 @@ TEST_F(ShellTest, RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; @@ -981,13 +971,8 @@ TEST_F(ShellTest, OnPlatformViewDestroyDisablesThreadMerger) { RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; @@ -1048,13 +1033,8 @@ TEST_F(ShellTest, OnPlatformViewDestroyAfterMergingThreads) { RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; @@ -1117,13 +1097,8 @@ TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; @@ -1185,13 +1160,8 @@ TEST_F(ShellTest, RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; PumpOneFrame(shell.get(), 100, 100, builder); @@ -1229,13 +1199,8 @@ TEST_F(ShellTest, OnPlatformViewDestroyWithoutRasterThreadMerger) { RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; PumpOneFrame(shell.get(), 100, 100, builder); @@ -1295,13 +1260,8 @@ TEST_F(ShellTest, OnPlatformViewDestroyWithStaticThreadMerging) { RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; PumpOneFrame(shell.get(), 100, 100, builder); @@ -1341,13 +1301,8 @@ TEST_F(ShellTest, GetUsedThisFrameShouldBeSetBeforeEndFrame) { RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; PumpOneFrame(shell.get(), 100, 100, builder); @@ -2083,13 +2038,8 @@ TEST_F(ShellTest, Screenshot) { RunEngine(shell.get(), std::move(configuration)); LayerTreeBuilder builder = [&](const std::shared_ptr& root) { - fml::RefPtr queue = fml::MakeRefCounted( - this->GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(10, 10), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(80, 80), queue}), - false, false); + SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; @@ -2405,6 +2355,8 @@ TEST_F(ShellTest, RasterizerScreenshot) { } TEST_F(ShellTest, RasterizerMakeRasterSnapshot) { + GTEST_SKIP() << "This test needs to create an isolate"; + Settings settings = CreateSettingsForFixture(); auto configuration = RunConfiguration::InferFromSettings(settings); auto task_runner = CreateNewThread(); @@ -2441,13 +2393,8 @@ TEST_F(ShellTest, OnServiceProtocolEstimateRasterCacheMemoryWorks) { // 1. Construct a picture and a picture layer to be raster cached. sk_sp display_list = MakeSizedDisplayList(10, 10); - fml::RefPtr queue = fml::MakeRefCounted( - GetCurrentTaskRunner(), fml::TimeDelta::Zero()); auto display_list_layer = std::make_shared( - SkPoint::Make(0, 0), - flutter::SkiaGPUObject( - {MakeSizedDisplayList(100, 100), queue}), - false, false); + SkPoint::Make(0, 0), MakeSizedDisplayList(100, 100), false, false); display_list_layer->set_paint_bounds(SkRect::MakeWH(100, 100)); // 2. Rasterize the picture and the picture layer in the raster cache. diff --git a/shell/common/snapshot_controller_skia.cc b/shell/common/snapshot_controller_skia.cc index e5f0c49a00bb4..2afaa60652b2d 100644 --- a/shell/common/snapshot_controller_skia.cc +++ b/shell/common/snapshot_controller_skia.cc @@ -7,6 +7,7 @@ #include "display_list/image/dl_image.h" #include "flutter/flow/surface.h" #include "flutter/fml/trace_event.h" +#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "flutter/shell/common/snapshot_controller.h" #include "third_party/skia/include/core/SkColorSpace.h" #include "third_party/skia/include/core/SkSurface.h" @@ -120,7 +121,7 @@ sk_sp SnapshotControllerSkia::DoMakeRasterSnapshot( })); } - return DlImage::Make(result); + return DlImageGPU::Make(result); } sk_sp SnapshotControllerSkia::MakeRasterSnapshot( diff --git a/shell/platform/android/android_external_texture_gl.cc b/shell/platform/android/android_external_texture_gl.cc index 3e40b99c3d582..6199aadc8d545 100644 --- a/shell/platform/android/android_external_texture_gl.cc +++ b/shell/platform/android/android_external_texture_gl.cc @@ -9,6 +9,7 @@ #include #include "flutter/display_list/effects/dl_color_source.h" +#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "third_party/skia/include/core/SkAlphaType.h" #include "third_party/skia/include/core/SkColorSpace.h" #include "third_party/skia/include/core/SkColorType.h" @@ -73,7 +74,7 @@ void AndroidExternalTextureGL::Paint(PaintContext& context, context.canvas->Translate(bounds.x(), bounds.y() + bounds.height()); context.canvas->Scale(bounds.width(), -bounds.height()); - auto dl_image = DlImage::Make(image); + auto dl_image = DlImageGPU::Make(image); if (!transform.isIdentity()) { DlImageColorSource source(dl_image, DlTileMode::kRepeat, DlTileMode::kRepeat, sampling, &transform); diff --git a/shell/platform/darwin/graphics/BUILD.gn b/shell/platform/darwin/graphics/BUILD.gn index fbb5a7a5b1035..5df74492bdce5 100644 --- a/shell/platform/darwin/graphics/BUILD.gn +++ b/shell/platform/darwin/graphics/BUILD.gn @@ -22,6 +22,7 @@ source_set("graphics") { "//flutter/common/graphics", "//flutter/display_list", "//flutter/fml", + "//flutter/lib/ui:ui", "//flutter/shell/common", "//flutter/shell/platform/darwin/common:framework_shared", ] diff --git a/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm b/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm index 01f64b351a538..049fe81695c02 100644 --- a/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm +++ b/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm @@ -4,6 +4,7 @@ #import "flutter/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.h" #include "flutter/display_list/image/dl_image.h" +#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "impeller/base/validation.h" #include "impeller/display_list/display_list_image_impeller.h" #include "impeller/renderer/backend/metal/texture_mtl.h" @@ -227,7 +228,7 @@ - (void)onTextureUnregistered { return nullptr; } - return flutter::DlImage::Make(skImage); + return flutter::DlImageGPU::Make(skImage); } - (sk_sp)wrapRGBAExternalPixelBuffer:(CVPixelBufferRef)pixelBuffer @@ -272,7 +273,7 @@ - (void)onTextureUnregistered { if (!skImage) { return nullptr; } - return flutter::DlImage::Make(skImage); + return flutter::DlImageGPU::Make(skImage); } @end diff --git a/shell/platform/embedder/embedder_external_texture_gl.cc b/shell/platform/embedder/embedder_external_texture_gl.cc index 04b069c5fa73e..4bc22995925c1 100644 --- a/shell/platform/embedder/embedder_external_texture_gl.cc +++ b/shell/platform/embedder/embedder_external_texture_gl.cc @@ -5,6 +5,7 @@ #include "flutter/shell/platform/embedder/embedder_external_texture_gl.h" #include "flutter/fml/logging.h" +#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "include/core/SkCanvas.h" #include "include/core/SkPaint.h" #include "third_party/skia/include/core/SkAlphaType.h" @@ -101,7 +102,7 @@ sk_sp EmbedderExternalTextureGL::ResolveTexture( return nullptr; } - return DlImage::Make(std::move(image)); + return DlImageGPU::Make(std::move(image)); } // |flutter::Texture| diff --git a/shell/platform/embedder/embedder_external_texture_metal.mm b/shell/platform/embedder/embedder_external_texture_metal.mm index ac759d5211ef6..f5d735a964313 100644 --- a/shell/platform/embedder/embedder_external_texture_metal.mm +++ b/shell/platform/embedder/embedder_external_texture_metal.mm @@ -6,6 +6,7 @@ #include "flow/layers/layer.h" #include "flutter/fml/logging.h" +#include "flutter/lib/ui/painting/display_list_image_gpu.h" #import "flutter/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.h" #include "third_party/skia/include/core/SkImage.h" #include "third_party/skia/include/core/SkSize.h" @@ -100,7 +101,7 @@ static bool ValidNumTextures(int expected, int actual) { FML_LOG(ERROR) << "Could not create external texture: " << texture_id; } - return DlImage::Make(std::move(image)); + return DlImageGPU::Make(std::move(image)); } // |flutter::Texture| diff --git a/shell/platform/embedder/tests/embedder_metal_unittests.mm b/shell/platform/embedder/tests/embedder_metal_unittests.mm index 7974dc32cf5ea..1fc2abcd7c0aa 100644 --- a/shell/platform/embedder/tests/embedder_metal_unittests.mm +++ b/shell/platform/embedder/tests/embedder_metal_unittests.mm @@ -70,6 +70,8 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr } TEST_F(EmbedderTest, ExternalTextureMetal) { + GTEST_SKIP() << "This test needs to create an isolate"; + EmbedderTestContextMetal& context = reinterpret_cast( GetEmbedderContext(EmbedderTestContextType::kMetalContext)); @@ -491,6 +493,8 @@ void Collect() { } TEST_F(EmbedderTest, ExternalTextureMetalRefreshedTooOften) { + GTEST_SKIP() << "This test needs to create an isolate"; + EmbedderTestContextMetal& context = reinterpret_cast( GetEmbedderContext(EmbedderTestContextType::kMetalContext)); From 1df6059c8e385ed8b60e8997c40b97b13cde367e Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Mon, 17 Apr 2023 01:30:21 -0700 Subject: [PATCH 2/3] optimize ImageShader::shader() method --- lib/ui/painting/image_shader.cc | 16 ++++------------ lib/ui/painting/image_shader.h | 2 +- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/lib/ui/painting/image_shader.cc b/lib/ui/painting/image_shader.cc index 0135f481e1393..5093f51433c0e 100644 --- a/lib/ui/painting/image_shader.cc +++ b/lib/ui/painting/image_shader.cc @@ -41,24 +41,16 @@ Dart_Handle ImageShader::initWithImage(CanvasImage* image, DlImageSampling sampling = sampling_is_locked_ ? ImageFilter::SamplingFromIndex(filter_quality_index) : DlImageSampling::kLinear; - cached_shader_ = - sk_make_sp(image_, tmx, tmy, sampling, &local_matrix); + cached_shader_ = std::make_shared( + image_, tmx, tmy, sampling, &local_matrix); FML_DCHECK(cached_shader_->isUIThreadSafe()); return Dart_Null(); } std::shared_ptr ImageShader::shader(DlImageSampling sampling) { - if (sampling_is_locked_) { - return cached_shader_->with_sampling(cached_shader_->sampling()); + if (sampling_is_locked_ || sampling == cached_shader_->sampling()) { + return cached_shader_; } - // It might seem that if the sampling is locked we can just return the - // cached version, but since we need to hold the cached shader in a - // Skia GPU wrapper, and that wrapper requires an sk_sp<>, we are holding - // an sk_sp<> version of the shared object and we need a shared_ptr version. - // So, either way, we need the with_sampling() method to shared_ptr'ify - // our copy. - // If we can get rid of the need for the GPU unref queue, then this can all - // be simplified down to just a shared_ptr. return cached_shader_->with_sampling(sampling); } diff --git a/lib/ui/painting/image_shader.h b/lib/ui/painting/image_shader.h index c5e8fc2c3435f..61b4a851ebb15 100644 --- a/lib/ui/painting/image_shader.h +++ b/lib/ui/painting/image_shader.h @@ -43,7 +43,7 @@ class ImageShader : public Shader { sk_sp image_; bool sampling_is_locked_; - sk_sp cached_shader_; + std::shared_ptr cached_shader_; }; } // namespace flutter From 6b47eba2d32b82d55768ab107bda8006629bfb9f Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Mon, 17 Apr 2023 14:44:32 -0700 Subject: [PATCH 3/3] back off unnecessary use of DlImageGPU and remove obsolete includes --- flow/BUILD.gn | 2 -- flow/layers/display_list_raster_cache_item.cc | 2 +- flow/testing/diff_context_test.cc | 5 +--- flow/testing/diff_context_test.h | 9 ++---- flow/testing/mock_texture.cc | 2 +- flow/testing/skia_gpu_object_layer_test.cc | 18 ----------- flow/testing/skia_gpu_object_layer_test.h | 30 ------------------- .../display_list_image_impeller.cc | 2 +- .../display_list_deferred_image_gpu_skia.h | 1 - lib/ui/painting/display_list_image_gpu.cc | 10 ------- lib/ui/painting/display_list_image_gpu.h | 1 - lib/ui/painting/image.h | 2 -- lib/ui/painting/multi_frame_codec.cc | 1 + lib/ui/painting/picture.cc | 5 +++- lib/ui/painting/picture.h | 1 - lib/ui/snapshot_delegate.h | 1 - lib/ui/ui_dart_state.h | 12 -------- shell/common/shell_unittests.cc | 2 -- shell/common/snapshot_controller.h | 3 ++ shell/common/snapshot_controller_skia.cc | 5 ++-- .../android/android_external_texture_gl.cc | 3 +- shell/platform/darwin/graphics/BUILD.gn | 1 - .../FlutterDarwinExternalTextureMetal.mm | 8 +++-- .../embedder/embedder_external_texture_gl.cc | 4 +-- .../embedder_external_texture_metal.mm | 4 +-- .../tests/embedder_metal_unittests.mm | 4 --- 26 files changed, 27 insertions(+), 111 deletions(-) delete mode 100644 flow/testing/skia_gpu_object_layer_test.cc delete mode 100644 flow/testing/skia_gpu_object_layer_test.h diff --git a/flow/BUILD.gn b/flow/BUILD.gn index 1adceefde4bc2..f996f0707e04c 100644 --- a/flow/BUILD.gn +++ b/flow/BUILD.gn @@ -123,8 +123,6 @@ if (enable_unittests) { "testing/mock_raster_cache.h", "testing/mock_texture.cc", "testing/mock_texture.h", - "testing/skia_gpu_object_layer_test.cc", - "testing/skia_gpu_object_layer_test.h", ] public_deps = [ diff --git a/flow/layers/display_list_raster_cache_item.cc b/flow/layers/display_list_raster_cache_item.cc index 7c6e95af2eb9f..b295c85ea2761 100644 --- a/flow/layers/display_list_raster_cache_item.cc +++ b/flow/layers/display_list_raster_cache_item.cc @@ -14,7 +14,7 @@ #include "flutter/flow/raster_cache_item.h" #include "flutter/flow/raster_cache_key.h" #include "flutter/flow/raster_cache_util.h" -#include "flutter/flow/skia_gpu_object.h" +#include "third_party/skia/include/gpu/GrDirectContext.h" namespace flutter { diff --git a/flow/testing/diff_context_test.cc b/flow/testing/diff_context_test.cc index b704be110a223..9860c7668871c 100644 --- a/flow/testing/diff_context_test.cc +++ b/flow/testing/diff_context_test.cc @@ -10,10 +10,7 @@ namespace flutter { namespace testing { -DiffContextTest::DiffContextTest() - : unref_queue_(fml::MakeRefCounted( - GetCurrentTaskRunner(), - fml::TimeDelta::FromSeconds(0))) {} +DiffContextTest::DiffContextTest() {} Damage DiffContextTest::DiffLayerTree(MockLayerTree& layer_tree, const MockLayerTree& old_layer_tree, diff --git a/flow/testing/diff_context_test.h b/flow/testing/diff_context_test.h index 0c4223cfce52b..f606e42bca87b 100644 --- a/flow/testing/diff_context_test.h +++ b/flow/testing/diff_context_test.h @@ -5,7 +5,7 @@ #include "flutter/flow/layers/container_layer.h" #include "flutter/flow/layers/display_list_layer.h" #include "flutter/flow/layers/opacity_layer.h" -#include "flutter/flow/testing/skia_gpu_object_layer_test.h" +#include "flutter/flow/testing/layer_test.h" #include "third_party/skia/include/core/SkPicture.h" #include "third_party/skia/include/core/SkPictureRecorder.h" @@ -31,7 +31,7 @@ class MockLayerTree { SkISize size_; }; -class DiffContextTest : public ThreadTest { +class DiffContextTest : public LayerTest { public: DiffContextTest(); @@ -62,11 +62,6 @@ class DiffContextTest : public ThreadTest { std::initializer_list> layers, SkAlpha alpha, const SkPoint& offset = SkPoint::Make(0, 0)); - - fml::RefPtr unref_queue() { return unref_queue_; } - - private: - fml::RefPtr unref_queue_; }; } // namespace testing diff --git a/flow/testing/mock_texture.cc b/flow/testing/mock_texture.cc index ceb8e88fd69c9..7414c660b8df8 100644 --- a/flow/testing/mock_texture.cc +++ b/flow/testing/mock_texture.cc @@ -4,7 +4,7 @@ #include "flutter/flow/testing/mock_texture.h" #include "flutter/flow/layers/layer.h" -#include "flutter/flow/testing/skia_gpu_object_layer_test.h" +#include "flutter/testing/display_list_testing.h" namespace flutter { namespace testing { diff --git a/flow/testing/skia_gpu_object_layer_test.cc b/flow/testing/skia_gpu_object_layer_test.cc deleted file mode 100644 index 1fca8ec8f3b81..0000000000000 --- a/flow/testing/skia_gpu_object_layer_test.cc +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "flutter/flow/testing/skia_gpu_object_layer_test.h" - -#include "flutter/fml/time/time_delta.h" - -namespace flutter { -namespace testing { - -SkiaGPUObjectLayerTest::SkiaGPUObjectLayerTest() - : unref_queue_(fml::MakeRefCounted( - GetCurrentTaskRunner(), - fml::TimeDelta::FromSeconds(0))) {} - -} // namespace testing -} // namespace flutter diff --git a/flow/testing/skia_gpu_object_layer_test.h b/flow/testing/skia_gpu_object_layer_test.h deleted file mode 100644 index d573ac0b41007..0000000000000 --- a/flow/testing/skia_gpu_object_layer_test.h +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef FLOW_TESTING_SKIA_GPU_OBJECT_LAYER_TEST_H_ -#define FLOW_TESTING_SKIA_GPU_OBJECT_LAYER_TEST_H_ - -#include "flutter/flow/skia_gpu_object.h" -#include "flutter/flow/testing/layer_test.h" -#include "flutter/testing/thread_test.h" - -namespace flutter { -namespace testing { - -// This fixture allows generating tests that create |SkiaGPUObject|'s which -// are destroyed on a |SkiaUnrefQueue|. -class SkiaGPUObjectLayerTest : public LayerTestBase { - public: - SkiaGPUObjectLayerTest(); - - fml::RefPtr unref_queue() { return unref_queue_; } - - private: - fml::RefPtr unref_queue_; -}; - -} // namespace testing -} // namespace flutter - -#endif // FLOW_TESTING_SKIA_GPU_OBJECT_LAYER_TEST_H_ diff --git a/impeller/display_list/display_list_image_impeller.cc b/impeller/display_list/display_list_image_impeller.cc index 6cc69188b342a..8a9d77b0005b5 100644 --- a/impeller/display_list/display_list_image_impeller.cc +++ b/impeller/display_list/display_list_image_impeller.cc @@ -66,7 +66,7 @@ bool DlImageImpeller::isTextureBacked() const { // |DlImage| bool DlImageImpeller::isUIThreadSafe() const { - // Impeller textures are always ... thread-safe :/ + // Impeller textures are always thread-safe return true; } diff --git a/lib/ui/painting/display_list_deferred_image_gpu_skia.h b/lib/ui/painting/display_list_deferred_image_gpu_skia.h index 44c9d360f86d7..2d6c9b53c5cf9 100644 --- a/lib/ui/painting/display_list_deferred_image_gpu_skia.h +++ b/lib/ui/painting/display_list_deferred_image_gpu_skia.h @@ -12,7 +12,6 @@ #include "flutter/display_list/display_list.h" #include "flutter/display_list/image/dl_image.h" #include "flutter/flow/layers/layer_tree.h" -#include "flutter/flow/skia_gpu_object.h" #include "flutter/fml/macros.h" #include "flutter/fml/memory/weak_ptr.h" #include "flutter/lib/ui/io_manager.h" diff --git a/lib/ui/painting/display_list_image_gpu.cc b/lib/ui/painting/display_list_image_gpu.cc index b06ce691c7998..b4e87090534d3 100644 --- a/lib/ui/painting/display_list_image_gpu.cc +++ b/lib/ui/painting/display_list_image_gpu.cc @@ -4,8 +4,6 @@ #include "flutter/lib/ui/painting/display_list_image_gpu.h" -#include "flutter/lib/ui/ui_dart_state.h" - namespace flutter { sk_sp DlImageGPU::Make(SkiaGPUObject image) { @@ -15,14 +13,6 @@ sk_sp DlImageGPU::Make(SkiaGPUObject image) { return sk_sp(new DlImageGPU(std::move(image))); } -sk_sp DlImageGPU::Make(sk_sp image) { - if (!image) { - return nullptr; - } - return sk_sp( - new DlImageGPU(UIDartState::CreateGPUObject(std::move(image)))); -} - DlImageGPU::DlImageGPU(SkiaGPUObject image) : image_(std::move(image)) {} diff --git a/lib/ui/painting/display_list_image_gpu.h b/lib/ui/painting/display_list_image_gpu.h index 47d2acde39b53..4290c724c8f20 100644 --- a/lib/ui/painting/display_list_image_gpu.h +++ b/lib/ui/painting/display_list_image_gpu.h @@ -14,7 +14,6 @@ namespace flutter { class DlImageGPU final : public DlImage { public: static sk_sp Make(SkiaGPUObject image); - static sk_sp Make(sk_sp image); // |DlImage| ~DlImageGPU() override; diff --git a/lib/ui/painting/image.h b/lib/ui/painting/image.h index 2352180b742de..18a7200739813 100644 --- a/lib/ui/painting/image.h +++ b/lib/ui/painting/image.h @@ -6,9 +6,7 @@ #define FLUTTER_LIB_UI_PAINTING_IMAGE_H_ #include "flutter/display_list/image/dl_image.h" -#include "flutter/flow/skia_gpu_object.h" #include "flutter/lib/ui/dart_wrapper.h" -#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "flutter/lib/ui/ui_dart_state.h" #include "third_party/skia/include/core/SkImage.h" diff --git a/lib/ui/painting/multi_frame_codec.cc b/lib/ui/painting/multi_frame_codec.cc index 796ea4645f8b1..c52b4a72dbc2f 100644 --- a/lib/ui/painting/multi_frame_codec.cc +++ b/lib/ui/painting/multi_frame_codec.cc @@ -7,6 +7,7 @@ #include #include "flutter/fml/make_copyable.h" +#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "flutter/lib/ui/painting/image.h" #if IMPELLER_SUPPORTS_RENDERING #include "flutter/lib/ui/painting/image_decoder_impeller.h" diff --git a/lib/ui/painting/picture.cc b/lib/ui/painting/picture.cc index 3703c270a0a79..53a9371277555 100644 --- a/lib/ui/painting/picture.cc +++ b/lib/ui/painting/picture.cc @@ -14,6 +14,7 @@ #if IMPELLER_SUPPORTS_RENDERING #include "flutter/lib/ui/painting/display_list_deferred_image_gpu_impeller.h" #endif // IMPELLER_SUPPORTS_RENDERING +#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "third_party/tonic/converter/dart_converter.h" #include "third_party/tonic/dart_args.h" #include "third_party/tonic/dart_binding_macros.h" @@ -175,7 +176,9 @@ Dart_Handle Picture::RasterizeToImage(const sk_sp& display_list, return; } - if (image->skia_image()) { + if (!image->isUIThreadSafe()) { + // All images with impeller textures should already be safe. + FML_DCHECK(image->impeller_texture() == nullptr); image = DlImageGPU::Make({image->skia_image(), std::move(unref_queue)}); } diff --git a/lib/ui/painting/picture.h b/lib/ui/painting/picture.h index edffcaacf4d1b..d0c2a0123fa1d 100644 --- a/lib/ui/painting/picture.h +++ b/lib/ui/painting/picture.h @@ -7,7 +7,6 @@ #include "flutter/display_list/display_list.h" #include "flutter/flow/layers/layer_tree.h" -#include "flutter/flow/skia_gpu_object.h" #include "flutter/lib/ui/dart_wrapper.h" #include "flutter/lib/ui/painting/image.h" #include "flutter/lib/ui/ui_dart_state.h" diff --git a/lib/ui/snapshot_delegate.h b/lib/ui/snapshot_delegate.h index 6f9fc113a330e..0640fe0dd9c07 100644 --- a/lib/ui/snapshot_delegate.h +++ b/lib/ui/snapshot_delegate.h @@ -9,7 +9,6 @@ #include "flutter/common/graphics/texture.h" #include "flutter/display_list/display_list.h" -#include "flutter/flow/skia_gpu_object.h" #include "third_party/skia/include/core/SkImage.h" #include "third_party/skia/include/core/SkPicture.h" #include "third_party/skia/include/core/SkPromiseImageTexture.h" diff --git a/lib/ui/ui_dart_state.h b/lib/ui/ui_dart_state.h index c384cd420f1c3..63824a552eee0 100644 --- a/lib/ui/ui_dart_state.h +++ b/lib/ui/ui_dart_state.h @@ -11,7 +11,6 @@ #include "flutter/common/settings.h" #include "flutter/common/task_runners.h" -#include "flutter/flow/skia_gpu_object.h" #include "flutter/fml/build_config.h" #include "flutter/fml/memory/weak_ptr.h" #include "flutter/fml/synchronization/waitable_event.h" @@ -153,17 +152,6 @@ class UIDartState : public tonic::DartState { // @param[in] message The message to be logged. void LogMessage(const std::string& tag, const std::string& message) const; - template - static flutter::SkiaGPUObject CreateGPUObject(sk_sp object) { - if (!object) { - return {}; - } - auto* state = UIDartState::Current(); - FML_DCHECK(state); - auto queue = state->GetSkiaUnrefQueue(); - return {std::move(object), std::move(queue)}; - }; - UnhandledExceptionCallback unhandled_exception_callback() const { return unhandled_exception_callback_; } diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 293767a25ef55..d85750e70d612 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -2355,8 +2355,6 @@ TEST_F(ShellTest, RasterizerScreenshot) { } TEST_F(ShellTest, RasterizerMakeRasterSnapshot) { - GTEST_SKIP() << "This test needs to create an isolate"; - Settings settings = CreateSettingsForFixture(); auto configuration = RunConfiguration::InferFromSettings(settings); auto task_runner = CreateNewThread(); diff --git a/shell/common/snapshot_controller.h b/shell/common/snapshot_controller.h index 54ad5e0b061c7..c402c3ec709fa 100644 --- a/shell/common/snapshot_controller.h +++ b/shell/common/snapshot_controller.h @@ -36,6 +36,9 @@ class SnapshotController { virtual ~SnapshotController() = default; + // Note that this image is not guaranteed to be UIThreadSafe and must + // be converted to a DlImageGPU if it is to be handed back to the UI + // thread. virtual sk_sp MakeRasterSnapshot(sk_sp display_list, SkISize size) = 0; diff --git a/shell/common/snapshot_controller_skia.cc b/shell/common/snapshot_controller_skia.cc index 2afaa60652b2d..e9fc66cde4e5a 100644 --- a/shell/common/snapshot_controller_skia.cc +++ b/shell/common/snapshot_controller_skia.cc @@ -7,7 +7,6 @@ #include "display_list/image/dl_image.h" #include "flutter/flow/surface.h" #include "flutter/fml/trace_event.h" -#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "flutter/shell/common/snapshot_controller.h" #include "third_party/skia/include/core/SkColorSpace.h" #include "third_party/skia/include/core/SkSurface.h" @@ -121,7 +120,9 @@ sk_sp SnapshotControllerSkia::DoMakeRasterSnapshot( })); } - return DlImageGPU::Make(result); + // It is up to the caller to create a DlImageGPU version of this image + // if the result will interact with the UI thread. + return DlImage::Make(result); } sk_sp SnapshotControllerSkia::MakeRasterSnapshot( diff --git a/shell/platform/android/android_external_texture_gl.cc b/shell/platform/android/android_external_texture_gl.cc index 6199aadc8d545..3e40b99c3d582 100644 --- a/shell/platform/android/android_external_texture_gl.cc +++ b/shell/platform/android/android_external_texture_gl.cc @@ -9,7 +9,6 @@ #include #include "flutter/display_list/effects/dl_color_source.h" -#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "third_party/skia/include/core/SkAlphaType.h" #include "third_party/skia/include/core/SkColorSpace.h" #include "third_party/skia/include/core/SkColorType.h" @@ -74,7 +73,7 @@ void AndroidExternalTextureGL::Paint(PaintContext& context, context.canvas->Translate(bounds.x(), bounds.y() + bounds.height()); context.canvas->Scale(bounds.width(), -bounds.height()); - auto dl_image = DlImageGPU::Make(image); + auto dl_image = DlImage::Make(image); if (!transform.isIdentity()) { DlImageColorSource source(dl_image, DlTileMode::kRepeat, DlTileMode::kRepeat, sampling, &transform); diff --git a/shell/platform/darwin/graphics/BUILD.gn b/shell/platform/darwin/graphics/BUILD.gn index 5df74492bdce5..fbb5a7a5b1035 100644 --- a/shell/platform/darwin/graphics/BUILD.gn +++ b/shell/platform/darwin/graphics/BUILD.gn @@ -22,7 +22,6 @@ source_set("graphics") { "//flutter/common/graphics", "//flutter/display_list", "//flutter/fml", - "//flutter/lib/ui:ui", "//flutter/shell/common", "//flutter/shell/platform/darwin/common:framework_shared", ] diff --git a/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm b/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm index 049fe81695c02..3a8a5030b71cf 100644 --- a/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm +++ b/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm @@ -4,7 +4,6 @@ #import "flutter/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.h" #include "flutter/display_list/image/dl_image.h" -#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "impeller/base/validation.h" #include "impeller/display_list/display_list_image_impeller.h" #include "impeller/renderer/backend/metal/texture_mtl.h" @@ -228,7 +227,8 @@ - (void)onTextureUnregistered { return nullptr; } - return flutter::DlImageGPU::Make(skImage); + // This image should not escape local use by this flutter::Texture implementation + return flutter::DlImage::Make(skImage); } - (sk_sp)wrapRGBAExternalPixelBuffer:(CVPixelBufferRef)pixelBuffer @@ -273,7 +273,9 @@ - (void)onTextureUnregistered { if (!skImage) { return nullptr; } - return flutter::DlImageGPU::Make(skImage); + + // This image should not escape local use by this flutter::Texture implementation + return flutter::DlImage::Make(skImage); } @end diff --git a/shell/platform/embedder/embedder_external_texture_gl.cc b/shell/platform/embedder/embedder_external_texture_gl.cc index 4bc22995925c1..53bcc3e92e5b7 100644 --- a/shell/platform/embedder/embedder_external_texture_gl.cc +++ b/shell/platform/embedder/embedder_external_texture_gl.cc @@ -5,7 +5,6 @@ #include "flutter/shell/platform/embedder/embedder_external_texture_gl.h" #include "flutter/fml/logging.h" -#include "flutter/lib/ui/painting/display_list_image_gpu.h" #include "include/core/SkCanvas.h" #include "include/core/SkPaint.h" #include "third_party/skia/include/core/SkAlphaType.h" @@ -102,7 +101,8 @@ sk_sp EmbedderExternalTextureGL::ResolveTexture( return nullptr; } - return DlImageGPU::Make(std::move(image)); + // This image should not escape local use by EmbedderExternalTextureGL + return DlImage::Make(std::move(image)); } // |flutter::Texture| diff --git a/shell/platform/embedder/embedder_external_texture_metal.mm b/shell/platform/embedder/embedder_external_texture_metal.mm index f5d735a964313..4e825e8cacde2 100644 --- a/shell/platform/embedder/embedder_external_texture_metal.mm +++ b/shell/platform/embedder/embedder_external_texture_metal.mm @@ -6,7 +6,6 @@ #include "flow/layers/layer.h" #include "flutter/fml/logging.h" -#include "flutter/lib/ui/painting/display_list_image_gpu.h" #import "flutter/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.h" #include "third_party/skia/include/core/SkImage.h" #include "third_party/skia/include/core/SkSize.h" @@ -101,7 +100,8 @@ static bool ValidNumTextures(int expected, int actual) { FML_LOG(ERROR) << "Could not create external texture: " << texture_id; } - return DlImageGPU::Make(std::move(image)); + // This image should not escape local use by EmbedderExternalTextureMetal + return DlImage::Make(std::move(image)); } // |flutter::Texture| diff --git a/shell/platform/embedder/tests/embedder_metal_unittests.mm b/shell/platform/embedder/tests/embedder_metal_unittests.mm index 1fc2abcd7c0aa..7974dc32cf5ea 100644 --- a/shell/platform/embedder/tests/embedder_metal_unittests.mm +++ b/shell/platform/embedder/tests/embedder_metal_unittests.mm @@ -70,8 +70,6 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr } TEST_F(EmbedderTest, ExternalTextureMetal) { - GTEST_SKIP() << "This test needs to create an isolate"; - EmbedderTestContextMetal& context = reinterpret_cast( GetEmbedderContext(EmbedderTestContextType::kMetalContext)); @@ -493,8 +491,6 @@ void Collect() { } TEST_F(EmbedderTest, ExternalTextureMetalRefreshedTooOften) { - GTEST_SKIP() << "This test needs to create an isolate"; - EmbedderTestContextMetal& context = reinterpret_cast( GetEmbedderContext(EmbedderTestContextType::kMetalContext));