focus SkiaGPUObject wrappers on only those objects that need the prot…

…ection (flutter#41237)

With this PR we no longer need to hold DisplayLists in GPUObject wrappers and they can be disposed instantly instead of queueing on the Unref thread.

This will definitely be a win for Impeller as none of the objects used in a frame now require queueing, but the performance impact on apps running on top of skia is less clear if they depend on a lot of images inside their DisplayLists that still need to be queued to be freed. After getting further in the work, it looks like only decoded images need to use the protected DlImage wrappers and most of those should survive many frames before they are disposed. That should hopefully leave very few unrefs happening per frame.

~There are 3 unit tests in `shell_unittests.cc` and `embedder_metal_unittests.mm` that are now GSKIP'd as they now invoke code that needs a fully initialized UIDartState in order to protect their images. I will look into fixing the tests and/or making the code they invoke provide protection without relying on UIDartState.~ (This looks to be fixed in the latest commit by simply not creating DlImageGPUs all over the source base and simply catching only those that end up in UI data structures. There is actually existing code in one of the modules that feeds ui.Image with an answer to wrap the image in a DlImageGPU if it has a skia image anyway, so most of these additional uses of DlImageGPU that were having trouble getting the Skia unref queue just didn't need it anyway.)

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<const DlRTree> 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() {

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<const DlRTree> 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<const DlRTree> rtree_;
 
   void Dispatch(DlOpReceiver& ctx,

display_list/dl_builder.cc

@@ -73,9 +73,10 @@ sk_sp<DisplayList> DisplayListBuilder::Build() {
   nested_bytes_ = nested_op_count_ = 0;
   storage_.realloc(bytes);
   bool compatible = layer_stack_.back().is_group_opacity_compatible();
-  return sk_sp<DisplayList>(new DisplayList(std::move(storage_), bytes, count,
-                                            nested_bytes, nested_count,
-                                            bounds(), compatible, rtree()));
+  bool is_safe = is_ui_thread_safe_;
+  return sk_sp<DisplayList>(
+      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<ClearColorSourceOp>(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<DlImage> image,
       ? Push<DrawImageWithAttrOp>(0, 1, image, point, sampling)
       : Push<DrawImageOp>(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<DlImage> image,
   Push<DrawImageRectOp>(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<DlImage> image,
       ? Push<DrawImageNineWithAttrOp>(0, 1, image, center, dst, filter)
       : Push<DrawImageNineOp>(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<DlImage> 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<DisplayList> display_list,
                                          SkScalar opacity) {
   DlPaint current_paint = current_;
   Push<DrawDisplayListOp>(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.

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 <typename T, typename... Args>
   void* Push(size_t extra, int op_inc, Args&&... args);
 

display_list/effects/dl_color_source.h

@@ -109,6 +109,16 @@ class DlColorSource : public DlAttribute<DlColorSource, DlColorSourceType> {
 
   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<DlColorSource> shared() const override {
     return std::make_shared<DlColorColorSource>(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<DlColorSource> 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<DlColorSource> 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<DlColorSource> 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<DlColorSource> 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<DlColorSource> shared() const override {

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.
   ///

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();

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;
 

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 = [

flow/layers/display_list_layer.cc

@@ -16,15 +16,14 @@
 namespace flutter {
 
 DisplayListLayer::DisplayListLayer(const SkPoint& offset,
-                                   SkiaGPUObject<DisplayList> display_list,
+                                   sk_sp<DisplayList> 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

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<DisplayList> display_list,
+                   sk_sp<DisplayList> 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<DisplayList> display_list_;
+  sk_sp<DisplayList> display_list_;
 
   static bool Compare(DiffContext::Statistics& statistics,
                       const DisplayListLayer* l1,