ANGLE share handle compositing makes preserveDrawingBuffer test fail

RESOLVED FIXED in mozilla34

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla34
All
Windows 8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8464406 [details] [diff] [review]
patch 0: Fix style issues in touched code.
Attachment #8464406 - Flags: review?(bjacob)
(Assignee)

Comment 2

3 years ago
Created attachment 8464407 [details] [diff] [review]
patch 1: Fix compositing.
Attachment #8464407 - Flags: review?(dglastonbury)
(Assignee)

Updated

3 years ago
Blocks: 1045957
(Assignee)

Comment 3

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b60441ba3ccd
Attachment #8464407 - Flags: review?(dglastonbury) → review+
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) {
        ...
        
    }
}
Attachment #8464406 - Flags: review?(bjacob) → review+
(Assignee)

Comment 5

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
Created attachment 8464812 [details] [diff] [review]
patch 0: Fix style issues in touched code.

r=kamidphish
Attachment #8464406 - Attachment is obsolete: true
Attachment #8464812 - Flags: review+
(Assignee)

Comment 7

3 years ago
 https://hg.mozilla.org/integration/mozilla-inbound/rev/834a458dae0b
 https://hg.mozilla.org/integration/mozilla-inbound/rev/aef0101ff775
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/834a458dae0b
(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
Keywords: leave-open
(Assignee)

Comment 11

3 years ago
Alright, I found the issue.
(Assignee)

Comment 12

3 years ago
Created attachment 8468976 [details] [diff] [review]
patch 2: Pick RGBA/RGBX
Attachment #8468976 - Flags: review?(dglastonbury)
(Assignee)

Comment 13

3 years ago
Created attachment 8468977 [details] [diff] [review]
patch 3: Allow blitting internal FB 0.
Attachment #8468977 - Flags: review?(dglastonbury)
(Assignee)

Comment 14

3 years ago
Created attachment 8468979 [details] [diff] [review]
patch 3.5: Fix issues caused by patch 3 on B2G Emu
Attachment #8464407 - Attachment is obsolete: true
Attachment #8468979 - Flags: review?(dglastonbury)
(Assignee)

Comment 15

3 years ago
Created attachment 8468980 [details] [diff] [review]
patch 4: Style fixes
Attachment #8468980 - Flags: review?(dglastonbury)
Attachment #8468976 - Flags: review?(dglastonbury) → review+
Attachment #8468977 - Flags: review?(dglastonbury) → review+
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?
Attachment #8468979 - Flags: review?(dglastonbury) → review-
Attachment #8468980 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 17

3 years ago
(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.
(Assignee)

Comment 18

3 years ago
Created attachment 8469558 [details] [diff] [review]
patch 3.5: Fix issues caused by patch 3 on B2G Emu

Oops, this was the wrong patch.
Attachment #8468979 - Attachment is obsolete: true
Attachment #8469558 - Flags: review?(dglastonbury)
(Assignee)

Comment 19

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4fedbc53a342
Attachment #8469558 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 20

3 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.