Closed Bug 630672 Opened 13 years ago Closed 13 years ago

implement WebGL OES_texture_float extension

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6
Tracking Status
firefox6 - ---
blocking2.0 --- .x+

People

(Reporter: vlad, Assigned: vlad)

References

Details

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

Attachments

(4 files, 1 obsolete file)

Attached patch implement OES_texture_float (obsolete) — Splinter Review
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.
Attached patch v2, fixedSplinter Review
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.
Assignee: nobody → vladimir
Attachment #508883 - Attachment is obsolete: true
Attached patch v2.0001Splinter Review
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?
I'd like to suggest that this is important. See for example http://codeflow.org/entries/2011/apr/16/webgl-capabilities-analyzed/
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.
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.
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.
Actually, the test hasn't change, it just happens to pass on my machine. I wonder why it doesn't pass on yours :-/
I didn't try locally, I saw that error on tryserver. No idea why it would happen there but not on your machine...
OK, and GetExtension() does not actually return NULL for OES_texture_float; it's just the comments there that are really confusing.
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.
Can you give me the tryserver link?
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 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.
Attachment #525256 - Flags: review+
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.
(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.
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
OK, this failure is intermittent and if I register this test multiple times (00_test_list.txt) I can reproduce it here on Linux.
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.
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.
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.
(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.
(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.
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.
(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).
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
Attachment #531325 - Flags: review?(joe)
Green on try, only need review.
Comment on attachment 531325 [details] [diff] [review]
fixes (pass test, break reference cycle, fix leak)

Review of attachment 531325 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #531325 - Flags: review?(joe) → review+
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?
Keywords: checkin-needed
Yes; I plan to land these today.
Target Milestone: --- → mozilla6
Depends on: 670260
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
(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).
No longer blocks: webgl-ext
Whiteboard: webgl-extension
Blocks: 912280
You need to log in before you can comment on or make changes to this bug.