Closed Bug 1045955 Opened 5 years ago Closed 5 years ago

ANGLE share handle compositing makes preserveDrawingBuffer test fail

Categories

(Core :: Canvas: WebGL, defect)

All
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jgilbert, Assigned: jgilbert)

References

()

Details

Attachments

(5 files, 3 obsolete files)

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.
Attachment #8464406 - Flags: review?(bjacob)
Attached patch patch 1: Fix compositing. (obsolete) — Splinter Review
Attachment #8464407 - Flags: review?(dglastonbury)
Blocks: 1045957
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+
(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.
r=kamidphish
Attachment #8464406 - Attachment is obsolete: true
Attachment #8464812 - Flags: review+
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
(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
Alright, I found the issue.
Attachment #8468976 - Flags: review?(dglastonbury)
Attachment #8468977 - Flags: review?(dglastonbury)
Attachment #8464407 - Attachment is obsolete: true
Attachment #8468979 - Flags: review?(dglastonbury)
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+
(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.
Oops, this was the wrong patch.
Attachment #8468979 - Attachment is obsolete: true
Attachment #8469558 - Flags: review?(dglastonbury)
Attachment #8469558 - Flags: review?(dglastonbury) → review+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.