Bug 90397 - ARB_program_interface_query: glGetProgramResourceiv() returns wrong value for GL_REFERENCED_BY_*_SHADER prop for GL_UNIFORM for members of an interface block with an instance name
Summary: ARB_program_interface_query: glGetProgramResourceiv() returns wrong value for...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Tapani Pälli
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-11 07:14 UTC by Samuel Iglesias Gonsálvez
Modified: 2015-05-19 05:17 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
patch to fix the issue (1.37 KB, patch)
2015-05-11 07:59 UTC, Tapani Pälli
Details | Splinter Review
patch to GetProgramResourceIndex() to fix arrays of UBOs behavior (1.54 KB, patch)
2015-05-13 10:41 UTC, Samuel Iglesias Gonsálvez
Details | Splinter Review

Description Samuel Iglesias Gonsálvez 2015-05-11 07:14:45 UTC
I was doing some testing of GL_BUFFER_VARIABLE <programInterface> parameter in glGetProgramResourceiv() checking different property queries, because I need that support for testing my GL_ARB_shader_storage_buffer_object work.

I realized that queries for GL_REFERENCED_BY_*_SHADER (GL_REFERENCED_BY_VERTEX_SHADER, GL_REFERENCED_BY_FRAGMENT_SHADER...) are not returning the proper value when the variable is a member of an interface block with an instance name. This is happening to GL_UNIFORM in current master branch.

For example: using the following vertex shader, glGetProgramResourceiv(GL_REFERENCED_BY_VERTEX_SHADER) query for GL_UNIFORM's "ubo_std140.s[0].b[0]" should return 1 but it returns 0. ATI proprietary driver returns 1.

    #version 330
    #extension GL_ARB_uniform_buffer_object : require

    struct B {mat2 b[3]; float c;};
    layout(row_major, std140) uniform ubo_std140 {
	    vec4 v;
	    B s[2];
    } a_std140;

    in vec4 piglit_vertex;

    void main() {
	    gl_Position = piglit_vertex;
	    mat2 a = a_std140.s[0].b[0];
	    gl_Position.x = a[0][0];
    }

Tested on Mesa master: abf3fefa1aa734844e0ca8e95e8c3a501909aa33
Comment 1 Tapani Pälli 2015-05-11 07:30:12 UTC
Yep, the problem is that their stage reference bitmask is empty. This is a debug print of active resources for your shader. They are considered active but not referenced. Will have to see how to tackle this.

--- 8< ---

PROGRAM RESOURCE LIST
IN   piglit_vertex [x_______]
OUT  gl_Position [x_______]
UNI  ubo_std140.v [________]
UNI  ubo_std140.s[0].b [________]
UNI  ubo_std140.s[0].c [________]
UNI  ubo_std140.s[1].b [________]
UNI  ubo_std140.s[1].c [________]
UBO  ubo_std140 [________]
Comment 2 Tapani Pälli 2015-05-11 07:59:46 UTC
Created attachment 115690 [details] [review]
patch to fix the issue

Does this fix the issue for you?
Comment 3 Samuel Iglesias Gonsálvez 2015-05-11 08:22:18 UTC
(In reply to Tapani Pälli from comment #2)
> Created attachment 115690 [details] [review] [review]
> patch to fix the issue
> 
> Does this fix the issue for you?

Yes, it does :-D

Thanks!
Comment 4 Samuel Iglesias Gonsálvez 2015-05-13 10:41:47 UTC
Created attachment 115737 [details] [review]
patch to GetProgramResourceIndex() to fix arrays of UBOs behavior

I found another issue when modifying arb_program_interface_query-getprogramresourceiv to test an uniform inside an array of UBOs.

I modified a shader which is defined inside common.h. Because of that, I updated other tests' code to work with that change. In particular, resource-query.c.

Shader code:

	#version 150

	uniform fs_uniform_block {
		vec4 fs_color;
		float fs_array[4];
	};

	uniform fs_array_uniform_block {
		vec4 fs_color;
		float fs_array[4];
	} ubo[4];

	in vec4 fs_input1;
	out vec4 fs_output0;
	out vec4 fs_output1;

	void main() {
	   fs_output0 = fs_color * fs_input1 * fs_array[2] *
		        ubo[0].fs_array[2] * ubo[2].fs_array[2];
	   fs_output1 = fs_color * fs_input1 * fs_array[3] *
		        ubo[1].fs_array[3] * ubo[3].fs_array[3];
	}


I found that glGetProgramResourceName() is returning the name "fs_array_uniform_block[0]", "fs_array_uniform_block[1]", "fs_array_uniform_block[2]", and "fs_array_uniform_block[3]" when asked with the corresponding block index as those blocks are active resources in the shader.

resource-query test checks again that those names corresponds to active resources by comparing the former index against the one returned by glGetProgramResourceIndex(name).

Unfortunately, Mesa is returning GL_INVALID_INDEX to fs_array_uniform_block[{1,2,3}] (other than zero),  while NVIDIA passes the test for that case.

Reading GL_ARB_program_interface_query, I found the following:

   "Note that if an
    interface enumerates a single active resource list entry for an array
    variable (e.g., "a[0]"), a <name> identifying any array element other than
    the first (e.g., "a[1]") is not considered to match."

I understand that this is referring to uniform/buffer variables but not uniform/shader-storage blocks, but I might be wrong.

I would like to have your opinion regarding if it is valid or not spec-wise. What do you think?

P.S: Attached to this, it's the patch that fixes this case.
Comment 5 Tapani Pälli 2015-05-18 09:21:41 UTC
(In reply to Samuel Iglesias from comment #4)
> I understand that this is referring to uniform/buffer variables but not
> uniform/shader-storage blocks, but I might be wrong.
> 
> I would like to have your opinion regarding if it is valid or not spec-wise.
> What do you think?
> 
> P.S: Attached to this, it's the patch that fixes this case.

I'm somewhat surprised if this rule is not be common for all resources. The text in OpenGL Core 4.5 says (right below function declaration):

"If name exactly matches the name string of one of the active resources for
programInterface, the index of the matched resource is returned. Additionally, if
name would exactly match the name string of an active resource if "[0]" were
appended to name, the index of the matched resource is returned. Otherwise, name
is considered not to be the name of an active resource, and INVALID_INDEX is
returned.

Note that if an interface enumerates a single active resource list entry for an array variable (e.g., "a[0]"), a name identifying any array element other than the first (e.g., "a[1]") is not considered to match."

Just to make sure I understand correctly, when querying the index for fs_array_uniform_block[{1,2,3}] you are giving GL_UNIFORM as the interface?
Comment 6 Samuel Iglesias Gonsálvez 2015-05-18 09:42:29 UTC
(In reply to Tapani Pälli from comment #5)
> (In reply to Samuel Iglesias from comment #4)
> > I understand that this is referring to uniform/buffer variables but not
> > uniform/shader-storage blocks, but I might be wrong.
> > 
> > I would like to have your opinion regarding if it is valid or not spec-wise.
> > What do you think?
> > 
> > P.S: Attached to this, it's the patch that fixes this case.
> 
> I'm somewhat surprised if this rule is not be common for all resources. The
> text in OpenGL Core 4.5 says (right below function declaration):
> 
> "If name exactly matches the name string of one of the active resources for
> programInterface, the index of the matched resource is returned.
> Additionally, if
> name would exactly match the name string of an active resource if "[0]" were
> appended to name, the index of the matched resource is returned. Otherwise,
> name
> is considered not to be the name of an active resource, and INVALID_INDEX is
> returned.
> 
> Note that if an interface enumerates a single active resource list entry for
> an array variable (e.g., "a[0]"), a name identifying any array element other
> than the first (e.g., "a[1]") is not considered to match."
> 
> Just to make sure I understand correctly, when querying the index for
> fs_array_uniform_block[{1,2,3}] you are giving GL_UNIFORM as the interface?

Nope, the interface type is GL_UNIFORM_BLOCK.

Let me recapitulate, simplifying what I wrote:

* I am querying the index for a <programInterface> equals to GL_UNIFORM_BLOCK and for the following names -> fs_array_uniform_block[{1,2,3}].

* The OpenGL Core 4.5 spec snippet you pasted says the same than the ARB_program_interface_query extension spec.

* As I said, NVIDIA doesn't give any error. Mesa returns INVALID_INDEX because of valid_program_resource_index_name() function as, for each name, the array index included in the name is not zero (in the example, they are 1, 2 and 3).

* I have doubts about what to do in this case, spec-wise.

I hope this is clearer now :-)

Sam
Comment 7 Tapani Pälli 2015-05-18 10:27:43 UTC
(In reply to Samuel Iglesias from comment #6)
> (In reply to Tapani Pälli from comment #5)
> > (In reply to Samuel Iglesias from comment #4)
> > > I understand that this is referring to uniform/buffer variables but not
> > > uniform/shader-storage blocks, but I might be wrong.
> > > 
> > > I would like to have your opinion regarding if it is valid or not spec-wise.
> > > What do you think?
> > > 
> > > P.S: Attached to this, it's the patch that fixes this case.
> > 
> > I'm somewhat surprised if this rule is not be common for all resources. The
> > text in OpenGL Core 4.5 says (right below function declaration):
> > 
> > "If name exactly matches the name string of one of the active resources for
> > programInterface, the index of the matched resource is returned.
> > Additionally, if
> > name would exactly match the name string of an active resource if "[0]" were
> > appended to name, the index of the matched resource is returned. Otherwise,
> > name
> > is considered not to be the name of an active resource, and INVALID_INDEX is
> > returned.
> > 
> > Note that if an interface enumerates a single active resource list entry for
> > an array variable (e.g., "a[0]"), a name identifying any array element other
> > than the first (e.g., "a[1]") is not considered to match."
> > 
> > Just to make sure I understand correctly, when querying the index for
> > fs_array_uniform_block[{1,2,3}] you are giving GL_UNIFORM as the interface?
> 
> Nope, the interface type is GL_UNIFORM_BLOCK.
> 
> Let me recapitulate, simplifying what I wrote:
> 
> * I am querying the index for a <programInterface> equals to
> GL_UNIFORM_BLOCK and for the following names ->
> fs_array_uniform_block[{1,2,3}].
> 
> * The OpenGL Core 4.5 spec snippet you pasted says the same than the
> ARB_program_interface_query extension spec.

Right, I just wanted to highlight this again as this explicitly states the rule and does not state that it would be valid only for some resource types. IMO it covers them all.
 
> * As I said, NVIDIA doesn't give any error. Mesa returns INVALID_INDEX
> because of valid_program_resource_index_name() function as, for each name,
> the array index included in the name is not zero (in the example, they are
> 1, 2 and 3).

Could you attach or link to the modified Piglit test case? I'm wondering a bit if glGetProgramResourceName returns the name with array index and if so, why.
 
> * I have doubts about what to do in this case, spec-wise.
> 
> I hope this is clearer now :-)

Yes!
Comment 8 Samuel Iglesias Gonsálvez 2015-05-18 10:35:15 UTC
(In reply to Tapani Pälli from comment #7)
> (In reply to Samuel Iglesias from comment #6)
> > (In reply to Tapani Pälli from comment #5)
> > > (In reply to Samuel Iglesias from comment #4)
> > > > I understand that this is referring to uniform/buffer variables but not
> > > > uniform/shader-storage blocks, but I might be wrong.
> > > > 
> > > > I would like to have your opinion regarding if it is valid or not spec-wise.
> > > > What do you think?
> > > > 
> > > > P.S: Attached to this, it's the patch that fixes this case.
> > > 
> > > I'm somewhat surprised if this rule is not be common for all resources. The
> > > text in OpenGL Core 4.5 says (right below function declaration):
> > > 
> > > "If name exactly matches the name string of one of the active resources for
> > > programInterface, the index of the matched resource is returned.
> > > Additionally, if
> > > name would exactly match the name string of an active resource if "[0]" were
> > > appended to name, the index of the matched resource is returned. Otherwise,
> > > name
> > > is considered not to be the name of an active resource, and INVALID_INDEX is
> > > returned.
> > > 
> > > Note that if an interface enumerates a single active resource list entry for
> > > an array variable (e.g., "a[0]"), a name identifying any array element other
> > > than the first (e.g., "a[1]") is not considered to match."
> > > 
> > > Just to make sure I understand correctly, when querying the index for
> > > fs_array_uniform_block[{1,2,3}] you are giving GL_UNIFORM as the interface?
> > 
> > Nope, the interface type is GL_UNIFORM_BLOCK.
> > 
> > Let me recapitulate, simplifying what I wrote:
> > 
> > * I am querying the index for a <programInterface> equals to
> > GL_UNIFORM_BLOCK and for the following names ->
> > fs_array_uniform_block[{1,2,3}].
> > 
> > * The OpenGL Core 4.5 spec snippet you pasted says the same than the
> > ARB_program_interface_query extension spec.
> 
> Right, I just wanted to highlight this again as this explicitly states the
> rule and does not state that it would be valid only for some resource types.
> IMO it covers them all.
>  
> > * As I said, NVIDIA doesn't give any error. Mesa returns INVALID_INDEX
> > because of valid_program_resource_index_name() function as, for each name,
> > the array index included in the name is not zero (in the example, they are
> > 1, 2 and 3).
> 
> Could you attach or link to the modified Piglit test case? I'm wondering a
> bit if glGetProgramResourceName returns the name with array index and if so,
> why.
>

I published a branch with the modified test, please see last commit:

git clone -b arb_uniform_buffer_object-patches https://github.com/samuelig/piglit.git

File: tests/spec/arb_program_interface_query/resource-query.c
Comment 9 Tapani Pälli 2015-05-19 05:12:37 UTC
Thanks for the test. I do agree with the patch, UBO arrays should be treated differently.

Reviewed-by: Tapani Pälli <tapani.palli@intel.com>

(I'm resolving this bug as the patch has been pushed to master and ubo array is a separate issue)
Comment 10 Samuel Iglesias Gonsálvez 2015-05-19 05:17:18 UTC
(In reply to Tapani Pälli from comment #9)
> Thanks for the test. I do agree with the patch, UBO arrays should be treated
> differently.
> 
> Reviewed-by: Tapani Pälli <tapani.palli@intel.com>
> 
> (I'm resolving this bug as the patch has been pushed to master and ubo array
> is a separate issue)

OK, thanks.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.