887318 - SkiaGL fails to readback pixels on Adreno

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

It looks like readback of the texture fails on Adreno and possibly PowerVR, causing lots of bustage. We either get all zeros or garbage.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

SkiaGL always tries to readback using GL_BGRA, which is unsupported on a lot of stuff. We'll need something similar to what we have in GLContext to read with RGBA and converting.

Comment 2

[George Wright (:gw280) (needinfo me!)]

(╯°□°）╯︵ ┻━┻

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SkiaGL readback when BGRA unavailable

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SkiaGL readback when BGRA unavailable

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 768298 [details] [diff] [review]
Fix SkiaGL readback when BGRA unavailable

Review of attachment 768298 [details] [diff] [review]:
-----------------------------------------------------------------

I got pinged on the previous patch for this, so I figured I'd check in.

::: gfx/skia/src/gpu/gl/GrGLCaps.cpp
@@ +125,5 @@
>          }
> +
> +        if (ctxInfo.hasExtension("GL_EXT_read_format_bgra")) {
> +            GrGLint readformat = 0;
> +            GR_GL_GetIntegerv(gli, GR_GL_IMPLEMENTATION_COLOR_READ_FORMAT, &readformat);

This is unfortunately not how this extension works. The value you get back here is actually dependent on the framebuffer that you have bound, and will change depending on what's backing the framebuffer.

::: gfx/skia/src/gpu/gl/GrGpuGL.cpp
@@ +1393,5 @@
> +    uint8_t* src = (uint8_t*)buffer;
> +    uint8_t* end = src + len;
> +
> +    for (; src != end; src += 4) {
> +        tmp[0] = src[2];

Why not `std::swap(src[0], src[2])`?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 768298 [details] [diff] [review]
> Fix SkiaGL readback when BGRA unavailable
> 
> Review of attachment 768298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I got pinged on the previous patch for this, so I figured I'd check in.
> 
> ::: gfx/skia/src/gpu/gl/GrGLCaps.cpp
> @@ +125,5 @@
> >          }
> > +
> > +        if (ctxInfo.hasExtension("GL_EXT_read_format_bgra")) {
> > +            GrGLint readformat = 0;
> > +            GR_GL_GetIntegerv(gli, GR_GL_IMPLEMENTATION_COLOR_READ_FORMAT, &readformat);
> 
> This is unfortunately not how this extension works. The value you get back
> here is actually dependent on the framebuffer that you have bound, and will
> change depending on what's backing the framebuffer.

Crap. So I need to do that right before the glReadPixels()?

> 
> ::: gfx/skia/src/gpu/gl/GrGpuGL.cpp
> @@ +1393,5 @@
> > +    uint8_t* src = (uint8_t*)buffer;
> > +    uint8_t* end = src + len;
> > +
> > +    for (; src != end; src += 4) {
> > +        tmp[0] = src[2];
> 
> Why not `std::swap(src[0], src[2])`?

Forgot about that, thanks.

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #6)
> Crap. So I need to do that right before the glReadPixels()?
Yep. It should be the same for the life of the framebuffer, but it can be different every time you switch or modify framebuffers. We should also assert that the READ_TYPE is UNSIGNED_BYTE as we expect.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SkiaGL readback when BGRA unavailable

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 768740 [details] [diff] [review]
Fix SkiaGL readback when BGRA unavailable

Review of attachment 768740 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/skia/src/gpu/gl/GrGpuGL.cpp
@@ +1387,5 @@
>          return false;
>      }
>  }
>  
> +static void swizzleRow(void* buffer, int len) {

I recommend changing `len` to `byteLen`.

@@ +1391,5 @@
> +static void swizzleRow(void* buffer, int len) {
> +    uint8_t* src = (uint8_t*)buffer;
> +    uint8_t* end = src + len;
> +
> +    for (; src != end; src += 4) {

This needs to assert that `end - src` is a multiple of four.

@@ +1408,5 @@
>      bool flipY = kBottomLeft_GrSurfaceOrigin == target->origin();
> +    bool needSwizzle = false;
> +
> +    if (kBGRA_8888_GrPixelConfig == config &&
> +        kDesktop_GrGLBinding != this->glBinding() &&

Gross, but yoda-code is the style in here, it looks like.

@@ +1421,5 @@
> +
> +            if (readformat != GR_GL_BGRA || readtype != GR_GL_UNSIGNED_BYTE) {
> +                needSwizzle = true;
> +            }
> +        } else needSwizzle = true;

Missing a newline here, and technically should have braces around it, too.

Also, this doesn't look correct. We'll hit this path if any of these are true:
1) config != kBGRA_8888
2) glBinding() == kDesktop
3) hasExtension("GL_EXT_bgra") (Remove this one, as EXT_bgra went core in GL2.0. Also see bug 743918)

These look like the conditions where we should *not* swizzle, since we already have bgra.

Really, we should only be swizzling if we need BGRA, and are on GLES, but unable to use BGRA. This would probably be clearer if we used a helper function for CanReadBGRA(). Alternatively, do this inline here, with something like:
needSwizzle = true
if config is BGRA:
  canReadBGRA = false
  if desktop:
    canReadBGRA = true
  else if has EXT_read_format_bgra:
    if implFormat is BGRA and implType is UByte:
      canReadBGRA = true

  if canReadBGRA:
    needSwizzle = true

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> Comment on attachment 768740 [details] [diff] [review]
> Fix SkiaGL readback when BGRA unavailable
> 
> Review of attachment 768740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/skia/src/gpu/gl/GrGpuGL.cpp
> @@ +1387,5 @@
> >          return false;
> >      }
> >  }
> >  
> > +static void swizzleRow(void* buffer, int len) {
> 
> I recommend changing `len` to `byteLen`.
> 

OK

> @@ +1391,5 @@
> > +static void swizzleRow(void* buffer, int len) {
> > +    uint8_t* src = (uint8_t*)buffer;
> > +    uint8_t* end = src + len;
> > +
> > +    for (; src != end; src += 4) {
> 
> This needs to assert that `end - src` is a multiple of four.

Yup done.

> 
> @@ +1408,5 @@
> >      bool flipY = kBottomLeft_GrSurfaceOrigin == target->origin();
> > +    bool needSwizzle = false;
> > +
> > +    if (kBGRA_8888_GrPixelConfig == config &&
> > +        kDesktop_GrGLBinding != this->glBinding() &&
> 
> Gross, but yoda-code is the style in here, it looks like.
> 
> @@ +1421,5 @@
> > +
> > +            if (readformat != GR_GL_BGRA || readtype != GR_GL_UNSIGNED_BYTE) {
> > +                needSwizzle = true;
> > +            }
> > +        } else needSwizzle = true;
> 
> Missing a newline here, and technically should have braces around it, too.

Fixed

> 
> Also, this doesn't look correct. We'll hit this path if any of these are
> true:
> 1) config != kBGRA_8888
> 2) glBinding() == kDesktop
> 3) hasExtension("GL_EXT_bgra") (Remove this one, as EXT_bgra went core in
> GL2.0. Also see bug 743918)
> 
> These look like the conditions where we should *not* swizzle, since we
> already have bgra.

Hmm, I think you misread?

> 
> Really, we should only be swizzling if we need BGRA, and are on GLES, but
> unable to use BGRA. This would probably be clearer if we used a helper
> function for CanReadBGRA(). Alternatively, do this inline here, with
> something like:
> needSwizzle = true
> if config is BGRA:
>   canReadBGRA = false
>   if desktop:
>     canReadBGRA = true
>   else if has EXT_read_format_bgra:
>     if implFormat is BGRA and implType is UByte:
>       canReadBGRA = true
> 
>   if canReadBGRA:
>     needSwizzle = true

Right, that's basically what it is now. I agree that maybe a separate canReadBGRA() thing may be more clear. I added the EXT_bgra check, though, because Tegra3 has that but does not seem to have EXT_read_format_bgra. I gather from the spec that if you have EXT_bgra then ReadPixels will work with that? It does work on Tegra3.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SkiaGL readback when BGRA unavailable

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0c86ed08c2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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