[Impeller] support GLES 3.0 MSAA without extension. #56705

jonahwilliams · 2024-11-19T16:06:53Z

Adds multisampling support for GLES devices without GL_EXT_multisampled_render_to_texture provided they are at least GLES 3.0 to support mutlisampled render buffers.

Fixes flutter/flutter#158360
Fixes flutter/flutter#157951

TBD: should we prefer renderbuffer 3.0 approach over multisample_render_to_texture?

flutter-dashboard · 2024-11-19T16:06:56Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

jonahwilliams · 2024-11-19T16:34:34Z

impeller/display_list/canvas.cc

+  // Unlike other backends the blit to restore the onscreen fails for GLES
+  // if the src is a multisample framebuffer, even if we specifically target
+  // the non-multisampled resolve of the dst.
+  bool is_gles_and_must_skip_blit = renderer_.GetContext()->GetBackendType() ==


It seems like once we have a multisampled renderbuffer attached to a texture blits will just fail :(

jonahwilliams · 2024-11-19T16:35:28Z

FYI @bdero , I think with this approach we can let flutter gpu use a non-weird MSAA approach for GLES 3 (via ANGLE) which should cover all of the windows desktop usage at least.

jonahwilliams · 2024-11-19T16:35:56Z

impeller/renderer/backend/gles/texture_gles.cc

    case PixelFormat::kB8G8R8A8UNormInt:
    case PixelFormat::kR8G8B8A8UNormInt:
-      return GL_RGBA4;
+      return GL_RGBA8;


This was fun to find lol

jonahwilliams · 2024-11-19T16:37:15Z

impeller/renderer/backend/gles/capabilities_gles.cc

    GLint value = 0;
    gl.GetIntegerv(GL_MAX_SAMPLES_EXT, &value);
    supports_offscreen_msaa_ = value >= 4;
+  } else if (desc->GetGlVersion().major_version >= 3 && desc->IsES()) {


I think this is ES only? I don't have a desktop GL test environment.

flutter-dashboard · 2024-11-19T16:45:25Z

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 #56705 at sha 26a1b71

gaaclarke

Looking super promising. This is giving us MSAA for the render buffer right? Is it also going to give MSAA for offscreen frame buffers? That's what we need for the golden's to be antialiased.

gaaclarke · 2024-11-19T17:40:22Z

impeller/renderer/backend/gles/capabilities_gles.cc

    supports_offscreen_msaa_ = value >= 4;
+  } else if (desc->GetGlVersion().major_version >= 3 && desc->IsES()) {
+    GLint value = 0;
+    gl.GetIntegerv(GL_MAX_SAMPLES_EXT, &value);


I don't think you can safely query this without first verifying that GL_EXT_framebuffer_multisample is present.

I believe this is core in 3.0:

https://registry.khronos.org/OpenGL-Refpages/es3.0/html/glRenderbufferStorageMultisample.xhtml

I don't see GL_MAX_SAMPLES_EXT mentioned there. The only place I see it mentioned is in documentation for that extension (ex: https://www.khronos.org/opengl/wiki/GL_EXT_framebuffer_multisample). I'm pretty sure it's not standard because it has the _EXT postfix.

oh, right that. I think we can just drop the ext

can confirm GL_MAX_SAMPLES_EXT and GL_MAX_SAMPLES have the same value. I can change the latter to GL_MAX_SAMPLES but it doesn't actually matter, I think they should both just be GL_MAX_SAMPLES

gaaclarke · 2024-11-19T17:45:22Z

impeller/renderer/backend/gles/texture_gles.cc

+                        ? TextureGLES::Type::kTextureMultisampled
+                        : TextureGLES::Type::kRenderBufferMultisampled)


This seems odd that it is conflating textures with render buffers. This isn't anything new introduced in this PR though.

The render buffer AFAIK is like the "Device transient" texture, though there is some weirdness with how we treat the implicit msaa stuff

gaaclarke · 2024-11-19T17:48:34Z

impeller/renderer/backend/gles/texture_gles.cc

+          // with regular GLES 3.0 multisampled renderbuffers/textures.
+          if (gl.GetCapabilities()->SupportsImplicitResolvingMSAA()) {
+            gl.RenderbufferStorageMultisampleEXT(
+                GL_RENDERBUFFER,               // target


please use proper argument comments to get them checked by the linter (ex. /*foobar=*/)

doesn't look like the linter is checking these, maybe something to do with the proc table drop argument names? either that or its a C ABI.

will switch the comment stye regardless

Weird, I double checked we do have bugprone-argument-comment in .clang-tidy.

gaaclarke · 2024-11-19T17:53:11Z

impeller/renderer/backend/gles/render_pass_gles.cc

+    // Perform multisample resolve via blit.
+    // Create and bind a resolve FBO.
+    GLuint resolve_fbo;
+    gl.GenFramebuffers(1u, &resolve_fbo);


Is this generating and deleting a frame buffer each frame?

for offscreen fbos we always generate and delete the fbo AFAIK.

gaaclarke · 2024-11-19T17:54:51Z

impeller/renderer/backend/gles/render_pass_gles.cc

+  }
+
  if (gl.DiscardFramebufferEXT.IsAvailable()) {
+    gl.BindFramebuffer(GL_FRAMEBUFFER, fbo);


It's weird this logic is changing because the new logic above it is in in a conditional, will this be the right thing to do if the conditional above evaluates to true or false?

it should be safe to rebind the same fbo but I'll move this to the end of the blit instead.

gaaclarke · 2024-11-19T17:58:56Z

This is giving us MSAA for the render buffer right? Is it also going to give MSAA for offscreen frame buffers? That's what we need for the golden's to be antialiased.

Nevermind, looks like it is based on the golden results.

gaaclarke · 2024-11-19T18:13:33Z

How is this going to be have on systems using opengles and which have support for offscreen msaa (for example here:

engine/impeller/display_list/dl_dispatcher.cc

Line 1248 in d5820a6

if (context.GetContext()->GetCapabilities()->SupportsOffscreenMSAA()) {

)?

jonahwilliams · 2024-11-19T18:40:45Z

@gaaclarke CreateOffscreenMSAA conditionally handles implicit (extension) vs non-implicit MSAA

gaaclarke

This is looking good to me. I just have one more suggestion on trying to make this code a bit easier to understand and maintain. It's a bit hard to follow as is.

gaaclarke · 2024-11-19T19:28:29Z

impeller/renderer/backend/gles/render_pass_gles.cc

+  if (pass_data.resolve_attachment &&
+      pass_data.resolve_attachment != pass_data.color_attachment &&
+      !is_default_fbo) {


Can we make this branch use the same conditional that is being used in the other code? Something like:

if (!gl.GetCapabilities()->SupportsImplicitResolvingMSAA()) { FML_DCHECK(pass_data.resolve_attachment); }

This feature lives across multiple files, it would be nice if there was an easy way to find all the different components that go into making this work. So instead of conditionalizing on the state, conditionalize on how it should behave and assert the state.

flutter-dashboard · 2024-11-19T19:58:21Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #56705 at sha 82c0fce

gaaclarke

lgtm, nice work

flutter-dashboard · 2024-11-19T21:48:14Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #56705 at sha 93beb95

…r/engine#56705)

jonahwilliams · 2024-11-21T23:13:14Z

reason for revert: goldens occassionally fail to render anything.

This reverts commit ec065be.

#56741) Reverts: #56705 Initiated by: jonahwilliams Reason for reverting: goldens occassionally fail to render anything. Original PR Author: jonahwilliams Reviewed By: {gaaclarke} This change reverts the following previous change: Adds multisampling support for GLES devices without GL_EXT_multisampled_render_to_texture provided they are at least GLES 3.0 to support mutlisampled render buffers. Fixes flutter/flutter#158360 Fixes flutter/flutter#157951 TBD: should we prefer renderbuffer 3.0 approach over multisample_render_to_texture?

…r/engine#56705)

…159308) flutter/engine@d1a0806...6f941c9 2024-11-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] support GLES 3.0 MSAA without extension. (#56705)" (flutter/engine#56741) 2024-11-21 [email protected] Roll Dart SDK from dde57dc75c15 to b36e4d731d67 (1 revision) (flutter/engine#56723) 2024-11-21 [email protected] [native assets] Consume `NativeAssetsManifest.json` (flutter/engine#56727) 2024-11-21 [email protected] [Impeller] support GLES 3.0 MSAA without extension. (flutter/engine#56705) 2024-11-21 [email protected] Updated some impeller benchmark urls (flutter/engine#56721) 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

…6705) Adds multisampling support for GLES devices without GL_EXT_multisampled_render_to_texture provided they are at least GLES 3.0 to support mutlisampled render buffers. Fixes flutter#158360 Fixes flutter#157951 TBD: should we prefer renderbuffer 3.0 approach over multisample_render_to_texture?

…#56705)" (flutter/engine#56741) Reverts: flutter/engine#56705 Initiated by: jonahwilliams Reason for reverting: goldens occassionally fail to render anything. Original PR Author: jonahwilliams Reviewed By: {gaaclarke} This change reverts the following previous change: Adds multisampling support for GLES devices without GL_EXT_multisampled_render_to_texture provided they are at least GLES 3.0 to support mutlisampled render buffers. Fixes flutter#158360 Fixes flutter#157951 TBD: should we prefer renderbuffer 3.0 approach over multisample_render_to_texture?

[Impeller] support GLES 3.0 multisampling.

26a1b71

github-actions bot added the e: impeller label Nov 19, 2024

jonahwilliams changed the title ~~[Impeller] support GLES 3.0 multisampling.~~ [Impeller] support GLES 3.0 MSAA without extension. Nov 19, 2024

jonahwilliams commented Nov 19, 2024

View reviewed changes

jonahwilliams requested review from chinmaygarde and gaaclarke November 19, 2024 16:34

jonahwilliams commented Nov 19, 2024

View reviewed changes

flutter-dashboard bot added the will affect goldens label Nov 19, 2024

gaaclarke reviewed Nov 19, 2024

View reviewed changes

Comments and conditional.

82c0fce

gaaclarke reviewed Nov 19, 2024

View reviewed changes

jonahwilliams added 2 commits November 19, 2024 12:17

fix conditional.

9561d91

Update render_pass_gles.cc

93beb95

gaaclarke approved these changes Nov 19, 2024

View reviewed changes

Merge branch 'main' into see_if_golden_broken

757ce6a

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

auto-submit bot merged commit ec065be into flutter:main Nov 21, 2024
31 checks passed

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter Engine from d1a08064e193 to 764b91d7d75a (3 revisions) flutter/flutter#159298

Closed

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

ec065be3a [Impeller] support GLES 3.0 MSAA without extension. (flutte…

ba9299e

…r/engine#56705)

jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Nov 21, 2024

auto-submit bot pushed a commit that referenced this pull request Nov 21, 2024

Revert "[Impeller] support GLES 3.0 MSAA without extension. (#56705)"

04da399

This reverts commit ec065be.

auto-submit bot mentioned this pull request Nov 21, 2024

Reverts "[Impeller] support GLES 3.0 MSAA without extension. (#56705)" #56741

Merged

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Nov 21, 2024

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Flutter Engine from d1a08064e193 to c3482189e568 (4 revisions) flutter/flutter#159305

Closed

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

ec065be3a [Impeller] support GLES 3.0 MSAA without extension. (flutte…

3861f35

…r/engine#56705)

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

Roll Flutter Engine from d1a08064e193 to 6f941c961b05 (5 revisions) flutter/flutter#159308

Merged

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

ec065be3a [Impeller] support GLES 3.0 MSAA without extension. (flutte…

9a8db60

…r/engine#56705)

jonahwilliams mentioned this pull request Dec 8, 2024

[Impeller] Reland: support GLES 3.0 MSAA without extension. #57053

Closed

		? TextureGLES::Type::kTextureMultisampled
		: TextureGLES::Type::kRenderBufferMultisampled)

[Impeller] support GLES 3.0 MSAA without extension. #56705

[Impeller] support GLES 3.0 MSAA without extension. #56705

Uh oh!

Conversation

jonahwilliams commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Nov 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Nov 19, 2024

Uh oh!

gaaclarke 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!

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!

gaaclarke commented Nov 19, 2024

Uh oh!

gaaclarke commented Nov 19, 2024

Uh oh!

jonahwilliams commented Nov 19, 2024

Uh oh!

gaaclarke 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!

flutter-dashboard bot commented Nov 19, 2024

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Nov 19, 2024

Uh oh!

Uh oh!

jonahwilliams commented Nov 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jonahwilliams commented Nov 19, 2024 •

edited

Loading