Make the GL Program handles strongly typed

ASSIGNED
Assigned to

Status

()

defect
ASSIGNED
5 years ago
5 years ago

People

(Reporter: corentin, Assigned: corentin)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140611075517
Blocks: 984926
OS: Linux → All
Hardware: x86_64 → All
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!
Attachment #8457826 - Flags: feedback?(nical.bugzilla)
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).
Attachment #8457826 - Flags: feedback?(nical.bugzilla)
Attachment #8457826 - Flags: feedback?(jgilbert)
Attachment #8457826 - Flags: feedback+
Assignee: nobody → corentin
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.
Attachment #8457826 - Attachment is obsolete: true
Attachment #8457826 - Flags: feedback?(jgilbert)
Attachment #8459988 - Flags: review?(jgilbert)
I forgot to compile a last time before making the last patch.
Addressed Nical's comments and adding Jeff for review.
Attachment #8459988 - Attachment is obsolete: true
Attachment #8459988 - Flags: review?(jgilbert)
Attachment #8459995 - Flags: review?(jgilbert)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attachment #8459995 - Flags: review?(jgilbert)
Attachment #8459995 - Flags: review-
Attachment #8459995 - Flags: feedback+
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.
Attachment #8459995 - Attachment is obsolete: true
Attachment #8463827 - Flags: review?(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 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.
Attachment #8463827 - Flags: review?(jgilbert) → review-
Update to yse a private member + accessor instead of a const member, as discussed on IRC.
Attachment #8463827 - Attachment is obsolete: true
Attachment #8464437 - Flags: review?(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()`.
Attachment #8464437 - Flags: review?(jgilbert) → review+
Attachment #8464437 - Attachment is obsolete: true
Attachment #8465173 - Flags: review?(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.
Attachment #8465173 - Flags: review?(jgilbert) → review+
r=jgilbert
Attachment #8465173 - Attachment is obsolete: true
Attachment #8465925 - Flags: review+
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
Status: NEW → ASSIGNED
 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
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? :-)
You need to log in before you can comment on or make changes to this bug.