[Windows] Improve texture format logic #54329

loic-sharma · 2024-08-02T23:00:54Z

This improves Flutter Window's texture format logic:

If the device has extension GL_EXT_texture_format_BGRA8888: backing stores' texture will be created with format GL_BGRA_EXT instead of GL_RGBA8. This is what fixes [Windows] Flutter does not render any GUI and shows black window instead flutter#150546.
Otherwise: backing stores' texture will be created with format GL_RGBA and the engine will use the color type kRGBA_8888_SkColorType when creating the SkSurface.

Previously the engine always used color type kN32_SkColorType, which could be either RGBA or BGRA depending on the machine that compiled the engine. This caused Skia validation errors as the texture's format did not match the Skia surface's color type.

I tested this by editing Flutter Windows to force it down this code path.

Huge kudos to @chinmaygarde for the OpenGL expertise!

Warning

Unfortunately, we are unable to test this.
This bug appears to only affect older devices; none of our devices reproduce this issue.
We also do not have the infrastructure to do a native screenshot test on Windows.
I will get a test exemption for this change.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

loic-sharma · 2024-08-02T23:04:41Z

shell/platform/embedder/embedder.cc

  auto surface = SkSurfaces::WrapBackendTexture(
      context,                      // context
      backend_texture,              // back-end texture
      kBottomLeft_GrSurfaceOrigin,  // surface origin
      1,                            // sample count
-      kN32_SkColorType,             // color type


kN32_SkColorType is either BGRA8 or RGBA8 depending on the machine that builds the engine. This caused Skia to reject RGBA8 textures if the engine was built on a machine where BGRA8 is the native color format.

Oh wow - nice catch.

loic-sharma · 2024-08-02T23:06:03Z

shell/platform/windows/compositor_opengl.cc

  gl_->BindTexture(GL_TEXTURE_2D, 0);

-  gl_->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0_EXT,


GL_COLOR_ATTACHMENT0 is the same value as GL_COLOR_ATTACHMENT0_EXT but available in OpenGL ES 2.0.

shell/platform/windows/compositor_opengl.h

flutter-dashboard · 2024-08-02T23:20:55Z

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

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.

chinmaygarde · 2024-08-02T23:26:13Z

Since testing this is so tenuous, have you verified the fallback works locally? Just by assuming the extension is unavailable?

loic-sharma · 2024-08-02T23:32:21Z

Since testing this is so tenuous, have you verified the fallback works locally? Just by assuming the extension is unavailable?

Yup that's right, I edited the code to act as if my machine doesn't have GL_EXT_texture_format_BGRA8888. Previously this failed due to Skia's validation, with this patch the app now works as expected.

cbracken

stuartmorgan-g · 2024-08-08T17:05:13Z

test-exempt: requires hardware that's not available in CI

chinmaygarde · 2024-08-08T20:10:48Z

This looks good to go but we don't have a confirmation from a customer that this patch works on their devices. There does seem to be a conflict though so it would be good to resolve that so we can land this as soon as we hear back.

krll-kov · 2024-08-09T11:13:17Z

This looks good to go but we don't have a confirmation from a customer that this patch works on their devices. There does seem to be a conflict though so it would be good to resolve that so we can land this as soon as we hear back.

But we've already tested example app in the issue flutter/flutter#150546

loic-sharma · 2024-08-09T16:31:19Z

@chinmaygarde Updated! FYI, I didn't remove the kN32_SkColorType on the EGL surface code path. This would require adding a new format parameter to its config, see this discussion: https://github.com/flutter/engine/pull/43683/files#r1263989268

@Kirill-21 There's a very large customer that can reproduce this issue. We'd like them to test this change to verify this doesn't have unexpected regressions. I recognize this delay is frustrating, we're trying to land this as fast as possible!

loic-sharma · 2024-08-12T17:51:13Z

Unfortunately, our partner hasn't verified this patch after 10 days. Several community members confirmed this fixed what appears to be the same issue. I will assume this fixes our partner's issue as well.

…153313) flutter/engine@bcf2dcc...5d8ee52 2024-08-12 matej.knopp@gmail.com macOS: Fix crash in attributedSubstringForProposedRange with out of bounds range (flutter/engine#54469) 2024-08-12 737941+loic-sharma@users.noreply.github.com [Windows] Improve texture format logic (flutter/engine#54329) 2024-08-12 bdero@google.com Revert "[Impeller] remove scene3d support." (flutter/engine#54502) 2024-08-12 skia-flutter-autoroll@skia.org Roll Skia from f77adcef7c1c to ec7558d41b34 (1 revision) (flutter/engine#54507) 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 jsimmons@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

flutteractionsbot · 2024-08-12T22:17:46Z

Failed to create CP due to merge conflicts.
You will need to create the PR manually. See the cherrypick wiki for more info.

@chinmaygarde

This improves Flutter Window's texture format logic: 1. **If the device has extension `GL_EXT_texture_format_BGRA8888`**: backing stores' texture will be created with format `GL_BGRA_EXT` instead of `GL_RGBA8`. This is what fixes flutter/flutter#150546. 2. **Otherwise**: backing stores' texture will be created with format `GL_RGBA` and the engine will use the color type `kRGBA_8888_SkColorType` when creating the `SkSurface`. Previously the engine always used color type `kN32_SkColorType`, which could be either RGBA or BGRA depending on the machine that compiled the engine. This caused Skia validation errors as the texture's format did not match the Skia surface's color type. I tested this by editing Flutter Windows to force it down this code path. Huge kudos to @chinmaygarde for the OpenGL expertise! Fixes: flutter/flutter#150546 > [!WARNING] > Unfortunately, we are unable to test this. > This bug appears to only affect older devices; none of our devices reproduce this issue. > We also do not have the infrastructure to do a native screenshot test on Windows. > I will get a test exemption for this change. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

flutter-dashboard · 2024-08-13T16:07:10Z

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.

…lutter#153313) flutter/engine@bcf2dcc...5d8ee52 2024-08-12 matej.knopp@gmail.com macOS: Fix crash in attributedSubstringForProposedRange with out of bounds range (flutter/engine#54469) 2024-08-12 737941+loic-sharma@users.noreply.github.com [Windows] Improve texture format logic (flutter/engine#54329) 2024-08-12 bdero@google.com Revert "[Impeller] remove scene3d support." (flutter/engine#54502) 2024-08-12 skia-flutter-autoroll@skia.org Roll Skia from f77adcef7c1c to ec7558d41b34 (1 revision) (flutter/engine#54507) 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 jsimmons@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

JemmyWang94 · 2024-08-19T09:39:45Z

May I ask if there are any new developments.

…lutter#153313) flutter/engine@bcf2dcc...5d8ee52 2024-08-12 matej.knopp@gmail.com macOS: Fix crash in attributedSubstringForProposedRange with out of bounds range (flutter/engine#54469) 2024-08-12 737941+loic-sharma@users.noreply.github.com [Windows] Improve texture format logic (flutter/engine#54329) 2024-08-12 bdero@google.com Revert "[Impeller] remove scene3d support." (flutter/engine#54502) 2024-08-12 skia-flutter-autoroll@skia.org Roll Skia from f77adcef7c1c to ec7558d41b34 (1 revision) (flutter/engine#54507) 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 jsimmons@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

github-actions bot added affects: desktop platform-windows labels Aug 2, 2024

loic-sharma mentioned this pull request Aug 2, 2024

[Windows] Use GL_RGBA as backing store internal format #54255

Closed

8 tasks

loic-sharma commented Aug 2, 2024

View reviewed changes

chinmaygarde approved these changes Aug 2, 2024

View reviewed changes

loic-sharma requested review from cbracken and yaakovschectman August 2, 2024 23:12

chinmaygarde reviewed Aug 2, 2024

View reviewed changes

shell/platform/windows/compositor_opengl.h Outdated Show resolved Hide resolved

loic-sharma force-pushed the issue_150546_bgra_format branch 3 times, most recently from b8df3c8 to 1b0afaa Compare August 2, 2024 23:20

loic-sharma marked this pull request as ready for review August 2, 2024 23:20

cbracken approved these changes Aug 2, 2024

View reviewed changes

yaakovschectman approved these changes Aug 5, 2024

View reviewed changes

loic-sharma force-pushed the issue_150546_bgra_format branch from 1b0afaa to 5e60ca5 Compare August 9, 2024 16:28

[Windows] Improve texture format logic

5e60ca5

chinmaygarde approved these changes Aug 9, 2024

View reviewed changes

loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 12, 2024

auto-submit bot merged commit 4887a4d into flutter:main Aug 12, 2024
29 checks passed

loic-sharma mentioned this pull request Aug 12, 2024

Add format field to EGL surface backing store #54499

Merged

8 tasks

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

Roll Flutter Engine from bcf2dcc09a13 to 4887a4df9067 (3 revisions) flutter/flutter#153303

Closed

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

4887a4df9 [Windows] Improve texture format logic (flutter/engine#54329)

2b24883

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

Roll Flutter Engine from bcf2dcc09a13 to 5d8ee52e985b (4 revisions) flutter/flutter#153313

Merged

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

4887a4df9 [Windows] Improve texture format logic (flutter/engine#54329)

21c1609

loic-sharma added the cp: stable cherry pick to the stable release candidate branch label Aug 12, 2024

loic-sharma mentioned this pull request Aug 12, 2024

[CP] [Windows] Fix black screen bug on older devices flutter/flutter#153321

Closed

loic-sharma mentioned this pull request Sep 13, 2024

Match Windows logic for picking RGB/BGR textures. #55121

Merged

		gl_->BindTexture(GL_TEXTURE_2D, 0);

		gl_->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0_EXT,

[Windows] Improve texture format logic #54329

[Windows] Improve texture format logic #54329

Uh oh!

Conversation

loic-sharma commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

loic-sharma Aug 2, 2024 • 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!

cbracken Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

loic-sharma Aug 2, 2024 • 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!

Uh oh!

flutter-dashboard bot commented Aug 2, 2024

Uh oh!

chinmaygarde commented Aug 2, 2024

Uh oh!

loic-sharma commented Aug 2, 2024

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Aug 8, 2024

Uh oh!

chinmaygarde commented Aug 8, 2024

Uh oh!

krll-kov commented Aug 9, 2024

Uh oh!

loic-sharma commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loic-sharma commented Aug 12, 2024

Uh oh!

Uh oh!

flutteractionsbot commented Aug 12, 2024

Uh oh!

flutter-dashboard bot commented Aug 13, 2024

Uh oh!

JemmyWang94 commented Aug 19, 2024

Uh oh!

Uh oh!

loic-sharma commented Aug 2, 2024 •

edited

Loading

loic-sharma Aug 2, 2024 •

edited

Loading

loic-sharma Aug 2, 2024 •

edited

Loading

loic-sharma commented Aug 9, 2024 •

edited

Loading