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

WIP - GLES: Download depth values via shader #11676

Merged
merged 5 commits into from
Dec 19, 2018

Conversation

unknownbrackets
Copy link
Collaborator

@unknownbrackets unknownbrackets commented Dec 17, 2018

This doesn't work on mobile, but I'm not sure why. It works on desktop (took some tweaking to get the depth values read accurately.) This works on depth and mobile now, at least NVIDIA and Adreno. Not actually triggered yet.

Obviously, for depth depal cases, we wouldn't download at all. But making depth downloadable is very useful to compare hardware tests of behavior, and is inevitably used by some games (probably Silent Hill.)

This is a cleanup of an old branch I was using. Notably, I looked at Aras' approach - it just didn't match the values correctly on desktop, though I'm sure it's a bit faster. I am worried about scenarios like Phantasy Star text boxes, and anyway inaccurate values will be trouble for tests.

Depth itself is working, so I think everything is okay in depth texture land. But the shader is just getting 0xc34f as the depth on Adreno, always.... somewhat mysteriously (no matter what value I clear it to.) NVIDIA (GL or GLES) gets it right.

Any ideas what I'm missing?

-[Unknown]

@hrydgard
Copy link
Owner

Could be yet another Adreno hardware bug (or possibly some obscure GLES thing).

I'm working on an in-app GPU functionality tester, which I will use to (in a roundabout way, don't ask) report the stencil/depth/discard bugs. Looks like another candidate for it.

uniform sampler2D tex;
void main() {
float depth = texture2D(tex, v_texcoord0).r;
float offset = 0.5 * (u_depthScaleFactor - 1.0) * (1.0 / u_depthScaleFactor);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you can just divide by u_depthScaleFactor, computing the inverse within parenthesis doesn't gain us anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think I just copied this from c++ for now, was trying to get it work first before optimizing or simplifying really. Could probably pass offset as a separate uniform to make sure the shader is simple.

-[Unknown]

// At this point, clamped maps [0, 1] to [0, 65535].
float clamped = clamp((depth - offset) * u_depthScaleFactor, 0.0, 1.0);

vec4 enc = vec4(16777215.0, 16777215.0 / 256.0, 16777215.0 / 65536.0, 16777215.0 / 16777216.0) * clamped;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the trouble on Adreno may be some kind of precision issue with this, or the mod below. Sad if so, but you know...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I suppose if they all become infinity or something broken, maybe that explains why every depth value turns into the same result. Will try.

Would be curious to know if this behaves any different on Mali or PowerVR.

-[Unknown]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending these constants as uniforms fixed it. Probably better anyway...

-[Unknown]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One would think the compiler would evaluate these at full precision, but apparently not then... Bizarre.

Fixes it on Adreno, no need to do the constant math in the shader.
@hrydgard
Copy link
Owner

Did you really have to add u_depthTo8 for it to work, or was it more of a might as well do it? Anyway, seems fine.

@hrydgard hrydgard merged commit 683a4e0 into hrydgard:master Dec 19, 2018
@hrydgard
Copy link
Owner

Hm, just realized this was still tagged WIP - oh well :)

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

Successfully merging this pull request may close these issues.

2 participants