SkiaGL fails to readback pixels on Adreno

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

It looks like readback of the texture fails on Adreno and possibly PowerVR, causing lots of bustage. We either get all zeros or garbage.
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.
(╯°□°)╯︵ ┻━┻
Attachment #768160 - Flags: review?(jgilbert)
Attachment #768160 - Attachment is obsolete: true
Attachment #768160 - Flags: review?(matt.woodrow)
Attachment #768160 - Flags: review?(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])`?
Attachment #768298 - Flags: review-
(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.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 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
Attachment #768740 - Flags: review?(jgilbert) → review-
(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.
Attachment #768740 - Attachment is obsolete: true
Attachment #768972 - Flags: review?(jgilbert) → review+
Attachment #768298 - Attachment is obsolete: true
Attachment #768298 - Flags: review?(matt.woodrow)
https://hg.mozilla.org/mozilla-central/rev/fb0c86ed08c2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.