[Impeller] Add Picture::RenderToImage #33864

bdero · 2022-06-07T02:25:11Z

Been meaning to add this for a while to speed up some our of fancier tests (like color wheel). Just noticed flutter/flutter#104718 as I was making this PR. I don't know what the context of that issue is or how it might interface with the dispatcher, but perhaps this is in the right direction anyhow?

Screen.Recording.2022-06-06.at.7.20.22.PM.mov

dnfield · 2022-06-07T03:27:03Z

impeller/aiks/picture.cc

+    return nullptr;
+  }
+
+  return std::make_shared<Image>(target.GetRenderTargetTexture());


This is a GPU resident texture right?

The API exposed via toImage expects a CPU resident image.

We can mark the new texture as host visible and have Image handle the shared buffer transfer internally. Would prefer to punt that down the road to when we add in stuff like ReadPixels (if/when we need it).

This isn't exposed up through the Dart API AFAICT, right?

If so, let's just document the behavior in a comment here.

Nah this is only Impeller internal right now. Added a comment about the visibility.

chinmaygarde · 2022-06-08T00:09:38Z

impeller/aiks/picture.h

 struct Picture {
  std::unique_ptr<EntityPass> pass;
+
+  std::shared_ptr<Image> RenderToImage(AiksContext& renderer);


Nit: AiksContext& context?

Another idea could be to flip it and ask the AiksContext to snapshot the picture as an image.

chinmaygarde · 2022-06-08T00:11:19Z

impeller/aiks/picture.cc


-//
+std::shared_ptr<Image> Picture::RenderToImage(AiksContext& renderer) {
+  auto coverage = pass->GetElementsCoverage();


Since this is a utility method, add validity checks. Some stuff that comes to mind:

Null checks for pass.

Check for empty coverage. You won't be able to create a zero sized offscreen texture anyway.

Done. Added check for render target + coverage. Gotta be more careful about these coverage optionals -- I've forgotten to check them before.

chinmaygarde · 2022-06-08T00:11:26Z

impeller/aiks/picture.cc

+      *renderer.context_, ISize(coverage->GetRight(), coverage->GetBottom()));
+
+  if (!renderer.Render(*this, target)) {
+    return nullptr;


VALIDATION_LOG here?

chinmaygarde · 2022-06-08T00:12:06Z

impeller/aiks/picture.cc

+std::shared_ptr<Image> Picture::RenderToImage(AiksContext& renderer) {
+  auto coverage = pass->GetElementsCoverage();
+  auto target = RenderTarget::CreateOffscreen(
+      *renderer.context_, ISize(coverage->GetRight(), coverage->GetBottom()));


This will create an unnecessarily large texture if the coverage has an offset element. In that case, pass entities would need to be copied and offset applied (I believe we already do this in the entity pass).

This is the behavior of the picture.toImage. I found it a tad less confusing to reason about as opposed to returning the position as part of the result or otherwise forcing the caller to fetch coverage from the picture separately. Up to you though -- I don't really have a strong opinion.

A good middle ground might be to add a convenience method for walking the picture and absorbing the top left coverage margin as you mention.

In that case, I think we should return an impeller::Snapshot in //impeller/entity/contents. Returning the position as part of image is then taken care of. Introducing a potentially large allocation overhead to make the API easier to reason makes me nervous. Especially when we have followed the right pattern elsewhere and have utilities for this purpose already.

If you do decide to use the snapshot, perhaps just call the method Picture::Snapshot as well and also move //impeller/entity/contents/snapshot to someplace more easily discoverable. Perhaps //impeller/renderer/snapshot? I think it's a good way of thinking about snapshotting and you have already thought through the various ways in which snapshotting can be used.

SGTM, done.

chinmaygarde · 2022-06-08T00:13:01Z

impeller/aiks/picture.cc

+    return nullptr;
+  }
+
+  return std::make_shared<Image>(target.GetRenderTargetTexture());


Check that the texture is not null. Add a validation log if it is and return nullptr.

IMO, we should, by convention, not return invalid resources out of factories that return resources via a pointer. Instead, favoring returning null. This makes the caller not have to do a null and IsValid invocation pair.

chinmaygarde · 2022-06-08T00:14:30Z

impeller/aiks/aiks_context.h

  std::unique_ptr<ContentContext> content_context_;
  bool is_valid_ = false;

+  friend Picture;


If the caller needs the impeller::Context from the AiksContext, you could just add a getter (completely reasonable IMO) instead of adding a friend.

chinmaygarde · 2022-06-08T18:21:10Z

Left a comment about snapshotting. Not sure if this is a bit of a yak-shave though.

chinmaygarde · 2022-06-09T20:21:57Z

cc @bdero Ping on the open question on the usage of an impeller::Snapshot. If you'd rather handle it in a later patch, LGTM here. I don't want to block other work that perhaps builds on top of this.

bdero · 2022-06-13T23:35:27Z

Sorry for the delay, just got around to fixing this up.

fluttergithubbot · 2022-06-14T07:18:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux Unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

bdero added affects: engine e: impeller labels Jun 7, 2022

bdero requested review from chinmaygarde, dnfield and zanderso June 7, 2022 02:25

dnfield reviewed Jun 7, 2022

View reviewed changes

chinmaygarde reviewed Jun 8, 2022

View reviewed changes

bdero force-pushed the bdero/picture-to-image branch from 4a01f85 to c0a7891 Compare June 8, 2022 03:06

chinmaygarde assigned bdero Jun 8, 2022

chinmaygarde approved these changes Jun 9, 2022

View reviewed changes

bdero added 4 commits June 13, 2022 23:15

Picture::RenderToImage

b6e3dc3

Address comments

13e9f73

Return snapshot

122df18

Move snapshot to renderer

6bb5b25

bdero force-pushed the bdero/picture-to-image branch from c932ced to 6bb5b25 Compare June 14, 2022 06:16

bdero added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 14, 2022

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 14, 2022

Format

85aa628

bdero merged commit 9c067e6 into flutter:main Jun 14, 2022

engine-flutter-autoroll mentioned this pull request Jun 14, 2022

Roll Flutter Engine from edc2aa4e894b to 5ad8879327be (23 revisions) flutter/flutter#105978

Merged

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

9c067e6 [Impeller] Add Picture::RenderToImage (flutter/engine#33864)

ffdfc34

houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022

[Impeller] Add Picture::RenderToImage (flutter#33864)

2f9bb61

[Impeller] Add Picture::RenderToImage #33864

[Impeller] Add Picture::RenderToImage #33864

Uh oh!

Conversation

bdero commented Jun 7, 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!

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!

chinmaygarde commented Jun 8, 2022

Uh oh!

chinmaygarde commented Jun 9, 2022

Uh oh!

bdero commented Jun 13, 2022

Uh oh!

fluttergithubbot commented Jun 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants