Bug 51678 – Support for OES_standard_derivatives

• Component: WebGL
• Status: RESOLVED FIXED

[Marcin Ignac]

I was trying to implement hatching shader in WebGL and discovered that OES_standard_derivatives extension is not supported yet and functions like dFdx()  or dFdy() can't be used. I was surprised because it is supported by OpenGL ES 2.0. From what I see in the source of http://www.khronos.org/registry/webgl/doc/spec/extensions/ it is one of the planned extensions. Any timeline for supporting it?

[Ben Vanik]

Working on a patch for this that'll enable the extension in both WebKit and Chromium.

Extension spec:
http://www.khronos.org/registry/webgl/doc/spec/extensions/OES_standard_derivatives/

Conformance test:
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/oes-standard-derivatives.html

[Ben Vanik]

Created attachment 79541 [details]
Implementation of OES_standard_derivatives

Implementation of the OES_standard_derivatives WebGL extension.

Changes are modeled off of the existing OESTextureFloat extension. New files, extension retrieval, etc all match the existing code.

Changed ANGLEWebKitBridge to allow for multiple sets of the ANGLE shader compiler options. This supports the enabling of the standard derivatives flag when the extension is enabled. Refactored the cleanup code to make the destruction of the compilers (if they had been created) cleaner.

Tested with the WebGL conformance test:
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/oes-standard-derivatives.html
Passes on WebKit/OSX, Chromium/OSX, and Chromium/Windows.

[Kenneth Russell]

Comment on attachment 79541 [details]
Implementation of OES_standard_derivatives

This looks perfect. Thanks for implementing it. The only questionable part is making Extensions3DOpenGL a friend of GraphicsContext3D on the Mac, but I actually think that's cleaner than providing an accessor method for the ANGLEWebKitBridge, which is a class we have been thinking about throwing out entirely.

[WebKit Commit Bot]

Comment on attachment 79541 [details]
Implementation of OES_standard_derivatives

Rejecting attachment 79541 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'ap..." exit_code: 2

Last 500 characters of output:
Bridge.h
patching file Source/WebCore/platform/graphics/Extensions3D.h
patching file Source/WebCore/platform/graphics/GraphicsContext3D.h
patching file Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp
patching file Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.h
patching file Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7505215

[Kenneth Russell]

Darn it. I'll try to land this by hand later today.

[Ben Vanik]

RE the friending, I thought the same thing but saw a similar pattern used in Extensions3DChromium with GraphicsContext3DInternal - when modifying Extensions3DOpenGL to take the a weak pointer for the same purposes I figured I'd reuse that.
Now that everyone is friending things maybe there's some refactoring that can be done to make things cleaner, but I figured I'd avoid changing too much :)

[Zhenyao Mo]

Comment on attachment 79541 [details]
Implementation of OES_standard_derivatives

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

If you haven't landed it already, there are two style violations.

> Source/WebCore/html/canvas/OESStandardDerivatives.cpp:34
> +OESStandardDerivatives::OESStandardDerivatives() : WebGLExtension()

This is against webkit style

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:44
> +      m_context(context)

This is against webkit style.

[Kenneth Russell]

(In reply to comment #7)
> (From update of attachment 79541 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79541&action=review
> 
> If you haven't landed it already, there are two style violations.
> 
> > Source/WebCore/html/canvas/OESStandardDerivatives.cpp:34
> > +OESStandardDerivatives::OESStandardDerivatives() : WebGLExtension()
> 
> This is against webkit style

I don't see a firm rule about one-line constructor declarations in the WebKit style guide. Since the style checker didn't flag this I think it's OK.

> > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:44
> > +      m_context(context)
> 
> This is against webkit style.

You're right, generally the commas go first. I'll fix this in the process of generating a new patch to try to land via the commit queue.

[Kenneth Russell]

Created attachment 79674 [details]
Patch

[Kenneth Russell]

Comment on attachment 79674 [details]
Patch

Merged with top of tree. Attempting to re-land via commit queue.

[WebKit Commit Bot]

Comment on attachment 79674 [details]
Patch

Clearing flags on attachment: 79674

Committed r76324: <http://trac.webkit.org/changeset/76324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This broke Mac builds because it added an idl file to the project Resources. How did it get through the commit queue?

[Eric Seidel (no email)]

Not sure.  Does the check-for-resources script catch things on the first build or only a re-build?

The commit-queue nodes all run on Snow Leopard.

[Kenneth Russell]

(In reply to comment #13)
> This broke Mac builds because it added an idl file to the project Resources.

Sorry about the build breakage; how was it reproduced? Has it been cleaned up at this point, or does something more need to be done?