Bug 66067 - Trine 2's fragment normal buffer is mixtextured on Radeon HD 6770 (Juniper)
Summary: Trine 2's fragment normal buffer is mixtextured on Radeon HD 6770 (Juniper)
Status: RESOLVED MOVED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/r600 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Default DRI bug account
QA Contact:
URL:
Whiteboard:
Keywords:
: 78262 (view as bug list)
Depends on:
Blocks: 77449
  Show dependency treegraph
 
Reported: 2013-06-23 05:46 UTC by Nicholas Miell
Modified: 2019-09-18 19:04 UTC (History)
9 users (show)

See Also:
i915 platform:
i915 features:


Attachments
rendering with Mesa d282f4ea9b99e4eefec8ce0664cdf49d53d7b052 (1.98 MB, image/png)
2013-06-23 05:46 UTC, Nicholas Miell
Details
rendering with Catalyst (2.16 MB, image/png)
2013-06-23 05:46 UTC, Nicholas Miell
Details
Call 5654455, GL_COLOR_ATTACMENT0, Mesa (1.19 MB, image/png)
2013-07-05 05:30 UTC, Nicholas Miell
Details
Call 5654455, GL_COLOR_ATTACHMENT0, Catalyst (2.37 MB, image/png)
2013-07-05 05:30 UTC, Nicholas Miell
Details
Hack to force SAMPLE instruction even for 2D shadow samplers (839 bytes, patch)
2013-09-17 23:26 UTC, Grigori Goronzy
Details | Splinter Review
radeonsi: Handle sampler depth compare mode (4.95 KB, patch)
2014-07-09 08:53 UTC, Michel Dänzer
Details | Splinter Review
LD_PRELOAD library to fix Trine's ARB shaders (4.45 KB, text/plain)
2014-12-13 05:26 UTC, Daniel Scharrer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nicholas Miell 2013-06-23 05:46:15 UTC
Created attachment 81244 [details]
rendering with Mesa d282f4ea9b99e4eefec8ce0664cdf49d53d7b052

Trine 2 colors/lighting are wrong with Mesa 9.2 and current git (d282f4ea9b99e4eefec8ce0664cdf49d53d7b052) on a Radeon HD 6770 Juniper XT.
Game renders correctly using the Catalyst drivers.

Colors are wrong enough to make some puzzles unsolvable because portions of the screen are too dark to see.

Sample apitrace is at
https://docs.google.com/file/d/0B8G3Ivdx_-JNTV96YjgyNWplZ0U/edit?usp=sharing

Attached screenshots were generated using glretrace -b -S 5670984
Comment 1 Nicholas Miell 2013-06-23 05:46:48 UTC
Created attachment 81245 [details]
rendering with Catalyst
Comment 2 Alex Deucher 2013-06-23 16:45:47 UTC
This may be related to bug 36236.
Comment 3 Nicholas Miell 2013-06-23 18:20:44 UTC
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.
Comment 4 Nicholas Miell 2013-07-05 05:30:12 UTC
Created attachment 82060 [details]
Call 5654455, GL_COLOR_ATTACMENT0, Mesa
Comment 5 Nicholas Miell 2013-07-05 05:30:51 UTC
Created attachment 82061 [details]
Call 5654455, GL_COLOR_ATTACHMENT0, Catalyst
Comment 6 Nicholas Miell 2013-07-06 05:59:04 UTC
It finally occurred to me to test the apitrace with Mesa LIBGL_ALWAYS_SOFTWARE=1 and it was rendered correctly.
Comment 7 Grigori Goronzy 2013-09-17 23:25:00 UTC
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.
Comment 8 Grigori Goronzy 2013-09-17 23:26:26 UTC
Created attachment 86032 [details] [review]
Hack to force SAMPLE instruction even for 2D shadow samplers
Comment 9 Nicholas Miell 2013-09-18 00:52:48 UTC
(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.
Comment 10 Nicholas Miell 2013-09-18 01:37:14 UTC
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.
Comment 11 Grigori Goronzy 2013-09-18 01:55:04 UTC
> 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.
Comment 12 Nicholas Miell 2013-09-18 04:16:15 UTC
(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.
Comment 13 Roland Scheidegger 2013-09-21 01:09:56 UTC
(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.
Comment 14 Sylvain BERTRAND 2014-05-05 15:27:53 UTC
*** Bug 78262 has been marked as a duplicate of this bug. ***
Comment 15 smoki 2014-07-04 03:13:14 UTC
 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 :).
Comment 16 Nicholas Miell 2014-07-04 04:18:28 UTC
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.
Comment 17 smoki 2014-07-04 06:25:25 UTC
 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 :).
Comment 18 Marek Olšák 2014-07-04 11:27:07 UTC
Radeon drivers look at the texture instruction to determine if shadow sampling should be used. The type of the texture doesn't matter.
Comment 19 Grigori Goronzy 2014-07-04 11:52:14 UTC
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. :(
Comment 20 Marek Olšák 2014-07-04 12:45:38 UTC
(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.
Comment 21 Michel Dänzer 2014-07-09 08:53:23 UTC
Created attachment 102470 [details] [review]
radeonsi: Handle sampler depth compare mode

This Mesa patch seems to fix the apitrace for me. Can you confirm?
Comment 22 Michel Dänzer 2014-07-09 08:54:36 UTC
Note that this patch is radeonsi specific, I'm afraid the same approach is not possible with r600g.
Comment 23 smoki 2014-07-09 10:54:20 UTC
(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 :).
Comment 24 Marek Olšák 2014-07-09 12:16:44 UTC
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.
Comment 25 smoki 2014-07-09 13:39:05 UTC
(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 :).
Comment 26 Nicholas Miell 2014-07-09 23:09:38 UTC
(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.
Comment 27 smoki 2014-07-10 00:03:54 UTC
 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.
Comment 28 Daniel Scharrer 2014-12-13 05:26:45 UTC
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?
Comment 29 Nicholas Miell 2014-12-13 05:38:04 UTC
Does it ever actually sample from a depth texture or is unconditionally converting the SHADOW2D to 2D safe?
Comment 30 Daniel Scharrer 2014-12-13 09:21:01 UTC
(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.
Comment 31 Daniel Scharrer 2015-09-13 20:02:45 UTC
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
Comment 32 Nicholas Miell 2015-09-13 22:47:24 UTC
(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.
Comment 33 Marek Olšák 2015-09-30 13:04:01 UTC
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.
Comment 34 Daniel Scharrer 2016-02-13 01:02:24 UTC
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.
Comment 35 Roland Scheidegger 2016-02-13 03:57:38 UTC
(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.
Comment 36 Roland Scheidegger 2016-02-13 04:02:11 UTC
(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).
Comment 37 Nicholas Miell 2016-02-13 04:29:34 UTC
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.
Comment 38 Roland Scheidegger 2016-02-13 18:01:56 UTC
(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...
Comment 39 GitLab Migration User 2019-09-18 19:04:01 UTC
-- 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.


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.