Closed
Bug 630672
Opened 13 years ago
Closed 13 years ago
implement WebGL OES_texture_float extension
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: vlad, Assigned: vlad)
References
Details
(Keywords: dev-doc-complete, Whiteboard: webgl-extension)
Attachments
(4 files, 1 obsolete file)
36.71 KB,
patch
|
Details | Diff | Splinter Review | |
53.92 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
54.80 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
4.37 KB,
patch
|
joe
:
review+
|
Details | Diff | 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.
Assignee | ||
Comment 1•13 years ago
|
||
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
Comment 2•13 years ago
|
||
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•13 years ago
|
||
I'd like to suggest that this is important. See for example http://codeflow.org/entries/2011/apr/16/webgl-capabilities-analyzed/
tracking-firefox6:
--- → ?
Comment 4•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
OK, and GetExtension() does not actually return NULL for OES_texture_float; it's just the comments there that are really confusing.
Comment 11•13 years ago
|
||
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•13 years ago
|
||
Can you give me the tryserver link?
Comment 13•13 years ago
|
||
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•13 years ago
|
||
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+
Comment 15•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 20•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
Attachment #531321 -
Flags: review+
Comment 27•13 years ago
|
||
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)
Comment 28•13 years ago
|
||
Green on try, only need review.
Comment 29•13 years ago
|
||
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+
Comment 30•13 years ago
|
||
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
Comment 31•13 years ago
|
||
Yes; I plan to land these today.
Comment 32•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/032e308c10a2 http://hg.mozilla.org/mozilla-central/rev/0ecc706a8b12
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Keywords: checkin-needed → dev-doc-needed
Target Milestone: --- → mozilla6
Comment 33•13 years ago
|
||
Documented: https://developer.mozilla.org/en/WebGL#Gecko_6.0_notes
Keywords: dev-doc-needed → dev-doc-complete
Comment 34•13 years ago
|
||
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•13 years ago
|
||
(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).
You need to log in
before you can comment on or make changes to this bug.
Description
•