745840 - Rework WebGL uniform/attrib setters, remove the huge macros

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Kill these macros:

   OBTAIN_UNIFORM_LOCATION
   SIMPLE_ARRAY_METHOD_NO_COUNT
   SIMPLE_ARRAY_METHOD_UNIFORM
   SIMPLE_MATRIX_METHOD_UNIFORM

etc. Factor out common code in helper functions instead of macros.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Do we want to do this before the new-binding work or after?  I haven't gotten to the methods using this stuff yet, I think....

Comment 2

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

It can wait until after you're done: it's not an emergency, we're busy with other things, and it won't make your work a lot easier by itself.

Comment 3

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

So, anyone willing to take that bug:

the main file to modify here is

http://hg.mozilla.org/mozilla-central/file/tip/content/canvas/src/WebGLContextGL.cpp

In this file, search for the macro nams listed in comment 0. See how these macros are used to generate several of the WebGLContext methods corresponding to WebGL uniform setters (for example WebGLContext::Uniform4fv implements the javascript webgl uniform4fv function). That's ugly. The goal of this bug is to get rid of these huge macros. If common code needs to be factored in a shared helper, that should be a function rather than a macro.

Comment 4

[Peter Panaguiton]

Hi Benoit,

I'll take a look at it and check it out! I'll come and comment here if I'm stuck or anything.

Comment 5

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

Hi Peter, Saurabh expressed interest earlier today -- please check with him. To both of you: do not forget to assign bugs to yourself once you plan to work on them, as a means of letting others know.

It does seem that there is demand for more WebGL mentored bugs :-) Now is an excellent time to start coding on the WebGL implementation, there's lots to be done, but I have fallen behind on filing mentored bugs. Feel free to email me or query me on IRC to get me to file more mentored bugs faster.

Comment 6

[Saurabh Anand [:sawrubh]]

Attachment: Patch v1

Comment 7

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

Comment on attachment 671450 [details] [diff] [review]
Patch v1

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

Please configure your mercurial with this in the [diff] section:

[diff]
git = 1
showfunc = 1
unified = 8

See https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Configuration

::: content/canvas/src/WebGLContext.h
@@ -577,4 @@
>          if (!mWebGLError)
>              mWebGLError = *currentGLError;
>      }
> -    

Please do not make any whitespace changes except in the precise areas that you are otherwise modifying. This makes patches easier to review and rebase and makes hg annotate more usable.

::: content/canvas/src/WebGLContextGL.cpp
@@ +23,4 @@
>  
>  #include "WebGLTexelConversions.h"
>  #include "WebGLValidateStrings.h"
> +#include <string>

We normally use our own string classes (nsString, nsCString) rather than std strings. You would need a very specific reason to.

@@ +3612,5 @@
>  }
>  
> +void
> +WebGLContext::ObtainUniformLocation(const char* info, WebGLUniformLocation *location_object,
> +                                    int& returnFlag)

It seems that returnFlag is only assigned 1 or 0 here, so it should probably be a boolean and be named to something more explicit like "success" or "error". And it would be more intuitive if "true" meant "success".

Also, since the function doesn't already have a return value, why not return this as its return value, instead of taking an output-parameter?

@@ +3626,5 @@
> +    if (mCurrentProgram != location_object->Program())
> +        return ErrorInvalidOperation("%s: this uniform location doesn't correspond to the current program", info);
> +    if (mCurrentProgram->Generation() != location_object->ProgramGeneration())
> +        return ErrorInvalidOperation("%s: This uniform location is obsolete since the program has been relinked", info);
> +    returnFlag = 0;

Yup, that looks good. Since this function only does validation, it should be renamed to something that starts in Validate... .

@@ +3632,5 @@
>  
> +void
> +WebGLContext::SimpleArrayMethodNoCount(const char* name, int cnt, uint32_t arrayLength, int& returnFlag)
> +{
> +    returnFlag = 1;

Same remarks apply here.

@@ +3638,5 @@
> +        return;
> +    }
> +    if (arrayLength < cnt) {
> +        return ErrorInvalidOperation((std::string(name) + ": array must be >= %d elements").c_str(),
> +                                     cnt);

You don't need std::string for that, you can do ErrorInvalidOperation("%s: message", name);

I'll stop here for now. The general approach looks good otherwise.

@@ +3641,5 @@
> +        return ErrorInvalidOperation((std::string(name) + ": array must be >= %d elements").c_str(),
> +                                     cnt);
> +    }
> +    returnFlag = 0;
> +    MakeContextCurrent();

Don't put the MakeContextCurrent() here. Better keep this function doing only validation.

The MakeContextCurrent() call should always be done at the same place, just before, the actual GL call (gl->fFoo()).

Comment 8

[Saurabh Anand [:sawrubh]]

Attachment: Patch v2

Comment 9

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

Comment on attachment 671537 [details] [diff] [review]
Patch v2

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

This is looking really good!

Let's do one more round to do the following additional improvements:

::: content/canvas/src/WebGLContext.h
@@ +1028,5 @@
>                                 WebGLboolean transpose, uint32_t arrayLength,
>                                 const float* data);
>  
>      void UseProgram(WebGLProgram *prog);
> +    bool ValidateArrayMethodNoCount(const char* name, int cnt, uint32_t arrayLength);

I feel that these function names are bad. Not your fault ---  the macro names were bad. But let's take this occasion to make them better.

For example, it seems that (but please verify it) ValidateArrayMethodNoCount is only used by Attrib setters. So it's probably better renamed to ValidateAttribArraySetter.

@@ +1030,5 @@
>  
>      void UseProgram(WebGLProgram *prog);
> +    bool ValidateArrayMethodNoCount(const char* name, int cnt, uint32_t arrayLength);
> +    bool ValidateArrayMethodUniform(const char* name, int expectedElemSize, WebGLUniformLocation *location_object,
> +                                    GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength);

Likewise: ValidateUniformArraySetter

@@ +1033,5 @@
> +    bool ValidateArrayMethodUniform(const char* name, int expectedElemSize, WebGLUniformLocation *location_object,
> +                                    GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength);
> +    bool ValidateMatrixMethodUniform(const char* name, int dim, WebGLUniformLocation *location_object,
> +                                     GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength,
> +                                     WebGLboolean aTranspose);

ValidateUniformMatrixArraySetter

@@ +1034,5 @@
> +                                    GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength);
> +    bool ValidateMatrixMethodUniform(const char* name, int dim, WebGLUniformLocation *location_object,
> +                                     GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength,
> +                                     WebGLboolean aTranspose);
> +    bool ValidateMethodUniform(const char* name, WebGLUniformLocation *location_object, GLint& location);

ValidateUniformSetter  (since this seems to be used specifically by non-array setters)

Comment 10

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

Comment on attachment 671537 [details] [diff] [review]
Patch v2

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3681,5 @@
> +        arrayLength != expectedElemSize) {
> +        ErrorInvalidOperation(
> +            "%s: expected an array of length exactly"
> +            " %d (since this uniform is not an array"
> +            " uniform), got an array of length %d", name,

Also, can you refactor this string to use fewer, longer lines --- use at least 80 columns before wrapping.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Surely it should be 'use up to about 80 columns', not that 80 is our minimum.
I usually target an 80-char maximum, but exceptionally let some lines grow up to more like 100, generally because of long function names. (Thanks OpenGL! :P)

Comment 12

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

Guess you're right :) I usually target 150 characters columns, but that does irk people.

Comment 13

[Saurabh Anand [:sawrubh]]

Attachment: Patch v3

Comment 14

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

Comment on attachment 671585 [details] [diff] [review]
Patch v3

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

One style nit below -- I believe it's actually quite important for readability.

Also, I forgot this before: in our current file organization (which isn't great, but that's a separate issue) these new WebGLContext::Validate* methods should go to the separate WebGLContextValidate.cpp file.

::: content/canvas/src/WebGLContextGL.cpp
@@ +3848,5 @@
> +    uint32_t numElementsToUpload;
> +    GLint location;
> +    if (!ValidateUniformArraySetter("Uniform1iv", 1, location_object, location,
> +                                    numElementsToUpload, arrayLength))
> +        return;

After a multi-line if() condition, curly braces {...} are really mandatory.

(In fact I don't remember if our coding style makes them always mandatory and maybe we're just not following it in this file --- but at least for multi-line if() we do follow it.)

Comment 15

[Saurabh Anand [:sawrubh]]

Attachment: Patch v4

Will ask for review after the try run comes back.

Comment 16

[Saurabh Anand [:sawrubh]]

https://tbpl.mozilla.org/?tree=Try&rev=9faad40b474e

Comment 17

[Saurabh Anand [:sawrubh]]

The previous run had converted warnings as errors regarding comparing of int to unsigned int. I have made some changes and done another try run :

https://tbpl.mozilla.org/?tree=Try&rev=efde7e324278

Will post the patch after the try run is green.

Comment 18

[Saurabh Anand [:sawrubh]]

Attachment: Patch v5

Most probably fixed the build errors on Linux during the try run of the previous patch, by using uint32_t instead of int.

Comment 19

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

It's green, congratz.

Comment 20

[Saurabh Anand [:sawrubh]]

Comment on attachment 671802 [details] [diff] [review]
Patch v5

The try run looks green \o/.

Waiting for review and then please land it.

Comment 21

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/8ce5daee0470

Thanks!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ce5daee0470