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

jonahwilliams · 2022-07-13T01:26:35Z

ToImage/toImageSync doesn't currently include the output of textures, due to LayerTree::Flatten using a no-op texture registry. We should provide one so texture backed platform views work.

(This implementation is just a hack so i can test it out.) Also unsure if there were other reasons why this wouldn't work

FYI @dnfield @zanderso

…ure_view

jonahwilliams · 2022-07-13T06:39:09Z

or maybe it will, need to actually build on Android and test

dnfield · 2022-07-13T20:00:00Z

flow/layers/layer_tree.cc


+sk_sp<DisplayList> LayerTree::FlattenWithContext(
+    const SkRect& bounds,
+    TextureRegistry* texture_registry) {


I think you can probably just add this as an optional parameter on Flatten

Yeah, that makes sense!

dnfield · 2022-07-13T20:01:27Z

lib/ui/compositing/scene.cc

-  auto picture = layer_tree_->Flatten(SkRect::MakeWH(width, height));
+  auto picture = layer_tree_->FlattenWithContext(
+      SkRect::MakeWH(width, height),
+      snapshot_delegate.get()->GetTextureRegistry());


You can drop .get() here, but you should null check this. I think if the snapshot delegate is nullptr we can just proceed without the texture registry.

dnfield · 2022-07-13T20:01:35Z

lib/ui/compositing/scene.cc

                               Dart_Handle raw_image_handle) {
  TRACE_EVENT0("flutter", "Scene::toImageSync");
+  auto* dart_state = UIDartState::Current();
+  auto snapshot_delegate = dart_state->GetSnapshotDelegate();


nullcheck on dart_state.

These can be deleted now

jonahwilliams · 2022-07-13T20:03:48Z

I got this almost working (as in, it works but eventually crashes), the problem is that the texture registry needs to be accessed on the raster thread, so the flatten call needs to go in the posted task

jonahwilliams · 2022-07-13T20:45:43Z

lib/ui/compositing/scene.cc

+  fml::TaskRunner::RunNowOrPostTask(
+      raster_task_runner,
+      [snapshot_delegate, unref_queue, dl_image = std::move(dl_image),
+       layer_tree = std::move(layer_tree.get()), width, height]() {


What I'm doing to layer_tree doesn't seem right, but it did compile....

I don't think we can safely call takeLayerTree here. That makes the scene unusable in the future.

That said, I think we'll have to make the layer_tree_ a shared_ptr for this to work to share it across threads.

Also you don't need to move a raw pointer.

You'll want something like layer_tree = layer_tree_, and make the ivar a std::shared_ptr.

dnfield · 2022-07-13T21:07:17Z

flow/layers/layer_tree.h

+  sk_sp<DisplayList> FlattenWithContext(const SkRect& bounds,
+                                        TextureRegistry* texture_registry);


just remove this now

oop, why did this compile lol

…ne into hack_for_texture_view

jonahwilliams · 2022-07-13T22:10:40Z

lib/ui/compositing/scene.cc

+
 std::unique_ptr<flutter::LayerTree> Scene::takeLayerTree() {
-  return std::move(layer_tree_);
+  return nullptr;


Changing to shared_ptr almost works, except that everything that uses this API expects a unique_ptr.

Maybe I can hold onto the parts used to construct the LayerTree and then build either a shared_ptr or unique_ptr on the fly?

The root layer used to construct the layer tree is itself a shared_ptr...

What's it look like to just convert all those callsites to shared?

its a lot, let me dig in some more.

I updated the PR with all the changes needed to compile this

dnfield · 2022-07-15T05:11:30Z

lib/ui/compositing/scene.cc

+       layer_tree = layer_tree_, width, height]() {
+        auto display_list =
+            layer_tree->Flatten(SkRect::MakeWH(width, height),
+                                snapshot_delegate.get()->GetTextureRegistry());


need a to check snapshot_delegate. It might have been collected by now.

Doneish. Do I need to set anything on the dl_image, or could we assume we're shutting down now?

dnfield

Will need a test.

dnfield · 2022-07-15T05:11:38Z

lib/ui/compositing/scene.cc

+
+  fml::TaskRunner::RunNowOrPostTask(
+      raster_task_runner,
+      [snapshot_delegate, unref_queue, dl_image = std::move(dl_image),


std::move snapshot_delegate and unref_queue.

dnfield · 2022-07-15T05:13:13Z

lib/ui/compositing/scene.h

+                            uint32_t height,
+                            Dart_Handle raw_image_handle);
+
+  std::shared_ptr<flutter::LayerTree> layer_tree_;


Consider documenting why this is a shared pointer - and that it's no longer valid after takeLayerTree.

dnfield · 2022-07-15T05:16:43Z

lib/ui/compositing/scene.cc

+void Scene::RasterizeToImageSync(uint32_t width,
+                                 uint32_t height,
+                                 Dart_Handle raw_image_handle) {
+  auto* dart_state = UIDartState::Current();


I know this is a private method, but we should probably at least FML_DCHECK(layer_tree_) here.

Also, you'll need to do a null check on dart_state and just bail out early if it's nullptr (I think that might be extra defensive but there ends up being weirdness around isolate teardown and when these objects get destroyed).

…ure_view

jonahwilliams · 2022-07-18T19:01:02Z

I added a test that Scene.toImageSync works with a texture layer, but since actual texture creation is platform specific the only way I could test it further would be to mock the texture out like the unit tests do. Any other ideas @dnfield ?

…ne into hack_for_texture_view

…ure_view

jonahwilliams · 2022-07-21T01:40:59Z

Testing this more today, I ran into some issues I hadn't seen before with the texture registry. I'll need to step back through the code and see where I made a mistake. This doesn't block the page transition PR though, since that can use a fallback for both TextureLayer and PlatformLayer until this is fixed (we'd retain the fallback for PlatformLayer)

…ure_view

jonahwilliams · 2022-07-21T22:44:12Z

Ahh I'm missing the grcontext!

jonahwilliams · 2022-07-21T23:26:01Z

Updated and works correctly now, we also need a GrContext to paint the texture

…ure_view

dnfield · 2022-08-04T20:12:06Z

flow/layers/layer_tree.cc


-sk_sp<DisplayList> LayerTree::Flatten(const SkRect& bounds) {
+sk_sp<DisplayList> LayerTree::Flatten(const SkRect& bounds,
+                                      TextureRegistry* texture_registry,


In #35073 I'm making this a shared_ptr everywhere. Could you rebase on top of that? I'd rather do that than have bare pointers floating around.

dnfield · 2022-08-04T20:12:23Z

flow/layers/layer_tree.h


-  sk_sp<DisplayList> Flatten(const SkRect& bounds);
+  sk_sp<DisplayList> Flatten(const SkRect& bounds,
+                             TextureRegistry* texture_registry = nullptr,


Ditto here - let's just make these smart pointer objects.

dnfield · 2022-08-04T20:16:14Z

lib/ui/compositing.dart

    }
-    return _futurize((_Callback<Image?> callback) => _toImage(width, height, (_Image? image) {
+    final _Image image = _Image._();
+    return _futurize((_Callback<Image?> callback) => _toImage(width, height, image, (_Image? image) {


We should just make this a voidcallback, right?

We're passing in an image and then passing the same handle back it seems.

jonahwilliams · 2022-08-06T17:21:57Z

Will update this after I can rebase on the toPictureSync change

jonahwilliams · 2022-08-10T17:53:15Z

I think we should probably icebox this until we figure out toImageSync for Image shaders/paints.

Otherwise we'll end up with different coverage on the sync and async paths, with the former able to access textures easily and the latter able to be used in image shaders

Jonah Williams added 2 commits July 7, 2022 15:36

delete dl flag from allowlist

c82840f

hhacky

856069a

This comment was marked as off-topic.

Sign in to view

Jonah Williams added 3 commits July 12, 2022 18:35

Merge branch 'master' of github.com:flutter/engine into hack_for_text…

679d7f2

…ure_view

Re-arrange interface

614836d

++

73197e9

Update snapshot_delegate.h

8ba46b2

dnfield reviewed Jul 13, 2022

View reviewed changes

jonahwilliams mentioned this pull request Jul 13, 2022

Scene.toImage/toImageSync does not include TextureLayer contents flutter/flutter#107576

Closed

run flatten on raster thread

11d1844

jonahwilliams commented Jul 13, 2022

View reviewed changes

jonahwilliams changed the title ~~Hack for texture view/toImage~~ Flatten LayerTree on raster thread to include texture registry Jul 13, 2022

Jonah Williams added 2 commits July 13, 2022 13:54

make FlattenWithContext an optional parameter on Flatten

68bb764

++

36bed16

jonahwilliams marked this pull request as ready for review July 13, 2022 20:57

jonahwilliams requested a review from dnfield July 13, 2022 20:57

dnfield reviewed Jul 13, 2022

View reviewed changes

Jonah Williams added 4 commits July 13, 2022 14:08

Update layer_tree.h

550a907

Merge branch 'hack_for_texture_view' of github.com:jonahwilliams/engi…

cd92c27

…ne into hack_for_texture_view

amost works

a8c43a0

almost

87c2edb

jonahwilliams commented Jul 13, 2022

View reviewed changes

Jonah Williams added 2 commits July 13, 2022 16:59

change to shared_ptr

1fb2718

++

b3c0e00

dnfield reviewed Jul 15, 2022

View reviewed changes

jonahwilliams mentioned this pull request Jul 18, 2022

Use toPictureSync for faster zoom page transition flutter/flutter#106621

Merged

Jonah Williams added 2 commits July 18, 2022 11:53

add nullchecks and texture test

f7c8de4

Merge branch 'master' of github.com:flutter/engine into hack_for_text…

f612d0e

…ure_view

Jonah Williams added 2 commits July 18, 2022 12:08

++

3d21528

Update compositing_test.dart

abdf5b8

jonahwilliams requested a review from dnfield July 18, 2022 20:45

Jonah Williams added 3 commits July 20, 2022 12:25

Merge branch 'hack_for_texture_view' of github.com:jonahwilliams/engi…

62437b4

…ne into hack_for_texture_view

Merge branch 'master' of github.com:flutter/engine into hack_for_text…

844c4dd

…ure_view

Work in progress unification of toImage and toImageSync

bcb3f44

Jonah Williams added 2 commits July 21, 2022 14:16

Merge branch 'master' of github.com:flutter/engine into hack_for_text…

b628bfd

…ure_view

++

535a8c8

Jonah Williams added 2 commits July 21, 2022 16:23

include gr direct context

9c0a286

++

2a1a51c

jonahwilliams changed the title ~~Flatten LayerTree on raster thread to include texture registry~~ Flatten LayerTree on raster thread to include texture registry and GrDirectContext Jul 21, 2022

Merge branch 'master' of github.com:flutter/engine into hack_for_text…

acfb369

…ure_view

dnfield reviewed Aug 4, 2022

View reviewed changes

jonahwilliams closed this Aug 10, 2022

jonahwilliams deleted the hack_for_texture_view branch August 10, 2022 17:53

jonahwilliams restored the hack_for_texture_view branch August 17, 2022 07:16

		sk_sp<DisplayList> FlattenWithContext(const SkRect& bounds,
		TextureRegistry* texture_registry);

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

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

Uh oh!

Conversation

jonahwilliams commented Jul 13, 2022

Uh oh!

This comment was marked as off-topic.

jonahwilliams commented Jul 13, 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!

jonahwilliams commented Jul 13, 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!

dnfield left a comment

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!

jonahwilliams commented Jul 18, 2022

Uh oh!

jonahwilliams commented Jul 21, 2022

Uh oh!

jonahwilliams commented Jul 21, 2022

Uh oh!

jonahwilliams commented Jul 21, 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!

jonahwilliams commented Aug 6, 2022

Uh oh!

jonahwilliams commented Aug 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects