[Webkit-unassigned] [Bug 58003] chromium support for glSetLatch and glWaitLatch between 3D contexts

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58003


Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #88914|review?                     |review-
               Flag|                            |




--- Comment #5 from Kenneth Russell <kbr at google.com>  2011-04-11 15:03:16 PST ---
(From update of attachment 88914)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.