The chromium code needs to synchronize between child and parent contexts. For example, currently the WebGL FBO is copied into the compositor context texture without synchronization. So the compositor may render the wrong frame.
Created attachment 88651 [details] Patch
Attachment 88651 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8341707
Created attachment 88683 [details] Patch
Created attachment 88914 [details] Patch
Comment on attachment 88914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88914&action=review This looks very good overall and as we've discussed offline the results are excellent, fixing multiple longstanding bugs. We should do the API change to wait/setLatchCHROMIUM that we've discussed offline. A few other relatively minor issues. Don't forget to regenerate the ChangeLogs. Also, I suggest using "webkit-patch upload" to upload your patch the next time as it will take care of some things like style checks. > Source/WebCore/ChangeLog:3 > + Reviewed by Ken Russell. Leave this line as "Reviewed by NOBODY (OOPS!)". The commit scripts will rewrite it after it's been reviewed. > Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:73 > + void waitLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId); > + void setLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId); Per offline discussion, let's remove the shmId argument now. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:177 > - m_a(a), m_b(b), m_c(c), m_d(d) > + m_a(a), m_b(b), m_c(c), m_d(d) This whitespace-only change doesn't belong in this patch. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:276 > + // Before drawLayers: This comment seems useless. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:277 > + if (hardwareCompositing()) { Even though your current code runs, for correctness, the following block of code needs to be run against the layer renderer's GraphicsContext3D and all of the child layers' contexts: if (m_context->getExtensions()->supports("GL_CHROMIUM_latch")) m_context->getExtensions()->ensureEnabled("GL_CHROMIUM_latch"); and the code which uses getChildToParentLatchCHROMIUM and waitLatchCHROMIUM needs to be guarded by a test: if (extensions->supports("GL_CHROMIUM_latch")) See e.g. Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:278 > + // The multithreaded compositor case does not work as long as Change this to "FIXME: ... case will not work". > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:281 > + // potentially clobbering the parent texture that is beibng renderered typo: beibng > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:303 > + // After drawLayers: This comment also seems useless. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:353 > + // Before updateCompositorResourcesRecursive, when the compositor runs in Please add a FIXME here. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:967 > - > + This whitespace-only change doesn't belong in this patch. I suggest you try to configure your editor. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1125 > +// TODO(jbates): when compositor is multithreaded and copyTexImage2D bug is fixed, WebKit uses FIXME rather than TODO(). > Source/WebKit/chromium/ChangeLog:3 > + Reviewed by Ken Russell. Per above, leave the OOPS line. > Source/WebKit/chromium/public/WebGraphicsContext3D.h:175 > + virtual void getParentToChildLatchCHROMIUM(WGC3Duint* latchId) {} // TODO = 0; > + virtual void getChildToParentLatchCHROMIUM(WGC3Duint* latchId) {} // TODO = 0; > + virtual void waitLatchCHROMIUM(WGC3Dint shmId, WGC3Duint latchId) {} // TODO = 0; > + virtual void setLatchCHROMIUM(WGC3Dint shmId, WGC3Duint latchId) {} // TODO = 0; TODO -> FIXME: Also, per offline discussion, let's remove the shmId argument, as it's being removed from the implementation. > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:254 > - > + Whitespace-only change. > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:806 > +DELEGATE_TO_IMPL_2(waitLatchCHROMIUM, GC3Dint, GC3Duint) > +DELEGATE_TO_IMPL_2(setLatchCHROMIUM, GC3Dint, GC3Duint) Per offline discussion, remove first argument. > Source/WebKit/chromium/src/GraphicsContext3DInternal.h:277 > + void waitLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId); > + void setLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId); Remove shmId argument.
Created attachment 89115 [details] Patch
Comment on attachment 88914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88914&action=review >> Source/WebCore/ChangeLog:3 >> + Reviewed by Ken Russell. > > Leave this line as "Reviewed by NOBODY (OOPS!)". The commit scripts will rewrite it after it's been reviewed. done >> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:73 >> + void setLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId); > > Per offline discussion, let's remove the shmId argument now. done >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:177 >> + m_a(a), m_b(b), m_c(c), m_d(d) > > This whitespace-only change doesn't belong in this patch. done >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:276 >> + // Before drawLayers: > > This comment seems useless. It's meant to correlate the code with drawLayers, so that people aren't inclined to move it up or down, etc. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:277 >> + if (hardwareCompositing()) { > > Even though your current code runs, for correctness, the following block of code needs to be run against the layer renderer's GraphicsContext3D and all of the child layers' contexts: > > if (m_context->getExtensions()->supports("GL_CHROMIUM_latch")) > m_context->getExtensions()->ensureEnabled("GL_CHROMIUM_latch"); > > and the code which uses getChildToParentLatchCHROMIUM and waitLatchCHROMIUM needs to be guarded by a test: > > if (extensions->supports("GL_CHROMIUM_latch")) > > See e.g. Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp. done >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:278 >> + // The multithreaded compositor case does not work as long as > > Change this to "FIXME: ... case will not work". done >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:281 >> + // potentially clobbering the parent texture that is beibng renderered > > typo: beibng done >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:353 >> + // Before updateCompositorResourcesRecursive, when the compositor runs in > > Please add a FIXME here. done >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:967 >> + > > This whitespace-only change doesn't belong in this patch. I suggest you try to configure your editor. done >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1125 >> +// TODO(jbates): when compositor is multithreaded and copyTexImage2D bug is fixed, > > WebKit uses FIXME rather than TODO(). done >> Source/WebKit/chromium/ChangeLog:3 >> + Reviewed by Ken Russell. > > Per above, leave the OOPS line. done >> Source/WebKit/chromium/public/WebGraphicsContext3D.h:175 >> + virtual void setLatchCHROMIUM(WGC3Dint shmId, WGC3Duint latchId) {} // TODO = 0; > > TODO -> FIXME: > > Also, per offline discussion, let's remove the shmId argument, as it's being removed from the implementation. done >> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:254 >> + > > Whitespace-only change. done >> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:806 >> +DELEGATE_TO_IMPL_2(setLatchCHROMIUM, GC3Dint, GC3Duint) > > Per offline discussion, remove first argument. done >> Source/WebKit/chromium/src/GraphicsContext3DInternal.h:277 >> + void setLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId); > > Remove shmId argument. done
Created attachment 89120 [details] Patch
Comment on attachment 89120 [details] Patch Looks great. Fantastic work on this. I think it's going to solve a lot of longstanding problems.
Comment on attachment 89120 [details] Patch Clearing flags on attachment: 89120 Committed r83552: <http://trac.webkit.org/changeset/83552>
All reviewed patches have been landed. Closing bug.