[Flutter GPU] Add missing MSAA stuff. #55424

bdero · 2024-09-25T05:30:51Z

bdero · 2024-09-25T05:48:40Z

The test seems to be exhibiting some weird behavior in Swiftshader. The content of the resolve texture is translucent and not antialiased.

Without MSAA:

With MSAA:

jonahwilliams · 2024-09-25T16:22:50Z

lib/gpu/lib/src/render_pass.dart

@@ -21,6 +21,32 @@ base class ColorAttachment {

  Texture texture;
  Texture? resolveTexture;
+
+  void _assertValid() {


I think this would be easier to read if you wrote it as a regular error checking function, throwing exceptions, then only called the function in an assert.

This is a regular pattern used in the framework, for example: https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/animation/animation_controller.dart#L551-L564

Thanks. I updated this. Let me know if you think I can improve the pattern more.

lib/gpu/lib/src/render_pass.dart

jonahwilliams · 2024-09-25T16:25:16Z

lib/gpu/render_pass.cc

  {
    impeller::DepthAttachment desc;
    desc.load_action = flutter::gpu::ToImpellerLoadAction(depth_load_action);
    desc.store_action = flutter::gpu::ToImpellerStoreAction(depth_store_action);
    desc.clear_depth = depth_clear_value;
    desc.texture = texture->GetTexture();
+    if (resolve_texture) {


I don't think depth/stencil attachments should have a resolve field.

Yeah we don't need to open this can of worms. Removed.

maybe I made a mistake when reading this but it looks like this was from the same attachment as the color attachment but maybe that isn't the case.

Feel free to open the can of worms as needed

jonahwilliams · 2024-10-08T23:18:57Z

lib/gpu/lib/src/context.dart

+  /// There is currently no way to render directly against the "onscreen"
+  /// texture that the framework renders to, so all Flutter GPU textures are
+  /// "offscreen".
+  bool get doesSupportOffscreenMSAA {


Just a thought. Maybe this doesn't actually need to be exposed at all.Just let folks construct the same MSAA or non-MSAA topolgy, and if its not supported then ignore the transient attachments and use the resolve attachment.

Practically speaking, the number of devices flutter targets that does not support MSAA is sooo small. I don't know if its worth carrying around in our API.

Additionaly, I don't know if this handles it but we need to do MSAA a bit differently for GLES. We don't ever actually allocate the transient attachment.

Oooh, I think for the common case of kMultisampleResolve, coercing to kStore and ignoring the multisample texture just about works. But unfortunately, things get funky with kStoreAndMultisampleResolve and I don't think we can reasonably assume the user's intent.

Additionaly, I don't know if this handles it but we need to do MSAA a bit differently for GLES. We don't ever actually allocate the transient attachment.

IIRC we did this in a way where it automagically does the right thing when you try to allocate a transient multisample texture on GLES (unless I'm thinking of the wrong thing). :)

If there's special HAL usage surrounding this, could you link to where we're doing it in the 2D renderer to help jog my memory?

I'm not sure if it would work automatically or not, see

engine/impeller/renderer/render_target.cc

Line 342 in 2d0eac0

if (context.GetCapabilities()->SupportsImplicitResolvingMSAA()) {

I think SupportsImplicitResolvingMSAA is just for MSAA against the onscreen texture

No, this is used for all offscreen MSAA on the GLES backend.

Oh I'm remembering this now. I don't know if I can easily hide this complexity under the API. With the way the HAL currently works, I would have to surface and document SupportsImplicitResolvingMSAA. The user is in an alternate reality where they have to creates, attach, and bind fake multisample textures.

I'm not sure the right way to fix this in the API, but maybe consider only supporting MSAA on metal/vulkan for now and coming back to GLES?

Yeah. I changed this to add a graceful fallback to not use MSAA when "normal" MSAA support is not possible. I kept the doesSupportOffscreenMSAA check, but now it also returns false if "implicit" MSAA is available.

A renderer could easily use this check to fallback to other AA techniques (such as FXAA) when MSAA is unavailable.

jonahwilliams

LGTM (but not to CI)

…lorAttachment.

flutter-dashboard · 2024-10-22T01:02:10Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #55424 at sha 00ee6f5

…157317) flutter/engine@b3e227b...8828844 2024-10-22 [email protected] [Flutter GPU] Add missing MSAA stuff. (flutter/engine#55424) 2024-10-22 [email protected] Roll Skia from 3a081993e2a7 to a71df7d9bc63 (24 revisions) (flutter/engine#56005) 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 [email protected],[email protected] 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

bdero added the flutter-gpu label Sep 25, 2024

bdero requested a review from chinmaygarde September 25, 2024 05:30

bdero self-assigned this Sep 25, 2024

bdero force-pushed the bdero/fluttergpu-msaa branch from 1b9c386 to e1b0e6a Compare September 25, 2024 05:47

jonahwilliams reviewed Sep 25, 2024

View reviewed changes

bdero force-pushed the bdero/fluttergpu-msaa branch from e1b0e6a to 281c668 Compare October 8, 2024 22:14

bdero requested a review from jonahwilliams October 8, 2024 22:17

jonahwilliams reviewed Oct 8, 2024

View reviewed changes

bdero force-pushed the bdero/fluttergpu-msaa branch from 281c668 to 33d9f51 Compare October 9, 2024 00:08

bdero force-pushed the bdero/fluttergpu-msaa branch from 33d9f51 to 2d39218 Compare October 19, 2024 01:24

bdero requested a review from jonahwilliams October 19, 2024 01:37

jonahwilliams approved these changes Oct 19, 2024

View reviewed changes

bdero added 5 commits October 21, 2024 14:13

[Flutter GPU] Add missing MSAA stuff.

8af165e

Use exceptions for errors

4a29748

Remove depthstencil resolve

10ef5b4

Gracefully fallback to not using MSAA when normal MSAA is not supported.

f64cd0d

Remove clearBindings hack

29bd4c4

bdero force-pushed the bdero/fluttergpu-msaa branch from b2de4d1 to 29bd4c4 Compare October 21, 2024 21:43

bdero added 3 commits October 21, 2024 16:17

The RenderPass isn't initialized yet, so pass the gpuContext to SetCo…

930646f

…lorAttachment.

Correct test skip logic

fe13382

Fix skip logic more

00ee6f5

flutter-dashboard bot added the will affect goldens label Oct 22, 2024

bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 22, 2024

auto-submit bot merged commit 8828844 into flutter:main Oct 22, 2024
33 checks passed

engine-flutter-autoroll mentioned this pull request Oct 22, 2024

Roll Flutter Engine from b3e227b9a5d9 to 88288446c395 (2 revisions) flutter/flutter#157317

Merged

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

88288446c [Flutter GPU] Add missing MSAA stuff. (flutter/engine#55424)

d973e05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flutter GPU] Add missing MSAA stuff. #55424

[Flutter GPU] Add missing MSAA stuff. #55424

bdero commented Sep 25, 2024

bdero commented Sep 25, 2024 •

edited

Loading

jonahwilliams Sep 25, 2024

bdero Oct 8, 2024

jonahwilliams Sep 25, 2024

bdero Oct 8, 2024

jonahwilliams Oct 8, 2024

jonahwilliams Oct 8, 2024

bdero Oct 9, 2024

jonahwilliams Oct 9, 2024

bdero Oct 9, 2024

jonahwilliams Oct 9, 2024

bdero Oct 9, 2024

jonahwilliams Oct 9, 2024

bdero Oct 19, 2024 •

edited

Loading

jonahwilliams left a comment

flutter-dashboard bot commented Oct 22, 2024

[Flutter GPU] Add missing MSAA stuff. #55424

[Flutter GPU] Add missing MSAA stuff. #55424

Conversation

bdero commented Sep 25, 2024

bdero commented Sep 25, 2024 • edited Loading

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

bdero Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Oct 22, 2024

bdero commented Sep 25, 2024 •

edited

Loading

bdero Oct 19, 2024 •

edited

Loading