684853 - Implement WebGL OES_standard_derivatives extension

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This demo got a lot of attention last week:

http://madebyevan.com/webgl-water/

The "caustics" effect works only in Chrome because we suck^Wdon't implement the OES_standard_derivatives extension.

http://www.khronos.org/registry/webgl/extensions/OES_standard_derivatives/

This is part of desktop OpenGL 2.0 and is an extension, also named OES_standard_derivatives, on OpenGL ES.

Should be just a matter of trivially exposing the extension plus hooking the FRAGMENT_SHADER_DERIVATIVE_HINT_OES pname in hint() and getParameter().

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Implementation steps:
* in WebGLContext.h add WebGL_OES_standard_derivatives below WebGL_OES_texture_float
* in WebGLContext.cpp edit GetExtension, GetSupportedExtensions, IsExtensionSupported to handle_standard_derivatives like OES_texture_float
* in WebGLContextGL.cpp edit Hint() and GetParameter() according to the extension spec.

Notice that we have the constant LOCAL_GL_FRAGMENT_SHADER_DERIVATIVE_HINT already defined for internal use in our C++ codebase. No need to introduce FRAGMENT_SHADER_DERIVATIVE_HINT_OES, it has the same value.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, OES_standard_derivatives implementation.

Implements OES_standard_derivatives. Includes fallbacks in case it isn't available. Passes all mochi tests.

The design may need some work as I don't know XPCOM very well/at all and probably did something wrong.

Comment 3

[Doug Sherk (:drs) (inactive)]

Also, the water demo looks like Chrome with this patch.

Comment 4

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, OES_standard_derivatives implementation.

Whoops, forgot to 'hg add' a couple of files. This should have everything.

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, OES_standard_derivatives implementation.

Cleaned up some commented-out code and removed a temporary fix I added specifically for my computer.

Comment 6

[nemo]

Just gonna CC so I can try stuff like:
http://www.neuroproductions.be/webgl/tadPoles/

Once it is done.
Love how they say "Sorry, no HTML5 sound API found :( " ...
Format wars have begun!

Comment 7

[Doug Sherk (:drs) (inactive)]

(In reply to nemo from comment #6)
> Just gonna CC so I can try stuff like:
> http://www.neuroproductions.be/webgl/tadPoles/
> 
> Once it is done.
> Love how they say "Sorry, no HTML5 sound API found :( " ...
> Format wars have begun!

You can apply the above patch yourself and build Fx if you want. This won't be landing for several weeks as it hasn't even been reviewed yet.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 559523 [details] [diff] [review]
Patch v1.1, OES_standard_derivatives implementation.

Review of attachment 559523 [details] [diff] [review]:
-----------------------------------------------------------------

Great work! Some nits, and 1 serious request for explanation about the ANGLE change.

::: content/canvas/src/Makefile.in
@@ +68,5 @@
>  	WebGLContext.cpp \
>  	WebGLContextGL.cpp \
>  	WebGLContextUtils.cpp \
>  	WebGLContextValidate.cpp \
> +	WebGLExtensionStandardDerivatives.cpp \

OK. I guess we want to move to more separate files anyway.

::: content/canvas/src/WebGLContext.cpp
@@ +37,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "WebGLContext.h"
> +#include "WebGLExtensions.h"

Same.

@@ +946,5 @@
> +        case WebGL_OES_texture_float: {
> +            MakeContextCurrent();
> +            isSupported = gl->IsExtensionSupported(gl->IsGLES2() ? 
> +                GLContext::OES_texture_float : GLContext::ARB_texture_float);
> +        }

No need for the curly braces here; I know we have some elsewhere but really no need unless you want to scope a local variable.

Also, I preferred the old formatting for the ?: , aligning the two possible results.

@@ +1243,5 @@
> +  //NS_INTERFACE_MAP_ENTRY(WebGLExtension)
> +  NS_INTERFACE_MAP_ENTRY(nsIWebGLExtensionStandardDerivatives)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, WebGLExtension)
> +  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(WebGLExtensionStandardDerivatives)
> +NS_INTERFACE_MAP_END_INHERITING(WebGLExtension)

You enjoying your time at mozilla ?

::: content/canvas/src/WebGLContext.h
@@ +480,3 @@
>          WebGLExtensionID_Max
>      };
> +    nsCOMPtr<WebGLExtension> mEnabledExtensions[WebGLExtensionID_Max];

OK. It does make more sense to store actual WebGLExtensions rather than interface base objects. Not sure what the original reasoning was (this was from Vlad's original texture_float patch). Maybe he did that when there was no WebGLExtension class?

::: content/canvas/src/WebGLContextGL.cpp
@@ +1952,5 @@
> +            }
> +            else
> +                return ErrorInvalidEnum("getParameter: parameter", pname);
> +        }
> +            break;

Again... let's get rid of the {...} after case: statements. Except of course if you have local variables to scope, but that isn't the case here (i is scoped in the if{} anyway).

@@ +3995,5 @@
>          resources.MaxTextureImageUnits = mGLMaxTextureImageUnits;
>          resources.MaxFragmentUniformVectors = mGLMaxFragmentUniformVectors;
>          resources.MaxDrawBuffers = 1;
> +        if (mEnabledExtensions[WebGL_OES_standard_derivatives])
> +            resources.OES_standard_derivatives = 1;

Oh so that's the ANGLE API to tell it to enable the extension in the shader compiler. OK. Kudos for figuring out ANGLE API on your own!

::: content/canvas/src/WebGLExtensionStandardDerivatives.cpp
@@ +20,5 @@
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Vladimir Vukicevic <[e-mail]> (original author)
> + *   Mark Steele <[e-mail]>

I don't think Mark contributed to this part, this is from Vlad's texture_float patch.

@@ +56,5 @@
> +#include "nsIPrivateDOMEvent.h"
> +#include "nsIDOMDataContainerEvent.h"
> +
> +#include "nsContentUtils.h"
> +#include "mozilla/Preferences.h"

Surely you don't need all those headers for this tiny file!

::: content/canvas/src/WebGLExtensions.h
@@ +20,5 @@
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Vladimir Vukicevic <[e-mail]> (original author)
> + *   Mark Steele <[e-mail]>

Same.

::: gfx/angle/src/compiler/Initialize.cpp
@@ +636,5 @@
>  void InitExtensionBehavior(const ShBuiltInResources& resources,
>                             TExtensionBehavior& extBehavior)
>  {
>      if (resources.OES_standard_derivatives)
> +        extBehavior["GL_OES_standard_derivatives"] = EBhDisable;

Wait! We're patching ANGLE now?

 1. this needs careful justification
 2. you'd then have to put a .patch file in gfx/angle and edit gfx/angle/README.mozilla
 3. why does chrome apparently not need to do that? (or do they keep a local patch against ANGLE?)

Comment 9

[Doug Sherk (:drs) (inactive)]

(In reply to Benoit Jacob [:bjacob] from comment #8)

> Great work! Some nits, and 1 serious request for explanation about the ANGLE
> change.

Thanks!

> @@ +1243,5 @@
> > +  //NS_INTERFACE_MAP_ENTRY(WebGLExtension)
> > +  NS_INTERFACE_MAP_ENTRY(nsIWebGLExtensionStandardDerivatives)
> > +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, WebGLExtension)
> > +  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(WebGLExtensionStandardDerivatives)
> > +NS_INTERFACE_MAP_END_INHERITING(WebGLExtension)
> 
> You enjoying your time at mozilla ?

I can safely say that those 5 lines took the majority of my time on this patch. :p

> ::: content/canvas/src/WebGLContext.h
> @@ +480,3 @@
> >          WebGLExtensionID_Max
> >      };
> > +    nsCOMPtr<WebGLExtension> mEnabledExtensions[WebGLExtensionID_Max];
> 
> OK. It does make more sense to store actual WebGLExtensions rather than
> interface base objects. Not sure what the original reasoning was (this was
> from Vlad's original texture_float patch). Maybe he did that when there was
> no WebGLExtension class?

nsIWebGLExtension is an ambiguous base of both WebGLExtension and nsIWebGLExtensionStandardDerivatives (both of which WebGLExtensionStandardDerivatives) derives from. Thus, it doesn't know which one to refer to when storing it. So my solution was to change it to a WebGLExtension array, since that's only derived once and sort of makes more sense anyways.

> ::: gfx/angle/src/compiler/Initialize.cpp
> @@ +636,5 @@
> >  void InitExtensionBehavior(const ShBuiltInResources& resources,
> >                             TExtensionBehavior& extBehavior)
> >  {
> >      if (resources.OES_standard_derivatives)
> > +        extBehavior["GL_OES_standard_derivatives"] = EBhDisable;
> 
> Wait! We're patching ANGLE now?
> 
>  1. this needs careful justification
>  2. you'd then have to put a .patch file in gfx/angle and edit
> gfx/angle/README.mozilla
>  3. why does chrome apparently not need to do that? (or do they keep a local
> patch against ANGLE?)

I figured that out more or less by trial and error but I just checked Chromium and they use the same code as my patch. Maybe they have a different version or something? Anyways, this change is needed to pass all of the conformance tests. I will do as you said with 1 and 2. Are we up to date on ANGLE? If not, maybe we should look into upgrading instead of changing this.

Comment 10

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.2, OES_standard_derivatives implementation.

Patch v1.2, OES_standard_derivatives implementation. Includes suggestions from bjacob.

Note that, when referring to the ANGLE changes, the conformance test that doesn't finish is on this page:
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/extensions/oes-standard-derivatives.html

Line 6 (as of writing):
PASS GL_OES_standard_derivatives not defined in shaders when extension disabled

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Oh OK. I see. So what you want is probably ANGLE r745:

   http://code.google.com/p/angleproject/source/detail?r=745

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

So, I've filed bug 690735 about upgrading ANGLE, so we can forget about the ANGLE part of your patch.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 563699 [details] [diff] [review]
Patch v1.2, OES_standard_derivatives implementation.

Review of attachment 563699 [details] [diff] [review]:
-----------------------------------------------------------------

r+; can land as-is, or do the ANGLE update first and land without the ANGLE patch.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Inbound, without custom ANGLE changes, on top of ANGLE updated to r774:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1798410cb04d

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 563699 [details] [diff] [review]
Patch v1.2, OES_standard_derivatives implementation.

>--- /dev/null
>+++ b/content/canvas/src/WebGLExtensionStandardDerivatives.cpp
>@@ -0,0 +1,58 @@
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ *   Mozilla Corporation.

This is not true. The ID for Mozilla employees is "the Mozilla Foundation". Your reviewer should have caught this.

Please fix asap in both files.

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1798410cb04d

fixed, but please address comment 15.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Ms2ger from comment #15)
> This is not true. The ID for Mozilla employees is "the Mozilla Foundation".
> Your reviewer should have caught this.
> 
> Please fix asap in both files.

I actually didn't know about that rule; this has been copied from other files so I'm not the only one to not know about this rule, unless it's new. Can you point me to some documentation around that?

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

bjacob@cahouette:~/mozilla-central/content$ grep 'Mozilla Corporation' -R . | wc -l
145
bjacob@cahouette:~/mozilla-central/content$ grep 'Mozilla Foundation' -R . | wc -l
224

So it would seem that 40% of our files under content/ are making the same mistake?

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Maybe Gervase knows about this.

Comment 20

[Josh Matthews [:jdm]]

Benoit, see http://blog.gerv.net/2010/02/mpl_initial_developer_for_mozilla_employ/

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

OK. There is an additional difficulty in the present case: the code in this new file isn't entirely new, it's starting off existing code from WebGLContext.h and WebGLContext.cpp, originally written by Vlad, and these file have 'Initial Developer' = 'Mozilla Corporation'. We preserved the mention of Vlad as contributor; shouldn't we also preserve the 'Initial Developer'?

Comment 22

[Doug Sherk (:drs) (inactive)]

(In reply to Benoit Jacob [:bjacob] from comment #21)
> OK. There is an additional difficulty in the present case: the code in this
> new file isn't entirely new, it's starting off existing code from
> WebGLContext.h and WebGLContext.cpp, originally written by Vlad, and these
> file have 'Initial Developer' = 'Mozilla Corporation'. We preserved the
> mention of Vlad as contributor; shouldn't we also preserve the 'Initial
> Developer'?

Indeed, I copied the header straight out of another file. If you could argue that this is my code instead though (since I wrote all of the new files without copying from anyone else's code, and just left their contributor notice because I didn't want to deal with whose it actually was), we could say that it's not "originally" from them so it should be MoFo.

Comment 23

[Doug Sherk (:drs) (inactive)]

Attachment: (On top of other patch) patch for author name

If we decide to go ahead and go from Mozilla Corporation -> Mozilla Foundation, we can use this. I labelled it as reviewed by bjacob because he reviewed the original patch.

Comment 24

[Gervase Markham [:gerv]]

Some files say Mozilla Corporation; they should say Mozilla Foundation, but it's not a big deal. Fix it if you remember, don't make the problem worse, don't spend a lot of time on it :-)

Gerv

Comment 25

[Doug Sherk (:drs) (inactive)]

I will submit a patch to change Mozilla Corporation -> Mozilla Foundation. I'm not sure why this is marked as dev-doc-needed, though. This patch is medium-large sized but in terms of actual functionality introduced it's fairly trivial and all stuff documented in Khronos specs. Is this referring to the changes in the WebGLExtension class and its derived classes?

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

dev-doc-needed means documentation for Web developers, like

   https://developer.mozilla.org/en/WebGL

I agree that in this case we probably don't need mozilla-specific documentation about that, as this extension is just exposing standard OpenGL functionality and trivial to use, and the spec has all the info that's needed,

   http://www.khronos.org/registry/webgl/extensions/OES_standard_derivatives/

OTOH there is one big dev-doc-needed topic in WebGL coming up, that's webglcontextlost / webglcontextrestored events, although Gregg has already posted a nice write-up on it:

   http://www.khronos.org/webgl/wiki/HandlingContextLost

Comment 27

[Eric Shepherd [:sheppy]]

Updated this page to mention the addition of this extension's support:

https://developer.mozilla.org/en/WebGL

Mentioned on Firefox 10 for developers.