The extension draft is here: http://www.khronos.org/registry/webgl/extensions/OES_element_index_uint An overview of how WebGL extensions are added is here: https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions

Hi, I'd like to work on this bug. I have a few questions though - the WebGLBuffer class has methods FindMax{Ubyte,Ushort}Element, which are used with DrawElements() to validate the element indices. Should they be accompanied by FindMaxUintElement when calling DrawElements with UNSIGNED_INT as the type? The methods also use two variables, mHasCachedMaxUshortElement and mCachedMaxUshortElement to avoid having to search the max element every time DrawElements is called. Is it acceptable to add Uint versions of the respective variables/methods, even though they may not be used at all if the extension isn't supported? And finally, as the extension is still a draft, should the extension be exposed as MOZ_OES_element_index_uint?

(In reply to Matias Juntunen (:dwarfcrank) from comment #1) > Hi, I'd like to work on this bug. > > I have a few questions though - the WebGLBuffer class has methods > FindMax{Ubyte,Ushort}Element, which are used with DrawElements() to validate > the element indices. Should they be accompanied by FindMaxUintElement when > calling DrawElements with UNSIGNED_INT as the type? Yes. Be very careful not to make a typo when writing this function, as it's critical for security. It is used to check that one is not trying to abuse glDrawElements to access arbitrary video memory. > > The methods also use two variables, mHasCachedMaxUshortElement and > mCachedMaxUshortElement to avoid having to search the max element every time > DrawElements is called. Is it acceptable to add Uint versions of the > respective variables/methods, even though they may not be used at all if the > extension isn't supported? Yes, it's acceptable: the overhead is tiny. The whole design here is not great, I admit. But I'm not very motivated to fix it, because we should eventually switch to a smarter approach: bug 732660. > > And finally, as the extension is still a draft, should the extension be > exposed as MOZ_OES_element_index_uint? Yes, for the extension string. No need to add MOZ_ prefix anywhere else than the string.

Ping :dwarfcrank, are you still working on this bug?

Bug 732660 has landed, so now is a very good time to work on this: the code is now simpler and is not expected to change much soon.

https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions has been updated to describe the new way to expose WebGL extensions.

Jon said he'd do it.

Automatically unassigning mentored bugs that haven't seen action for two weeks. The idea is to maximize the number of mentored bugs that newcomers can pick from.

The key places to change are: * class WebGLBuffer (currently in WebGLContext.h, soon to move to a separate file, see bug 801499) * WebGLContext::BufferData and BufferSubData in WebGLContextGL.cpp * WebGLElementArrayCache.cpp and corresponding .h file Also, no MOZ_ prefix here anymore. The extension has been promoted to community approved status.

If i am right, bug still listed for me. The clock is ticking, but i just wrote some wxamples for OpenGL, read abuot WebGL (OGL in javascript), wrote some basic in WebGL. I googled for info about this bug and my thoughts are about OpenGL ES and it's extensions. There are some nots at GFX team about implementing extensions, so i tried to read .\content\canvas\src\WebGLExtenstions.h Yet haven't clear vision, what it means (physically and, point, semantically), to implement extension. I guess, i didn't meet deadline in 2 weeks, but i still believe, that time isn't gone and i still can try fix it :) P.S. I don't get it clearly, what means "mentored bug". What is the trick? :) Can i ask some stupid advices 4 this bug or WebGL? And if answer is "yes", should i ask in this bug or use my email? P.P.S. This bugzilla comment is written in order to update bug status and also have aim to get feedback about deadline or unassigning or something like that.

Also, almost forgot: Big Tnx about implementations details!

A mentored bug means that at least some Mozilla developer, here jgilbert (but I guess I am co-mentoring this one), will personally take care to answer your questions here. Just ask any implementation question here as a bugzilla comment, and we will try to help you. This bug shouldn't require too much OpenGL knowledge. You just have to understand that drawElements can take indices of various types. By default only UNSIGNED_BYTE (8 bit) and UNSIGNED_SHORT (16 bit) indices are allowed. This extension is about adding support for UNSIGNED_INT (32 bit). If you look at the places mentioned in comment 8 you'll see mentioned of UNSIGNED_BYTE / uint8 and or UNSIGNED_SHORT / uint16. You'll just have to add equivalent code for UNSIGNED_INT / uint32. Oh and I forgot in comment 8: you will also have to extend the unit test, TestWebGLElementArrayCache.cpp, in the same way. The "2 week limit" is not a limit on the amount of time that you have to finish the work. It's just a time limit on mentored bugs that are assigned to someone but where absolutely nothing happens (not even a bugzilla comment). In that case, I just un-assign the bug, but of course the former assignee is still very welcome to fix it!

(In reply to Paladine from comment #9) > I googled for info about this bug and my thoughts are about OpenGL ES and > it's extensions. There are some nots at GFX team about implementing > extensions, so i tried to read .\content\canvas\src\WebGLExtenstions.h > Yet haven't clear vision, what it means (physically and, point, > semantically), to implement extension. Have you read the link given in comment 0: https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions This tells you how to implement a WebGL extension in Mozilla's codebase. Let us know if you have any question.

Yes, i read all commas (even 0 and 8) and even then I haven't clear vision. I agree, may be that makes me "bad reader" or "not too smart reader". There is pretty good link in comma 0, but I think, i shouldn't write code as "mimic" or something like that -- if i don't know, why it's needed or whats behind it. Tnx a lot for explaining and for thoughts about unit test extension. Too sad, the clock to 2 am and i haven't time to fix it now. :) I hope I fix it soon.

(In reply to Paladine from comment #13) > There is pretty good link in comma 0, but I think, i shouldn't write code as > "mimic" or something like that -- if i don't know, why it's needed or whats > behind it. That's a pretty interesting observation. Indeed the current documentation that we have in https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions is largely about mimicking existing code. We should try to expand a little bit on it with more insight into what that code does.

Eyes only. :) I'm not sure about it - i'm not tested it. Something wrong with my mach tool. I don't know how long it takes to fix build problem.

Comment on attachment 685111 [details] [diff] [review] OES_element_index_uint extension implementation - first try. Review of attachment 685111 [details] [diff] [review]: ----------------------------------------------------------------- That's a start, overall going in right direction, I hope the comments below help you complete this work. ::: content/canvas/compiledtest/TestWebGLElementArrayCache.cpp @@ +53,5 @@ > + case 4: return LOCAL_GL_UNSIGNED_INT; > + case 2: return LOCAL_GL_UNSIGNED_SHORT; > + case 1: return LOCAL_GL_UNSIGNED_BYTE; > + default: > + return LOCAL_GL_UNSIGNED_INT; //i'm sure, i'm better throw exception here instead of returning 'somewhat'. Since the default case should never happen, just fail the test there. In this test, you can do VERIFY(false). @@ +127,5 @@ > srand(0); // do not want a random seed here. > > CheckSanity<uint8_t>(); > CheckSanity<uint16_t>(); > + CheckSanity<uint32_t>(); This is not enough, there also is a spot (the main one!) in CheckValidate. A search for uint16 would find it. ::: content/canvas/src/WebGLContext.cpp @@ +941,5 @@ > return true; > case OES_texture_float: > return gl->IsExtensionSupported(gl->IsGLES2() ? GLContext::OES_texture_float > : GLContext::ARB_texture_float); > + case OES_element_index_uint: Indent with spaces, not tabs. @@ +944,5 @@ > : GLContext::ARB_texture_float); > + case OES_element_index_uint: > + /* > + When this extension is enabled: The drawElements entry point parameter type accepts the value UNSIGNED_INT (from http://www.khronos.org/registry/webgl/extensions/OES_element_index_uint/ ) > + */ No need for this comment here. @@ +945,5 @@ > + case OES_element_index_uint: > + /* > + When this extension is enabled: The drawElements entry point parameter type accepts the value UNSIGNED_INT (from http://www.khronos.org/registry/webgl/extensions/OES_element_index_uint/ ) > + */ > + //TODO: return true only if supported. (proposed: static assert, that depends on "is member function with UNSIGNED_INT or no"). Nope, can't be a static assertion as we want the same firefox build to run on all machines. We must detect at runtime whether the underlying OpenGL supports uint elements. See how other extensions here do: e.g. gl->IsExtensionSupported(GLContext::EXT_texture_filter_anisotropic). You may have to extend class GLContext to support checking for the right extensions. This is mentioned in https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions#Exposing_the_Extension @@ +1010,5 @@ > > WebGLExtensionID ext = WebGLExtensionID_unknown_extension; > > // step 1: figure what extension is wanted > + if (CompareWebGLExtensionName(name, "OES_element_index_uint")) Trailing whitespace? Also, it makes the patch simpler to read for me if you add this as an 'else if' at the end. @@ +1046,5 @@ > else if (CompareWebGLExtensionName(name, "WEBGL_debug_renderer_info")) > { > ext = WEBGL_debug_renderer_info; > } > + Trailing whitespace. ::: content/canvas/src/WebGLContextGL.cpp @@ +1476,5 @@ > CheckedUint32 checked_byteCount; > > WebGLsizei first = 0; > > + if (LOCAL_GL_UNSIGNED_INT == type) { in this condition you need && IsExtensionEnabled(...uint extension...) so that you only allow UNSIGNED_INT when the extension is enabled. We must not allow using a feature provided by an extension unless the user has enabled it by calling getExtension (in this respect WebGL extensions differ from OpenGL extensions which can be used readily). @@ +1477,5 @@ > > WebGLsizei first = 0; > > + if (LOCAL_GL_UNSIGNED_INT == type) { > + checked_byteCount = 4 * CheckedUint32(count); // 4 on all platforms? TODO: remove hardcode in this if-else: '4', '2' and so on. A OpenGL UNSIGNED_INT is always 32bits == 4 bytes, contrary to a C "unsigned int" which may be != 4 bytes. ::: content/canvas/src/WebGLElementArrayCache.cpp @@ +553,4 @@ > return aMallocSizeOf(this) + > mByteSize + > uint8TreeSize + > uint16TreeSize; You're missing a + uint32TreeSize here. ::: content/canvas/src/WebGLExtensionElementIndexUint.cpp @@ +9,5 @@ > + > + using namespace mozilla; > + > + WebGLExtensionElementIndexUint::WebGLExtensionElementIndexUint(WebGLContext* context) > + : WebGLExtensionBase(context) Trailing whitespace at lines 5, 9, 11, 13, 22 ::: content/canvas/src/WebGLExtensions.h @@ +91,5 @@ > +{ > +public: > + WebGLExtensionElementIndexUint(WebGLContext*); > + virtual ~WebGLExtensionElementIndexUint(); > + Trailing whitespace

Big tnx! So quickly reviewed! Btw :) >> >> WebGLExtensionID ext = WebGLExtensionID_unknown_extension; >> >> // step 1: figure what extension is wanted >> + if (CompareWebGLExtensionName(name, "OES_element_index_uint")) > >Trailing whitespace? Also, it makes the patch simpler to read for me if you add >this as an 'else if' at the end. Thought about alphabetical order....

I would approve a _separate_ patch fixing alphabetic order in all these places where we list extensions.

I hope, this patch would be last. I want to apologize: again i'm not tested it - still can't build FF.

(In reply to Paladine from comment #19) > I want to apologize: again i'm not tested it - still can't build FF. Oh. Realisically we need to solve this problem before we can go further. Not being able to build will make it exceedingly hard for you to make good patchs. What issue is preventing you from building Firefox? Do you have a hardware problem or just trouble with the documentation? Have you read https://developer.mozilla.org/en-US/docs/Simple_Firefox_build ?

y, and there were days, when i successfully builded FF. I got latest changes from mozilla-central, typed `./mach build`. Build lasted for 50+ minutes first time. For my own interest, i started build again - it lasted 12+ minutes. https://plus.google.com/photos/100955993203612668293/albums/5816241965125388929?authkey=CNqEwZTx6pPN3QE

I'm reinstalled tool collection `mozilla-build` several times. Then i cleaned all instances. And reinstalled to c:\mozilla-build, after added this path to system path. I apologize, coz i didn't read mach trobleshoot FAQ.

If mach doesn't work, you can also use make/pymake (pymake on Windows), after all it's still what a majority of people use.

U were right after all. I found typo error in bindings.conf, what caused build issue. Soon i'll go home and then after building ft debug (to read loaded extensions list) and after test execution i'll write something here and ask u to review again, ok? p.s. I hope, this is good plan. :)

Comment on attachment 686552 [details] [diff] [review] OES_element_index_uint extension implementation - little fixup. Yes, this is exactly how to proceed :-)

The build is ok and the tests are green, but one thing bothers me: i don't see extension load results in cerr. How i can figure out, is there debug configuration of firefox.exe? P.S. .mozconfig content is "ac_add_options --enable-debug"

WebGL extensions aren't shown on cerr. The best thing you can do is run the conformance test, https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/oes-element-index-uint.html and you can also go to http://webglreport.sf.net which gives a list of supported extensions.

If i'm right, GLContext.cpp:329, GLContext.cpp:557, GLContext.h:1353, GLContext.h:1368 - is answer 'why i expect them on cerr'. P.S. Thanks. I check the links.

I think, i failed again: This test verifies the functionality of the OES_element_index_uint extension, if it is available. On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". PASS WebGL context exists PASS No OES_element_index_uint support -- this is legal PASS OES_element_index_uint not listed as supported and getExtension failed -- this is legal PASS successfullyParsed is true TEST COMPLETE P.S. 'No extension support', 'getExtension failed' -- but test is green at these conditions. I think, there is bug in the test.

(In reply to Paladine from comment #28) > If i'm right, GLContext.cpp:329, GLContext.cpp:557, GLContext.h:1353, > GLContext.h:1368 - is answer 'why i expect them on cerr'. > > P.S. Thanks. I check the links. That only tells you that the OpenGL extension was found; it says nothing about WebGL. (In reply to Paladine from comment #29) > I think, i failed again: > > This test verifies the functionality of the OES_element_index_uint > extension, if it is available. > > On success, you will see a series of "PASS" messages, followed by "TEST > COMPLETE". > > PASS WebGL context exists > PASS No OES_element_index_uint support -- this is legal > PASS OES_element_index_uint not listed as supported and getExtension failed > -- this is legal > > PASS successfullyParsed is true > > TEST COMPLETE > > P.S. 'No extension support', 'getExtension failed' -- but test is green at > these conditions. I think, there is bug in the test. No. That means that the browser does not actually support the WebGL extension.

I didn't get it. 1) bool WebGLContext::IsExtensionSupported(...) { // ... switch(ext) { // ... case OES_element_index_uint: reutrun gl->IsExtensionSupported(GLContext::OES_element_index_uint); // ... Semantically - i think - this code means (if it is correct), that if OpenGL supports this extension, then WebGL too? Or not?

2) So if i will log all available OpenGL extensions, i will not get list of supported WebGL extensions? May be stupid question, but how i can get available WebGL extensions? Some kind of black magic? ;)

> No. That means that the browser does not actually support the WebGL extension. This is test agains the extension. Why it's green if it's not supported?

OK, I see the problem. On desktop-OpenGL (that is, not OpenGL ES), element_index_uint support is always available, it does not depend on an extension. GL_OES_element_index_uint is only on OpenGL ES. So you want to do a check for !gl->IsGLES2() in WebGLContext::IsExtensionSupported.

(In reply to Paladine from comment #32) > 2) > So if i will log all available OpenGL extensions, i will not get list of > supported WebGL extensions? Correct. > > May be stupid question, but how i can get available WebGL extensions? Some > kind of black magic? ;) This can be done by a script, for example, go to http://webglreport.sf.net/ , it gives the list at the bottom. (In reply to Paladine from comment #31) > Semantically - i think - this code means (if it is correct), that if OpenGL > supports this extension, then WebGL too? Or not? Yep, but see comment 34 for why that isn't the right logic here. Sorry I didn't think about that earlier. (In reply to Paladine from comment #33) > > No. That means that the browser does not actually support the WebGL extension. > > This is test agains the extension. > > Why it's green if it's not supported? Because it is legal for a WebGL implementation to not support an extension, as long as it does not claim to support it.

I'm right? 'case OES_element_index_uint: return gl->IsExtensionSupported(GLContext::OES_element_index_uint);' to 'case OES_element_index_uint: return gl->IsGLES2() && gl->IsExtensionSupported(GLContext::OES_element_index_uint);' ? I.e. GLContext class -- any GLContext? it can be desktop-OpenGL or OpenGL ES - for example, with probability 0.5 ? So i check 'if this is ES and extension is supported => return true'. 4 test: if i'm right - that test is against only this extension. So test says: 'current WebGL impl doesn't support it => it's not testable => OK' ? :) i'm just not sure, that 'if it cannot be tested' => OK :) Coz it's not common test, it's specialized (if i'm right).

(In reply to Paladine from comment #36) > I'm right? > > 'case OES_element_index_uint: > return > gl->IsExtensionSupported(GLContext::OES_element_index_uint);' > > to > > 'case OES_element_index_uint: > return gl->IsGLES2() && > gl->IsExtensionSupported(GLContext::OES_element_index_uint);' ? No: that would make us *only* support WebGL OES_element_index_uint on OpenGL ES. We want the opposite. On desktop OpenGL (not OpenGL ES) we unconditionally support this WebGL extension. > 4 test: if i'm right - that test is against only this extension. So test > says: 'current WebGL impl doesn't support it => it's not testable => OK' ? > :) i'm just not sure, that 'if it cannot be tested' => OK :) Coz it's not > common test, it's specialized (if i'm right). The test says: if the browser does not claim to support that extension, that's fine, we leave it alone then.

Also, what platform are you testing on?

I think it's common: Win7x64 :)

If i'm right, u propose something like: 'case OES_element_index_uint: return gl->IsGLES2() ? gl->IsExtensionSupported(GLContext::OES_element_index_uint) : true; ' Right? But i didn't get it again. >No: that would make us *only* support WebGL OES_element_index_uint on OpenGL ES. >We want the opposite. On desktop OpenGL (not OpenGL ES) we unconditionally support >this WebGL extension. The function have signature like 'bool WebGLContext::IsExtensionSupported(...' How it refers to desktop OpenGL? and if not refers, why i must check 'gl->IsGLES2()' ?

(In reply to Paladine from comment #40) > If i'm right, u propose something like: > 'case OES_element_index_uint: > return gl->IsGLES2() ? > gl->IsExtensionSupported(GLContext::OES_element_index_uint) : true; ' > > Right? Exactly. This simplifies as !gl->IsGLES2() || gl->IsExtensionSupported(GLContext::OES_element_index_uint) > But i didn't get it again. So, on Windows, by default, we actually use OpenGL ES (because that is why ANGLE gives us). So it could be that the reason why you are not getting the extension, is completely different. I'd suggest stepping through this in a debugger. Step through mozilla::WebGLContext::GetSupportedExtensions and mozilla::WebGLContext::IsExtensionSupported > > >No: that would make us *only* support WebGL OES_element_index_uint on OpenGL ES. >We want the opposite. On desktop OpenGL (not OpenGL ES) we unconditionally support >this WebGL extension. > > The function have signature like 'bool > WebGLContext::IsExtensionSupported(...' > > How it refers to desktop OpenGL? and if not refers, why i must check > 'gl->IsGLES2()' ? I'm not sure I understand the question. The job of WebGLContext::IsExtensionSupported is to decide whether we can support this WebGL extension. It does this by first checking whether we are on desktop OpenGL or OpenGL ES. On desktop OpenGL, it can simply return true, as it's always supported. On OpenGL ES, it has to check if the OES extension is supported by calling GLContext::IsExtensionSupported.

Ok, i think i know, what to do (1) fixup 2) use debugger). Sorry, may be i just ask wrong/stupid questions. I thought, the only way to use OpenGL in browser is in tab and there is one possibility of OpenGL: WebGL (OpenGL ES). So if i'm right, there are 2 possibilities of OpenGL context: it can be OpenGL ES or OpenGL desktop. I didn't have any thoughts, why i can get OpenGL-desktop and still don't have. This is not game, this is browser.... This is small blind spot for me in all this story :)

WebGL is the only thing that a script in a Web page can use. In order to implement WebGL, the browser itself has two possibilities: either use OpenGL ES or use desktop OpenGL. We use desktop OpenGL on Mac and on desktop Linux We use OpenGL ES on Windows and on Android. On Windows, OpenGL ES is provided by a library, ANGLE, that lives in gfx/angle/ and implements OpenGL ES on top of Direct3D. gl->IsGLES2() returns true if we are on OpenGL ES, false if we are on desktop OpenGL. Yes, please, investigate in a debugger what's happening around this code to explain why you are not getting the extension.

Now it's clear. Tnx a lot! Btw about debugging. It's not needed. I just screwed up. There are test run results for my desktop: http://www.everfall.com/paste/id.php?wh9z6kcrfu8g

There are test run result for my notebook: (same) http://www.everfall.com/paste/id.php?coknrror7ywm

I don't know, why i see some FAIL strings... and i don't know, is it ok or bad or very bad...

Comment on attachment 687544 [details] [diff] [review] OES_element_index_uint extension implementation - RC2 The FAIL are important to look at. I will review the patch once there are no FAIL's anymore. Let's look at the first one: FAIL gl.drawElements(gl.TRIANGLE_STRIP, 4, gl.UNSIGNED_INT, 4) expected: NO_ERROR. Was INVALID_OPERATION. Indeed, that should not generate an error once the OES_element_index_uint extension is enabled. To debug this, set a breakpoint on mozilla::WebGLContext::DrawElements and step through this funtion.

Ok, i will start debugging. Show must go on :) Tnx of test explaining! P.S. I think, this is bad test: it lies. Overall result is green. But feature doesn't work properly.

It is hard to write a test that will _guarantee_ that the feature works properly. If you have an idea of how to improve the test, you are very welcome to submit it to http://www.khronos.org/bugzilla/

I'm don't want call it "improvement" coz it doesn't. I admit, i screwed up again: I thought, this test have overall result, but seems it haven't and overall result is up to "test user" I will think... Btw I didn't understand: we cannot guarantee, that feature actually will work. Sorry, I'm blunt, but then why we're writing a code?) If result cann't be measured - this is wrong aim, right? P.S. Rly find't understand. do u mean "on all platforms and on any hardware"?

I just meant it's hard to write a test that would guarantee that rendering with uint indices works as expected: you would have to render some stuff, readPixels, analyze the rendering... so the tests contents itself with only testing things that are simpler to test. But even if it's hard to test, correct rendering matters because... the user would easily notice if it were wrong.

It's been almost 6 weeks, it'd be nice wrap this up. Do you have a patch that works, or a specific question that I can answer?

You asked, but let me explain my vision: 1. May be i'm wrong, but i thought (and you wrote too 'not needed OpenGL knowledge exactly') this is easy: by analody add tree for uint32_t. So i added and asked 4 review. In the same time i had firefox build issue. I fixed my own issue and launched tests - they 3/20 FAILed or something like that. Now i think, that this is not so easy and can not be done just 'by analogy'. Now debugging takes a lot of my time. This is on the one hand. (objective criteria) I have a patch 'by analogy', it not passes all tests. 2. On the other hand, clock is ticking, job isn't done. I understand. I'm not sure, can u help me with code fixup? (for example, a short time ago i asked for review). What your aims? I imagine this situation in sereral ways. They are: a) i'm screwed up, no same bugs for me, i'm leaving gfx. (coz i'm useless here) b) i'm screwed up, some other newbie bug for me (second chance) c) i will continue debug and fix this bug. (i'd prefer this, but i think, the trouble is question 'what time i need 4 debug?'. i don't know). P.S. Now i'm not sure, what exactly to ask, i need more time to think.

I think all I wrote was "This bug shouldn't require too much OpenGL knowledge." in comment 11. Certainly it requires working a little bit with GL, but not much. It seems that you are most of the way there and just need to do some debugging to understand the test failures. That debugging isn't very GL intensive. You just need to step through the functions you edited, especially WebGLContext::DrawElements and functions it calls, and see what they are doing. This is just generic C++ debugging, and you'll have to do this kind of debugging with any kind of nontrivial Gecko work --- nothing specific to Graphics here. The point of filing this mentored bug was that it would be a learning experience for someone and that it would save me time; so I don't want to debug your patch for you as that would defeat both purposes. I'll just un-assign the bug for now. I'll review the first patch that passes the conformance test.

Sorry, it seems i can't save your time... Now it's unassigned. Should i try to finish? Or i should give up? What do you think?

By all means, if you can debug this, that's great. Unassigning just means that other people are welcome to work on this too --- but you too are still 100% welcome to work on this and it definitely seems like you are just some debugging away from having it working.

Looking at the spec, bufferSubData should be returning INVALID_VALUE instead of INVALID_OPERATION.

I think the condition that checks whether this extension can be enabled could use some work. Maybe some sort of IsDesktopGL() equivalent? Try run incoming shortly, just waiting for my hg account to get reactivated.

(In reply to Jon Buckley [:jbuck] from comment #59) > Created attachment 690170 [details] [diff] [review] > Implement OES_element_index_uint > > I think the condition that checks whether this extension can be enabled > could use some work. Maybe some sort of IsDesktopGL() equivalent? !gl->IsGLES2() as discussed above.

Waiting for the try run before I review; is this Alexander's patch + fixes?

Here it goes: https://tbpl.mozilla.org/?tree=Try&rev=4af4ebaf823c I ran the tests locally on a Intel 3000 and they passed: https://dl.dropbox.com/u/4403845/Screenshots/5p.png It's my patch for both. You'll need both applied for tests to pass.

Screenshot link doesn't work.

Comment on attachment 690169 [details] [diff] [review] Fix bufferSubData Review of attachment 690169 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextGL.cpp @@ +480,5 @@ > if (byteOffset < 0) > return ErrorInvalidValue("bufferSubData: negative offset"); > > if (!boundBuffer) > + return ErrorInvalidValue("bufferData: no buffer bound!"); I disagree with this particular change: we always seem to generate InvalidOperation for this kind of error, and this is alse what the man page says: http://www.khronos.org/opengles/sdk/docs/man/xhtml/glBufferSubData.xml I can't find where this is described in the spec though. But if this change is to satisfy a conformance test, I'd rather fix the conformance test (they're on github, just do a pull request). Such a conformance test would be at odds with all the other conformance tests that we pass, and that must be requiring InvalidOperation on similar calls. @@ +485,4 @@ > > CheckedUint32 checked_neededByteLength = CheckedUint32(byteOffset) + data->Length(); > if (!checked_neededByteLength.isValid()) > + return ErrorInvalidValue("bufferSubData: integer overflow computing the needed byte length"); This change is good. @@ +489,3 @@ > > if (checked_neededByteLength.value() > boundBuffer->ByteLength()) > + return ErrorInvalidValue("bufferSubData: not enough data - operation requires %d bytes, but buffer only has %d bytes", good. @@ +518,5 @@ > if (byteOffset < 0) > return ErrorInvalidValue("bufferSubData: negative offset"); > > if (!boundBuffer) > + return ErrorInvalidValue("bufferSubData: no buffer bound!"); Bad, see above. @@ +523,4 @@ > > CheckedUint32 checked_neededByteLength = CheckedUint32(byteOffset) + data.Length(); > if (!checked_neededByteLength.isValid()) > + return ErrorInvalidValue("bufferSubData: integer overflow computing the needed byte length"); good. @@ +527,3 @@ > > if (checked_neededByteLength.value() > boundBuffer->ByteLength()) > + return ErrorInvalidValue("bufferSubData: not enough data -- operation requires %d bytes, but buffer only has %d bytes", good.

My Dropbox is straining to sync mozilla-central which is why that link isn't working yet... Local testing on an Intel 3000 and an AMD Radeon HD 6770M passes the conformance test with it showing as enabled. The try runs look green too. However, local testing on my windows desktop with a AMD Radeon 6950 shows two fails on that test: Test that client data is always copied during bufferData and bufferSubData calls FAIL gl.drawElements(gl.TRIANGLE_STRIP, 4, gl.UNSIGNED_INT, 4) expected: NO_ERROR. Was INVALID_OPERATION. PASS gl.drawElements(gl.TRIANGLE_STRIP, 4, gl.UNSIGNED_INT, 0) generated expected GL error: INVALID_OPERATION. PASS gl.drawElements(gl.TRIANGLE_STRIP, 4, gl.UNSIGNED_INT, 8) generated expected GL error: INVALID_OPERATION. FAIL gl.drawElements(gl.TRIANGLE_STRIP, 4, gl.UNSIGNED_INT, 4) expected: NO_ERROR. Was INVALID_OPERATION. PASS gl.drawElements(gl.TRIANGLE_STRIP, 4, gl.UNSIGNED_INT, 0) generated expected GL error: INVALID_OPERATION. PASS gl.drawElements(gl.TRIANGLE_STRIP, 4, gl.UNSIGNED_INT, 8) generated expected GL error: INVALID_OPERATION. Let me dig into this...

Comment on attachment 690170 [details] [diff] [review] Implement OES_element_index_uint Review of attachment 690170 [details] [diff] [review]: ----------------------------------------------------------------- Good work, almost good to go, just enough nits to require another pass: ::: content/canvas/compiledtest/TestWebGLElementArrayCache.cpp @@ +47,5 @@ > > template<typename T> > GLenum GLType() > { > + switch(sizeof(T)) Space after switch ::: content/canvas/src/WebGLContext.cpp @@ +945,5 @@ > + } > + else > + { > + return false; > + } Simplify this logic, see above comments. return !GLES2 || isextensionsupported @@ +1368,5 @@ > > nsTArray<nsString>& arr = retval.SetValue(); > > + if (IsExtensionSupported(cx, OES_element_index_uint)) > + arr.AppendElement(NS_LITERAL_STRING("OES_element_index_uint")); Some day, in a separate patch, it would be nice to sort this alphabetically. ::: content/canvas/src/WebGLContextGL.cpp @@ +1493,2 @@ > } else { > + return ErrorInvalidEnum("drawElements: type must be UNSIGNED_SHORT or UNSIGNED_BYTE or UNSIGNED_INT"); This message is going to be confusing if the issue is that the application forgot to getExtension(..._uint). I'd just keep it as-is, so that if people forget to enable the extension, that message will hint them in that direction. ::: content/canvas/src/WebGLElementArrayCache.cpp @@ +108,1 @@ > * every subsequent invalidation will have to invalidate both trees even if one of the two types is never 'both' and 'two' need to be updated here.

(In reply to Benoit Jacob [:bjacob] from comment #64) > Comment on attachment 690169 [details] [diff] [review] > Fix bufferSubData > > Review of attachment 690169 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/canvas/src/WebGLContextGL.cpp > @@ +480,5 @@ > > if (byteOffset < 0) > > return ErrorInvalidValue("bufferSubData: negative offset"); > > > > if (!boundBuffer) > > + return ErrorInvalidValue("bufferData: no buffer bound!"); > > I disagree with this particular change: we always seem to generate > InvalidOperation for this kind of error, and this is alse what the man page > says: > http://www.khronos.org/opengles/sdk/docs/man/xhtml/glBufferSubData.xml > > I can't find where this is described in the spec though. But if this change > is to satisfy a conformance test, I'd rather fix the conformance test > (they're on github, just do a pull request). Such a conformance test would > be at odds with all the other conformance tests that we pass, and that must > be requiring InvalidOperation on similar calls. Ah, my bad. I was reading http://www.khronos.org/registry/webgl/specs/latest/#5.14.5 where it says "If data is null then an INVALID_VALUE error is generated" but that's referring to the data argument, not the bound buffer. I'll revert the two incorrect changes in this patch.

Reverted invalid changes.

Try server build: https://tbpl.mozilla.org/?tree=Try&rev=ca8aca177534 I still need to investigate the cause of those Windows-only conformance test fails.

Before I forget again, the Windows tests are failing on: > ::: content/canvas/src/WebGLContextGL.cpp > @@ 1518,1524 @@ > if (!maxAllowedCount || > !mBoundElementArrayBuffer->Validate(type, maxAllowedCount - 1, first, count)) > { > return ErrorInvalidOperation( > "DrawElements: bound vertex attribute buffers do not have sufficient " > "size for given indices from the bound element array"); > }

Comment on attachment 690259 [details] [diff] [review] Part A - Fix bufferSubData v2 As discussed with Jon: - part of the failures are an issue in the test, for which Jon has a patch; - another failure is apparently a bug with the present patch Will review once there are no failures.

Pinging Benoit / Jon, what's left on this bug? From reading the comments it *seems* it's almost ready?

The patch is ready to go from a review perspective (minus probable bitrotting), but I ran into some test issues with ATI GPU in Windows. Lets see if I can replicate them now that the Web GL tests are updated...

jon, should we proceed to review or do you want to check that ATI issue first?

Yeah, lets get this reviewed. The fixes that I was playing with needed to be done in the conformance tests themselves, not m-c. Try server run: https://tbpl.mozilla.org/?tree=Try&rev=6efb5b22a836

Hm, Windows build failed because of: e:\builds\moz2_slave\try-w32-0000000000000000000000\build\config\rules.mk:851:0$ e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe e:/builds/moz2_slave/try-w32-0000000000000000000000/build/config/expandlibs_exec.py --depend .deps/TestAudioEventTimeline.exe.pp --target TestAudioEventTimeline.exe --uselist -- link -nologo -out:TestAudioEventTimeline.exe -pdb:TestAudioEventTimeline.pdb TestAudioEventTimeline.obj -LARGEADDRESSAWARE -NXCOMPAT -RELEASE -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF -LIBPATH:../../../../dist/lib -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -NODEFAULTLIB:msvcprt -NODEFAULTLIB:msvcprtd -DEFAULTLIB:mozcrt kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/dist/lib/xpcomglue_s.lib e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/dist/lib/xul.lib e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/dist/lib/mozalloc.lib e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/dist/lib/nspr4.lib e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/dist/lib/plc4.lib e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/dist/lib/plds4.lib e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/dist/lib/mozjs.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib TestWebGLElementArrayCache.cpp e:/builds/moz2_slave/try-w32-0000000000000000000000/build/content/canvas/compiledtest/TestWebGLElementArrayCache.cpp(126) : error C2220: warning treated as error - no 'object' file generated e:/builds/moz2_slave/try-w32-0000000000000000000000/build/content/canvas/compiledtest/TestWebGLElementArrayCache.cpp(138) : see reference to function template instantiation 'void CheckSanity<uint32_t>(void)' being compiled e:/builds/moz2_slave/try-w32-0000000000000000000000/build/content/canvas/compiledtest/TestWebGLElementArrayCache.cpp(126) : warning C4307: '+' : integral constant overflow The applicable line in the file is marked with a *: // bug 825205 if (sizeof(T) < sizeof(uint32_t)) { * uint32_t bigValWrappingToZero = uint32_t(T(-1)) + 1; VERIFY(c.Validate(type, bigValWrappingToZero, 0, numElems)); VERIFY(c.Validate(type, bigValWrappingToZero - 1, 0, numElems)); }

Trying a quick hack where I change the uint32_t to uint64_t to see if it builds on Windows, since my own Windows machine doesn't catch this error for some reason: https://tbpl.mozilla.org/?tree=Try&rev=0e894f34edb2

And a full try run: https://tbpl.mozilla.org/?tree=Try&rev=33b6be0f560a

Try server run: https://tbpl.mozilla.org/?tree=Try&rev=9e7bd1115f73 It actually builds on Windows this time!

Comment on attachment 746408 [details] [diff] [review] Part B - Separate sanity and uint overflow tests woooo tuesdays

Comment on attachment 746190 [details] Part C - Implement extension v4 tuesdays for everyone

Comment on attachment 746408 [details] [diff] [review] Part B - Separate sanity and uint overflow tests Review of attachment 746408 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/compiledtest/TestWebGLElementArrayCache.cpp @@ +140,5 @@ > > // bug 825205 > + uint32_t bigValWrappingToZero = uint32_t(T(-1)) + 1; > + VERIFY(c.Validate(type, bigValWrappingToZero, 0, numElems)); > + VERIFY(c.Validate(type, bigValWrappingToZero - 1, 0, numElems)); Can you please add: VERIFY(!c.Validate(type, 0, 0, numElems)); as a basic self-test to verify that this test would catch the bug that it is trying to guard against.

Comment on attachment 746410 [details] [diff] [review] Part C - Implement extension v5 Review of attachment 746410 [details] [diff] [review]: ----------------------------------------------------------------- All good. Thanks!

Carrying forward r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c02e6c6f0d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/e4fad18913b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc9b139557b

https://hg.mozilla.org/mozilla-central/rev/0c02e6c6f0d3 https://hg.mozilla.org/mozilla-central/rev/e4fad18913b3 https://hg.mozilla.org/mozilla-central/rev/6cc9b139557b