Firefox on GLES broken on recent Mesa releases
Categories
(Core :: Graphics: WebRender, defect)
Tracking
()
People
(Reporter: kusmabite, Unassigned, NeedInfo)
References
(Blocks 2 open bugs)
Details
Steps to reproduce:
Running Firefox on GLES (either on a system without desktop GL, or with the property gfx.egl.prefer-gles.enabled set to true) on Wayland with recent Mesa (non-released).
This regressed after Mesa added support for the GL_EXT_texture_storage extension, which Firefox makes some incorrect assumptions, leading to the problem.
Actual results:
Corrupted rendering.
The reason is that Firefox assumes the combination of GL_EXT_texture_format_BGRA8888 and GL_EXT_texture_storage and allows doing things like glTexImage2D() with an internalformat of GL_BGRA8_EXT, which is not something the respective specifications allow.
See https://github.com/mozilla/gecko-dev/blob/41edcdf7fe44678c5913a603a286b1fc3979d540/gfx/wr/webrender/src/device/gl.rs#L1513 for a comment describing the assumption.
The GL_EXT_texture_format_BGRA8888 extension does not even mention GL_BGRA8_EXT, only GL_BGRA_EXT. Those are not the same enum, even if they seem similar. See https://registry.khronos.org/OpenGL/extensions/EXT/EXT_texture_format_BGRA8888.txt for details.
GL_EXT_texture_storage allows using GL_BGRA8_EXT, but only as an internalformat for the glTexStorage*()-functions. See https://registry.khronos.org/OpenGL/extensions/EXT/EXT_texture_storage.txt for details.
In addition, it's kinda undefined if things like glGenerateMipmaps() would work on textures with internalformat GL_BGRA8_EXT without the APPLE_texture_format_BGRA8888 extension. The reason is that, while GL_EXT_texture_storage allows using GL_BGRA8_EXT when GL_EXT_texture_format_BGRA8888 is supported, neither of those extensions actually defines GL_BGRA8_EXT as color-renderable.
Expected results:
Firefox should not have tried to use GL_BGRA8_EXT for anything else than glTexStorage*(), which is where it's currently supported.
Reporter | ||
Comment 1•4 months ago
|
||
Some references:
The upstream Mesa issue report: https://gitlab.freedesktop.org/mesa/mesa/-/issues/10550
Corresponding Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1524322
Reporter | ||
Comment 2•4 months ago
|
||
I've created a Khronos issue to discuss if the spec should be changed instead: https://github.com/KhronosGroup/OpenGL-API/issues/92
Comment 3•4 months ago
|
||
The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.
Updated•4 months ago
|
Comment hidden (spam) |
Updated•3 months ago
|
Comment 5•2 months ago
|
||
Thanks for the bug report!
(In reply to Erik Faye-Lund from comment #0)
The reason is that Firefox assumes the combination of GL_EXT_texture_format_BGRA8888 and GL_EXT_texture_storage and allows doing things like glTexImage2D() with an internalformat of GL_BGRA8_EXT, which is not something the respective specifications allow.
See https://github.com/mozilla/gecko-dev/blob/41edcdf7fe44678c5913a603a286b1fc3979d540/gfx/wr/webrender/src/device/gl.rs#L1513 for a comment describing the assumption.
The GL_EXT_texture_format_BGRA8888 extension does not even mention GL_BGRA8_EXT, only GL_BGRA_EXT. Those are not the same enum, even if they seem similar. See https://registry.khronos.org/OpenGL/extensions/EXT/EXT_texture_format_BGRA8888.txt for details.
GL_EXT_texture_storage allows using GL_BGRA8_EXT, but only as an internalformat for the glTexStorage*()-functions. See https://registry.khronos.org/OpenGL/extensions/EXT/EXT_texture_storage.txt for details.
Perhaps the comment could be clearer but, unless I'm misunderstanding you, it agrees with what you just said. Only GL_EXT_texture_format_BGRA8888
means GL_BGRA8_EXT
is not a valid internalformat for either glTexImage*
or glTexStorage*
. Both GL_EXT_texture_format_BGRA8888
and GL_EXT_texture_storage
allows using GL_BGRA8_EXT
as the internalformat for glTexStorage*
. It would be clearer if we added "unless GL_EXT_texture_storage is also available in which case it is a valid internal format for glTexStorage".
Firefox should not have tried to use GL_BGRA8_EXT for anything else than glTexStorage*(), which is where it's currently supported.
This is the intention. There may be a bug somewhere, but I did just try to run on mesa but pretending that GL_EXT_texture_storage is supported, and I didn't see it happen. I see us calling glTexStorage2D with an internal format of GL_BGRA8_EXT
, and glTexSubImage2D with a format of GL_BGRA_EXT
. Can you get a backtrace?
In addition, it's kinda undefined if things like glGenerateMipmaps() would work on textures with internalformat GL_BGRA8_EXT without the APPLE_texture_format_BGRA8888 extension. The reason is that, while GL_EXT_texture_storage allows using GL_BGRA8_EXT when GL_EXT_texture_format_BGRA8888 is supported, neither of those extensions actually defines GL_BGRA8_EXT as color-renderable.
How does mesa currently handle this? We can avoid BGRA8 in cases where we may need mipmaps if required.
Reporter | ||
Comment 6•2 months ago
|
||
Calling glTexSubImage2D with a format of GL_BGRA_EXT is exactly what causes the issue. I explained this in a bit more detail in the Khronos issue here https://github.com/KhronosGroup/OpenGL-API/issues/92#issuecomment-1954458673
The problem is that the spec defines glTexSubImage2D
to have the same format/type error conditions as calling glTexImage2D
with the internal format previously specified. Here's the exact wording from the spec:
The same constraints and errors apply to the TexSubImage commands’ argument format and
the internalformat of the texture image being respecified as apply to the format
and internalformat arguments of its TexImage counterparts.
By my reading, this means the current version of GL_EXT_texture_format_BGRA8888
only really allows GL_BGRA
to be rendered to, not uploaded to directly. And GL_BGRA8_EXT
is not usable at all with just GL_EXT_texture_format_BGRA8888
.
How does mesa currently handle this? We can avoid BGRA8 in cases where we may need mipmaps if required.
Current versions allows using GL_BGRA8_EXT
for all glTex*Image*D
, but prints a warning to stdout about out-of-spec behavior on first use. We do not currently handle things like glGenerateMipmaps()
. But since the spec is in the progress of changing, we will implement the new and correct behavior of allowing both GL_BGRA8_EXT
and GL_BGRA
for both unconditionally. See https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27726 for the upcoming change.
Comment 7•2 months ago
|
||
(In reply to Erik Faye-Lund from comment #6)
Calling glTexSubImage2D with a format of GL_BGRA_EXT is exactly what causes the issue. I explained this in a bit more detail in the Khronos issue here https://github.com/KhronosGroup/OpenGL-API/issues/92#issuecomment-1954458673
Sorry that wasn't clear from your original comment but I understand your concern now, thank you for clarifying!
The problem is that the spec defines
glTexSubImage2D
to have the same format/type error conditions as callingglTexImage2D
with the internal format previously specified. Here's the exact wording from the spec:The same constraints and errors apply to the TexSubImage commands’ argument format and
the internalformat of the texture image being respecified as apply to the format
and internalformat arguments of its TexImage counterparts.
I believe this is covered in EXT_texture_storage:
OpenGL ES does not accept sized internal formats (e.g., RGBA8) and
instead derives an internal format from the <format> and <type>
parameters of TexImage2D. Since TexStorage* does not specify texel
data, the API doesn't include <format> and <type> parameters.
On an OpenGL ES implementation, the values in the <internalformat>
column in the tables below are accepted as <internalformat>
parameters, and base internal formats are not accepted. The
TexImage* calls in the TexStorage* pseudocode are modified so that
the <internalformat>, <format> and <type> parameters are
taken from the <format>, <format> and <type> columns (respectively)
in the tables below, according to the <internalformat>
specified in the TexStorage* command.
If EXT_texture_format_BGRA8888 or APPLE_texture_format_BGRA8888 is supported:
<internalformat> <format> <type>
---------------- -------- ------
BGRA8_EXT BGRA_EXT UNSIGNED_BYTE
Calling glTexStorage*
with an internalformat of BGRA8_EXT
is equivalent to calling glTexImage*
with an internalformat of BGRA_EXT
. Calling glTexSubImage*
with a format of BGRA_EXT
therefore satisfies the constraints.
Comment 8•2 months ago
•
|
||
By my reading, this means the current version of GL_EXT_texture_format_BGRA8888 only really allows GL_BGRA to be rendered to, not uploaded to directly
Unless, are you proposing that even using glTexImage*
with an internalformat of BGRA_EXT
then glTexSubImage*
with format BGRA_EXT
is not allowed either?
Reporter | ||
Comment 9•2 months ago
|
||
I believe this is covered in EXT_texture_storage:
Unfortunately, that's not the case. What you're quoting is about modifying the TexImage* calls in the pseudocode for the TexStorage* functions
But the spec also kinda spells it out:
Accepted by the <internalformat> parameter of TexStorage* when
implemented on OpenGL ES:[...]
(if EXT_texture_format_BGRA8888 or APPLE_texture_format_BGRA8888 is supported)
BGRA8_EXT 0x93A1
...as you can see, this is only added as allowed for TexStorage* by this spec. There's no wording adding this capability to the TexImage* APIs themselves there.
Unless, are you proposing that even using glTexImage* with an internalformat of BGRA_EXT then glTexSubImage* with format BGRA_EXT is not allowed either?
My bad, I got my wires crossed a bit there. I meant GL_BGRA8_EXT
, not GL_BGRA
.
Reporter | ||
Comment 10•2 months ago
|
||
Just to be clear:
EXT_texture_format_BGRA8888
adds full support forGL_BGRA
as a sized internal format, but does not even mentionGL_BGRA8_EXT
.EXT_texture_storage
combined withEXT_texture_format_BGRA8888
allowsGL_BGRA8_EXT
, but only for theglTexStorage*
APIs.
It seems to me like two mistakes has happened in the spec here:
EXT_texture_format_BGRA8888
definedGL_BGRA
as both a "normal" format and a sized internal format, instead of addingGL_BGRA8_EXT
as a separate sized internal format.- The spec-authors of
EXT_texture_storage
just assumedEXT_texture_format_BGRA8888
added support forGL_BGRA8_EXT
, like is what's been done for most other formats.
Comment 11•2 months ago
|
||
(In reply to Erik Faye-Lund from comment #9)
I believe this is covered in EXT_texture_storage:
Unfortunately, that's not the case. What you're quoting is about modifying the TexImage* calls in the pseudocode for the TexStorage* functions
And the pseudocode is the extension's description for the effect the commands have on the texture's format? I don't think firefox or chrome's interpretation is unreasonable, but thank you for your work updating the specs to clarify.
Do we expect the next stable mesa release to allow this, then? If so, as we've not found another implementation which doesn't allow this, then I don't think there's anything to do on our end? If mesa doesn't ship with this change then we can block EXT_texture_storage
on our end.
Description
•