Closed Bug 1244611 Opened 4 years ago Closed 4 years ago

Using named uniform buffer objects in the fragment shader fails

Categories

(Core :: Canvas: WebGL, defect)

47 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: brad.kotsopoulos, Assigned: brad.kotsopoulos)

References

Details

Attachments

(1 file, 5 obsolete files)

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: nobody → bkotsopoulos
Attachment #8725833 - Flags: review?(jmuizelaar)
Attachment #8725833 - Flags: review?(jgilbert)
Proposed patch relies on this ANGLE fix:
https://chromium.googlesource.com/angle/angle/+/39046169da60396c651b664aa67ea52a04146d2f

which will be included in the next ANGLE update.
Depends on: 1251375
Status: NEW → ASSIGNED
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-
Attachment #8725833 - Attachment is obsolete: true
Attachment #8725833 - Flags: review?(jgilbert)
Attachment #8728449 - Flags: review?(jmuizelaar)
Attachment #8728449 - Flags: review?(jgilbert)
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 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+
Attached patch Style changes made (obsolete) — Splinter Review
Attachment #8728462 - Attachment is obsolete: true
Attachment #8728462 - Flags: review?(jgilbert)
Attachment #8731265 - Flags: review?(jgilbert)
@jgilbert, can you please review?
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+
(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.
Attached patch ubo3.pat (obsolete) — Splinter Review
Attachment #8731265 - Attachment is obsolete: true
Attached patch ubo.patSplinter Review
Attachment #8733909 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c5a53b475538
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.