The extension proposal is there: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/extensions/proposals/EXT_texture_filter_anisotropic/index.html A good model for how to add support for a new WebGL extension is there: bug 684853. In particular: 1. Add the IDL from the spec to this file, with a new UUID: http://hg.mozilla.org/mozilla-central/file/tip/dom/interfaces/canvas/nsIDOMWebGLRenderingContext.idl Let's call this interface, nsIWebGLExtensionTextureFilterAnisotropic. A C++ header will be auto-generated from that IDL, and will define a nsIWebGLExtensionTextureFilterAnisotropic class. But that's just a base class. Below we'll manually define a WebGLExtensionTextureFilterAnisotropic (notice: no nsI prefix) class, inheriting nsIWebGLExtensionTextureFilterAnisotropic (see step XXX). 2. Edit these files, add goop similar to what there is for WebGLExtensionStandardDerivatives / nsIWebGLExtensionStandardDerivatives: http://hg.mozilla.org/mozilla-central/file/tip/dom/base/nsDOMClassInfo.cpp http://hg.mozilla.org/mozilla-central/file/tip/dom/base/nsDOMClassInfoClasses.h http://hg.mozilla.org/mozilla-central/file/tip/js/src/xpconnect/src/dom_quickstubs.qsconf 3. Edit this header and define the WebGLExtensionTextureFilterAnisotropic class, inheriting nsIWebGLExtensionTextureFilterAnisotropic, similar to what's being done for WebGLExtensionStandardDerivatives: http://hg.mozilla.org/mozilla-central/file/tip/content/canvas/src/WebGLExtensions.h You shouldn't have to do anything nontrivial there, as this class only has to expose the constants that were already defined in the IDL, so it's already inheriting them. I would implement the (empty) constructor and destructor inline there, to save the hassle of having to add a new .cpp file just for them. 4. Now let's do the work to actually expose this extension, in WebGL getExtension and getSupportedExtensions methods. The file to edit is http://hg.mozilla.org/mozilla-central/file/tip/content/canvas/src/WebGLContext.cpp The functions exposed to scripts are WebGLContext::GetExtension and WebGLContext::GetSupportedExtensions. They call another internal function that you'll have to edit as well: WebGLContext::IsExtensionSupported. That's where you'll have to decide whether this extension is supported or not by the client. If you have to query the GL_EXTENSIONS string of the underlying OpenGL context, call gl->IsExtensionSupported(enum value). Indeed, we are checking for a list of extensions when we create a OpenGL context, so that subsequent checking for them is really fast. If the OpenGL extensions you need to check for haven't yet been added to the list that we check for, you'll have to edit these files in a straightforward way (search for IsExtensionSupported): http://hg.mozilla.org/mozilla-central/file/tip/gfx/gl/GLContext.h http://hg.mozilla.org/mozilla-central/file/tip/gfx/gl/GLContext.cpp 5. Finally, let's implement what this extension actually does. The spec describes that. The file you have to edit to do that, is: http://hg.mozilla.org/mozilla-central/file/tip/content/canvas/src/WebGLContextGL.cpp Since all what this extension does is modify the behavior of some existing methods, it should be straightforward to see what to do there. The only thing is that you'll need to use these new symbolic constants, like TEXTURE_MAX_ANISOTROPY. Rather than trying to use the constants from the extension class, add #defines for them to this file: http://hg.mozilla.org/mozilla-central/file/tip/gfx/gl/GLDefs.h Notice we use LOCAL_GL_ prefixes there to avoid some conflicts. *** Miscellaneous remarks: 0. General info about getting started with Gfx hacking: https://wiki.mozilla.org/Platform/GFX/Contribute 1. MXR or DXR are useful code search tools. http://mxr.mozilla.org/mozilla-central/ http://dxr.mozilla.org/mozilla/ 2. Test your code by running the webgl conformance test suite (compare to a test run without your patch): https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/webgl-conformance-tests.html or as a mozilla mochitest: TEST_PATH=content/canvas/test/webgl/test_webgl_conformance_test_suite.html make -C obj-firefox-debug/ mochitest-plain assuming that you are in your mozilla-central directory, and that obj-firefox-debug is your object directory. 3. Unfortunately we don't have a test case for this particular extension! It would be very useful to write one, even more useful to write one that can be added to the conformance test suite.

I forgot to add, you'll also have to add goop like this to WebGLContext.cpp: (here is what was done for OES_standard_derivatives): NS_IMPL_ADDREF(WebGLExtensionStandardDerivatives) NS_IMPL_RELEASE(WebGLExtensionStandardDerivatives) DOMCI_DATA(WebGLExtensionStandardDerivatives, WebGLExtensionStandardDerivatives) NS_INTERFACE_MAP_BEGIN(WebGLExtensionStandardDerivatives) 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)

I have a few questions on this patch: 1) the implementation currently serves the new enums from the extension object. Is that correct? If not how would the enums be added to the webgl context? 2) The extension is added as MOZ_texture_filter_anisotropic, is that correct? 3) Only the namestrings parsing implements the MOZ prefix, the other code uses the expected (when the draft is finalized) name convention, is that ok? 4) In the file content/canvas/src/WebGLContextGL.cpp below line 2086 I added a case for LOCAL_GL_MAX_TEXTURE_MAX_ANISOTROPY_EXT, is that correct there? If so, where would I put the constant for MINVALUE_GL_MAX_TEXTURE_MAX_ANISOTROPY? 5) In the file content/canvas/src/WebGLContextGL.cpp below the line 2745 there would have to be a test for the texture max anisotropy not to be below 1. Where should the constant for that go? How can I make it issue an INVALID_VALUE? 6) Is there something that needs to be done for Direct3D or does the angleproject wrapper take care of that?

Forgot to mention, I did run the conformance test suite, but the results differed at each run (even of the same build). Sometimes a page timed out, othertimes tests would seem to have been updated in the space of a few minutes. The different builds performed largely the same on the conformance tests. I've not yet written a conformance test for this patch, but I intend to do it once I have a final version. I did write a testpage for the patch, which can be found there: http://codeflow.org/webgl/anisotropy (it should look like this http://codeflow.org/pictures/anisotropy.jpg)

Comment on attachment 598923 [details] [diff] [review] Draft patch implementing texture filter anisotropic feedback and review work in the following way: you set a ? on me, and I reply by setting it to + or - depending on whether I approve it.

(In reply to Florian Bösch from comment #2) > Created attachment 598923 [details] [diff] [review] > Draft patch implementing texture filter anisotropic > > I have a few questions on this patch: > > 1) the implementation currently serves the new enums from the extension > object. Is that correct? If not how would the enums be added to the webgl > context? Yes, that is what the extension spec asks for, and that is indeed what we want for a WebGL extension (that's what other exts like the s3tc one do). > > 2) The extension is added as MOZ_texture_filter_anisotropic, is that correct? I think it should be MOZ_EXT_ . The vendor prefix MOZ_ should be added to, not replace, the extension prefix EXT_. See the case of WEBGL_lose_context: http://www.khronos.org/registry/webgl/extensions/WEBGL_lose_context/ > > 3) Only the namestrings parsing implements the MOZ prefix, the other code > uses the expected (when the draft is finalized) name convention, is that ok? Yes. This way, the patch to drop the MOZ_ prefix will be minimal. > > 4) In the file content/canvas/src/WebGLContextGL.cpp below line 2086 I added > a case for LOCAL_GL_MAX_TEXTURE_MAX_ANISOTROPY_EXT, is that correct there? > If so, where would I put the constant for > MINVALUE_GL_MAX_TEXTURE_MAX_ANISOTROPY? Don't worry about that. This code is the implementation of the webgl.min_capability_mode preference. If you wanted to worry about that, you'd basically check that is the smallest value for glGetIntegerv(GL_MAX_TEXTURE_MAX_ANISOTROPY_EXT) on any system that supports anisotropic (or check if the specs say something about the minimum legal value), and return that. But it's not worth the hassle. The webgl.disable-extensions preference will take care of disabling all exts, including anisotropic. > > 5) In the file content/canvas/src/WebGLContextGL.cpp below the line 2745 > there would have to be a test for the texture max anisotropy not to be below > 1. Where should the constant for that go? No need to give a symbolic name to that constant if it's going to be always 1 and used at only one place. > How can I make it issue an INVALID_VALUE? call: ErrorInvalidValue("texParameter: message"); you can pass extra args, printf-style. notice that ErrorInvalidValue returns NS_OK which is what you almost always want to return even in case of error (returning anything else than NS_OK here would generate a JS exception). that's why a lot of code in that file just does: return ErrorInvalidValue("...."); > > 6) Is there something that needs to be done for Direct3D or does the > angleproject wrapper take care of that? On Windows, we use the implementation of OpenGL ES 2 provided by ANGLE (on top of D3D9). Nothing special needs to be done here, this is a GLES2 "like any other". I'd be interested to know if it supports the anisotropic extension.

(In reply to Florian Bösch from comment #3) > Forgot to mention, I did run the conformance test suite, but the results > differed at each run (even of the same build). Sometimes a page timed out, > othertimes tests would seem to have been updated in the space of a few > minutes. The different builds performed largely the same on the conformance > tests. Was this running the mochitest, or the test from khronos.org? Which pages failed or timed out? It's possible that you ran into OOM conditions (some test pages are abnormally intensive especially the quickCheckAPI* pages which fuzz all WebGL funcs). The mochitest is much more resilient as it forces a GC run after every test page. > > I've not yet written a conformance test for this patch, but I intend to do > it once I have a final version. Wonderful. The conformance test should go under conformance/extensions. > > I did write a testpage for the patch, which can be found there: > http://codeflow.org/webgl/anisotropy (it should look like this > http://codeflow.org/pictures/anisotropy.jpg) Great demo! (the image. the page gives me a error 404)

One remark: currently the only GL extension string you're checking for is "GL_EXT_texture_filter_anisotropic". Isn't there a different string for OpenGL ES? You can call gl->IsGLES2() to determine whether you're on ES (as opposed to desktop GL).

> I think it should be MOZ_EXT_ . Changed to MOZ_EXT_ > Don't worry about that. This code is the implementation of the webgl.min_capability_mode preference. Removed this case > No need to give a symbolic name to that constant if it's going to be always 1 and used at only one place. Added a test for the int and float param > call: ErrorInvalidValue("texParameter: message"); > you can pass extra args, printf-style. Added a boolean paramValueValid and a conditional branching on that to return ErrorInvalidValue > Was this running the mochitest, or the test from khronos.org? The test was from khronos.org > Which pages failed or timed out? It seemed random to me > (the image. the page gives me a error 404) Fixed > One remark: currently the only GL extension string you're checking for is "GL_EXT_texture_filter_anisotropic". Isn't there a different string for OpenGL ES? The extensions are defined there: - http://www.khronos.org/registry/gles/extensions/EXT/texture_filter_anisotropic.txt - http://www.opengl.org/registry/specs/EXT/texture_filter_anisotropic.txt Both define the name string as "GL_EXT_texture_filter_anisotropic" Other changes: - added a conformance suite - fixed handling in case of extension not supported Tested the conformance suite with mochi test: TEST_PATH=content/canvas/test/webgl/test_webgl_conformance_test_suite.html make -C obj-i686-pc-linux-gnu mochitest-plain Run without patch ----------------- Passed: 318 Failed: 6 Todo: 0 failed | Test failed, "contextA.getUniform(programS, locationSx) should be 3. Was 0." (URL: conformance/uniforms/uniform-location.html) failed | Test expected to fail, but passed: conformance/misc/uninitialized-test.html failed | Test expected to fail, but passed: conformance/programs/gl-get-active-attribute.html failed | Test expected to fail, but passed: conformance/uniforms/gl-uniform-bool.html failed | Test expected to fail, but passed: conformance/more/functions/uniformfArrayLen1.html failed | Test expected to fail, but passed: conformance/renderbuffers/framebuffer-object-attachment.html Run with patch and no patch conformance --------------------------------------- Passed: 318 Failed: 6 Todo: 0 failed | Test failed, "contextA.getUniform(programS, locationSx) should be 3. Was 0." (URL: conformance/uniforms/uniform-location.html) failed | Test expected to fail, but passed: conformance/misc/uninitialized-test.html failed | Test expected to fail, but passed: conformance/programs/gl-get-active-attribute.html failed | Test expected to fail, but passed: conformance/uniforms/gl-uniform-bool.html failed | Test expected to fail, but passed: conformance/more/functions/uniformfArrayLen1.html failed | Test expected to fail, but passed: conformance/renderbuffers/framebuffer-object-attachment.html Run with patch and patch conformance ------------------------------------ Passed: 319 Failed: 6 Todo: 0 failed | Test failed, "contextA.getUniform(programS, locationSx) should be 3. Was 0." (URL: conformance/uniforms/uniform-location.html) failed | Test expected to fail, but passed: conformance/misc/uninitialized-test.html failed | Test expected to fail, but passed: conformance/programs/gl-get-active-attribute.html failed | Test expected to fail, but passed: conformance/uniforms/gl-uniform-bool.html failed | Test expected to fail, but passed: conformance/more/functions/uniformfArrayLen1.html failed | Test expected to fail, but passed: conformance/renderbuffers/framebuffer-object-attachment.html

Tryserver run (results will come in over the next few hours): https://tbpl.mozilla.org/?tree=Try&rev=39c823803ee3

Added this ticket to angleproject to get Direct3D support for it http://code.google.com/p/angleproject/issues/detail?id=297

Comment on attachment 599167 [details] [diff] [review] revised patch based on feedback and tests Review of attachment 599167 [details] [diff] [review]: ----------------------------------------------------------------- Excellent patch, basically good to land as-is, but we need a quick second round, see comments below. In particular: - copyright yourself on the conformance test - some parts of your patch follow closely some existing code, that's not a great model to follow: please fix it as explained below. - a style nit ::: content/canvas/src/WebGLContext.cpp @@ +955,5 @@ > // We always support this extension. > isSupported = true; > break; > + case WebGL_EXT_texture_filter_anisotropic: > + MakeContextCurrent(); In fact, this MakeContextCurrent is not needed here (because the calls here are just looking up already computed values), and the above OES_texture_float case is also wrong to do it. Can you remove both? @@ +1441,5 @@ > +NS_INTERFACE_MAP_BEGIN(WebGLExtensionTextureFilterAnisotropic) > + NS_INTERFACE_MAP_ENTRY(nsIWebGLExtensionTextureFilterAnisotropic) > + NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, WebGLExtension) > + NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(WebGLExtensionTextureFilterAnisotropic) > +NS_INTERFACE_MAP_END_INHERITING(WebGLExtension) You know what, since you added a .cpp file for this extension, please move that goop there. We should already have split those huge files a long time ago. ::: content/canvas/src/WebGLContextGL.cpp @@ +2216,5 @@ > + gl->fGetFloatv(pname, &f); > + wrval->SetAsFloat(f); > + } > + else > + return ErrorInvalidEnum("getParameter: parameter", pname); Style nit: when there are {} after the if, there should also be {} after the else. Like this: if (a) { x; } else { y; } @@ +2763,5 @@ > + return ErrorInvalidValue("texParameteri: pname %x and param %x (decimal %d) is invalid", > + pname, intParam, intParam); > + else > + return ErrorInvalidValue("texParameterf: pname %x and floating-point param %e is invalid", > + pname, floatParam); Let's take this occasion to fix the current code: "floating-point" is redundant in this message and %g is a better choice than %e (more readable for usual magnitudes). Please fix this in the current code above, as well as in your new code. ::: content/canvas/src/WebGLContextNotSupported.cpp @@ +55,4 @@ > DOMCI_DATA(WebGLActiveInfo, void) > DOMCI_DATA(WebGLExtension, void) > DOMCI_DATA(WebGLExtensionStandardDerivatives, void) > +DOMCI_DATA(WebGLExtensionTextureFilterAnisotropic, void) Good catch! I forgot to mention this. ::: content/canvas/src/WebGLExtensions.h @@ +43,4 @@ > > class WebGLExtensionLoseContext; > class WebGLExtensionStandardDerivatives; > +class WebGLExtensionTextureFilterAnisotropic; Wow, these forward-declarations seem completely useless! Please remove them. ::: content/canvas/test/webgl/conformance/extensions/moz-ext-texture-filter-anisotropic.html @@ +1,2 @@ > +<!-- > +Copyright (c) 2011 The Chromium Authors. All rights reserved. Copyright Florian :-) @@ +6,5 @@ > +<!DOCTYPE html> > +<html> > +<head> > +<meta charset="utf-8"> > +<title>WebGL MOZ_EXT_texture_filter_anisotropic Conformance Tests</title> No need to put MOZ_ in the title. The prefix is only wanted in the extension string, nowhere else, to minimize the overhead it causes. @@ +19,5 @@ > +<canvas id="canvas" style="width: 50px; height: 50px;"> </canvas> > +<div id="console"></div> > + > +<script> > +description("This test verifies the functionality of the MOZ_EXT_texture_filter_anisotropic extension, if it is available."); Same, and again several occurences below.

(In reply to Benoit Jacob [:bjacob] from comment #11) > In fact, this MakeContextCurrent is not needed here (because the calls here > are just looking up already computed values), and the above > OES_texture_float case is also wrong to do it. Can you remove both? Removed > @@ +1441,5 @@ > > +NS_INTERFACE_MAP_BEGIN(WebGLExtensionTextureFilterAnisotropic) > > + NS_INTERFACE_MAP_ENTRY(nsIWebGLExtensionTextureFilterAnisotropic) > > + NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, WebGLExtension) > > + NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(WebGLExtensionTextureFilterAnisotropic) > > +NS_INTERFACE_MAP_END_INHERITING(WebGLExtension) This goop moved to the cpp file. > Style nit: when there are {} after the if, there should also be {} after the > else. Like this: > > if (a) { > x; > } else { > y; > } Adjusted to fit the desired style. > Let's take this occasion to fix the current code: "floating-point" is > redundant in this message and %g is a better choice than %e (more readable > for usual magnitudes). Please fix this in the current code above, as well as > in your new code. Deleted "floating-point" and replaces %e with %g > Wow, these forward-declarations seem completely useless! Please remove them. Removed the forward-declarations. > Copyright Florian :-) Corrected copyright in the testcase and in the new cpp file. > No need to put MOZ_ in the title. The prefix is only wanted in the extension > string, nowhere else, to minimize the overhead it causes. Renamed the conformance test to ext-texture-filter-anisotropic.html, replaced all occurences of MOZ_EXT with EXT except for the getExtension call and the test in the list of getSupportedExtensions. Ran the conformance test suite: Passed: 319 Failed: 6 Todo: 0 failed | Test failed, "contextA.getUniform(programS, locationSx) should be 3. Was 0." (URL: conformance/uniforms/uniform-location.html) failed | Test expected to fail, but passed: conformance/misc/uninitialized-test.html failed | Test expected to fail, but passed: conformance/programs/gl-get-active-attribute.html failed | Test expected to fail, but passed: conformance/uniforms/gl-uniform-bool.html failed | Test expected to fail, but passed: conformance/more/functions/uniformfArrayLen1.html failed | Test expected to fail, but passed: conformance/renderbuffers/framebuffer-object-attachment.html

Comment on attachment 599945 [details] [diff] [review] revised patch based on review Review of attachment 599945 [details] [diff] [review]: ----------------------------------------------------------------- Perfect! and your demo runs fine here on Linux "radeon" driver.

Due to missing EXT suffix in conformance with other extensions the enumerants should optain the suffix and the conformance test should be adjusted.

Added the _EXT suffix to the enumerants MAX_TEXTURE_MAX_ANISOTROPY and TEXTURE_MAX_ANISOTROPY. Fixed the conformance test and added additional handling of vendor prefixes or the canonical extension name.

Use the new MPL header: http://www.mozilla.org/MPL/headers/ for the file you added; that also avoids the errors you had in the MPL1.1 boilerplate.

Started with some documentation there: https://developer.mozilla.org/en/WebGL/Using_Extensions

I've added the version compatibility to https://developer.mozilla.org/en/WebGL and added a small section on how to use this extension in https://developer.mozilla.org/en/WebGL/Using_Extensions

Thanks for the docs, guys! I've listed this on Firefox 13 for developers. You guys rock!