Last Comment Bug 630672 - implement WebGL OES_texture_float extension
: implement WebGL OES_texture_float extension
Status: RESOLVED FIXED
webgl-extension
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
:
Mentors:
Depends on: 669013 670260
Blocks: 683216 912280
  Show dependency treegraph
 
Reported: 2011-02-01 13:58 PST by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2013-09-03 16:22 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
.x+


Attachments
implement OES_texture_float (35.53 KB, patch)
2011-02-01 13:58 PST, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details | Diff | Review
v2, fixed (36.71 KB, patch)
2011-02-03 17:02 PST, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details | Diff | Review
v2.0001 (53.92 KB, patch)
2011-04-11 18:21 PDT, Alon Zakai (:azakai)
jacob.benoit.1: review+
Details | Diff | Review
rebased again (54.80 KB, patch)
2011-05-10 07:19 PDT, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review+
Details | Diff | Review
fixes (pass test, break reference cycle, fix leak) (4.37 KB, patch)
2011-05-10 07:36 PDT, Benoit Jacob [:bjacob] (mostly away)
joe: review+
Details | Diff | Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2011-02-01 13:58:25 PST
Created attachment 508883 [details] [diff] [review]
implement OES_texture_float

We should add this in to .x; the patch passes tests with ANGLE, but work for some reason with native GL on windows.  It also sets up some framework for future extension implementations.
Comment 1 Vladimir Vukicevic [:vlad] [:vladv] 2011-02-03 17:02:06 PST
Created attachment 509645 [details] [diff] [review]
v2, fixed

Desktop GL requires the floating point internal formats (RGBA32F etc.) whereas ES doesn't (and requires that format == internalformat, thus GL_RGBA).  This fixes things with both ANGLE and desktop GL.
Comment 2 Alon Zakai (:azakai) 2011-04-11 18:21:02 PDT
Created attachment 525256 [details] [diff] [review]
v2.0001

I updated the v2 patch to latest m-c and fixed some minor compilation errors. Works great! I was able to run this demo

http://codeflow.org/entries/2011/apr/11/advanced-webgl-part-1/demo/

which requires OES_texture_float.

vlad, what is the next step for this bug?
Comment 3 Alon Zakai (:azakai) 2011-04-15 16:48:27 PDT
I'd like to suggest that this is important. See for example http://codeflow.org/entries/2011/apr/16/webgl-capabilities-analyzed/
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-04-16 15:57:11 PDT
I would be wary of giving too much weight to a moronic blog post. This post is basically a zillion charts to say what really just amounts to "oh noes float textures and usage of textures in vertex shaders are not standard features". This kind of post is exactly what makes me wary of adding _any_ extension to WebGL.

That said, yes, given that WebGL extensions exist, we pretty much have to implement them, and float textures are indeed the most important one. Will have a look at Vlad's patches ASAP.
Comment 5 Alon Zakai (:azakai) 2011-04-16 16:14:33 PDT
Meanwhile I pushed the patch to try:

1. There is a leak (mochitest-1), I suspect that WebGLExtension is in an undetected cycle with the context.

2. The WebGL conformance suite fails with

WebGL mochitest: starting page conformance/oes-texture-float.html
40489 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | Test failed, "gl.getExtension("OES_texture_float").myProperty should be 2 (of type number). Was undefined (of type undefined)." (URL: conformance/oes-texture-float.html)
WebGL test error: test page failure: conformance/oes-texture-float.html

in the part of the test regarding "Testing that getExtension() returns the same object each time". I suspect the problem is that we return an xpconnect wrapped C++ object, so it is a different object each time, not sure how we fix such things. But this is the last test in that file, so I think otherwise it would pass ok.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2011-04-16 16:40:13 PDT
About the WebGL conformance suite failure, this is because this patch implements GetExtension() in such a way that for OES_texture_float it returns the null object; the test tried adding a property to this object, which fails because one can't add properties to the null object. In order to pass this test, we can never return null objects for WebGL extensions.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2011-04-16 16:43:43 PDT
Wait! This test has been fixed upstream. The upstream test passes:
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/oes-texture-float.html
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-04-16 16:47:36 PDT
Actually, the test hasn't change, it just happens to pass on my machine. I wonder why it doesn't pass on yours :-/
Comment 9 Alon Zakai (:azakai) 2011-04-16 16:51:31 PDT
I didn't try locally, I saw that error on tryserver. No idea why it would happen there but not on your machine...
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2011-04-16 16:55:19 PDT
OK, and GetExtension() does not actually return NULL for OES_texture_float; it's just the comments there that are really confusing.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2011-04-16 16:57:06 PDT
The reason might be that the test slaves do not support float textures (they have NVIDIA Geforce 9400 cards). Another part of the joys of hardware-dependent Web APIs :-/ will emulate locally this behavior.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2011-04-16 17:00:55 PDT
Can you give me the tryserver link?
Comment 13 Alon Zakai (:azakai) 2011-04-16 17:04:58 PDT
Sure, tryserver rev is

http://tbpl.mozilla.org/?tree=MozillaTry&rev=c18de6fff118

and log for the failed run (only happened on win opt) is

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1302923128.1302923894.11990.gz

(there is another error before that in that log, looks like a random orange though).
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2011-04-16 20:11:32 PDT
Comment on attachment 525256 [details] [diff] [review]
v2.0001

R+, modulo fixing the errors found on tryserver, and also the following makes me sad:

> -    PRBool HasES2Compatibility() const {
> +    PRBool HasES2Compatibility() {

We already have many const-correctness issues, so let's not make them worse. Will address that in a follow-up patch.
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2011-04-16 21:37:50 PDT
New tryserver push:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=2dab2a5d5c12

I have broken the ref cycle between WebGLContext and WebGLExtension, by letting WebGLExtension inherit WebGLContextBoundObject instead of storing a refptr to the WebGLContext. Need to check that it's conformant behavior (the WebGLExtension does no longer keep the WebGLExtension alive) but that's what we do for all other kinds of WebGL objects. This solves the XPCOM_MEM_LEAK_LOG errors that your tryserver found in debug builds.

The other thing I put in this tryserver push is a lot of debug output in GetExtension so that we can hopefully figure the error on Win opt.
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2011-04-17 01:17:54 PDT
(In reply to comment #4)
> I would be wary of giving too much weight to a moronic blog post. This post is
> basically a zillion charts to say what really just amounts to "oh noes float
> textures and usage of textures in vertex shaders are not standard features".

(After discussion with the blog's author) OK, that wasn't constructive. The constructive version of that is the following:

 * float textures are only implemented in Chrome at the moment
 * usage of textures in vertex shaders is not supported in ANGLE at the moment, which is the default WebGL renderer on Windows in both Firefox and Chrome. More generally it's not a standard feature in OpenGL ES2.

Therefore, a WebGL page using both these features can only work in Chrome/Linux and Chrome/Mac at the moment.
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2011-04-17 02:02:11 PDT
The earlier tryserver build shows that the leak is fixed.

New tryserver build to understand the Win Opt failure:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=feccdd366b06
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2011-04-17 07:39:19 PDT
OK, this failure is intermittent and if I register this test multiple times (00_test_list.txt) I can reproduce it here on Linux.
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2011-04-18 06:00:20 PDT
The failure seems to be that the test does basically

  var obj = webgl.GetExtension("OES_texture_float");
  obj.someProperty = 123;
  var obj2 = webgl.GetExtension("OES_texture_float");
  assert (obj == obj2);

I.e. it expects that even if it injects a new property into the object, the next GetExtension call will still return that same object the next time, with the added property. I need to ask JS people what they think of that and how to implement that behavior in the JSAPI.
Comment 20 Vladimir Vukicevic [:vlad] [:vladv] 2011-04-18 07:05:24 PDT
Oh -- that's a general extension issue.  The JS-side object is being destroyed, even though the native/xpcom object is the same.  I had intended to remove that part from the test, because I don't think it's useful, but I think the fix should be simple -- mrbkap should know more.
Comment 21 Jason Orendorff [:jorendorff] 2011-04-19 20:41:43 PDT
Benoit and I looked briefly at this yesterday. He has confirmed in the debugger that the GetExtension method is storing the same C++ pointer in *retval each time. This should cause the same JS object to be created, as far as I know. The `obj.someProperty = 123` is irrelevant, since obj is live.

I don't know why this wouldn't work. Paging drbkap.
Comment 22 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-04-22 14:49:22 PDT
(In reply to comment #19)
> The failure seems to be that the test does basically
> 
>   var obj = webgl.GetExtension("OES_texture_float");
>   obj.someProperty = 123;

Looking through the tree, this seems to be an oversimplification of the test. The lines in question are:

gl.getExtension("OES_standard_derivatives").myProperty = 2;

and

shouldBe('gl.getExtension("OES_standard_derivatives").myProperty', '2');

with a forced GC in between.

So, what's happening is that the wrapper (with the expando) is getting collected and then recreated the second time the extension object is being exposed to JS.

It isn't terribly easy to fix this, but the DOM already has to do something like this for DOM nodes (see nsContentUtils::PreserveWrapper). It isn't a drop-in fix since there's a couple of interfaces you have to implement, and some other small things to get right to avoid leaks, but that might be your best bet.
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2011-04-24 08:47:00 PDT
(In reply to comment #22)
> (In reply to comment #19)
> > The failure seems to be that the test does basically
> > 
> >   var obj = webgl.GetExtension("OES_texture_float");
> >   obj.someProperty = 123;
> 
> Looking through the tree, this seems to be an oversimplification of the test.

Yes, sorry, I realized that afterward.

> So, what's happening is that the wrapper (with the expando) is getting
> collected and then recreated the second time the extension object is being
> exposed to JS.
> 
> It isn't terribly easy to fix this, but the DOM already has to do something
> like this for DOM nodes (see nsContentUtils::PreserveWrapper).

Thanks for the pointer, will try.
Comment 24 Asa Dotzler [:asa] 2011-05-06 13:24:56 PDT
tracking-firefox flags aren't for suggesting something is important. If you think it's important, work with the leads of that module to get it prioritized.
Comment 25 Alon Zakai (:azakai) 2011-05-09 15:16:16 PDT
(In reply to comment #22)
> It isn't terribly easy to fix this, but the DOM already has to do something
> like this for DOM nodes (see nsContentUtils::PreserveWrapper). It isn't a
> drop-in fix since there's a couple of interfaces you have to implement, and
> some other small things to get right to avoid leaks, but that might be your
> best bet.

Given this, I recommend that we remove the problematic part of the test, as vlad said he intended to do anyhow (writing a bunch of code to make the test pass has the cost of additional code complexity, and I don't see much of an benefit to justify that).
Comment 26 Benoit Jacob [:bjacob] (mostly away) 2011-05-10 07:19:42 PDT
Created attachment 531321 [details] [diff] [review]
rebased again
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2011-05-10 07:36:46 PDT
Created attachment 531325 [details] [diff] [review]
fixes (pass test, break reference cycle, fix leak)

Here's the follow-up patch fixing the above-discussed problems:
 * break the ref cycle reported in comment 5, see comment 15 for an explanation.
 * store the extensions in a plain array of pointer, instead of a nsTArray of pointers, since their number is known at compile time, and is currently equal to 1.
 * disable the part of the conformance test that requires same-objects-even-if-modified-in-between semantics. Wrote to the WebGL list about that:
https://www.khronos.org/webgl/public-mailing-list/archives/1105/msg00038.html

TBPL:
http://tbpl.mozilla.org/?tree=Try&rev=c642268c80ee
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2011-05-10 16:43:11 PDT
Green on try, only need review.
Comment 29 Joe Drew (not getting mail) 2011-05-18 08:44:44 PDT
Comment on attachment 531325 [details] [diff] [review]
fixes (pass test, break reference cycle, fix leak)

Review of attachment 531325 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 30 Alon Zakai (:azakai) 2011-05-19 16:51:12 PDT
I assume this is ready to land? I'm not sure of the structure of the patches here - are |rebased again| and |fixes| meant to be merged and pushed as one patch?
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2011-05-20 04:47:12 PDT
Yes; I plan to land these today.
Comment 33 Eric Shepherd [:sheppy] 2011-05-31 09:46:37 PDT
Documented:

https://developer.mozilla.org/en/WebGL#Gecko_6.0_notes
Comment 34 Lars Gunther 2011-11-23 16:28:04 PST
On my Nvidia M4000 using the default nouveau drivers on Fedora this does not work. Should I open a bug?

Testing on http://madebyevan.com/webgl-water/ I get the message "This demo requires the OES_texture_float_extension"


  Application Basics

        Name
        Firefox

        Version
        11.0a1

        User Agent
        Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111123 Firefox/11.0a1

        Profile Directory

          Open Containing Folder

        Enabled Plugins

          about:plugins

        Build Configuration

          about:buildconfig

        Crash Reports

          about:crashes

        Memory Use

          about:memory

  Extensions

        Name

        Version

        Enabled

        ID

  Modified Preferences

      Name

      Value

        browser.places.smartBookmarksVersion
        2

        browser.startup.homepage_override.buildID
        20111123031034

        browser.startup.homepage_override.mstone
        rv:11.0a1

        extensions.lastAppVersion
        11.0a1

        network.cookie.prefsMigrated
        true

        places.database.lastMaintenance
        1321614706

        places.history.expiration.transient_current_max_pages
        104858

        privacy.sanitize.migrateFx3Prefs
        true

        security.warn_viewing_mixed
        false

  Graphics

        Adapter Description
        nouveau -- Gallium 0.4 on NVC4

        Driver Version
        2.1 Mesa 7.11

        WebGL Renderer
        nouveau -- Gallium 0.4 on NVC4 -- 2.1 Mesa 7.11

        GPU Accelerated Windows
        0/1
Comment 35 Benoit Jacob [:bjacob] (mostly away) 2011-11-23 17:00:42 PST
(In reply to Lars Gunther from comment #34)
> On my Nvidia M4000 using the default nouveau drivers on Fedora this does not
> work. Should I open a bug?
[SNIP]
>   Graphics
> 
>         Adapter Description
>         nouveau -- Gallium 0.4 on NVC4

Nouveau is known to not work, the developers themselves say so,
  http://nouveau.freedesktop.org/wiki/MesaDrivers
No need to report bug to us about Nouveau; but maybe Nouveau developers might be interested (over at freedesktop.org).

Note You need to log in before you can comment on or make changes to this bug.