[Impeller] finish migration to new render pass API. #49740

jonahwilliams · 2024-01-12T01:50:59Z

Migrate the rest of the commands in impeller to use the new API. Hide RenderPass::AddCommand. On subsequent changes I will be able to begin making some of these methods virtual so we can add more direct pass through. Though the vulkan backend will be blocked on changes to descriptor sets: #49686

jonahwilliams · 2024-01-12T01:51:15Z

impeller/entity/contents/atlas_contents.cc

-    cmd.stencil_reference = entity.GetClipDepth();
-    auto options = OptionsFromPass(pass);
-    cmd.pipeline = renderer.GetPorterDuffBlendPipeline(options);
+#ifdef IMPELLER_DEBUG


I only added ifdefs where we do runtime string creation.

I prefer the macro style we had previously. Did you find the string creation wasn't getting stripped out?

FWIW this could be transformed to a static string too since we only have so many blend modes. I did that at some point but the PR got caught up and dropped.

Strings were still getting formatted, then dropped. We could add more static strings for these, I wouldn't be opposed. Feedback I got on the macro approach was that everyone hated it 😆

gaaclarke

LGTM, there is just one spot where there might be an accident logic change. Also, this introduces string copies which I think you were trying to avoid so maybe it's better to use the macro function still to set the debug labels.

gaaclarke · 2024-01-12T18:38:41Z

impeller/entity/contents/atlas_contents.cc

-    cmd.stencil_reference = entity.GetClipDepth();
-    auto options = OptionsFromPass(pass);
-    cmd.pipeline = renderer.GetPorterDuffBlendPipeline(options);
+#ifdef IMPELLER_DEBUG


I prefer the macro style we had previously. Did you find the string creation wasn't getting stripped out?

FWIW this could be transformed to a static string too since we only have so many blend modes. I did that at some point but the PR got caught up and dropped.

gaaclarke · 2024-01-12T18:41:51Z

impeller/entity/contents/clip_contents.cc

-      cmd.pipeline = renderer.GetClipPipeline(options);
-      pass.AddCommand(Command(cmd));
+      pass.SetPipeline(renderer.GetClipPipeline(options));
+      pass.SetStencilReference(entity.GetClipDepth());


Is this difference intentional? This used to be set outside of the conditional.

It shouldn't actually matter, this one is redundant since I call Draw immediately below.

gaaclarke · 2024-01-12T18:45:39Z

impeller/entity/contents/texture_contents.cc

  if (label_.empty()) {
-    DEBUG_COMMAND_INFO(cmd, "Texture Fill");
+    pass.SetCommandLabel("Texture Fill");


You just set it to the unnamed label in non debug builds.

It will get dropped anyway, because SetCommandLabel will no-op

gaaclarke · 2024-01-12T18:47:29Z

impeller/entity/contents/atlas_contents.cc

@@ -396,8 +397,7 @@ bool AtlasTextureContents::Render(const ContentContext& renderer,
    return true;
  }

-  Command cmd;
-  DEBUG_COMMAND_INFO(cmd, "AtlasTexture");
+  pass.SetCommandLabel("AtlasTexture");


FYI this is still incurring a copy because the parameter takes a std::string.

https://github.com/flutter/engine/blob/main/impeller/renderer/render_pass.h#L74 This takes a string view.

RenderPass::OnSetLabel takes an std::string, it doesn't hit that?

RenderPass label vs Command label

Maybe we should rename SetCommandLabel at some point since the concept of Command has been eliminated.

Yeah, I think that it would be better if the RenderPass label was provided during construction instead of via a Setter. Then we just need "SetLabel"

gaaclarke

lgtm with nit about being able to have static debug labels as a fallback

…ation

…e into finish_cmd_migration

…r/engine#49740)

…141489) flutter/engine@418c9e9...b8e5d47 2024-01-12 jonahwilliams@google.com [Impeller] move TrackedObjectsVK to separate file. (flutter/engine#49773) 2024-01-12 54558023+keyonghan@users.noreply.github.com Add gclient_variables back for linux_license and fix the excluded files (flutter/engine#49775) 2024-01-12 jonahwilliams@google.com [Impeller] Descriptor pool incremental allocation. (flutter/engine#49686) 2024-01-12 zanderso@users.noreply.github.com Relands: Refactors RBE support (flutter/engine#49660) 2024-01-12 jonahwilliams@google.com [Impeller] finish migration to new render pass API. (flutter/engine#49740) 2024-01-12 jonahwilliams@google.com [Impeller] remove Buffer type and associated abstractions. (flutter/engine#49702) 2024-01-12 54558023+keyonghan@users.noreply.github.com Remove `gclient_variables` from `linux_license.json` (flutter/engine#49766) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

[Impeller] finish migration to new render pass API.

b47d01e

jonahwilliams commented Jan 12, 2024

View reviewed changes

github-actions bot added the e: impeller label Jan 12, 2024

jonahwilliams added 2 commits January 11, 2024 19:16

++

5143ba9

++

52f7456

jonahwilliams requested review from bdero and gaaclarke January 12, 2024 18:23

gaaclarke reviewed Jan 12, 2024

View reviewed changes

Update clip_contents.cc

0536713

gaaclarke approved these changes Jan 12, 2024

View reviewed changes

bdero approved these changes Jan 12, 2024

View reviewed changes

jonahwilliams added 2 commits January 12, 2024 12:28

Merge branch 'main' of github.com:flutter/engine into finish_cmd_migr…

2ac88c2

…ation

Merge branch 'finish_cmd_migration' of github.com:jonahwilliams/engin…

c9fc88b

…e into finish_cmd_migration

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 12, 2024

auto-submit bot merged commit 35336ad into flutter:main Jan 12, 2024
28 checks passed

engine-flutter-autoroll mentioned this pull request Jan 12, 2024

Roll Flutter Engine from 418c9e918a22 to 35336ad64766 (3 revisions) flutter/flutter#141481

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 12, 2024

35336ad64 [Impeller] finish migration to new render pass API. (flutte…

cb820a3

…r/engine#49740)

engine-flutter-autoroll mentioned this pull request Jan 12, 2024

Roll Flutter Engine from 418c9e918a22 to b8e5d4776908 (7 revisions) flutter/flutter#141489

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 12, 2024

35336ad64 [Impeller] finish migration to new render pass API. (flutte…

15a6151

…r/engine#49740)

jonahwilliams deleted the finish_cmd_migration branch January 18, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] finish migration to new render pass API. #49740

[Impeller] finish migration to new render pass API. #49740

jonahwilliams commented Jan 12, 2024 •

edited

Loading

jonahwilliams Jan 12, 2024

gaaclarke Jan 12, 2024

jonahwilliams Jan 12, 2024

gaaclarke left a comment

gaaclarke Jan 12, 2024

gaaclarke Jan 12, 2024

jonahwilliams Jan 12, 2024

gaaclarke Jan 12, 2024

jonahwilliams Jan 12, 2024

gaaclarke Jan 12, 2024

jonahwilliams Jan 12, 2024

gaaclarke Jan 12, 2024

jonahwilliams Jan 12, 2024

gaaclarke Jan 12, 2024

jonahwilliams Jan 12, 2024

gaaclarke left a comment

[Impeller] finish migration to new render pass API. #49740

[Impeller] finish migration to new render pass API. #49740

Conversation

jonahwilliams commented Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

jonahwilliams commented Jan 12, 2024 •

edited

Loading