WebGL content swapped at wrong time in threaded compositing mode
Created attachment 133947 [details] Patch
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.
Comment on attachment 133947 [details] Patch Attachment 133947 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12145211
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?
Created attachment 133955 [details] mac build fix
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.
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.
Created attachment 133959 [details] Patch
(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.
Comment on attachment 133959 [details] Patch Thanks, that's a nice cleanup. r=me still.
Awesome, holding off on cq+ until the other EWSen cycle.
This patch breaks the antialiased display path (which appears to not break any layout tests). Investigating both...
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.
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.
Created attachment 135924 [details] Patch
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.
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?
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.
(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().
(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?
Created attachment 136043 [details] Patch
(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.
Comment on attachment 136043 [details] Patch The state saving and restoring code has still been removed in DrawingBuffer::clearFramebuffer.
(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.
(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.
Committed r113493: <http://trac.webkit.org/changeset/113493>