Closed Bug 684853 Opened 9 years ago Closed 8 years ago

Implement WebGL OES_standard_derivatives extension

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: bjacob, Assigned: drs)

References

Details

(Keywords: dev-doc-complete, Whiteboard: webgl-extension)

Attachments

(2 files, 3 obsolete files)

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().
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.
Assignee: nobody → dsherk
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.
Attachment #559370 - Flags: review?(bjacob)
Also, the water demo looks like Chrome with this patch.
Whoops, forgot to 'hg add' a couple of files. This should have everything.
Attachment #559370 - Attachment is obsolete: true
Attachment #559370 - Flags: review?(bjacob)
Attachment #559371 - Flags: review?(bjacob)
Cleaned up some commented-out code and removed a temporary fix I added specifically for my computer.
Attachment #559371 - Attachment is obsolete: true
Attachment #559371 - Flags: review?(bjacob)
Attachment #559523 - Flags: review?(bjacob)
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!
(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 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 <vladimir@pobox.com> (original author)
> + *   Mark Steele <mwsteele@gmail.com>

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 <vladimir@pobox.com> (original author)
> + *   Mark Steele <mwsteele@gmail.com>

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?)
Attachment #559523 - Flags: review?(bjacob) → review-
(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.
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
Attachment #559523 - Attachment is obsolete: true
Attachment #563699 - Flags: review?(bjacob)
Oh OK. I see. So what you want is probably ANGLE r745:

   http://code.google.com/p/angleproject/source/detail?r=745
Depends on: 690735
So, I've filed bug 690735 about upgrading ANGLE, so we can forget about the ANGLE part of your patch.
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.
Attachment #563699 - Flags: review?(bjacob) → review+
Inbound, without custom ANGLE changes, on top of ANGLE updated to r774:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1798410cb04d
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.
https://hg.mozilla.org/mozilla-central/rev/1798410cb04d

fixed, but please address comment 15.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(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?
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?
Maybe Gervase knows about this.
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'?
(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.
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.
Blocks: 656824
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
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?
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
Updated this page to mention the addition of this extension's support:

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

Mentioned on Firefox 10 for developers.
No longer blocks: webgl-ext
Whiteboard: webgl-extension
You need to log in before you can comment on or make changes to this bug.