The default bug view has changed. See this FAQ.

Implement WebGL OES_standard_derivatives extension

RESOLVED FIXED in mozilla10

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bjacob, Assigned: drs)

Tracking

({dev-doc-complete})

unspecified
mozilla10
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: webgl-extension)

Attachments

(2 attachments, 3 obsolete attachments)

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().
(Reporter)

Comment 1

6 years ago
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)

Updated

6 years ago
Assignee: nobody → dsherk
(Assignee)

Comment 2

6 years ago
Created attachment 559370 [details] [diff] [review]
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.
Attachment #559370 - Flags: review?(bjacob)
(Assignee)

Comment 3

6 years ago
Also, the water demo looks like Chrome with this patch.
(Assignee)

Comment 4

6 years ago
Created attachment 559371 [details] [diff] [review]
Patch v1.0, OES_standard_derivatives implementation.

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)
(Assignee)

Comment 5

6 years ago
Created attachment 559523 [details] [diff] [review]
Patch v1.1, OES_standard_derivatives implementation.

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)

Comment 6

6 years ago
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!
(Assignee)

Comment 7

6 years ago
(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.
(Reporter)

Comment 8

6 years ago
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-
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
Created attachment 563699 [details] [diff] [review]
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
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
(Reporter)

Updated

6 years ago
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
Last Resolved: 6 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.
Benoit, see http://blog.gerv.net/2010/02/mpl_initial_developer_for_mozilla_employ/
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'?
(Assignee)

Comment 22

6 years ago
(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.
(Assignee)

Comment 23

6 years ago
Created attachment 564136 [details] [diff] [review]
(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.
(Assignee)

Updated

6 years ago
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
Depends on: 693595
Keywords: dev-doc-needed
(Assignee)

Comment 25

6 years ago
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.
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 728319
(Reporter)

Updated

5 years ago
No longer blocks: 728319
Whiteboard: webgl-extension
You need to log in before you can comment on or make changes to this bug.