Bug 82275 – WebGL content swapped at wrong time in threaded compositing mode

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

WebGL content swapped at wrong time in threaded compositing mode

[James Robinson]

Created attachment 133947 [details]
Patch

[James Robinson]

Ken said he'd take a look at this. I've modified other DrawingBuffer implementations based on inspection and they all seem fine except for Blackberry, which seems to be clearly non-compiling on ToT so I've mostly left it alone.

[Build Bot]

Comment on attachment 133947 [details]
Patch

Attachment 133947 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12145211

[Kenneth Russell]

Comment on attachment 133947 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133947&action=review

Looks good overall. One minor comment. The Mac build failure is probably a minor issue. r=me

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.h:58
> +    static PassRefPtr<DrawingBuffer> create(GraphicsContext3D*, const IntSize&, bool seperateBackingTexture, bool alpha);

Typo: seperateBackingTexture -> separateBackingTexture. Should we define an enum instead of adding another anonymous bool argument to the constructor? Perhaps also retrofit the separateBackingTexture argument with one?

[James Robinson]

Created attachment 133955 [details]
mac build fix

[James Robinson]

Oops, I think I accidentally killed your review flag. I'm going to let EWS check that I didn't explode any other ports before landing.

[Kenneth Russell]

Comment on attachment 133955 [details]
mac build fix

No problem. The change to the Mac port looks OK, but consider the comments from the previous review.

[James Robinson]

Created attachment 133959 [details]
Patch

[James Robinson]

(In reply to comment #4)
> Typo: seperateBackingTexture -> separateBackingTexture. Should we define an enum instead of adding another anonymous bool argument to the constructor? Perhaps also retrofit the separateBackingTexture argument with one?

Good idea. I've switched both to enums - I struggled a bit on the name, but decided they were both requirements that the caller was making of creation. Let me know if that works for you.

[Kenneth Russell]

Comment on attachment 133959 [details]
Patch

Thanks, that's a nice cleanup. r=me still.

[James Robinson]

Awesome, holding off on cq+ until the other EWSen cycle.

[James Robinson]

This patch breaks the antialiased display path (which appears to not break any layout tests).  Investigating both...

[Kenneth Russell]

Apologies that the layout tests don't exercise multisampling. I thought they would, based on memory and code inspection, but I haven't traced through DRT and in-process command buffer implementation to understand what's going on. Please feel free to file a new bug about that and assign it to me.

[James Robinson]

Comment on attachment 133959 [details]
Patch

Clearing flags - this breaks the multisampled path on Chromium because FBOs aren't shared between contexts so we can't bind + blit from context other than the WebGL context.  I'll leave the resolve in paint..() and make sure that we always double buffer the color attachment and do either a swap or a copy in updateCompositorResources().  I'll also file a bug about why this isn't caught by running layout tests, since the multisampled output path is the default.

[James Robinson]

Created attachment 135924 [details]
Patch

[James Robinson]

This ended up being a bit larger than I had hoped, but I believe it works.  A few things to note:

1.) mesa doesn't support multisampling, so the pixel expectations for DRT are not multisampled. I've run this test manually to confirm that multisampling still works as expected.

2.) I've updated other DrawingBuffer implementations in a best-effort way, but it's pretty clear that many of these do not even compile with current code.

3.) This *still* doesn't swap at exactly the right time in the face of incremental uploads (canvas 2d has the same bug).  I'll address this for WebGL and canvas 2d in a follow-up patch since it'll only impact chromium code.

[Kenneth Russell]

Comment on attachment 135924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135924&action=review

Overall this looks very good. Nice work. There's only one small issue, but I feel pretty strongly that it should be taken care of (and re-testing done), thus the r-.

> Source/WebCore/ChangeLog:29
> +        since we're shadowing the relevant GL state on the WebGLRenderingContext anyway.

This one change concerns me. Noting that FECustomFilter.cpp is using DrawingBuffer, it is not a good idea to assume that the WebGLRenderingContext is the caller of DrawingBuffer::clearFramebuffer(). Minimally, this change should be split off from the rest of this patch, but I also think it should just not be done.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:145
> +    m_drawingBuffer = DrawingBuffer::create(m_context.get(), contextSize, DrawingBuffer::Discard, DrawingBuffer::Alpha);

Because this class uses DrawingBuffer I think it is not a good idea to assume that the WebGLRenderingContext will preserve all of the needed state during calls to DrawingBuffer::clearFramebuffer().

> LayoutTests/fast/canvas/webgl/webgl-composite-modes-repaint.html:4
> +This test is only useful as a pixel test. You should see two rows with 4 canvases in each. The top row of canvases should have a black background, the bottom row should have a pink background.

Is the comment about the pink background accurate in this test and the other one?

[James Robinson]

FECustomFilter doesn't use clearFramebuffer(), so I'm not sure what code or testing change you are proposing.  Do you want me to just delete the comment?  Presumably if somebody adds a call to clearFramebuffer() they will read the comment and do whatever they need to do to maintain their GL state invariant.

The pink comment in the layout test is out of date (I changed the background color to darkblue to make it more obvious if antialiasing was happening or not).  Will update.

[Kenneth Russell]

(In reply to comment #18)
> FECustomFilter doesn't use clearFramebuffer(), so I'm not sure what code or testing change you are proposing.  Do you want me to just delete the comment? 

No, I'd like you to restore the state saving and restoring code in DrawingBuffer::clearFramebuffer().

[James Robinson]

(In reply to comment #19)
> (In reply to comment #18)
> > FECustomFilter doesn't use clearFramebuffer(), so I'm not sure what code or testing change you are proposing.  Do you want me to just delete the comment? 
> 
> No, I'd like you to restore the state saving and restoring code in DrawingBuffer::clearFramebuffer().

Why?

[James Robinson]

Created attachment 136043 [details]
Patch

[Kenneth Russell]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > FECustomFilter doesn't use clearFramebuffer(), so I'm not sure what code or testing change you are proposing.  Do you want me to just delete the comment? 
> > 
> > No, I'd like you to restore the state saving and restoring code in DrawingBuffer::clearFramebuffer().
> 
> Why?

Because if other callers than WebGL start using the DrawingBuffer class, then having clearFramebuffer() destroy context state will be surprising and unexpected. I don't think there will be a huge performance improvement from eliminating these state queries. If there is, we can figure out alternatives.

[Kenneth Russell]

Comment on attachment 136043 [details]
Patch

The state saving and restoring code has still been removed in DrawingBuffer::clearFramebuffer.

[James Robinson]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (In reply to comment #18)
> > > > FECustomFilter doesn't use clearFramebuffer(), so I'm not sure what code or testing change you are proposing.  Do you want me to just delete the comment? 
> > > 
> > > No, I'd like you to restore the state saving and restoring code in DrawingBuffer::clearFramebuffer().
> > 
> > Why?
> 
> Because if other callers than WebGL start using the DrawingBuffer class, then having clearFramebuffer() destroy context state will be surprising and unexpected. I don't think there will be a huge performance improvement from eliminating these state queries. If there is, we can figure out alternatives.

It's in the header.

Pretty much every other operation on the DrawingBuffer (creating, reset(), commit(), bind(), createSecondaryBuffers(), paintRenderingResultsToImageData()) destroys context state.  It's far more surprising that clearFramebuffer() attempts to restore state currently when nothing else does and every caller is shadowing this state anyway.

[Kenneth Russell]

(In reply to comment #24)
> Pretty much every other operation on the DrawingBuffer (creating, reset(), commit(), bind(), createSecondaryBuffers(), paintRenderingResultsToImageData()) destroys context state.  It's far more surprising that clearFramebuffer() attempts to restore state currently when nothing else does and every caller is shadowing this state anyway.

All right, I hadn't realized that none of the other operations tried to preserve state. r=me then, and thanks for fixing this.

[James Robinson]

Committed r113493: <http://trac.webkit.org/changeset/113493>