[Impeller] Removed requirement for multisample buffers from egl setup. #40944
Conversation
|
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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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. |
gaaclarke
changed the title
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
While the offscreen MSAA stuff is guarded behind a flag till we implement checks for GL_EXT_multisampled_render_to_texture, this one deals with onscreen rendering. That is the main FBO. I believe doing this will disable MSAA on the onscreen present on devices. We don't want that. We need to check for 4 first and then fall back to 1. Can you verify the MSAA behavior in render doc or perhaps eyeball it (it may be hard to tell though).
chinmaygarde
added
the
autosubmit
chinmaygarde
removed
the
autosubmit
|
auto label is removed for flutter/engine, pr: 40944, due to This PR has not met approval requirements for merging. Changes were requested by {chinmaygarde}, please make the needed changes and resubmit this PR.
|
|
auto label is removed for flutter/engine, pr: 40944, due to - The status or check suite Windows windows_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
|
auto label is removed for flutter/engine, pr: 40944, due to - This pull request has changes requested by @chinmaygarde. Please resolve those before re-applying the label. |
|
Sorry about the autosub label. Added it by mistake. |
That sounds right. I didn't look closely since it is rendering kind of goofy. I'll tweak it this afternoon. |
impeller/toolkit/egl/display.cc
Outdated
| attributes.push_back(static_cast<EGLint>(config.stencil_bits)); | ||
| } | ||
|
|
||
| // On android emulators multisample buffer configs don't work, so we fallback |
There was a problem hiding this comment.
The stuff in //impeller/toolkit is meant to be platform agnostic. If a Samples::kFour config isn't found, this method will return nullptr to the caller. Can the caller can then try a different config? That way, this Android quirk doesn't affect other platforms.
I patched the Android stuff like so (don't know if it compiles but just a gist):
diff --git a/shell/platform/android/android_surface_gl_impeller.cc b/shell/platform/android/android_surface_gl_impeller.cc
index 3200822776..86437bb945 100644
--- a/shell/platform/android/android_surface_gl_impeller.cc
+++ b/shell/platform/android/android_surface_gl_impeller.cc
@@ -100,8 +100,17 @@ AndroidSurfaceGLImpeller::AndroidSurfaceGLImpeller(
desc.surface_type = impeller::egl::SurfaceType::kWindow;
auto onscreen_config = display->ChooseConfig(desc);
if (!onscreen_config) {
- FML_DLOG(ERROR) << "Could not choose onscreen config.";
- return;
+ // This is probably an Android simulator. Fallback to a single sample so we
+ // get something up on the screen.
+ desc.samples = impeller::egl::Samples::kOne;
+ onscreen_config = display->ChooseConfig(desc);
+ if (onscreen_config) {
+ FML_LOG(INFO) << "Warning: This device doesn't support MSAA for onscreen "
+ "framebuffers. Falling back to a single sample.";
+ } else {
+ FML_DLOG(ERROR) << "Could not choose onscreen config.";
+ return;
+ }
}
desc.surface_type = impeller::egl::SurfaceType::kPBuffer;
We actually don't want to use kOne because that would disable MSAA on the onscreen FBO. We are just doing it so something shows up on simulators. Let's do that only on Android and log a message when we do so we know something is expected to not work. Instead of silently breaking like here.
There was a problem hiding this comment.
Yea, this does the retry but I can move it up to the android layer if we want. Third time is a charm.
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
The stuff in android_surface_gl_impeller.cc lgtm. I don't think the update in display.cc is accurate though.
| if (sample_count > 1) { | ||
| attributes.push_back(EGL_SAMPLE_BUFFERS); | ||
| attributes.push_back(1); | ||
| attributes.push_back(EGL_SAMPLES); |
There was a problem hiding this comment.
We want to specify sample count to 1 explicitly right? If the caller wants 1 and we specify no opinion, the implementation is allowed to select anything it wants (I think). We will not be respecting the caller opinion for 1.
There was a problem hiding this comment.
You can't, egl fails on the emulator if you specify 1. You have to leave it undefined. I expect undefined would not give you MSAA. It isn't officially documented as such though.
There was a problem hiding this comment.
Do you want me to add an undefined option in the ConfigDescriptor to be explicit?
There was a problem hiding this comment.
egl fails on the emulator if you specify 1
Didn't know that.
Do you want me to add an undefined option in the ConfigDescriptor to be explicit?
No. This is fine.
LGTM still.
gaaclarke
added
the
autosubmit
|
blocked on flutter/flutter#124332 |
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/engine, pr: 40944, Failed to merge pr#: 40944 with Pull request could not be merged: Required status check "ci.yaml validation" is expected.. |
|
I'm following up in a different PR for unit tests since I had to refactor quite a bit to make it happen and this is already approved: #40979 |
flutter#40944) I reviewed the code and I don't think this requirement is necessary. Offscreen MSAA is already guarded by a flag and looking at the opengles code it doesn't seem to be using multisample buffers. When removing it the emulator no longer crashes and behaves identically to on device (which is rendering incorrect colors and crashing when pressing a the counter button). fixes flutter/flutter#105323 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] 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 Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
followup to #40944 issue flutter/flutter#105323 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
|
i am continously getting error of using impeller flutter run --enable-impeller Additional info: |
@RoliGautam21 you should file an issue with reproduction steps. There's nothing in that log that suggests it's related to this PR. |
3 participants
I reviewed the code and I don't think this requirement is necessary. Offscreen MSAA is already guarded by a flag and looking at the opengles code it doesn't seem to be using multisample buffers.
When removing it the emulator no longer crashes and behaves identically to on device (which is rendering incorrect colors and crashing when pressing a the counter button).
fixes flutter/flutter#105323
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.