Open Bug 1879496 Opened 2 years ago Updated 1 year ago

Firefox on GLES broken on recent Mesa releases

Categories

(Core :: Graphics: WebRender, defect)

Product:
Core
Component:
Graphics: WebRender
Type:
defect

Tracking

()

Status:
UNCONFIRMED

People

(Reporter: kusmabite, Unassigned)

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.

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

I've created a Khronos issue to discuss if the spec should be changed instead: https://github.com/KhronosGroup/OpenGL-API/issues/92

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.

Product: Firefox → WebExtensions
Component: Untriaged → Graphics: WebRender
Product: WebExtensions → Core

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.

Flags: needinfo?(kusmabite)

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.

Flags: needinfo?(kusmabite)

(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 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.

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.

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?

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.

Just to be clear:

  • EXT_texture_format_BGRA8888 adds full support for GL_BGRA as a sized internal format, but does not even mention GL_BGRA8_EXT.
  • EXT_texture_storage combined with EXT_texture_format_BGRA8888 allows GL_BGRA8_EXT, but only for the glTexStorage* APIs.

It seems to me like two mistakes has happened in the spec here:

  1. EXT_texture_format_BGRA8888 defined GL_BGRA as both a "normal" format and a sized internal format, instead of adding GL_BGRA8_EXT as a separate sized internal format.
  2. The spec-authors of EXT_texture_storage just assumed EXT_texture_format_BGRA8888 added support for GL_BGRA8_EXT, like is what's been done for most other formats.

(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.

Blocks: wr-todos

Redirect a needinfo that is pending on an inactive user to the triage owner.
:gw, since the bug doesn't have a severity set, could you please set the severity or close the bug?

For more information, please visit BugBot documentation.

Flags: needinfo?(lilislisoh000) → needinfo?(gwatson)

The EXT_texture_format_BGRA8888 spec got changed to allow what Firefox and Chrome has been assuming: https://github.com/KhronosGroup/OpenGL-Registry/pull/603

So I don't think there's anything more actionable needed here. Mesa got patched to allow it as soon as we discovered it, and the patches got back-ported to all affected releases.

Mesa got patched to allow it as soon as we discovered it, and the patches got back-ported to all affected releases.

Nice. Probably these versions are a good baseline then from which on FF should prefer GLES over GL on all architectures, not just arm64, following the rest of the ecosystem (like GTK4, Gstreamer etc.). I have something brewing for that in bug 1913548.

Flags: needinfo?(gwatson)
You need to log in before you can comment on or make changes to this bug.