Collect textures from toImageSync safely #35073

dnfield · 2022-08-02T00:33:43Z

This is very similar to #34931, but does not require new Skia API. All of the differences from that patch are in the last commit (d7c89e0)

This patch assumes that the DisplayListCanvasDispatcher is only ever used to render things to Skia. AFAICT, we make the same assumption when using TextureLayers and other Skia objects - @flar fyi.

This patch changes the implementation of toByteData because SkImage::MakeFromTexture does not appear to support SkImage::makeRasterImage on the GL and Vulkan backends (but does support it on Metal .. hah).

@jonahwilliams fyi

@bsalomon @egdaniel fyi

bsalomon · 2022-08-02T13:05:50Z

Glad this approach worked! makeRasterImage should work on GL and Vulkan, though.

dnfield · 2022-08-02T16:57:21Z

Moving this to draft while I work out some of the CI issues. I fixed a clang-tidy issue that appears to have broken some tests, and I'm having issues with hte color types on Linux

flutter-dashboard · 2022-08-02T17:02:42Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

jason-simmons · 2022-08-02T18:43:51Z

flow/testing/layer_test.h

  std::vector<RasterCacheItem*>& cacheable_items() { return cacheable_items_; }

-  TextureRegistry& texture_regitry() { return texture_registry_; }
+  std::shared_ptr<TextureRegistry> texture_regitry() {


typo: "registry"

jason-simmons · 2022-08-02T18:59:02Z

lib/ui/painting/display_list_deferred_image_gpu.h


  // |DlImage|
  // This method is only safe to call from the raster thread.
+  // The image created here must not be added to the unref queue.


I would make this warning more general - callers should not hold long term references to this image and should only use it for the immediate painting operation.

jason-simmons · 2022-08-02T19:12:28Z

common/graphics/texture.cc


+void TextureRegistry::RegisterContextDestroyedListener(
+    std::weak_ptr<ContextDestroyedListener> image) {
+  images_.push_back(std::move(image));


The images_ list will grow unbounded until the context is destroyed.

DlDeferredImageGPU holds a reference to the TextureRegistry - could the DlDeferredImageGPU dtor's raster thread task remove the ImageWrapper from the TextureRegistry?

IIRC a previous version of this patch had an unregister mechanism like that.

Ahh yeah. I was thinking of another place I used a pattern like this where it gets visited on every frame, but that won't happen here. I'll add back the unregister method.

Fixed this.

jason-simmons · 2022-08-02T20:14:47Z

lib/ui/painting/display_list_deferred_image_gpu.h

+
+   private:
+    GrBackendTexture texture_;
+    sk_sp<GrDirectContext> context_;


This is a potential risk, but I'm not sure what to do about it.

Holding a strong reference to the GrDirectContext here is going to make it harder to understand and control how the GrDirectContext is deleted during engine shutdown. Or conceivably someone could call GrDirectContext::abandonContext while one of these objects is still referencing the GrDirectContext.

On the Android embedder the engine group context sharing mechanism should ensure that the GrDirectContext is not abandoned until after the engine is shut down. So by that time the DlDeferredImageGpu instances should all be GCed and the TextureRegistry.images_ should be cleaned up.

But I don't know if there are other platforms where the GrDirectContext has a different lifecycle that this might conflict with (for example, the embedding library)

I'm wondering if these images should be treated specially instead of making them look like subclasses of DlImage?

Would it be feasible to create a display list object that explicitly wraps a GrBackendTexture?
Or an API where the caller must supply a GrDirectContext to get an SkImage, and the caller therefore knows that the resulting image is tied to the GrDirectContext and must not be held beyond the context's lifetime?

@chinmaygarde @flar

If they call abandonContext, they should notify the TextureRegistry::onGrContextDestroyed which will give this class a chance to delete the texture and then release its reference to GrDirectContext.

In fact, that will happen during rasterizer teardown and synchronously complete before the rasterizer lets go to its surface_ pointer, which holds the GrDirectContext.

I'm not really sure what benefit we'd get from having a special object that isn't a subclass of DlImage. It's probably possible but it would still hav eto go all the same places DlImage goes today (drawImage, drawImageRect, drawImageNine, drawAtlas, ImageShader).

TextureRegistry::onGrContextDestroyed is called during the Rasterizer::Teardown that happens when the engine loses its rendering surface (for example, when you pause and resume an app in the Android task switcher)

So if Dart code calls toImageSync and holds a reference to the resulting Image across a pause/resume cycle, then the Image will not render properly after the app resumes.

(onGrContextDestroyed is now a misnomer on Android - in the past the GrDirectContext was destroyed at that time, but with the engine group context sharing scheme the GrDirectContext is being kept alive beyond that call.)

I'm not sure when the right time would be to clean up any outstanding DlDeferredImageGPU::ImageWrapper textures. Doing it during or immediately before Rasterizer destruction might work, but I don't know if that risks keeping a GrDirectContext instance alive longer than intended on some platform.

I think it's fine to require that these images get recreated on application resume - they will throw if you try to draw them after at this point.

An application would be left with two choices - either explicitly make a host copy with toByteData to re-upload on resume, or re-create the image on resume with toImageSync again using the same picture object.

I tried experimenting with this, and it looks like that is not how these images currently behave. Drawing an image after a pause/resume produces a black rectangle with no exception.

Does DlDeferredImageGPU need to track whether OnGrContextDestroyed was called in order to implement this?

It should also be made clear in the API documentation that these are not general purpose images. They should only be used temporarily and should not be stored or cached anywhere.

Ah, I'm missing setting the error string when the context is destroyed.

Actually, throwing in this case might be too strict.

Even if the image is created, used, and then quickly discarded, there still might be a risk that the user pauses the app at the exact wrong moment and the OnGrContextDestroyed happens between creation and use. The error might need to be a warning logged when in debug mode.

Latest change re-creates the texture when the texture registry tells us the gr context has been restored. It does this by keeping the displaylist around.

dnfield · 2022-08-02T21:17:41Z

Linux is having issues with the GPU path, but that embedding is having issues in general right now. I've filed a bug to track enabling the GPU path on Linux and for now am having it do a slower software based path.

jason-simmons · 2022-08-02T21:55:59Z

lib/ui/painting/display_list_deferred_image_gpu.cc

+  FML_DCHECK(raster_task_runner_->RunsTasksOnCurrentThread());
+
+  if (texture_.isValid()) {
+    unref_queue_->DeleteTexture(std::move(texture_));


Reset texture_ to an invalid GrBackendTexture
(AFAICT GrBackendTexture does not have a move constructor that will invalidate texture_)

Also drop the context_ reference to the GrDirectContext

jason-simmons · 2022-08-03T23:08:08Z

common/graphics/texture.cc

+
+  for (const auto& [id, weak_image] : images_) {
+    if (auto image = weak_image.lock()) {
+      image->OnGrContextCreated();


Would it make sense to do this lazily for each image instead of doing it for all images when the context becomes available?

I think if we did it lazily it would be in the image itself rather than here - IOW the image object would have to track state and re-create the image on demand.

I'm inclined to think that it'd be better to just take time resuming than to potentially jank during an animation to re-create an image. I'm not really sure which will be better - if an application has a lot of these images it could significantly slow down resume, but there will be a number of problems with using that many images anyway e.g. memory constraints on the device.

jason-simmons · 2022-08-03T23:10:06Z

lib/ui/painting/display_list_deferred_image_gpu.cc

+        if (!wrapper) {
+          return;
+        }
+        auto result = wrapper->snapshot_delegate_->MakeGpuImage(


Check the validity of the snapshot delegate fml::WeakPtr before dereferencing

jason-simmons · 2022-08-03T23:12:18Z

lib/ui/painting/display_list_deferred_image_gpu.cc

 // |DlImage|
-DlDeferredImageGPU::~DlDeferredImageGPU() = default;
+DlDeferredImageGPU::~DlDeferredImageGPU() {
+  auto image_wrapper = std::atomic_load(&image_wrapper_);


The use of atomic_load with image_wrapper_ is no longer necessary.

image_wrapper_ is now const and is not mutated until the DlDeferredImageGPU is destructed.

Ahh yeah, done

jason-simmons · 2022-08-04T20:35:17Z

LGTM for concurrency and object lifetime issues

@chinmaygarde Any feedback on the API and its interaction with the rasterizer subsystem?

chinmaygarde · 2022-08-09T21:11:34Z

The tree became red after this patch but the failure is a very brittle test. Still investigating if what happened.

dnfield · 2022-08-09T21:22:00Z

I had to update that value as part of these changes but maybe there was a merge conflict

This reverts commit 4ac52f2.

…utter#35281)" This reverts commit 3f7ccb2.

…utter#35281) This reverts commit 4ac52f2.

dnfield added 11 commits July 25, 2022 15:15

asdf

d1216ab

Merge remote-tracking branch 'upstream/main' into gpu_img_safe

8ea17c7

..

66d7147

..

b489450

missing dep

b2d3e6d

Fix unref

ba24c46

use unref queue

05d2512

Cleanup pointer usage, mutex for new wrapper

e4cee79

atomics

2f695ab

Merge remote-tracking branch 'upstream/main' into gpu_img_safe_2

da2fe09

MakeFromTexture

d7c89e0

dnfield requested a review from jason-simmons August 2, 2022 00:33

dnfield marked this pull request as ready for review August 2, 2022 00:33

dnfield added 3 commits August 1, 2022 21:04

format

014bc8f

guard nullptr

d0b0d81

Fix mock raster cache construction issues

d2d7ba1

dnfield marked this pull request as draft August 2, 2022 16:57

sundry test issues

46f30db

jason-simmons reviewed Aug 2, 2022

View reviewed changes

dnfield mentioned this pull request Aug 2, 2022

Make toImageSync not use the software raster path on Linux flutter/flutter#108835

Closed

Make linux use raster path, jason review

dab4808

dnfield marked this pull request as ready for review August 2, 2022 21:17

jason-simmons reviewed Aug 2, 2022

View reviewed changes

dnfield added 2 commits August 2, 2022 15:14

Reset texture/context on destroy

773c116

Recreate texture on resume

2a3b709

jason-simmons reviewed Aug 3, 2022

View reviewed changes

dnfield added 3 commits August 3, 2022 17:15

Jason review

4c788a8

syntax error

ba9044c

I swear I know C++

a693a0c

dnfield mentioned this pull request Aug 4, 2022

Flatten LayerTree on raster thread to include texture registry and GrDirectContext #34624

Closed

zanderso requested a review from chinmaygarde August 4, 2022 20:29

iskakaushik mentioned this pull request Aug 8, 2022

Enable latency sensitive GC mode for high-impact animations flutter/flutter#109167

Closed

chinmaygarde approved these changes Aug 9, 2022

View reviewed changes

dnfield merged commit 4ac52f2 into flutter:main Aug 9, 2022

dnfield deleted the gpu_img_safe_2 branch August 9, 2022 20:22

dnfield added a commit that referenced this pull request Aug 9, 2022

Revert "Collect textures from toImageSync safely (#35073)"

27ce935

This reverts commit 4ac52f2.

dnfield mentioned this pull request Aug 9, 2022

Revert "Collect textures from toImageSync safely" #35281

Merged

dnfield added a commit that referenced this pull request Aug 9, 2022

Revert "Collect textures from toImageSync safely (#35073)" (#35281)

3f7ccb2

This reverts commit 4ac52f2.

dnfield added a commit to dnfield/engine that referenced this pull request Aug 9, 2022

Reland "Collect textures from toImageSync safely (flutter#35073)" (fl…

4707b69

…utter#35281)" This reverts commit 3f7ccb2.

dnfield mentioned this pull request Aug 9, 2022

Reland toImageSync #35283

Merged

engine-flutter-autoroll mentioned this pull request Aug 9, 2022

Roll Flutter Engine from 010813db12f6 to 3f7ccb24015b (3 revisions) flutter/flutter#109266

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 9, 2022

4ac52f2 Collect textures from toImageSync safely (flutter/engine#35073)

a666081

emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022

Collect textures from toImageSync safely (flutter#35073)

94d3ede

emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022

Revert "Collect textures from toImageSync safely (flutter#35073)" (fl…

7002417

…utter#35281) This reverts commit 4ac52f2.

Collect textures from toImageSync safely #35073

Collect textures from toImageSync safely #35073

Uh oh!

Conversation

dnfield commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsalomon commented Aug 2, 2022

Uh oh!

dnfield commented Aug 2, 2022

Uh oh!

flutter-dashboard bot commented Aug 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Aug 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jason-simmons commented Aug 4, 2022

Uh oh!

chinmaygarde commented Aug 9, 2022

Uh oh!

dnfield commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnfield commented Aug 2, 2022 •

edited

Loading