Closed
Bug 1244611
Opened 8 years ago
Closed 8 years ago
Using named uniform buffer objects in the fragment shader fails
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: brad.kotsopoulos, Assigned: brad.kotsopoulos)
References
Details
Attachments
(1 file, 5 obsolete files)
2.08 KB,
patch
|
Details | Diff | Splinter Review |
Relevant WebGL conformance test: https://www.khronos.org/registry/webgl/sdk/tests/conformance2/buffers/uniform-buffers.html -- Description Declaring unnamed uniform buffer objects like this works: uniform UBOData { float UBORed; float UBOGreen; float UBOBlue; }; Declaring named UBOS like this - where we name an instance - fails: uniform UBOData { float Red; float Green; float Blue; } UBOA; -- Likely Cause The uniforms in the unnamed UBOs are in the global namespace, whereas the uniforms in the named UBOs are in the namespace of the instance. They named ones are referenced using the instance name (ex. UBOA.Red). Uniforms that are referenced through the instance namespace use a hashed name rather than plain-text (ex. webgl-2a29ea169) When we query the active uniforms from the driver, we get it in a hashed form (ex. webgl-2a29ea169.webgl-2a29ed634). But when we look up this hashed name in the list of uniform variables that we got from the shader compiler, the name can't be found. It looks like the name from the driver is hashed like hash(UBOData).hash(Red) whereas the name from the compiler variables is hashed as hash(UBOData).hash(UBOData.Red)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bkotsopoulos
Assignee | ||
Comment 1•8 years ago
|
||
Relevant (to determine if this is an ANGLE issue or not): https://code.google.com/p/chromium/issues/detail?id=577368&q=uniform&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Cr%20Status%20Owner%20Summary%20OS%20Modified
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8725833 -
Flags: review?(jmuizelaar)
Attachment #8725833 -
Flags: review?(jgilbert)
Assignee | ||
Comment 3•8 years ago
|
||
Proposed patch relies on this ANGLE fix: https://chromium.googlesource.com/angle/angle/+/39046169da60396c651b664aa67ea52a04146d2f which will be included in the next ANGLE update.
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 4•8 years ago
|
||
Comment on attachment 8725833 [details] [diff] [review] Remove the uniform block name prefix before searching for uniform by mapped name Review of attachment 8725833 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLShaderValidator.cpp @@ +476,5 @@ > + mappedFieldName = mappedName.substr(pos + 1); > + } > + else { > + mappedFieldName = mappedName; > + } The hunk above can be lifted out of the inner loop.
Attachment #8725833 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8725833 -
Attachment is obsolete: true
Attachment #8725833 -
Flags: review?(jgilbert)
Attachment #8728449 -
Flags: review?(jmuizelaar)
Attachment #8728449 -
Flags: review?(jgilbert)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8728449 -
Attachment is obsolete: true
Attachment #8728449 -
Flags: review?(jmuizelaar)
Attachment #8728449 -
Flags: review?(jgilbert)
Attachment #8728462 -
Flags: review?(jmuizelaar)
Attachment #8728462 -
Flags: review?(jgilbert)
Comment 7•8 years ago
|
||
Comment on attachment 8728462 [details] [diff] [review] For named blocks, we look for a matching block name prefix before checking fields Review of attachment 8728462 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable to me with the style changes fixed. ::: dom/canvas/WebGLShaderValidator.cpp @@ +464,3 @@ > const std::vector<sh::InterfaceBlock>& interfaces = *ShGetInterfaceBlocks(mHandle); > for (const auto& interface : interfaces) { > + A bit of extra whitespace here. @@ +466,5 @@ > + > + // If the InterfaceBlock exposes an instanceName, all variables defined > + // within the block are qualified with the block name. > + std::string mappedFieldName; > + if ("" != interface.instanceName) { if (!interface.instanceName.empty()) { FWIW, if (interface.instanceName != "") would be preferred if empty() didn't exist. @@ +477,5 @@ > + const std::string mappedInterfaceBlockName = mappedName.substr(0, dotPos); > + if (interface.mappedName != mappedInterfaceBlockName) > + continue; > + > + mappedFieldName = mappedName.substr(dotPos + 1); Looks like this line is misindented @@ +479,5 @@ > + continue; > + > + mappedFieldName = mappedName.substr(dotPos + 1); > + } > + else { } else { @@ +491,3 @@ > continue; > > + if ("" != interface.instanceName) { if (!interface.instanceName.empty()) {
Attachment #8728462 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8728462 -
Attachment is obsolete: true
Attachment #8728462 -
Flags: review?(jgilbert)
Attachment #8731265 -
Flags: review?(jgilbert)
Assignee | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0117a8ccbbd
Assignee | ||
Comment 10•8 years ago
|
||
@jgilbert, can you please review?
Comment 11•8 years ago
|
||
Comment on attachment 8731265 [details] [diff] [review] Style changes made Review of attachment 8731265 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLShaderValidator.cpp @@ +464,3 @@ > const std::vector<sh::InterfaceBlock>& interfaces = *ShGetInterfaceBlocks(mHandle); > for (const auto& interface : interfaces) { > + EOL whitespace @@ +464,4 @@ > const std::vector<sh::InterfaceBlock>& interfaces = *ShGetInterfaceBlocks(mHandle); > for (const auto& interface : interfaces) { > + > + // If the InterfaceBlock exposes an instanceName, all variables defined s/exposes/has/ @@ +464,5 @@ > const std::vector<sh::InterfaceBlock>& interfaces = *ShGetInterfaceBlocks(mHandle); > for (const auto& interface : interfaces) { > + > + // If the InterfaceBlock exposes an instanceName, all variables defined > + // within the block are qualified with the block name. s/block name/instanceName/ += (as opposed to being placed in the global scope) @@ +466,5 @@ > + > + // If the InterfaceBlock exposes an instanceName, all variables defined > + // within the block are qualified with the block name. > + std::string mappedFieldName; > + if (!interface.instanceName.empty()) { Pull this out into `hasInstanceName`. @@ +477,5 @@ > + const std::string mappedInterfaceBlockName = mappedName.substr(0, dotPos); > + if (interface.mappedName != mappedInterfaceBlockName) > + continue; > + > + mappedFieldName = mappedName.substr(dotPos + 1); Fix indentation, please!
Attachment #8731265 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #11) > @@ +464,5 @@ > > const std::vector<sh::InterfaceBlock>& interfaces = *ShGetInterfaceBlocks(mHandle); > > for (const auto& interface : interfaces) { > > + > > + // If the InterfaceBlock exposes an instanceName, all variables defined > > + // within the block are qualified with the block name. > > s/block name/instanceName/ > > += (as opposed to being placed in the global scope) > The shading language expects the variable to be qualified with the instanceName, but as far as GL and the introspection API is concerned, it uses the block name. The change is made to the shader source in the translator.
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8731265 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8733909 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a53b475538
Keywords: checkin-needed
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c5a53b475538
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•