[Impeller] Change Renderer utility to pass RenderTarget to callback; render non-pipeline blend modes #32982

bdero · 2022-04-28T10:40:36Z

Attach blend filters on the fly to support rendering with "advanced" (non-pipeline) blend modes. Advanced blends manipulate color, and behave like kSourceOver alpha-wise. They require sampling the source/destination textures as inputs and rendering to an intermediate texture, whereas the "pipeline" blends are simple Porter-Duff alpha compositing operations that can be performed by configuring builtin pipeline blend options.

bdero · 2022-05-03T05:15:37Z

Alright, I think this PR is in a pretty good spot for review.

Advanced blends require binding the "current" pass resolve texture and rendering the result to an intermediate texture. And so the active render pass (which attaches the backdrop texture in question for writing) needs to execute before the blend pass (which binds the backdrop texture for reading), and then subsequent writes to the texture need to happen in a new RenderPass which executes after the blend.

To make this possible, I changed the Renderer utility to pass RenderTargets (which is essentially a RenderPass descriptor with texture handles) to the render callback instead of creating and passing one RenderPass. This punts the responsibility of creating RenderPasses to the caller. The RenderTarget makes its way to EntityPass, which now interleaves render passes following the pattern explained above.

Rendering all elements of the EntityPass (whether entities or subpasses) now happens in the same codepath. Prior to rendering, advanced blends are applied.

There is a small fanout of other problems I've noticed along the way that can be best addressed in follow-up PRs. These are the ones relevant to advanced blends (and are visible in the test below):

When the BlendFilter draws advanced blends, the backdrop texture is being transformed incorrectly.
kScreen looks like it's outputting colors that are too dark -- smells like a problem in the way I'm handling premultiplied alpha.

Fancy new color wheel blend test for Aiks (compare with https://fiddle.skia.org/c/@BlendModes):

Screen.Recording.2022-05-02.at.10.10.27.PM.mov

chinmaygarde · 2022-05-03T16:28:05Z

Awesome. I'll take a stab at reviewing this today!

bdero · 2022-05-04T23:24:52Z

Rebased this past the GLES 2 changes. I plan to address problems with the blend filter itself in a follow-up.

bdero · 2022-05-05T19:08:59Z

impeller/playground/backend/metal/playground_impl_mtl.mm

+  if (@available(macOS 10.13, *)) {
+    // When calling nextDrawable, wait for the texture to become available
+    // indefinitely.
+    data_->metal_layer.allowsNextDrawableTimeout = NO;


These two layer properties were needed to make the surface textures readily bindable. Both of them are enabled in the iOS Flutter embedder, which enables Skia to do the same thing. framebufferOnly makes the surface textures readable. Without allowsNextDrawableTimeout, multi-pass rendering seemed to result in the texture being presented before all of the passes finish executing.

We do set framebufferOnly in the Skia renderer but allowsNextDrawableTimeout is definitely not being set. I think we should investigate the need for setting this.

Removed this in favor of just rendering to a third texture in this situation.

bdero · 2022-05-05T19:15:44Z

This also makes collapsed passes create a new command buffer/RenderPass, but that could be improved by passing an optional "starter" RenderPass to child EntityPass::Render calls.

chinmaygarde · 2022-05-06T00:44:27Z

Can we schedule some time tomorrow to go over this PR, have a few thoughts to clarify.

chinmaygarde · 2022-05-06T19:06:34Z

impeller/renderer/backend/metal/surface_mtl.mm

@@ -61,7 +61,8 @@
  color0_resolve_tex_desc.format = color_format;
  color0_resolve_tex_desc.size = color0_tex_desc.size;
  color0_resolve_tex_desc.usage =
-      static_cast<uint64_t>(TextureUsage::kRenderTarget);
+      static_cast<uint64_t>(TextureUsage::kRenderTarget) |


Like the color_format var above, I think we should read this from current_drawable.texture.usage instead of making it match in two places. Then, a validation can be added here saying the texture could not be read from.

I suppose I am not sure why the usage of the resolve texture matters. Shouldn't it be on the usage flags of the color0_tex_desc descriptor?

Changed this to not read from the surface texture.

chinmaygarde · 2022-05-06T19:09:36Z

impeller/renderer/surface.h

@@ -26,7 +26,7 @@ class Surface {

  bool IsValid() const;

-  const RenderTarget& GetTargetRenderPassDescriptor() const;
+  RenderTarget& GetTargetRenderPassDescriptor();


This is perplexing. How would mutating the render target of a surface work?

Discussed offline -- this was my (overly) aggressive aversion to copying striking again. ;)

chinmaygarde · 2022-05-06T19:20:33Z

impeller/entity/entity_pass.cc

+      // input ("source"), writing the result to an intermediate texture, and
+      // finally copying the data from the intermediate texture back to the
+      // render target texture. And so all of the commands that have written to
+      // the render target texture so far need to execute before it's binded for


bdero · 2022-05-07T03:17:00Z

@chinmaygarde I rewrote this to render the root EntityPass to a secondary texture so that we don't have to disable framebufferOnly. As I started adding a simple texture->texture blit to the render context but realized GLES 2 doesn't have glBlitFramebuffer. :/ Know if any good tricks in GLES 2 for fast blits?

I'm probably overthinking it since this'll only happen once per frame (and only if advanced blends are used on the root pass specifically), so maybe just rolling with a raster pipeline for the final blit is fine for now.

chinmaygarde · 2022-05-09T18:37:11Z

Do you really need glBlitFramebuffer if you can approximate its behavior with a draw call by sampling from the source?

Speaking of glBlitFramebuffer, I don't think there is an equivalent way to do this in renderer framework. I think we should add this feature now (and implement it using a draw in ES2 perhaps).

bdero · 2022-05-09T22:36:08Z

Do you really need glBlitFramebuffer if you can approximate its behavior with a draw call by sampling from the source?

Yeah, this is what I meant by using a raster pipeline. We don't need blits, but was hoping we'd support it at some point.

I changed this implementation to render to a third texture when any advanced blends are used in the root EntityPass. This is good to review again!

Fix validation failures Allow for multiple passes against the on screen target Add fancy color wheel blend test Tweak test Fix compare URL Improve comments

…llback; render non-pipeline blend modes (flutter/engine#32982)

chinmaygarde added the e: impeller label Apr 28, 2022

zanderso requested a review from chinmaygarde April 28, 2022 20:29

bdero force-pushed the bdero/render-advanced-blends branch from ef06683 to 5b8156e Compare April 30, 2022 01:47

bdero changed the title ~~WIP: [Impeller] Render non-pipeline blend modes~~ [Impeller] Render non-pipeline blend modes May 3, 2022

bdero requested a review from dnfield May 4, 2022 18:00

bdero force-pushed the bdero/render-advanced-blends branch 2 times, most recently from 698c878 to 424e1f2 Compare May 4, 2022 23:22

bdero changed the title ~~[Impeller] Render non-pipeline blend modes~~ [Impeller] Change Renderer utility to pass RenderTarget to callback; render non-pipeline blend modes May 5, 2022

bdero commented May 5, 2022

View reviewed changes

chinmaygarde reviewed May 6, 2022

View reviewed changes

bdero force-pushed the bdero/render-advanced-blends branch 2 times, most recently from 3276bfe to ac90180 Compare May 9, 2022 22:30

bdero requested a review from chinmaygarde May 9, 2022 22:36

bdero added 2 commits May 9, 2022 16:39

Render advanced blends

bb028b3

Fix validation failures Allow for multiple passes against the on screen target Add fancy color wheel blend test Tweak test Fix compare URL Improve comments

Address comments

367ba1e

bdero force-pushed the bdero/render-advanced-blends branch from ac90180 to 367ba1e Compare May 9, 2022 23:40

chinmaygarde approved these changes May 9, 2022

View reviewed changes

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 May 10, 2022

fluttergithubbot merged commit 3f51133 into flutter:main May 10, 2022

engine-flutter-autoroll mentioned this pull request May 10, 2022

Roll Flutter Engine from fcf0e806b390 to 0b5a6a95259b (3 revisions) flutter/flutter#103391

Merged

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

3f51133 [Impeller] Change Renderer utility to pass RenderTarget to ca…

aaf5dad

…llback; render non-pipeline blend modes (flutter/engine#32982)

bdero mentioned this pull request May 12, 2022

[Impeller] Add ColorBurn, fix SourceOver alpha, make default pipeline initialization robust #33289

Merged

[Impeller] Change Renderer utility to pass RenderTarget to callback; render non-pipeline blend modes #32982

[Impeller] Change Renderer utility to pass RenderTarget to callback; render non-pipeline blend modes #32982

Uh oh!

Conversation

bdero commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdero commented May 3, 2022

Uh oh!

chinmaygarde commented May 3, 2022

Uh oh!

bdero commented May 4, 2022

Uh oh!

bdero May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bdero commented May 5, 2022

Uh oh!

chinmaygarde commented May 6, 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!

bdero commented May 7, 2022

Uh oh!

chinmaygarde commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdero commented May 9, 2022

Uh oh!

Uh oh!

bdero commented Apr 28, 2022 •

edited

Loading

bdero May 5, 2022 •

edited

Loading

chinmaygarde commented May 9, 2022 •

edited

Loading