android: fix broken rendering in emulators #31727
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Android's OpenGL emulation layer (goldfish-opengl) has pre-processing
logic that looks for samplers of the type `samplerExternalOES` and
does a simple textual [replacement to change the type][1] to `sampler2D`
before compilation. It also [marks the sampler][2] as 'replaced' so
it can emulate the correct type at runtime.
However, this logic can lead to false positives when the sampler is
declared inside conditional macros. Hence, the sampler's type can be
incorrectly marked as `samplerExternalOES` even though the #if, #ifdef
conditional logic would have declared the type as `sampler2D`.
This seems to be a [known limitation][3].
WebRender (in particular the shared.glsl include) has such conditional
declaration of the texture units used from the shaders.
In particular, the sampler [sColor0 here][4] is declared within ifdefs
to have different types depending on the flags enabled, to allow the
shader to work with different image target kinds.
WebRender also maintain two versions of the compiled shaders in its cache:
1. An unoptimized version with all the conditional logic preserved
in the source until the shader is compiled at runtime on Android,
when the shader is actually used.
2. Multiple optimized versions for combinations of features required
These versions are produced during Servo [build][5]. Thus the
optimized versions eliminate most of the conditional declarations
at build time.
The bug in Servo with current code is because, [by default][6], WebRender
uses the *unoptimized* versions of the shaders. This means the conditional
GLSL source is evaluated at runtime by the Android emulator and thus ends
up with the incorrect type for the sColor0 sampler unit, which breaks the
[texture sampling in the fragment shader][7], causing it to always return
Vec4(0, 0, 0, 1) and rendering all elements on the page black.
This change forces WebRender to use the *optimized* version as a
workaround - the optimized versions have unconditional code for the
sampler declarations so are not susceptible to the emulator issue.
[1]: https://android.googlesource.com/device/generic/goldfish-opengl/+/refs/tags/android-platform-11.0.0_r40/system/GLESv2_enc/GL2Encoder.cpp#1644
[2]: https://android.googlesource.com/device/generic/goldfish-opengl/+/refs/tags/android-platform-11.0.0_r40/system/GLESv2_enc/GL2Encoder.cpp#1673
[3]: https://android.googlesource.com/device/generic/goldfish-opengl/+/refs/tags/android-platform-11.0.0_r40/system/GLESv2_enc/GL2Encoder.cpp#1571
[4]: https://github.com/servo/webrender/blob/b36399019cadcadeeed53759c96870577d4da136/webrender/res/shared.glsl#L206
[5]: https://github.com/servo/webrender/blob/b36399019cadcadeeed53759c96870577d4da136/webrender/build.rs#L289
[6]: https://github.com/servo/webrender/blob/b36399019cadcadeeed53759c96870577d4da136/webrender/src/renderer/init.rs#L214
[7]: https://github.com/servo/webrender/blob/b36399019cadcadeeed53759c96870577d4da136/webrender/res/composite.glsl#L189
Fixes servo#31726.
Signed-off-by: Mukilan Thiyagarajan <mukilan@igalia.com>
mukilan
commented
Mar 18, 2024
| // on Android emulators with unoptimized shaders. This is due to a known | ||
| // issue in the emulator's OpenGL emulation layer. | ||
| // See: https://github.com/servo/servo/issues/31726 | ||
| use_optimized_shaders: true, |
There was a problem hiding this comment.
I've manually tested the NixOS and Android builds but I'm no sure if this change could potentially break other platforms. WebRender does have fallback logic to use unoptimized versions if the optimized version is not present in the build. However, let me know if we should instead enable this only for android i.e use_optimized_shaders: cfg!(target_os = "android") instead.
|
🔨 Triggering try run (#8323421004) for Linux WPT |
|
Wow! Excellent detective work. It seems likely that we'd prefer to use something called "optimized shaders" for production and release builds. Have you been able to report this problem upstream in WebRender? |
|
Test results for linux-wpt-layout-2020 from try job (#8323421004): Flaky unexpected result (12)
Stable unexpected results that are known to be intermittent (15)
|
|
✨ Try run (#8323421004) succeeded. |
mrobinson
approved these changes
Mar 18, 2024
|
Thanks for the review, @mrobinson!
Not yet. I will create an issue in https://bugzilla.mozilla.org |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Looks like a network failure in 2013 test and a single test in 2020 failed: Since the try job passed, I'll add the PR to the queue again. |
Do you mind opening an I-Intermittent issue for this failure? |
|
What a wild bug. Thanks for the explanation! |
|
JFYI, I've reported the issue upstream with steps to reproduce using wrench |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Android's OpenGL emulation layer (goldfish-opengl) has pre-processing logic that looks for samplers of the type
samplerExternalOESand does a simple textual replacement to change the type tosampler2Dbefore compilation. It also marks the sampler as 'replaced' so it can emulate the correct type at runtime.However, this logic can lead to false positives when the sampler is declared inside conditional macros. Hence, the sampler's type can be incorrectly marked as
samplerExternalOESeven though the #if, #ifdef conditional logic would have declared the type assampler2D. This seems to be a known limitation.WebRender (in particular the shared.glsl include) has such conditional declaration of the texture units used from the shaders. In particular, the sampler sColor0 here is declared within ifdefs to have different types depending on the flags enabled, to allow the shader to work with different image target kinds.
WebRender also maintain two versions of the compiled shaders in its cache:
The bug in Servo with current code is because, by default, WebRender uses the unoptimized versions of the shaders. This means the conditional GLSL source is evaluated at runtime by the Android emulator and thus ends up with the incorrect type for the sColor0 sampler unit, which breaks the texture sampling in the fragment shader, causing it to always return Vec4(0, 0, 0, 1) and rendering all elements on the page black.
This change forces WebRender to use the optimized version as a workaround - the optimized versions have unconditional code for the sampler declarations so are not susceptible to the emulator issue.
Fixes #31726.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors