Bug 58003 – chromium support for glSetLatch and glWaitLatch between 3D contexts

• Component: WebGL
• Status: RESOLVED FIXED

[John Bates]

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.

[John Bates]

Created attachment 88651 [details]
Patch

[WebKit Review Bot]

Attachment 88651 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8341707

[John Bates]

Created attachment 88683 [details]
Patch

[John Bates]

Created attachment 88914 [details]
Patch

[Kenneth Russell]

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.

[John Bates]

Created attachment 89115 [details]
Patch

[John Bates]

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

[John Bates]

Created attachment 89120 [details]
Patch

[Kenneth Russell]

Comment on attachment 89120 [details]
Patch

Looks great. Fantastic work on this. I think it's going to solve a lot of longstanding problems.

[WebKit Commit Bot]

Comment on attachment 89120 [details]
Patch

Clearing flags on attachment: 89120

Committed r83552: <http://trac.webkit.org/changeset/83552>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.