1704783 - Red and green channels appear swapped for some images on PowerVR

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Jamie Nicol [:jnicol]]

Attachment: Screenshot_20210408-143021_Firefox Nightly.jpg

First time trying webrender on PowerVR (Samsung A10s, PowerVR Rogue GE8320).

The red and green channels in the google play logo appear swapped. However, the larger image and the gradient appear fine. This is due to swizzling: the google play logo is small and will therefore be uploaded to the shared cache, whereas the gradient is from a render task and the larger image will be in the standalone cache.

Weirdly, one time I resumed the app from the screen being off, and the google play logo and gradient had switched - the gradient had it's channels reversed but the google play logo looked normal. I have not been able to reproduce that since. I think the safest bet is probably to disable swizzling on PowerVR devices.

Currently on PowerVR we take this branch when deciding texture format support. The device supports BGRA format textures, but only supports glTexStorage for non-BGRA formats. This is the same configuration we use on Adreno, therefore it's unlikely the configuration is buggy in our code and more likely to be a driver bug to do with swizzling.

This configuration means "When we decide to use a BGRA format texture, use glTexImage2D with the unsized GL_BGRA_EXT format. But use RGBA as the default color format." This means for standalone cache entries we create the texture as the same format as the data to be uploaded, usually BGRA, and no swizzling is required. But for shared cache entries we create RGBA textures and set the swizzle flag on the entry.

However, if we disable swizzling support then we still create RGBA textures for the shared cache, but because swizzling is not supported we hit a GL error 0x0502 in glTexSubImage2D() when we attempt to upload to it. This is because we pass GL_BGRA as the external format when uploading to a texture with a GL_RGBA8 internal format, and conversion during upload is illegal in GLES. So the main thing to fix here is to set color_formats to TextureFormatPair::from(ImageFormat::BGRA8) if swizzling is not supported.

Tangentially, I'm somewhat surprised that we ignore the value of color_formats for standalone textures. I thought that at one point the conclusion was that we wanted to prefer using glTexStorage2D over glTexImage2D as much as possible. So wouldn't using glTexStorage2D + RGBA8 internal format + swizzling be the preferred choice for standalone textures (when supported)? Do you remember if this was a deliberate decision, Dzmitry?

Comment 2

[Jamie Nicol [:jnicol]]

Tangentially, I'm somewhat surprised that we ignore the value of color_formats for standalone textures. I thought that at one point the conclusion was that we wanted to prefer using glTexStorage2D over glTexImage2D as much as possible. So wouldn't using glTexStorage2D + RGBA8 internal format + swizzling be the preferred choice for standalone textures (when supported)?

To solve this, we can move the gl::GlType::Gles if supports_texture_swizzle branch of the match statement above the gl::GlType::Gles if supports_gles_bgra && !avoid_tex_image branch. Then we will use RGBA + glTexStorage + swizzling for BGRA standalone textures.

Then we can change the supports_gles_bgra && !avoid_tex_image branch (which will now only be taken if swizzling is unsupported) to return ImageFormat::BGRA8 instead of ImageFormat::RGBA8 for color_formats. Then I think we get the desired behaviour for all configurations.

We should also change the bottom branch of the match to pretend BGRA data is RGBA even though swizzling is unsupported (and log a warning). It will in theory result in swapped channels when rendering, but that's preferable to no image being rendered at all due to the uploads failing. (As GLES doesn't support conversion during upload.) There shouldn't be any devices for which this branch is taken anyway.

Comment 3

[Jamie Nicol [:jnicol]]

Attachment: Bug 1704783 - Disable texture swizzling support on PowerVR Rogue devices. r?kvark

Running webrender on PowerVR Rogue devices results in some images
being rendered with swapped R and B channels. This is because we use
texture swizzling for images in the shared texture cache, and
presumably the driver has a buggy implementation. This patch disables
texture swizzling on all PowerVR Rogue devices as a precaution.

It also ensures that we always use BGRA format textures in cases where
texture swizzling is unsupported, otherwise we encounter GL errors
when attempting to upload BGRA data to RGBA format textures, as GLES
does not support format conversion during upload. Furthermore, we make
it so that using RGBA+glTexStorage+swizzling is preferred to
BGRA+glTexImage in cases where swizzling is actually supported, as we
generally want to prefer glTexStorage to glTexImage.

Lastly, in cases where neither BGRA or swizzling are supported, rather
than attempting to upload BGRA data to RGBA textures (which will
result in a GL error) we pretend the BGRA data is RGBA. This will
result in the channels being swapped when rendering, but that is
preferable to the alternative of images not being uploaded at
all. This configuration shouldn't actually exist in the wild anyway.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5045813af7d1
Disable texture swizzling support on PowerVR Rogue devices. r=kvark

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5045813af7d1