738867 - Implement WebGL OES_element_index_uint extension

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Description

[Kelsey Gilbert [:jgilbert]]

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

Comment 1

[Matias Juntunen (:dwarfcrank)]

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?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

(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.

Comment 3

[Jon Buckley]

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

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Jon said he'd do it.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 9

[Alexander Enaldiev]

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.

Comment 10

[Alexander Enaldiev]

Also, almost forgot: Big Tnx about implementations details!

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

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!

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

(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.

Comment 13

[Alexander Enaldiev]

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.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

(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.

Comment 15

[Alexander Enaldiev]

Attachment: OES_element_index_uint extension implementation - first try.

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 16

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 17

[Alexander Enaldiev]

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....

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 19

[Alexander Enaldiev]

Attachment: OES_element_index_uint extension implementation - little fixup.

I hope, this patch would be last.

I want to apologize: again i'm not tested it - still can't build FF.

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

(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 ?

Comment 21

[Alexander Enaldiev]

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

Comment 22

[Alexander Enaldiev]

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.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 24

[Alexander Enaldiev]

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 25

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 686552 [details] [diff] [review]
OES_element_index_uint extension implementation - little fixup.

Yes, this is exactly how to proceed :-)

Comment 26

[Alexander Enaldiev]

Attachment: OES_element_index_uint extension implementation - release candidate :)

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"

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 28

[Alexander Enaldiev]

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.

Comment 29

[Alexander Enaldiev]

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.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

(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.

Comment 31

[Alexander Enaldiev]

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?

Comment 32

[Alexander Enaldiev]

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? ;)

Comment 33

[Alexander Enaldiev]

> 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?

Comment 34

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

(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.

Comment 36

[Alexander Enaldiev]

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).

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

(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.

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

Also, what platform are you testing on?

Comment 39

[Alexander Enaldiev]

I think it's common: Win7x64 :)

Comment 40

[Alexander Enaldiev]

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()' ?

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

(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.

Comment 42

[Alexander Enaldiev]

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 :)

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 44

[Alexander Enaldiev]

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

Comment 45

[Alexander Enaldiev]

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

Comment 46

[Alexander Enaldiev]

Attachment: OES_element_index_uint extension implementation - RC2

Comment 47

[Alexander Enaldiev]

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

Comment 48

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 49

[Alexander Enaldiev]

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.

Comment 50

[Benoit Jacob [:bjacob] (mostly away)]

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/

Comment 51

[Alexander Enaldiev]

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"?

Comment 52

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 53

[Benoit Jacob [:bjacob] (mostly away)]

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?

Comment 54

[Alexander Enaldiev]

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.

Comment 55

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 56

[Alexander Enaldiev]

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?

Comment 57

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 58

[Jon Buckley]

Attachment: Fix bufferSubData

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

Comment 59

[Jon Buckley]

Attachment: 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?

Try run incoming shortly, just waiting for my hg account to get reactivated.

Comment 60

[Benoit Jacob [:bjacob] (mostly away)]

(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.

Comment 61

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 62

[Jon Buckley]

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.

Comment 63

[Josh Matthews [:jdm]]

Screenshot link doesn't work.

Comment 64

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 65

[Jon Buckley]

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 66

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 67

[Jon Buckley]

(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.

Comment 68

[Jon Buckley]

Attachment: Part A - Fix bufferSubData v2

Reverted invalid changes.

Comment 69

[Jon Buckley]

Attachment: Part B - Implement extension v2

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.

Comment 70

[Jon Buckley]

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 71

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 72

[Anthony Pesch]

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

Comment 73

[Jon Buckley]

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...

Comment 74

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 75

[Jon Buckley]

Attachment: Part B - Implement extension v3

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

Comment 76

[Jon Buckley]

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));
  }

Comment 77

[Jon Buckley]

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

Comment 78

[Jon Buckley]

Attachment: Part B - Separate sanity and uint overflow tests

Comment 79

[Jon Buckley]

Attachment: Part C - Implement extension v4

Comment 80

[Jon Buckley]

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

Comment 81

[Jon Buckley]

Attachment: Part B - Separate sanity and uint overflow tests

Comment 82

[Jon Buckley]

Attachment: Part C - Implement extension v5

Try server run: https://tbpl.mozilla.org/?tree=Try&rev=9e7bd1115f73

It actually builds on Windows this time!

Comment 83

[Jon Buckley]

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

woooo tuesdays

Comment 84

[Jon Buckley]

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

tuesdays for everyone

Comment 85

[Benoit Jacob [:bjacob] (mostly away)]

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 86

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 746410 [details] [diff] [review]
Part C - Implement extension v5

Review of attachment 746410 [details] [diff] [review]:
-----------------------------------------------------------------

All good. Thanks!

Comment 87

[Jon Buckley]

Attachment: Part B - Separate sanity and uint overflow tests v2

Carrying forward r+

Comment 88

[Ryan VanderMeulen [:RyanVM]]

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

Comment 89

[Ryan VanderMeulen [:RyanVM]]

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