Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fshack: The gamma ramp data doesn't query the actual array layout which can lead to incorrect rendering #133

Open
gerddie opened this issue Jan 21, 2022 · 3 comments

Comments

@gerddie
Copy link

gerddie commented Jan 21, 2022

Debugging an issue running Tomb Raider II by using the mesa/virgl driver within a Qemu VM showed a problem with the gamma correction as done with fshack: The shader doesn't specify any layout for the gamma array values which results that in this case the array values being allocated with a stride of 16 byte (vec4), but when the data is uploaded by using glBufferData it expects that the array is densely packed, and the result is that the "gamma corrected" output has only an incorrect red component.

According to OpenGL 4.6 (Core profile) May 14, 2018, section 7.6.2.2. the uniforms contained within a uniform block are extracted from buffer storage in an implementation-dependent manner and the offsets and alignments can by queried by using glGetActiveUniformsiv, and this latter step is missing here.

To avoid the querying and the dynamic setup of the data to be passed into the buffer one can declare the UBO as std140, and in this case the alignment of each array element is vec4 (according to the same section in the spec).

With that one could, for instance, use a shader like

static const char *fs_hack_gamma_frag_shader_src =
"#version 330\n"
"\n"
"uniform sampler2D tex;\n"
"in vec2 texCoord;\n"
"layout (std140) uniform ramp {\n"
"    vec3 values[256];\n"
"};\n"
"\n"
"layout(location = 0) out vec4 outColor;\n"
"\n"
"void main(void)\n"
"{\n"
"    vec4 lookup = texture(tex, texCoord) * 255.0;\n"
"    outColor.r = values[int(lookup.r)].x;\n"
"    outColor.g = values[int(lookup.g)].y;\n"
"    outColor.b = values[int(lookup.b)].z;\n"
"    outColor.a = 1.0;\n"
"}\n"
;

And pack the data passed into an array of 4 * 256 * sizeof(float) ordered [r,g,b,0].

@gerddie
Copy link
Author

gerddie commented Jan 21, 2022

I was able to reproduce the issue directly on the hardware, i.e. a r600 based graphics card that is a vec4 based architecture.

@aeikum
Copy link
Collaborator

aeikum commented Jan 21, 2022

@gerddie Thanks for the report! We'll take a look at this soon. We're currently in the middle of a major Wine upgrade, so may take some time.

@gofman
Copy link
Contributor

gofman commented Jan 21, 2022

Thanks, the changes based on this report are in Experimental ([bleeding edge] branch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants