1705349 - Meaning of supports_texture_storage variable in webrender::Device::new is unclear

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Jamie Nicol [:jnicol]]

This is a follow up from bug 1704783's review, but a regression from bug 1612941.

The variable supports_texture_storage has 2 different meanings depending on whether we are running Gl or Gles. For Gl, it indeed indicates whether glTexStorage is supported. For Gles, it indicates whether the extension GL_EXT_texture_storage is supported.

In Gles 3, glTexStorage is always supported for most formats. The relevance of whether the GL_EXT_texture_storage extension is supported is that it means glTexStorage can be used with GL_BGRA8.

(This is because the GL_EXT_texture_format_BGRA8888 extension was written against GLES 2 + GL_EXT_texture_storage, and has never been updated for GLES 3.)

This is why the code originally tested directly for the extension support when deciding whether to allow glTexStorage with GL_BGRA8, and doesn't bother checking in the other branches where we use glTexStorage with other formats. But now that we use this variable it's super confusing. In the GL branches we must check for supports_texture_storage before using texture storage, but not in the Gles branches??? but we do check it for BGRA?? :/

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: Bug 1705349 - Avoid confusing usage of supports_texture_storage variable. r?kvark

The variable supports_texture_storage during webrender device
initialization has 2 different meanings depending on whether we are
running GL or GLES. For GL, it indeed indicates whether glTexStorage
is supported. For GLES, however, it indicates whether the extension
GL_EXT_texture_storage is supported.

In GLES 3, glTexStorage is always supported for most formats. The
relevance of whether the GL_EXT_texture_storage extension is supported
is that it means glTexStorage can be used with GL_BGRA8. (This is
because the GL_EXT_texture_format_BGRA8888 extension was written
against GLES 2 + GL_EXT_texture_storage, and has never been updated
for GLES 3.)

This is why in the GL case we must check supports_texture_storage
before allowing texture storage usage, but in the GLES case we do not
bother checking, except for when using BGRA. This is confusing.

To make this less confusing, this patch checks directly for the
extension support in the GLES + BGRA case. We do not bother checking
the value of supports_texture_storage in any non-BGRA GLES cases, as
it is always true and would only further complicate logic that is
already too complex. If this is required in the future (eg to work
around a buggy glTexStorage implementation) we can always add it then.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdd3092ac613
Avoid confusing usage of supports_texture_storage variable. r=kvark

Comment 3

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/bdd3092ac613