Open
Bug 1039950
Opened 7 years ago
Updated 7 years ago
Make the GL Program handles strongly typed
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
ASSIGNED
People
(Reporter: corentin, Assigned: corentin)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 6 obsolete files)
32.11 KB,
patch
|
corentin
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release) Build ID: 20140611075517
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
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 2•7 years ago
|
||
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+
Updated•7 years ago
|
Assignee: nobody → corentin
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8457826 -
Attachment is obsolete: true
Attachment #8457826 -
Flags: feedback?(jgilbert)
Attachment #8459988 -
Flags: review?(jgilbert)
Assignee | ||
Comment 5•7 years ago
|
||
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)
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 6•7 years ago
|
||
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+
Assignee | ||
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
(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•7 years ago
|
||
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-
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
Attachment #8464437 -
Attachment is obsolete: true
Attachment #8465173 -
Flags: review?(jgilbert)
Comment 13•7 years ago
|
||
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+
Assignee | ||
Comment 14•7 years ago
|
||
r=jgilbert
Attachment #8465173 -
Attachment is obsolete: true
Attachment #8465925 -
Flags: review+
Comment 16•7 years ago
|
||
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
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 17•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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.
Description
•