Bug 66067

Created attachment 81245 [details]
rendering with Catalyst

This may be related to bug 36236.

Bug 36236 is about Trine, not Trine 2, but possibly.

There's also bug 60553, which involves Trine 2, but a Northerns Islands GPU and differences in rendering when fullscreen vs. windowed.

Created attachment 82060 [details]
Call 5654455, GL_COLOR_ATTACMENT0, Mesa

Created attachment 82061 [details]
Call 5654455, GL_COLOR_ATTACHMENT0, Catalyst

It finally occurred to me to test the apitrace with Mesa LIBGL_ALWAYS_SOFTWARE=1 and it was rendered correctly.

This actually seems to be a bug in Trine 2. The fragment shader uses a shadow sampler to sample the normal texture, which is RGBA. This cannot work as shadow sampling is only defined for depth textures. According to the ARB_fragment_program_shadow specification (Trine 2 uses those old assembly-like shaders for some reason), it is undefined behaviour to sample from non-depth textures. Obviously, Trine 2 relies on a certain behavior, i.e. that shadow samplers are silently changed into normal 2D samplers when not sampling from depth textures.

I have contacted Frozenbyte support, let's see if they'll do something about. Otherwise we need to check if it is reasonably possible and sensible to implement these undefined cases like other drivers. This might get ugly, though.

If you just want to play the game and don't mind hacking mesa, try the nasty patch I attached. This will break other applications, but makes Trine 2 work.

Created attachment 86032 [details] [review]
Hack to force SAMPLE instruction even for 2D shadow samplers

(In reply to comment #7)
>Trine 2 uses those old assembly-like shaders for some reason

It's the minimum spec target profile for Nvidia's Cg compiler.

> This cannot work
> as shadow sampling is only defined for depth textures. According to the
> ARB_fragment_program_shadow specification (Trine 2 uses those old
> assembly-like shaders for some reason), it is undefined behaviour to sample
> from non-depth textures.

ARB_fragment_program_shadow may leave it undefined, but the GL spec says the texture unit operates in the normal manner and texture comparison is bypassed if the currently bound texture's base internal format is not DEPTH_COMPONENT or DEPTH_STENCIL.

To quote the Cg tutorial:

"Notice that the code for the shadow map query is virtually identical to the code for the projective texture lookup in Example 9-6. To a Cg programmer, shadow maps are like other projective textures, except that instead of returning a texture color,tex2Dproj returns a value representing shadow coverage. The underlying hardware implementation automatically recognizes when a texture is a depth texture (instead of a color texture) and performs the shadow map comparison instead of an ordinary texture fetch."

I suspect that the Cg compiler always compiles tex2Dproj to a TEX instruction with the SHADOW2D target and relies on OpenGL implementations always transparently using normal sampling on non-depth non-stencil textures.

> ARB_fragment_program_shadow may leave it undefined, but the GL spec ...

Which spec exactly? GL specifications only cover GLSL shaders. ARB_fragment_program is separate from that and has its own rules. The extension specification clearly states that behaviour is undefined.

Besides, OpenGL contradicts itself a little bit: in GL 4.4 spec, 15.2.1 it states that behaviour is undefined for this case, too. It makes much more sense anyway, shadow sampling is wildly different from sampling a texture normally in terms of what the sampling result look like.

(In reply to comment #11)
> > ARB_fragment_program_shadow may leave it undefined, but the GL spec ...
> 
> Which spec exactly? GL specifications only cover GLSL shaders.
> ARB_fragment_program is separate from that and has its own rules. The
> extension specification clearly states that behaviour is undefined.
> 
Section 8.23.1 of the GL 4.4 spec, which I paraphrased in comment #9.

(In reply to comment #12)
> (In reply to comment #11)
> > > ARB_fragment_program_shadow may leave it undefined, but the GL spec ...
> > 
> > Which spec exactly? GL specifications only cover GLSL shaders.
> > ARB_fragment_program is separate from that and has its own rules. The
> > extension specification clearly states that behaviour is undefined.
> > 
> Section 8.23.1 of the GL 4.4 spec, which I paraphrased in comment #9.

This only covers the DEPTH_TEXTURE_MODE but not the shadow target of arb_fragment_program_shadow.
To quote from there:
How should ARB_fragment_program_shadow function?
        a. Simply remove the interaction with ARB_shadow so that
           TEXTURE_COMPARE_MODE behaves exactly as specified in the
           OpenGL 1.4 specification.
        b. Add "SHADOW" targets to texture lookup instructions.
           TEXTURE_COMPARE_MODE is ignored.  For samples from a SHADOW target
           TEXTURE_COMPARE_MODE is treated as COMPARE_R_TO_TEXTURE;
           otherwise, it is treated as NONE.
        c. Like (b), but with undefined results if TEXTURE_COMPARE_MODE and/or
           the internal format of the texture does not match the target.
        d. A hybrid of (a) and (b), where the SHADOW target means to use the
           TEXTURE_COMPARE_MODE state.

      RESOLVED - Option c, undefined behavior when the target and
      mode do not match.

So if the cg compiler really does something as you suggested it is simply crazy to expect this to work. At least I see absolutely nothing why that GL wording somehow would apply to this (non-core) extension.

*** Bug 78262 has been marked as a duplicate of this bug. ***

 With current driver stack, kernel 3.16-rc3, mesa-10.3-devel (up to commit 	370184e813b25b463ad3dc9ca814231c98b95864) and llvm 3.5 snapshot (svn svn212056) on radeonsi (Radeon 8400), etc... much of this darkness in Trine 2 disappear... i mean render much nearly correct :), only some little parts remain being black :).

It isn't an issue of darkness (although that's one way this bug manifests itself) the problem is the the lighting in the scene is generally wrong.

The game expects shadow sampling from a non-depth texture to silently fallback to ordinary sampling, and it expects this behavior because that's what the real vendor drives do in this circumstance.

This is very easy to observe even without using apitrace and looking at components of the FBO: just create a box with the wizard character and watch the gears animate -- they'll pulse between dark and glowing instead of being correctly lit by the light sources in the scene. You can see this by comparing the first two screenshots attached to this bug -- in the Catalyst rendered scene, the cubes in the lower right are lit by the adjacent light, in the Mesa scene they're essentially random.

 Yes just checked out your trace and got the same screenshot... yes sorry you are right, but seems to me radeonsi was having a more black even than this seems like onother bug which got fixed on top of this bug (Sylvain from comment 14 may confirm that with radeonsi). It was like this https://bugs.freedesktop.org/attachment.cgi?id=99363 and now is much better, but not complitely correct yet.

 Sorry it is just that radeonsi is now in the same state as r600 to actually have this bug you describe :).

Radeon drivers look at the texture instruction to determine if shadow sampling should be used. The type of the texture doesn't matter.

By the way, I really wonder how this manages to work correctly with the blobs. Are they fixing up the shader on demand somehow?

Trine 2 (the Steam version) was updated a while ago, but sadly they haven't fixed this, despite promising to do so. :(

(In reply to comment #19)
> By the way, I really wonder how this manages to work correctly with the
> blobs. Are they fixing up the shader on demand somehow?

No idea. Catalyst should be broken too unless it recompiles shaders. The GL spec says that if GL_TEXTURE_COMPARE_MODE doesn't match the sampler type, the behavior is undefined.

Created attachment 102470 [details] [review]
radeonsi: Handle sampler depth compare mode

This Mesa patch seems to fix the apitrace for me. Can you confirm?

Note that this patch is radeonsi specific, I'm afraid the same approach is not possible with r600g.

(In reply to comment #21)
> Created attachment 102470 [details] [review] [review]
> radeonsi: Handle sampler depth compare mode
> 
> This Mesa patch seems to fix the apitrace for me. Can you confirm?


 I can confirm :) i tried this patch and this works fine for me on Kabini (Radeon 8400), 'glretrace -b -S 5670984' produce same image as Catalyst. Cool :).

Hi Michel,

I'm going to switch all sampler descriptors to vNi32, so it would be nice if the code used that type instead of vNi8.

Also, the function deserves a comment explaining how it works.

(In reply to comment #23)
> (In reply to comment #21)
> > Created attachment 102470 [details] [review] [review] [review]
> > radeonsi: Handle sampler depth compare mode
> > 
> > This Mesa patch seems to fix the apitrace for me. Can you confirm?
> 
> 
>  I can confirm :) i tried this patch and this works fine for me on Kabini
> (Radeon 8400), 'glretrace -b -S 5670984' produce same image as Catalyst.
> Cool :).

 BTW just to be aware of i see some side effects of this patch, for example performance in Unigine Sanctuary goes down by ~40% for me :).

(In reply to comment #25)
>  BTW just to be aware of i see some side effects of this patch, for example
> performance in Unigine Sanctuary goes down by ~40% for me :).

Probably because it adds a branch to every single sampler.

 Probably. Anyway with this patch somehow Unigine Sanctuary high shader is affected, that huge performance drop happens only on Shaders:High setings, but as i see performance is not reduced at all on Medium or Low shaders setting.

Created attachment 110801 [details]
LD_PRELOAD library to fix Trine's ARB shaders

Trine Enchanted Edition (port/remake of Trine 1 in the Trine 2 engine) has the same lighting problem.

Since the attached radeonsi patch no longer applies, I have created a small LD_PRELOAD library to change all instances of " SHADOW2D;" to "       2D;" in ARB shaders. Using this confirms that it's still the same bug.

Obviously fglrx somehow silently falls back to non-shadow samplers. Is this also the case for the nvidia driver with ARB shaders or does that just get fed Cg shaders directly? If so, would it be feasible for Mesa to include something like the "Handle sampler depth compare mode" but for ARB shaders only? Or at least a drirc option - either to decide based on the compare mode or to disable shadow samplers completely?

Does it ever actually sample from a depth texture or is unconditionally converting the SHADOW2D to 2D safe?

(In reply to Nicholas Miell from comment #29)
> Does it ever actually sample from a depth texture or is unconditionally
> converting the SHADOW2D to 2D safe?

In the apitraces I have, depth formats (GL_DEPTH_COMPONENT* and GL_DEPTH*_STENCIL*) are only ever used for renderbuffers. I also haven't noticed any visual problems with the shadows other than ones that are also there with fglrx.

No idea if this is true for the whole game and for all video settings (I only tried Very High graphics detail + FXAA + 2xSSAA). It can also of course change with future updates.

I have found another game that hits this: he Book of Unwritten Tales: The
Critter Chronicles. Again, my workaround works for me. Unlike Trine however, this game ships its Cg shader sources as plain text files.

Changing the tex2Dproj() calls to use the correct overload by replacing "tex2Dproj(cookieMap, IN.LightSpacePos)" with "tex2Dproj(cookieMap, IN.LightSpacePos.xyw)" in kAGE_phong.cgfx and kAGE_phong_simple.cgfx fixes it for me. I've sent an email to the developer so hopefully this will be fixed in the game.

Meanwhile Trine is still broken on radeonsi. Since it works with fglrx, do we know how it handles mismatched GL_TEXTURE_COMPARE_MODE and sampler in the shader?

> I suspect that the Cg compiler always compiles tex2Dproj to a TEX instruction with the SHADOW2D target and relies on OpenGL implementations always transparently using normal sampling on non-depth non-stencil textures.

It selects between the 2D and SHADOW2D samplers based on the number of components in the coordinate vector. This matches the documentation [1] which, unlike the tutorial you quoted, makes no mention of the shadow test depending on the texture format or compare mode.

[1] http://http.developer.nvidia.com/Cg/tex2Dproj.html

(In reply to Daniel Scharrer from comment #31)
> It selects between the 2D and SHADOW2D samplers based on the number of
> components in the coordinate vector. This matches the documentation [1]
> which, unlike the tutorial you quoted, makes no mention of the shadow test
> depending on the texture format or compare mode.
> 
> [1] http://http.developer.nvidia.com/Cg/tex2Dproj.html

Yeah, my understanding and explanation as recorded in this bug is scattershot, misleading, and wrong.

I've meant to update it with an accurate summary of the problem, but there seemed to be hostility towards the idea of matching nvidia's behavior instead of demanding everybody else change their (technically buggy but in practice perfectly correct) software so I haven't bothered.

Our hardware doesn't have a compare mode state in the sampler. We tried to emulate it in the shader, but the performance was bad. Thus, there is no plan to fix misbehaving apps at the moment.

Came across another game that does this wrong: Never Alone (http://store.steampowered.com/app/295790) However this one does need shadow comparison in other shaders.

I've contacted the developers about this (as I have for the other affected games) but I fear that this problem will just keep coming up - especially with the NVIDIA driver ignoring the shadow sampler from the shader and Catalyst also having a workaround (at least the Trine games rendered correctly with it the last time I tried). Additionally at one of the developers I have contacted suggested they are unlikely to release another patch so this will likely remain broken.

Would it be possible to - instead of branching in the shader - create shader variants if the TEXTURE_COMPARE_MODE does not match up with the use in the shader?

Again, would be good to know how the blob handles this.

(In reply to Daniel Scharrer from comment #34)
> Came across another game that does this wrong: Never Alone
> (http://store.steampowered.com/app/295790) However this one does need shadow
> comparison in other shaders.
> 
> I've contacted the developers about this (as I have for the other affected
> games) but I fear that this problem will just keep coming up - especially
> with the NVIDIA driver ignoring the shadow sampler from the shader and
> Catalyst also having a workaround (at least the Trine games rendered
> correctly with it the last time I tried). Additionally at one of the
> developers I have contacted suggested they are unlikely to release another
> patch so this will likely remain broken.
> 
> Would it be possible to - instead of branching in the shader - create shader
> variants if the TEXTURE_COMPARE_MODE does not match up with the use in the
> shader?
Possible, sure. Essentially you'd have to put a mask into the shader key indicating which texture units have the PIPE_TEX_COMPARE_R_TO_TEXTURE bit set in the sampler (well, for used units and only those which actually have shadow targets). But then you'd have a dependency on sampler changes so you'd need to recheck that on changes there and recompile (well, recompilation shouldn't be an issue, as you'd only ever have to do it for those totally broken shaders, and only once for each shader).

I blame cg - makes it super easy to mistakenly use the shadow variant of a texture instruction. At least I haven't seen anyone mistakenly using shadow variant with glsl... I'm wondering though why anyone is still using that stuff.

> Again, would be good to know how the blob handles this.
It would be possible to recognize this bug only on ARB_program shaders, so you only have some extra cost there - that is make some shader key there and recompile if it doesn't match currently bound samplers. This actually should be easier - core gl requires depth_compare_mode being ignored for non-depth textures (which is why the sampler code in mesa/st checks the base format and only sets depth compare mode with depth textures) but there is no such requirement in ARB_fp_shadow as this needs to match (well, just as it needs to match the target in the shader... just don't tell me the apps don't get this right neither...). This means you don't have a dependency on the bound textures, only samplers. I suppose it could be done in the mesa state tracker that way outside of drivers but I'm not sure if anyone is really thrilled... Those shaders are really simply terribly broken, broken, broken (and people are probably less interested in trying to come up with some quite hacky, creative workarounds if it's really clearly the fault of others...) - there's very good reasons behavior is undefined for such shaders.

(In reply to Roland Scheidegger from comment #35)
Actually don't you get some comment in the ARB_fp_program it was compiled from cg? You could just take that as a hint you're going to see that bug and invoke the additional checks only then (as I don't think you'd ever see that bug outside of cg generated programs).

You could look for "# cgc version" near the beginning of the program source (almost certainly the second line), but that assumes the shaders weren't post-processed to strip out all the comments.

Alternately, you could just disable it entirely by default and include a driconf option.

The fact remains that the games work correctly on other OpenGL implementations and are ancient enough that they're no longer supported and will never be fixed.

(In reply to Nicholas Miell from comment #37)
> You could look for "# cgc version" near the beginning of the program source
> (almost certainly the second line), but that assumes the shaders weren't
> post-processed to strip out all the comments.
> 
> Alternately, you could just disable it entirely by default and include a
> driconf option.

Actually I suppose in theory it would be possible to do this in the state tracker with relatively little overhead. You'd just have to validate the shadow targets the first time you draw for arb_fp shaders iff they have shadow sampler dcls and change them to non-shadow if the samplers don't have compare mode set (it would have to be conditional on a driconf variable though that way, because if you only validate the first time it would break conformant apps which happen to have the samplers wrong the first time they draw which could for instance happen if they try to force early compilation). Afterwards no further validation should be required.
Not sure it's all that feasible though, for instance shader validation happens before texture/sampler validation so you'd need to be careful, probably would make the code somewhat ugly just for fixing broken shaders...

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/444.