1045955 - ANGLE share handle compositing makes preserveDrawingBuffer test fail

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

I found out that this test on trunk:
https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-attribute-preserve-drawing-buffer.html

It doesn't appear to be failing for our test slaves. I'm not sure why not. That's pretty bad, in terms of having test coverage.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: Fix style issues in touched code.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Fix compositing.

Comment 3

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=b60441ba3ccd

Comment 4

[u480271]

Comment on attachment 8464406 [details] [diff] [review]
patch 0: Fix style issues in touched code.

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

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +263,5 @@
>  
> +    bool needsRBSwap = false;
> +    if (aDest->GetFormat() == SurfaceFormat::B8G8R8A8 ||
> +        aDest->GetFormat() == SurfaceFormat::B8G8R8X8) {
> +        needsRBSwap = true;

bool needsRBSwap = (aDest->GetFormat() == SurfaceFormat::B8G8R8A8 ||
                    aDest->GetFormat() == SurfaceFormat::B8G8R8X8);

@@ +269,5 @@
>  
> +    bool needsConvertTo16Bits = false;
> +    if (aDest->GetFormat() == SurfaceFormat::R5G6B5) {
> +        needsConvertTo16Bits = true;
> +    }

bool needsConvertTo16Bits = (aDest->GetFormat() == SurfaceFormat::R5G6B5);

@@ +282,5 @@
> +            uint8_t g = srcRow[1];
> +            uint8_t b = needsRBSwap ? srcRow[0] : srcRow[2];
> +            uint8_t a = srcRow[3];
> +
> +            if (needsConvertTo16Bits) {

I don't know how often this function is called, but why branch inside the hot loop?

if (needsConvertTo16Bits) {
    while (srcRow != srcRowEnd) {
        ...
        *(uint16_t*) destRow = PackRGB565(r, g, b);
    }
} else {
    while (srcRow != srcRowEnd) {
        ...
        
    }
}

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #4)
> Comment on attachment 8464406 [details] [diff] [review]
> patch 0: Fix style issues in touched code.
> 
> Review of attachment 8464406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLReadTexImageHelper.cpp
> @@ +263,5 @@
> >  
> > +    bool needsRBSwap = false;
> > +    if (aDest->GetFormat() == SurfaceFormat::B8G8R8A8 ||
> > +        aDest->GetFormat() == SurfaceFormat::B8G8R8X8) {
> > +        needsRBSwap = true;
> 
> bool needsRBSwap = (aDest->GetFormat() == SurfaceFormat::B8G8R8A8 ||
>                     aDest->GetFormat() == SurfaceFormat::B8G8R8X8);
Yep.
> 
> @@ +269,5 @@
> >  
> > +    bool needsConvertTo16Bits = false;
> > +    if (aDest->GetFormat() == SurfaceFormat::R5G6B5) {
> > +        needsConvertTo16Bits = true;
> > +    }
> 
> bool needsConvertTo16Bits = (aDest->GetFormat() == SurfaceFormat::R5G6B5);
Yep.
> 
> @@ +282,5 @@
> > +            uint8_t g = srcRow[1];
> > +            uint8_t b = needsRBSwap ? srcRow[0] : srcRow[2];
> > +            uint8_t a = srcRow[3];
> > +
> > +            if (needsConvertTo16Bits) {
> 
> I don't know how often this function is called, but why branch inside the
> hot loop?
> 
> if (needsConvertTo16Bits) {
>     while (srcRow != srcRowEnd) {
>         ...
>         *(uint16_t*) destRow = PackRGB565(r, g, b);
>     }
> } else {
>     while (srcRow != srcRowEnd) {
>         ...
>         
>     }
> }
Because it was there already. I got half-way though making a better-optimized version of this, but then decided that we should just use a common fast pixel format converter somewhere in the tree. This is out of scope for the current bug, so I just kept it as it was.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: Fix style issues in touched code.

r=kamidphish

Comment 7

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/834a458dae0b
 https://hg.mozilla.org/integration/mozilla-inbound/rev/aef0101ff775

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G reftest failures. I left the style fixup in, so adding leave-open to the keywords so the bug isn't resolved when that merges to m-c. You'll want to remove it when re-pushing to inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba2784ebc197

https://tbpl.mozilla.org/php/getParsedLog.php?id=44910023&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=44909193&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=44910456&tree=Mozilla-Inbound

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/834a458dae0b

Comment 10

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Because it was there already. I got half-way though making a
> better-optimized version of this, but then decided that we should just use a
> common fast pixel format converter somewhere in the tree. This is out of
> scope for the current bug, so I just kept it as it was.

Oh sure, I didn't expect you to fix that one. It was more for prosperity. I wish we had a system not linked to bugzilla where we could leave code reviews. Kinda like this: http://www.bounceapp.com/116414

Comment 11

[Kelsey Gilbert [:jgilbert]]

Alright, I found the issue.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Pick RGBA/RGBX

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Allow blitting internal FB 0.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3.5: Fix issues caused by patch 3 on B2G Emu

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: Style fixes

Comment 16

[u480271]

Comment on attachment 8468979 [details] [diff] [review]
patch 3.5: Fix issues caused by patch 3 on B2G Emu

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

::: gfx/gl/GLBlitHelper.cpp
@@ +556,5 @@
>  
>      ScopedBindFramebuffer boundFB(mGL);
>      ScopedGLState scissor(mGL, LOCAL_GL_SCISSOR_TEST, false);
>  
> +    if (internalFBs && mGL->Screen()) {

If you ask for internalFBs and there is no Screen(), should it fallback or do nothing and report error?

Comment 17

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #16)
> Comment on attachment 8468979 [details] [diff] [review]
> patch 3.5: Fix issues caused by patch 3 on B2G Emu
> 
> Review of attachment 8468979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLBlitHelper.cpp
> @@ +556,5 @@
> >  
> >      ScopedBindFramebuffer boundFB(mGL);
> >      ScopedGLState scissor(mGL, LOCAL_GL_SCISSOR_TEST, false);
> >  
> > +    if (internalFBs && mGL->Screen()) {
> 
> If you ask for internalFBs and there is no Screen(), should it fallback or
> do nothing and report error?

It should only ask for internal if mGL->Screen() is non-null. I can add an explicit assert for this.

Comment 18

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3.5: Fix issues caused by patch 3 on B2G Emu

Oops, this was the wrong patch.

Comment 19

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=4fedbc53a342

Comment 20

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2260b0f9e424
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/27af4489326f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7593d2b0bd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b065f0ebf575

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2260b0f9e424
https://hg.mozilla.org/mozilla-central/rev/27af4489326f
https://hg.mozilla.org/mozilla-central/rev/ea7593d2b0bd
https://hg.mozilla.org/mozilla-central/rev/b065f0ebf575