1039950 - Make the GL Program handles strongly typed

Status: ASSIGNED

Description

[Corentin Wallez]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140611075517

Comment 1

[Corentin Wallez]

Attachment: First shot at doing strongly typed handles

Putting the patch for feedback, I'm not entirely happy with it yet:
- It is not very clear that the handles are initialized to 0
- GetIntergv(LOCAL_GL_ACTIVE_PROGRAM, program) needs program to be a GLint* so we have to get the program in two steps
- ZeroObject would be nicer than ZeroObject() (easy to fix)

Thanks!

Comment 2

[Nicolas Silva [:nical]]

Comment on attachment 8457826 [details] [diff] [review]
First shot at doing strongly typed handles

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

Looks good! I have a few comments but nothing dramatic. Jeff owns this code so he'll do the final review. Transferring the feedback request to him (you'll still have to set the actual "review?" flag to get the patch landed).

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +584,5 @@
>  
>      do {
>          mGL->fGetIntegerv(LOCAL_GL_RENDERBUFFER_BINDING, &oldrb);
>          mGL->fGetIntegerv(LOCAL_GL_FRAMEBUFFER_BINDING, &oldfb);
> +        mGL->fGetIntegerv(LOCAL_GL_CURRENT_PROGRAM, &oldprogHandle);

nit: you can write
mGL->fGetIntegerv(LOCAL_GL_CURRENT_PROGRAM, &oldprog.mHandle);
to avoid declaring two variables in this case.

::: gfx/gl/GLTypes.h
@@ +79,5 @@
> + * Some exmaples:
> + *   TextureHandle tex; // tex is 0
> + *   tex = 42 // error
> + *   tex = TextureHandle(42); // tex is 42
> + *   tex = ZeroObject(); // tex is 0 again

You can remove ZeroObject. It's not immediately obvious why we need it and I find that it's more natural to do tex = TextureHandle() to assign a null handle.

@@ +90,5 @@
> +
> +template<int Type> struct ObjectHandle {
> +    ObjectHandle(): glHandle(0) {}
> +    // The explicit to prevent automatic conversion when calling functions
> +    explicit ObjectHandle(GLuint handle): glHandle(handle) {} 

nit: trailing space here

@@ +96,5 @@
> +    operator bool() const {
> +        return glHandle != 0;
> +    }
> +
> +    GLuint glHandle;

Member variables start with a 'm' in Gecko's coding guidelines (ex: mHandle).

Comment 3

[Nicolas Silva [:nical]]

Corentin, I think you can address the comments I had in your patch and submit it for review directly (Jeff should be the reviewer). Don't hesitate to ping Jeff or me in the bug if review is taking several days.

Comment 4

[Corentin Wallez]

Attachment: Proposed patch for strongly typed prgram object handles

Comment 5

[Corentin Wallez]

Attachment: Proposed patch for strongly typed program object handles

I forgot to compile a last time before making the last patch.
Addressed Nical's comments and adding Jeff for review.

Comment 6

[Jeff Gilbert [:jgilbert]]

Comment on attachment 8459995 [details] [diff] [review]
Proposed patch for strongly typed program object handles

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

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +584,5 @@
>  
>      do {
>          mGL->fGetIntegerv(LOCAL_GL_RENDERBUFFER_BINDING, &oldrb);
>          mGL->fGetIntegerv(LOCAL_GL_FRAMEBUFFER_BINDING, &oldfb);
> +        mGL->fGetIntegerv(LOCAL_GL_CURRENT_PROGRAM, &oldprog.mName);

Yeah, give these an overload so they can accept `&oldprog` directly.

::: gfx/gl/GLTypes.h
@@ +93,5 @@
> +        return mName != 0;
> +    }
> +
> +    // Names are uint in the spec but can sometimes be returned with a GLint* (as in glGetIntergeriv).
> +    // We use a GLint here because the conversion GLuint* -> GLint* is invalied whereas GLuint -> GLint is.

Use GLuint regardless, but provide overloads for the glGet[...] functions.

@@ +94,5 @@
> +    }
> +
> +    // Names are uint in the spec but can sometimes be returned with a GLint* (as in glGetIntergeriv).
> +    // We use a GLint here because the conversion GLuint* -> GLint* is invalied whereas GLuint -> GLint is.
> +    GLint mName;

This should be const.

@@ +97,5 @@
> +    // We use a GLint here because the conversion GLuint* -> GLint* is invalied whereas GLuint -> GLint is.
> +    GLint mName;
> +};
> +
> +typedef ObjectHandle<ProgramObject> ProgramHandle;

I think this should be `GLProgram`. I think using `FooHandle` is more noise than we need. I think it's fine that we end up with gl::GLFoo in external code.

Comment 7

[Corentin Wallez]

Attachment: strongly-typed-program-handle.4.patch

Addresses all of Jeff's comments apart from the constness: it prevents things like GetIntegerv(GL_ACTIVE_PROGRAM, _) to work (and also any sort of assignement operator).

It might be better to make mName private and add an accessor.

Comment 8

[Jeff Gilbert [:jgilbert]]

(In reply to Corentin Wallez from comment #7)
> Created attachment 8463827 [details] [diff] [review]
> strongly-typed-program-handle.4.patch
> 
> Addresses all of Jeff's comments apart from the constness: it prevents
> things like GetIntegerv(GL_ACTIVE_PROGRAM, _) to work (and also any sort of
> assignement operator).
> 
> It might be better to make mName private and add an accessor.
For a const member, I think direct access is fine.

Comment 9

[Jeff Gilbert [:jgilbert]]

Comment on attachment 8463827 [details] [diff] [review]
strongly-typed-program-handle.4.patch

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

Normally, I won't r- over a const opportunity, but this is important since we're accessing this member directly.

::: gfx/gl/GLTypes.h
@@ +92,5 @@
> +    operator bool() const {
> +        return mName != 0;
> +    }
> +
> +    GLuint mName;

Make this const.

Comment 10

[Corentin Wallez]

Attachment: strongly-typed-program-handle.5.patch

Update to yse a private member + accessor instead of a const member, as discussed on IRC.

Comment 11

[Jeff Gilbert [:jgilbert]]

Comment on attachment 8464437 [details] [diff] [review]
strongly-typed-program-handle.5.patch

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

::: gfx/gl/GLTypes.h
@@ +94,5 @@
> +    operator bool() const {
> +        return mName != 0;
> +    }
> +
> +    GLuint GetName() const {

I prefer just `Name()`.

Comment 12

[Corentin Wallez]

Attachment: strongly-typed-program-handle.6.patch

Comment 13

[Jeff Gilbert [:jgilbert]]

Comment on attachment 8465173 [details] [diff] [review]
strongly-typed-program-handle.6.patch

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

::: gfx/gl/GLTypes.h
@@ +84,5 @@
> + *   f(42); // error
> + *   f(GLTexture()) // ok, it is the 0 handle
> + */
> +
> +template<int Type> struct ObjectHandle {

Technically, the style should be:
template<foo bar>
struct Bat
{

( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes )

Don't ask for review again if you fix this, just carry forward my r+ so we can land this.

Comment 14

[Corentin Wallez]

Attachment: strongly-typed-program-handle.7.patch

r=jgilbert

Comment 15

[Nicolas Silva [:nical]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound

Comment 16

[Ed Morley [:emorley]]

Unfortunately I had to back this out for warnings-as-errors build failures on Windows:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b4c58f7a3d4b
eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45241400&tree=Mozilla-Inbound
c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\obj-firefox\dist\include\OGLShaderProgram.h(262) : error C2220: warning treated as error - no 'object' file generated
c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\obj-firefox\dist\include\OGLShaderProgram.h(262) : warning C4804: '>' : unsafe use of type 'bool' in operation
c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/config/rules.mk:948: recipe for target 'Unified_cpp_gfx_gl0.obj' failed
mozmake.exe[5]: *** [Unified_cpp_gfx_gl0.obj] Error 2

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff07dcb918fd

Comment 17

[Ed Morley [:emorley]]

Also on other platforms too:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45241918&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45241945&tree=Mozilla-Inbound

Comment 18

[Nicolas Silva [:nical]]

 c:\builds\moz2_slave\try-w32-0000000000000000000000\build\gfx\layers\opengl/OGLShaderProgram.cpp(395) : error C2220: warning treated as error - no 'object' file generated 

I'd rather not overload the bool operator. Here, the warning is showing a real bug.
Also I tried to run with GL layers on Linux and rendering is busted with the patch, I keep getting this assertion:

[13869] ###!!! ASSERTION: SetUniform with wrong program active!: 'gl::GLProgram(currentProgram) == mProgram', file ../../dist/include/OGLShaderProgram.h, line 436
[13869] ###!!! ASSERTION: SetUniform with wrong program active!: 'gl::GLProgram(currentProgram) == mProgram', file ../../dist/include/OGLShaderProgram.h, line 464

Comment 19

[Ed Morley [:emorley]]

There are also other wegbl related test failures showing up:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b4c58f7a3d4b

nical, would it be ok for you to push this to try before this relands? :-)