1499785 - [android] glyphs and images are black rectangles

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

On GeckoView with WR enabled (via extra patches) the glyphs and images are black rectangles. Additionally on my Xperia XZ1 I see flickering black stuff on top of the glyphs as I scroll, although I wasn't seeing that on my Z3C but I didn't try all the builds there. This bug is about the glyphs and images being black specifically, I'll file another bug about the flickering which seems like an independent issue.

Here are bunch of builds based on android nightly m-c revisions:
https://treeherder.mozilla.org/#/jobs?repo=try&author=kgupta%40mozilla.com&group_state=expanded&fromchange=d5d68cf6e33f568ee806bd1a868188d8587ced57&tochange=53892fbb31ba1fff82fb70a62b0725dd6ee30ca8

The blackness started between Oct 13 and Oct 14, which gives me this regression range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=94a62c1aad52&tochange=d49587f5ccd3

Note a whole lot there, so I'll bisect more.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Confirmed the Oct 13 - 14 regression range on my Z3C as well.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=9876ea4f4e6f3c4a8cac28ac728e3be06babd702 is good
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=6d915b4bef1b97fb7d5a3783d16bb3402cf5afcf is bad

So it's a regression from the WR update in bug 1498650, which is basically servo/webrender#3195.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Screenshot

This is what the "bad" case looks like (from Z3C)

Comment 4

[Bobby Holley (:bholley)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Created attachment 9017991 [details]
> Screenshot
> 
> This is what the "bad" case looks like (from Z3C)

Yeah, this presumably broke with the switch to immutable storage in bug 1496168.

I saw something similar when I had a bug in that stack that basically prevented the textures from being allocated - we were using GL_RGBA instead of GL_RGBA8 as the internal format (unsized instead of sized), which glTexImage* allows but glTexStorage* forbids.

So my guess is that the glTexStorage calls aren't working for whatever reason, and therefore textures aren't being allocated properly. Are you able to dump driver messages to see if an error was logged?

Comment 5

[Bobby Holley (:bholley)]

It's also possible we may have regressed [1] somehow. In general, the only GLES context we have any testing for is windows/angle, so it's possible that some of the extension availability is different and we're hitting an untested configuration.

[1] https://github.com/servo/webrender/pull/2731

Comment 6

[Jamie Nicol [:jnicol] (on leave until 21st October)]

I've been playing around with different combinations of internal and external formats, for both TexStorage and TexImage. And it seems like glTexImage2D() with the unsized GL_BGRA is the only one which works correctly.

Reading the relevant extension specs [1][2] seems to indicate that GL_EXT_texture_format_BGRA8888 adds support for GL_BGRA8_EXT to glTexStorage2DExt from the EXT_texture_storage extension, but not the functions built in to GLES 3. When investigating this thread I came across a Chromium bug report [3] which seems to have arrived at the same conclusion, although it is quite old. So it looks like we might need to revert to glTexImage2D on android.

[1] https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_texture_format_BGRA8888.txt
[2] https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_texture_storage.txt
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=407034

Comment 7

[Bobby Holley (:bholley)]

Is the issue specifically around BGRA+glTexStorage, or around glTexStorage in general? Losing glTexStorage on Android would suck.

Jeff, any thoughts here?

Comment 8

[Jamie Nicol [:jnicol] (on leave until 21st October)]

Just BGRA + TexStorage as far as I can tell, so yes we'd only have to revert to TexImage for BGRA textures. I would like to be wrong about this though, only know what I've googled and tested today.

Comment 9

[Kelsey Gilbert [:jgilbert]]

TexStorage+RGBA8+GL_TEXTURE_SWIZZLE_R?
If we always upload BGRA into RGBA, we can just swizzle sampling GL_TEXTURE_SWIZZLE_R<->B.

BGRA isn't supported by vanilla ES, and we should prioritize complying with vanilla ES to reduce our complexity, and reduce likelyhood of driver issues.

Honestly it would simplify things if we were predominantly RGBA instead of BGRA. I don't think the upload-time hit on some platforms (but not others) of using RGBA instead of BGRA is worth this trouble. 

How intractable is just using RGBA?

Comment 10

[Bobby Holley (:bholley)]

NI Andrew, since IIUC most of our BGRA comes from ImageLib.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

It should not be hard to make the decoders configurable to spit out RBGA/RGBX instead of BGRA/BGRX. I can do that. There are probably lot of places outside of imagelib to audit that would need to be able to handle RBGA/RGBX (including WebRender itself).

Comment 12

[Dzmitry Malyshau [:kvark]]

> Honestly it would simplify things if we were predominantly RGBA instead of
> BGRA. I don't think the upload-time hit on some platforms (but not others)
> of using RGBA instead of BGRA is worth this trouble. 

Indeed, it would be convenient for us. And it wouldn't matter for any post-GL APIs out there, anyway, so it's a tempting approach. I am concerned though about a driver performance hit we'd be seeing on Apple:

> The best format and data type combinations to use for texture data are:
>    GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV
>    GL_BGRA, GL_UNSIGNED_SHORT_1_5_5_5_REV)
>    GL_YCBCR_422_APPLE, GL_UNSIGNED_SHORT_8_8_REV_APPLE
> The combination GL_RGBA and GL_UNSIGNED_BYTE needs to be swizzled by many cards when the data is loaded, so it's not recommended.

Given that we know (at init time) whether BGRA is supported (or preferred), can we just ask ImageLib to provide the data in the most efficient format? That would give us some flexibility.

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Dzmitry Malyshau [:kvark] from comment #12)
> 
> Given that we know (at init time) whether BGRA is supported (or preferred),
> can we just ask ImageLib to provide the data in the most efficient format?
> That would give us some flexibility.

I can provide whatever format we prefer for whatever platform.

Comment 14

[Jamie Nicol [:jnicol] (on leave until 21st October)]

Looks like gles also doesn't support automatic swizzling during upload, ie internalformat=RGBA8 external=BGRA. So the current fallback path for when GL_EXT_texture_format_BGRA8888 isn't supported will fail. It seems like that is very widely supported, however.

It seems like we care more about immutable storage than possibly having to swizzle data, so how about the following?:

* Use glTexStorage wherever possible (gles, or gl when GL_ARB_texture_storage is supported). This is the current behaviour.
* When GL_EXT_texture_format_BGRA8888 and GL_EXT_texture_storage are supported, use internal=BGRA8 external=BGRA.
  * This seems to apply to windows using angle but not most android devices, as they have gles 3's glTexStorage rather than the extension.
  * In the future we can also do this when GL_APPLE_texture_format_BGRA8888 is supported.
* Else use internal=RBGA8 external=RGBA, and set GL_TEXTURE_SWIZZLE_R/B.

I believe this will work on gles3 and gl with any combination of available extensions, and allows the optimal path on angle (and mac, in the future). It's also pretty simple to implement.

Comment 15

[Dzmitry Malyshau [:kvark]]

Jamie, your suggested plan sounds good to me.
Also, here is some relevant reading: https://www.g-truc.net/post-0734.html

Comment 16

[Jamie Nicol [:jnicol] (on leave until 21st October)]

Since this is a webrender only change, do I create a pull request against servo/webrender? rather than a phabricator review on mozilla-central.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Yes, the current workflow means you have to get it landed in servo/webrender. Easiest way is to submit a PR, although Bobby has been makng changes in m-c, getting review via phabricator and then making/merging a PR. It's a bit easier for him since he has contributor permissions on servo/webrender.

Comment 18

[Jamie Nicol [:jnicol] (on leave until 21st October)]

Unfortunately my suggestion in comment 14 has a problem. Pretending the data is RGBA and setting the swizzle parameters works for BGRA image data which we upload. But it means that we also swizzle when rendering cached items which webrender has rasterized itself, such as borders.

So for now we probably want to fall back to glTexImage for BGRA on android, and then perhaps we want to work towards using RGBA instead.

Comment 19

[Bobby Holley (:bholley)]

Ok. We should get a bug on file to get RGBA working before we ship on Android, or verify that the relevant drivers aren't reserving space for mipmaps. Otherwise, we risk using 33% more memory than we need to, which isn't good.

Comment 20

[Dzmitry Malyshau [:kvark]]

(In reply to Jamie Nicol [:jnicol] from comment #18)
> Pretending the data is RGBA and setting the swizzle parameters works for
> BGRA image data which we upload. But it means that we also swizzle when
> rendering cached items which webrender has rasterized itself, such as borders.

Isn't the swizzling set at a texture/sampler unit level? We'd set the swizzle on the input textures that come with BGRA data, and not set the swizzle on WebRender's texture cache and render targets (obviously).

Comment 21

[Jamie Nicol [:jnicol] (on leave until 21st October)]

Yes it is set at texture/sampler unit level. If we know that one texture will only be used for BGRA image data we upload, and one texture will only be used for caching items rasterized by webrender, then that should be possible. Are these items always cached in different textures?

Comment 22

[Bobby Holley (:bholley)]

BGRA images can end up in the texture cache, which is shared between all sufficiently-small textures with 32-bit color, so I don't think dzmitry's suggestion would work, unless I'm missing something.

Comment 23

[Dzmitry Malyshau [:kvark]]

(In reply to Bobby Holley (:bholley) from comment #22)
> BGRA images can end up in the texture cache, which is shared between all
> sufficiently-small textures with 32-bit color, so I don't think dzmitry's
> suggestion would work, unless I'm missing something.

We discussed this a bit with Jamie. In the long run we figured we need uniform RGBA support in Gecko and WR. In the short run, we can sacrifice the 33% of memory on those Android GLES devices that don't support BGRA and use glTexImage.

There are multiple other ways to work around it, but nothing as ideal as proper support for RGBA.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Fixed by WR update in bug 1504672