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.

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

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.

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

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?

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

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

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

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.

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?

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

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

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

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

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.

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

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

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.

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.

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.

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

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?

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.

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

Fixed by WR update in bug 1504672