Closed
Bug 1048747
Opened 10 years ago
Closed 8 years ago
WebGL2 - Implement Uniform Buffer Objects
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
People
(Reporter: u480271, Assigned: u480271)
References
Details
(Keywords: dev-doc-complete)
Attachments
(10 files, 2 obsolete files)
10.79 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
10.60 KB,
patch
|
jgilbert
:
review-
|
Details | Diff | Splinter Review |
10.48 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
2.02 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
11.07 KB,
patch
|
jgilbert
:
review-
smaug
:
review-
|
Details | Diff | Splinter Review |
1.69 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
15.10 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
12.62 KB,
patch
|
jgilbert
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
2.82 KB,
patch
|
jgilbert
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
25.71 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
Updated•10 years ago
|
Keywords: dev-doc-needed
Keywords: leave-open
Add GL feature detection of uniform buffer functions.
Attachment #8469811 -
Flags: review?(jgilbert)
Adds GL extension checks for glGetInteger64i_v function.
Attachment #8469812 -
Flags: review?(jgilbert)
Attachment #8469813 -
Flags: review?(jgilbert)
Updated•9 years ago
|
Attachment #8469811 -
Flags: review?(jgilbert) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8469812 [details] [diff] [review] Patch 2 - WebGL2 - Implement GetInteger64i_v extension checks. Review of attachment 8469812 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +382,5 @@ > { (PRFuncPtr*) &mSymbols.fGetAttachedShaders, { "GetAttachedShaders", "GetAttachedShadersARB", nullptr } }, > { (PRFuncPtr*) &mSymbols.fGetAttribLocation, { "GetAttribLocation", "GetAttribLocationARB", nullptr } }, > { (PRFuncPtr*) &mSymbols.fGetIntegerv, { "GetIntegerv", nullptr } }, > + { (PRFuncPtr*) &mSymbols.fGetIntegeri_v, { "GetIntegeri_v", "GetIntegerIndexedvEXT", "GetIntegerIndexedvNV", nullptr } }, > + { (PRFuncPtr*) &mSymbols.fGetInteger64i_v, { "GetInteger64i_v", nullptr } }, Huh? We should not be requiring these symbols for GLContext creation.
Attachment #8469812 -
Flags: review?(jgilbert) → review-
Comment 5•9 years ago
|
||
Comment on attachment 8469813 [details] [diff] [review] Patch 3 - WebGL2 - Implement uniform block/buffer. Review of attachment 8469813 [details] [diff] [review]: ----------------------------------------------------------------- I think I'm going to r- non-thread-safe things from now on. WebGL in workers is coming about on the same timeframe as WebGL2, maybe sooner. ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +183,5 @@ > + > + switch (target) { > + case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_BINDING: > + case LOCAL_GL_UNIFORM_BUFFER_BINDING: > + { This style is growing on me. It sorta-kinda matches the style for `public:` and friends. I think we should probably keep the `{` on the previous line, though. @@ +218,5 @@ > + if (!ValidateObject("getUniformIndics: program", program)) > + return; > + > + if (!uniformNames.Length()) > + return; I think the early return here is unnecessary, so we should just let it though, knowing it won't execute the loop. @@ +291,5 @@ > + case LOCAL_GL_UNIFORM_BLOCK_NAME_LENGTH: > + case LOCAL_GL_UNIFORM_BLOCK_ACTIVE_UNIFORMS: > + case LOCAL_GL_UNIFORM_BLOCK_REFERENCED_BY_VERTEX_SHADER: > + case LOCAL_GL_UNIFORM_BLOCK_REFERENCED_BY_FRAGMENT_SHADER: > + { Bring this `{` scope up one indent. @@ +310,5 @@ > + gl->fGetActiveUniformBlockiv(progname, uniformBlockIndex, pname, (GLint*) JS_GetUint32ArrayData(obj)); > + retval.set(JS::ObjectOrNullValue(obj)); > + return; > + } > + } This looks like an error. (It's not, but let's avoid this by indenting case scopes. @@ +317,4 @@ > } > > +#define WEBGL_MAX_UNIFORM_BLOCK_NAME_LENGTH 256 > +static char nameBuffer[WEBGL_MAX_UNIFORM_BLOCK_NAME_LENGTH]; We should avoid adding non-thread-safe things at this point. Just put it on the stack. Get* funcs are fine to leave less optimized.
Attachment #8469813 -
Flags: review?(jgilbert) → review-
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/54f8ddeb0783 https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/eae3e0248ba1 https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/f4bde51acbf0
https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/1c897f3058ed https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/87dc104c9838 https://hg.mozilla.org/users/dglastonbury_mozilla.com/mozilla-central/rev/597a5cd4b47e
Attachment #8469812 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
Clean up taking :jgilbert comments. Split out feature checks for GetIntegeri_v and GetInteger64i_v because they don't appear in GL in the same revisions or extensions.
Attachment #8494842 -
Flags: review?(jgilbert)
Assignee | ||
Comment 11•9 years ago
|
||
So GL_EXT_texture_storage has the following: In particular, note that OpenGL ES 1.x/2.0 do not have proxy textures, 1D textures, or 3D textures, and thus only the TexStorage2DEXT entry point is required. If OES_texture_3D is supported, the TexStorage3DEXT entry point is also required. So I'm going to remove the GL_EXT_texture_storage check because it's not good enough for what we need.
Updated•9 years ago
|
Attachment #8494842 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 12•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e2763315acd8
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6688b3916fc3
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8524368 -
Flags: review?(jgilbert)
Updated•9 years ago
|
Attachment #8524368 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 16•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5154511552bc
Assignee | ||
Comment 17•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1b294c3790ab
Assignee | ||
Comment 18•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a84e0bdb8145
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d18e1e60d52
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8527392 -
Flags: review?(jgilbert)
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8527394 -
Flags: review?(jgilbert)
Comment 23•9 years ago
|
||
Comment on attachment 8527392 [details] [diff] [review] [WebGL2] Implement uniform block/buffer Review of attachment 8527392 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +340,5 @@ > + switch (target) { > + case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_BINDING: > + case LOCAL_GL_UNIFORM_BUFFER_BINDING: > + { > + GLint data; Init to zero for safety. @@ +351,5 @@ > + case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_SIZE: > + case LOCAL_GL_UNIFORM_BUFFER_START: > + case LOCAL_GL_UNIFORM_BUFFER_SIZE: > + { > + GLint64 data; Init to zero for safety. @@ +385,5 @@ > + > + for (size_t n = 0; n < count; n++) { > + const nsString& name = uniformNames[n]; > + GLuint index = 0; > + const GLchar* glname = NS_LossyConvertUTF16toASCII(name).get(); You have to have an active local instance of NS_LossyConvertUTF16toASCII. NS_LossyConvertUTF16toASCII foo(name); const char* bar = foo.get(); @@ +463,5 @@ > + case LOCAL_GL_UNIFORM_BLOCK_NAME_LENGTH: > + case LOCAL_GL_UNIFORM_BLOCK_ACTIVE_UNIFORMS: > + case LOCAL_GL_UNIFORM_BLOCK_REFERENCED_BY_VERTEX_SHADER: > + case LOCAL_GL_UNIFORM_BLOCK_REFERENCED_BY_FRAGMENT_SHADER: > + { case FOO: { // bar } @@ +483,5 @@ > + gl->fGetActiveUniformBlockiv(progname, uniformBlockIndex, pname, (GLint*) JS_GetUint32ArrayData(obj, nogc)); > + retval.set(JS::ObjectOrNullValue(obj)); > + return; > + } > + } Ick. @@ +490,4 @@ > } > > +#define WEBGL_MAX_UNIFORM_BLOCK_NAME_LENGTH 256 > +static char nameBuffer[WEBGL_MAX_UNIFORM_BLOCK_NAME_LENGTH]; Don't use non-threadsafe statics. Just use an nsAutoCString and SetLength.
Attachment #8527392 -
Flags: review?(jgilbert) → review-
Updated•9 years ago
|
Attachment #8527394 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #23) > Comment on attachment 8527392 [details] [diff] [review] > [WebGL2] Implement uniform block/buffer > > Review of attachment 8527392 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/WebGL2ContextUniforms.cpp > @@ +340,5 @@ > > + switch (target) { > > + case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_BINDING: > > + case LOCAL_GL_UNIFORM_BUFFER_BINDING: > > + { > > + GLint data; > > Init to zero for safety. I hate the { } inside a case clause. I'm going to pull them out. > @@ +483,5 @@ > > + } > > + } This one a bit harder to remove, so I indented the block.
Assignee | ||
Comment 26•9 years ago
|
||
Address comments + wrap long lines at 90 characters.
Attachment #8531809 -
Flags: review?(jgilbert)
Assignee | ||
Comment 27•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b3a28e9d77ef
Comment 28•9 years ago
|
||
Comment on attachment 8531809 [details] [diff] [review] review comments Review of attachment 8531809 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +31,5 @@ > } > > bool > +WebGL2Context::ValidateAttribPointerType(bool integerMode, GLenum type, > + GLsizei* alignment, const char* info) If `alignment` is an out-var, please prefix with `out_`, especially since `info` comes after it, and is an in-var. @@ +392,5 @@ > MakeContextCurrent(); > > for (size_t n = 0; n < count; n++) { > + NS_LossyConvertUTF16toASCII name(uniformNames[n]); > + const GLchar* glname = name.get(); I think BeginReading() is preferable, but get() may work too.
Attachment #8531809 -
Flags: review?(jgilbert) → review+
Attachment #8527392 -
Flags: review?(bugs)
Assignee | ||
Comment 29•9 years ago
|
||
Olli, Require a review of webidl change to getActiveUniformBlockParameter. getActiveUniformBlockParameter allocates a JS array for result which can fail and throw. There is precedence in WebGL1, where we have adorned some IDL with [throw] which isn't in the public spec.
Flags: needinfo?(bugs)
Comment 30•9 years ago
|
||
Comment on attachment 8527392 [details] [diff] [review] [WebGL2] Implement uniform block/buffer Why we need to return 'any'? Would the following work? [Throws] (long or sequence<{some sane type here}>)? getActiveUniformBlockParameter(WebGLProgram? program, GLuint uniformBlockIndex, GLenum pname); Similar question about GetIndexedParameter. The less we use JSAPI the better.
Flags: needinfo?(bugs)
Attachment #8527392 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 31•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fb550d9d5609
Assignee | ||
Comment 32•9 years ago
|
||
Implemented Olli's advice about the functions that return "any". Found and fixed bug in getIndexedParameter.
Attachment #8534803 -
Flags: review?(jgilbert)
Attachment #8534803 -
Flags: review?(bugs)
Comment 33•9 years ago
|
||
Comment on attachment 8534803 [details] [diff] [review] reduce scope of getActiveUniformBlockParameter (at some point it would be nice to get this all converted to use Mozilla coding style. {} with ifs and 2 space indentation, aFoo naming for arguments, etc. Especially lack of aFoo naming makes reading the code harder.) > GLuint progname = program->GLName(); > GLint param = 0; >+ JS::RootedObject array(cx); Why you define this here > case LOCAL_GL_UNIFORM_BLOCK_ACTIVE_UNIFORM_INDICES: >+ if (!GetUniformBlockActiveUniforms(gl, cx, this, progname, uniformBlockIndex, >+ &array)) When you use it only within this case: So, move it to above the 'if' ? > { >- GLint length = 0; >- gl->fGetActiveUniformBlockiv(progname, uniformBlockIndex, >- LOCAL_GL_UNIFORM_BLOCK_ACTIVE_UNIFORMS, &length); >- JSObject* obj = Uint32Array::Create(cx, this, length, nullptr); >- if (!obj) { >- rv = NS_ERROR_OUT_OF_MEMORY; >- } >- JS::AutoCheckCannotGC nogc; >- gl->fGetActiveUniformBlockiv(progname, uniformBlockIndex, pname, >- (GLint*) JS_GetUint32ArrayData(obj, nogc)); >- retval.set(JS::ObjectOrNullValue(obj)); >+ rv = NS_ERROR_OUT_OF_MEMORY; > return; Not clear to me why you created a helper method for this, but I guess it is fine. >+++ b/dom/webidl/WebGL2RenderingContext.webidl Btw, what does the spec say? Should the .webidl in the spec be changed too?
Attachment #8534803 -
Flags: review?(bugs) → review+
Comment 34•9 years ago
|
||
Comment on attachment 8534803 [details] [diff] [review] reduce scope of getActiveUniformBlockParameter Review of attachment 8534803 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +488,4 @@ > void > WebGL2Context::GetActiveUniformBlockParameter(JSContext* cx, WebGLProgram* program, > GLuint uniformBlockIndex, GLenum pname, > + Nullable<dom::OwningUnsignedLongOrUint32ArrayOrBoolean>& retval, This is what magic gets us, I guess.
Attachment #8534803 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 35•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee50dac0cb7e
Comment 36•9 years ago
|
||
sorry had to back this out since this might have caused https://treeherder.mozilla.org/logviewer.html#?job_id=4642792&repo=mozilla-inbound at least with this push
Assignee | ||
Comment 37•9 years ago
|
||
Alright, I've fixed the rooting hazard. http://tbpl.mozilla.org/?tree=Try&rev=cfdd17bde752
Assignee | ||
Comment 38•9 years ago
|
||
I was extracting the pointer to Uint32Array data the wrong way. I hope this is the correct way. It does fix the rooting hazard.
Attachment #8537005 -
Flags: review?(jgilbert)
Attachment #8537005 -
Flags: review?(bugs)
Updated•9 years ago
|
Attachment #8537005 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 39•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb46363d636
Updated•9 years ago
|
Attachment #8537005 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 41•9 years ago
|
||
Attachment #8579115 -
Flags: review?(jgilbert)
Comment 42•9 years ago
|
||
Comment on attachment 8579115 [details] [diff] [review] Cleanup GetUniformBlockIndex. Review of attachment 8579115 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGL2ContextUniforms.cpp @@ +10,5 @@ > #include "WebGLVertexArray.h" > #include "WebGLVertexAttribData.h" > #include "mozilla/dom/WebGL2RenderingContextBinding.h" > > +namespace mozilla { Thanks! ::: dom/canvas/WebGLProgram.cpp @@ +510,5 @@ > + if (!ParseName(userName, &baseUserName, &isArray, &arrayIndex)) > + return LOCAL_GL_INVALID_INDEX; > + > + const WebGLActiveInfo* activeInfo; > + if (!LinkInfo()->FindUniform(baseUserName, &activeInfo)) { Woah, LinkInfo() might be null here. See below in GetUniformLocation, where the second tests is `if (!IsLinked) => INVALID_OPERATION`. @@ +520,5 @@ > + > + nsAutoCString mappedName(baseMappedName); > + if (isArray) { > + mappedName.AppendLiteral("["); > + mappedName.AppendInt(uint32_t(arrayIndex)); I think we need to check that arrayIndex is valid, as well. (I think this info is also hanging off the ActiveInfo object)
Attachment #8579115 -
Flags: review?(jgilbert) → review-
Assignee | ||
Comment 43•9 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #42) > ::: dom/canvas/WebGLProgram.cpp > @@ +510,5 @@ > > + if (!ParseName(userName, &baseUserName, &isArray, &arrayIndex)) > > + return LOCAL_GL_INVALID_INDEX; > > + > > + const WebGLActiveInfo* activeInfo; > > + if (!LinkInfo()->FindUniform(baseUserName, &activeInfo)) { > > Woah, LinkInfo() might be null here. See below in GetUniformLocation, where > the second tests is `if (!IsLinked) => INVALID_OPERATION`. I swear I put that check in... > @@ +520,5 @@ > > + > > + nsAutoCString mappedName(baseMappedName); > > + if (isArray) { > > + mappedName.AppendLiteral("["); > > + mappedName.AppendInt(uint32_t(arrayIndex)); > > I think we need to check that arrayIndex is valid, as well. (I think this > info is also hanging off the ActiveInfo object) I'm not sure. I don't think that arrays are valid for uniform block names. I was mostly just cribbing from GetUniformLocation.
Assignee | ||
Comment 44•9 years ago
|
||
Comment 43
Assignee | ||
Comment 45•9 years ago
|
||
Can you elaborate on what we need to do check that arrayIndex is valid? Is the array count kept in WebGLActiveInfo?
Flags: needinfo?(jgilbert)
Comment 46•9 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #45) > Can you elaborate on what we need to do check that arrayIndex is valid? Is > the array count kept in WebGLActiveInfo? We talked about this, but it's worth dumping in the bug. The array count would be in WebGLActiveInfo. We need to generated the list of these for the program when we create the program's LinkInfo. (by querying the driver for info about them)
Flags: needinfo?(jgilbert)
Assignee | ||
Comment 47•9 years ago
|
||
Attachment #8593655 -
Flags: review?(jgilbert)
Assignee | ||
Comment 48•9 years ago
|
||
try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d0626bcc72e
Assignee | ||
Comment 49•9 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #48) > try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d0626bcc72e Well that's a bust.
Attachment #8579115 -
Attachment is obsolete: true
Comment 50•9 years ago
|
||
Comment on attachment 8593655 [details] [diff] [review] Completely rework how uniform interface blocks are handled. r=jgilbert Review of attachment 8593655 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLProgram.cpp @@ +113,5 @@ > + (GLint*)&maxUniformBlockLenWithNull); > + if (maxUniformBlockLenWithNull < 1) > + maxUniformBlockLenWithNull = 1; > + } > + whitespace! ::: dom/canvas/WebGLProgram.h @@ +55,5 @@ > std::map<nsCString, const WebGLActiveInfo*> attribMap; > std::map<nsCString, const WebGLActiveInfo*> uniformMap; > std::map<nsCString, const nsCString>* fragDataMap; > > + std::vector<nsRefPtr<UniformBlockInfo>> uniformBlocks; Prefer `RefPtr` to `nsRefPtr`. @@ +85,5 @@ > return true; > } > > + bool FindUniformBlock(const nsCString& baseUserName, > + const UniformBlockInfo** const out_info) const `RefPtr<const UniformBlockInfo>* const out_info`?
Attachment #8593655 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 51•9 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #50) > Prefer `RefPtr` to `nsRefPtr`. I cribbed from activeAttribs and activeUniforms which are nsRefPtr.
Assignee | ||
Comment 52•9 years ago
|
||
try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=21167facbca8
Assignee | ||
Comment 56•8 years ago
|
||
I was keeping this open to add more patches on an `as needed` basis. I see a follow up about getting to pass UBO conformance tests, so I'll close this and deal with each, specific failure in a separate bug.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(dglastonbury)
Keywords: leave-open
Resolution: --- → FIXED
Comment 57•8 years ago
|
||
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext#Uniform_buffer_objects
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•