Don't use BGRA8 unless GL_EXT_texture_storage is available

[jamienicol]

https://bugzilla.mozilla.org/show_bug.cgi?id=1499785

We recently changed to use the glTexStorage* family of functions,
along with sized internal format types, such as BGRA8. Unfortunately,
the GL_EXT_texture_format_BGRA8888 extension does not provide BGRA8 as
a valid format unless the GL_EXT_texture_storage extension is also
available, which is not usually the case on GLES3 as glTexStorage*
are built in.

Unfortunately this means we must fall back to using glTexImage* rather
than glTexStorage* when dealing with BGRA data. Longer term it will
make sense to ensure the provided data is in RGBA format, as the
benifits of immutable storage are desirable.

---

This change is 

[nox]

Why is this here? You can just use Option<TexStorageSupport> elsewhere.

[jamienicol]

I can't claim to be an expert on idiomatic rust, but my logic is that the level of support for glTexStorage is a tri-state: No support, support for non-bgra, and full support. In terms of how we use the variable, there is nothing special about the None case, nor in common about the two would-be Some cases. So using an Option feels less conceptually correct, and would require messier matching code, in my opinion.

[kvark]

Listed my concerns below

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nox and @jamienicol)

---

webrender/src/device/gl.rs, line 744 at r1 (raw file):

Previously, jamienicol (Jamie Nicol) wrote…

I can't claim to be an expert on idiomatic rust, but my logic is that the level of support for glTexStorage is a tri-state: No support, support for non-bgra, and full support. In terms of how we use the variable, there is nothing special about the None case, nor in common about the two would-be Some cases. So using an Option feels less conceptually correct, and would require messier matching code, in my opinion.

I don't mind the enum here. It's just that None as a variant of anything but Option can be confusing at times, but I think it's fine here.

---

webrender/src/device/gl.rs, line 911 at r1 (raw file):

        // [2] https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_texture_format_BGRA8888.txt
        // [3] https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_texture_storage.txt
        let supports_bgra = supports_extension(&extensions, "GL_EXT_texture_format_BGRA8888");

I think this should be removed now? It collides with your new config enum

---

webrender/src/device/gl.rs, line 915 at r1 (raw file):

            gl::GlType::Gl =>
                if supports_extension(&extensions, "GL_ARB_texture_storage") {
                    TexStorageSupport::NonBGRA8

shouldn't this be Full?

---

webrender/src/device/gl.rs, line 924 at r1 (raw file):

                    TexStorageSupport::Full
                } else {
                    TexStorageSupport::NonBGRA8

shouldn't we only have that when GL_EXT_texture_storage is supported?

---

webrender/src/device/gl.rs, line 1407 at r1 (raw file):

        // unnecessary mipmap storage and generally improves performance with
        // stronger invariants.
        let use_texture_storage = match self.texture_storage_support {

great!

[jamienicol]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nox and @jamienicol)

---

webrender/src/device/gl.rs, line 744 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I don't mind the enum here. It's just that None as a variant of anything but Option can be confusing at times, but I think it's fine here.

Ah okay. Would "Never" or "NotSupported" be a better name then?

---

webrender/src/device/gl.rs, line 911 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think this should be removed now? It collides with your new config enum

We still need this to decide whether to use RGBA8 as the internal format (swizzling during upload). I'll try again to see if I can come up with some more simple logic for all of this. But unfortunately the conditions aren't that simple.

---

webrender/src/device/gl.rs, line 915 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

shouldn't this be Full?

Hmm, yes it should be. In this case BGRA isn't supported as an internal format, so we will use RGBA instead. Which means we can in fact use glTexStorage.

---

webrender/src/device/gl.rs, line 924 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

shouldn't we only have that when GL_EXT_texture_storage is supported?

GLES 3 always supports glTexStorage for standard formats. However, it only supports glTexStorage with BGRA8 when GL_EXT_texture_storage && GL_EXT_texture_format_BGRA8888

[kvark]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nox, @kvark, and @jamienicol)

---

webrender/src/device/gl.rs, line 744 at r1 (raw file):

Previously, jamienicol (Jamie Nicol) wrote…

Ah okay. Would "Never" or "NotSupported" be a better name then?

I don't think we should change None to anything here. Does @nox have a preference?

---

webrender/src/device/gl.rs, line 924 at r1 (raw file):

Previously, jamienicol (Jamie Nicol) wrote…

GLES 3 always supports glTexStorage for standard formats. However, it only supports glTexStorage with BGRA8 when GL_EXT_texture_storage && GL_EXT_texture_format_BGRA8888

if it always supports glTexStorage, why do we check for GL_EXT_texture_storage ?

[jamienicol]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nox, @kvark, and @jamienicol)

---

webrender/src/device/gl.rs, line 924 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if it always supports glTexStorage, why do we check for GL_EXT_texture_storage ?

Because without GL_EXT_texture_storage, we have glTexStorage but it doesn't work with BGRA. Only with GL_EXT_texture_storage (and GL_EXT_texture_format_BGRA8888) do we have a glTexStorage that does work with BGRA.

[jamienicol]

Okay, the latest version restructures the logic and adds some more comments so hopefully it's a bit more obvious. And I fixed it so we do indeed use TexStorage when BGRA is not supported (ie the internal format is RGBA).

I also renamed TexStorageSupport to TexStorageUsage, because I think it makes more sense for that last case: TexStorage is still not technically supported for BGRA internal formats, but we are using it for our BGRA textures (because they have RGBA internal formats). Then I could happily rename "None" to "Never" to avoid that issue.

[kvark]

Got one additional concern

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nox, @kvark, and @jamienicol)

---

webrender/src/device/gl.rs, line 944 at r2 (raw file):

            // Since the internal format will actually be RGBA, if texture
            // storage is supported we can use it for such textures.
            assert_ne!(gl.get_type(), gl::GlType::Gles, "gles must have compatible internal and external formats");

this assertion doesn't appear correct. if we don't support BGRA we can still be running on GLES

[jamienicol]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nox, @kvark, and @jamienicol)

---

webrender/src/device/gl.rs, line 944 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

this assertion doesn't appear correct. if we don't support BGRA we can still be running on GLES

Yes we can, but texture uploads won't work because GLES cannot convert between formats during upload. Practically speaking, it seems like pretty much every android device does support BGRA, except the emulator. Again, the real solution is to use RGBA. I can remove the assertion in the mean time if you want though.

[kvark]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nox and @jamienicol)

---

webrender/src/device/gl.rs, line 944 at r2 (raw file):

Previously, jamienicol (Jamie Nicol) wrote…

Yes we can, but texture uploads won't work because GLES cannot convert between formats during upload. Practically speaking, it seems like pretty much every android device does support BGRA, except the emulator. Again, the real solution is to use RGBA. I can remove the assertion in the mean time if you want though.

I'm just confused by the fact that we are in supports_bgra if/else arms, while in fact we expect those to be GL/GLES arms.

[kvark]

@jamienicol could you launch a try?

[jamienicol]

supports_bgra is the condition which affects our decision. The assertions just make sure we're not in a situation we can't handle. It just so happens that they correlate (because until we support RGBA, we can't properly handle !supports_bgra on gles) but logically bgra support is what we're testing for.

Try run looks good: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=209780494&revision=019bb574894c88bb74c22e17c13b3b2de231d05f

[kvark]

Thanks!
@bors-servo r+

[bors-servo]

📌 Commit 70d8885 has been approved by kvark

[bors-servo]

⌛ Testing commit 70d8885 with merge 236adf7...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 236adf7 to master...