Emulator

I messed up the links in the initial issue: last and second link are supposed to be fliped.

Can you maybe post the effect texture with Texture to RAM so we can see how different it is?

Yes definitely, here are a few things I noticed.
The texture in question is definitely supposed to be the Depth texture used for the DOF effect. It is however improperly computed because it seems to have extra white vertical lines in it for every other line which is correctly computed.

The EFB to ram seems to almost correctly generate this texture (except for the white lines). So there is a deeper issue here.

The reason this texture is less noticeable with EFB to ram is because the depth is actually in the texture opposed to strange ramp patterns seen in the EFB to texture.

You will also notice the source textures being smaller than the EFB to texture.

Stage before DOF:
http://minus.com/m4IjLPzKF#1

Depth texture itself:
http://minus.com/m4IjLPzKF#2

First SetRenderTarget after DOF:
http://minus.com/m4IjLPzKF#3

Is that with native IR resolution or x2? That's definitely a strange bug, maybe the EFB to RAM encoder is broken.

This capture was made with 2x native IR.

Can you reproduce these vertical lines with 1x native IR?

It does reproduce even with the native resolution. However the captures I've made so far were made with the DX9 plugin.

On DX11 I do see the same ramp pattern over my screen when using EFB to Texture hacks.
But it does not seem to exhibit the same vertical line behavior as the DX9 plugin when using EFB to ram.

I will take captures from DX11 later today.

This patch removed the banding from EFB to Texture for me (linux, Open GL):

diff --git a/Source/Core/VideoCommon/Src/TextureCacheBase.cpp b/Source/Core/VideoCommon/Src/TextureCacheBase.cpp
index d3b7ca1..fb1a8f6 100644
--- a/Source/Core/VideoCommon/Src/TextureCacheBase.cpp
+++ b/Source/Core/VideoCommon/Src/TextureCacheBase.cpp
@@ -473,7 +473,7 @@ void TextureCache::CopyRenderTargetToTexture(u32 dstAddr, unsigned int dstFormat
break;

            case 12: // Z16L

•                   colmat[2] = colmat[6] = colmat[10] = colmat[13] = 1.0f;
  

•                   colmat[3] = colmat[7] = colmat[11] = colmat[14] = 1.0f;
                    cbufid = 6;
                    break;
  

Testing patch now, will report changes on dx11

While i don't have a clue what kamm.christian changed he certainly seems to have fixed the issue for all the video backends with efb to texture. Below is a proper patch file for anyone to test with the latest dolphin version.

And a link to a x64 build for further testing:
http://www.mediafire.com/?aagf5fjkhdw5a3u

Lines removed on Dx11 :)

@kamm.christian: could you explain a bit why your patch fixes that? I checked the color matrix values and can't find out why the previous values are wrong.

kamm.christian, could you give more context on what you've done?

I took a look at the Shader that gets generated to consume these parameters, but have a feel the new values just generate a white depth texture. (far away texture)
I say this because the scene gets over blurred on DX11. You can see a difference between the EFB to RAM and TEXTURE very clearly. And you need to be hugging objects before the blur goes away. (I was not able to get PIX to work with the DX11 stream)

DX9 does not seem to have any DOF effect anymore.

Could you give a small breakdown of what your values are supposed to do?

Thanks

Given the observations reported here the patch is likely to be incorrect.

I saw that the PIXELFMT_Z24 to Z16L conversion was used for the depth texture and started looking at whether something might be wrong with the colmat values that I could fix without reading the surrounding code. I noticed that the indices for Z8M are +1 from Z8 and Z8L are +2 from Z8. So I tried making Z16L +2 from Z16.

Since that produces a depth effect I didn't look further. I haven't read up on how the shader that accepts the colmat values actually uses them.

Just a small correction, the effect is not entirely broken with direct 3d9, it works indoors, it doesn't while outdoors.

I disagree... it is definitely at least kind of broken in D3D9.

Outside Earth Temple: http://img830.imageshack.us/img830/1885/soue011.png
Inside Earth Temple: http://img713.imageshack.us/img713/6236/soue012.png

Outside there is no DOF at all. Inside there is far too much.

Compare with 7719 D3D11 + EFB Texture... look at the rug next to Link: http://img195.imageshack.us/img195/9601/soue016.png

•                    colmat[2] = colmat[6] = colmat[10] = colmat[13] = 1.0f;
  

•                   colmat[3] = colmat[7] = colmat[11] = colmat[14] = 1.0f;
  

Ok i played a bit, changing stuff and rebuilding, and from the 4 values changed from above only the third is the one that matters when it comes to the banding issue (colmat 10 ->11). Unfortunately it is also the one that creates the problem with too much dof effect.

Ho ho ho, success!
Instead of increasing the third value i decreased it to 9 and things work properly with direct 3d 11! Gotta test the rest...

colmat[2] = colmat[6] = colmat[9] = colmat[13] = 1.0f;

It works fine with direct 3d 9 too, i think it is perfect now. Here is a build:
http://www.mediafire.com/?5ic6pfrorf4rwsk

I can verify that setting colmat[9] to 1 also gives me the same DOF effect as texcache-rewrite with OpenGL. A more consistent choice for colmat might be

colmat[1] = colmat[5] = colmat[9] = colmat[14] = 1.0f;

which gives me the same results. That way all the color channels use the same mid-frequency value while the alpha channel holds the high-freq one. So it'd be the same as the current setting - only reversed.

good work!, the result speaks for itself. no more need for the texcache-rewrite build for zelda ss.

I don't want to sound pessimist, but doesn't this break other games ?

Anyway good job guys, hope it'll be committed soon :)

Yep, this smells like random code, and we all know how well that works.

As long as nobody can explain the colmat coeffs change correctly we can't commit that. For example, why are we duplicating a part of the depth value in r, g, and b? TCR does not do that as far as I know but packs two Z16L values into one output color.

Delroth, compared to having people use old builds and complaining around for old issues that are actually fixed, i don't think it is such a bad idea trying your luck with this and see where it goes...

I just checked the texcache-rewrite code about that (finally found it, VirtualEfbCopy.cpp) and it is indeed using GGGB like the values that kamm.christian is providing in comment 21.

However it's not documented and the comments in the TCR version explicitly say that it is untested, so I don't really know what to do about this. We probably need to code some samples to run on real hardware and compare with our results. For example, I still don't get why we should repeat the G value instead of using GB00.

TCR is not even consistent about that btw.

DX11 does GB00 according to this:
http://code.google.com/p/dolphin-emu/source/browse/Source/Plugins/Plugin_VideoDX11/Src/VirtualEFBCopy.cpp?name=texcache-rewrite#246

DX9 and GL do GGGB according to this:
http://code.google.com/p/dolphin-emu/source/browse/Source/Plugins/Plugin_VideoDX9/Src/VirtualEFBCopy.cpp?name=texcache-rewrite#293

Are there any noticeable differences between DX9/DX11 with EFB to Texture on a texcache-rewrite enabled build? Can't test that here, I don't have Windows installed.

Theres an issue with the values given in comment 21, while it looks good in almost everywhere there are afew areas where things do not look right, and the lines reappear.

Using Dx11 with comment 21:
http://i.imgur.com/XxWYj.jpg

Using Dx11 with texcache-rewrite:
http://i.imgur.com/YAqN2.jpg

Notice the border of the beam is much smoother on tcr, and is accurately represented. Going to test DX9 now.

DX9 looks funky, but the edges of beam are normal in both versions:

Comment 21:
http://i.imgur.com/j9zAs.jpg

TCR:
http://i.imgur.com/Mlxy0.jpg

Masterg.

Could you post screenshots of:
1: Latest version of Dolphin without patch.
2: Latest version with patch.
3: TCR.

The first set of screenshots dx11 from 221 so lemme recompile without patch.

Delroth now this is beginning to sound like a political issue :) However I do agree with you that without understanding exactly why the texture swizzling is broken for Z16L it should not be committed.

Therefore I suggest we discuss exactly what the Copy_depth_buffer shader does, and how it uses the colmatrix values.

Let us step through the implementation for D3D9 cause that's where I captured this from:

srcFormat = PIXELFMT_Z24
dstFormat = 12 // Some one could add enums for this, with names like Z16L, DEPTH_TEXTURE_16BIT?
colmat[2] = colmat[6] = colmat[10] = colmat[13] = 1.0f;
cbufid = 6;
scalebyhal = true // Because from the pix capture there is a call to SetSamplerState(0, D3DSAMP_MINFILTER, D3DTEXF_LINEAR) and I had msaa turned off.

void TextureCache::TCacheEntry::FromRenderTarget(u32 dstAddr, unsigned int dstFormat,
unsigned int srcFormat, const EFBRectangle& srcRect,
bool isIntensity, bool scaleByHalf, unsigned int cbufid,
const float *colmat)

We are going for the depth texture cause of srcFormat.
We will be calling GetDepthMatrixProgram(0, true);

The next few lines are a pix capture of the render to texture operation.
8298 <0x40000070> IDirect3DDevice9::SetRenderState(D3DRS_COLORWRITEENABLE, D3DCOLORWRITEENABLE_RED | D3DCOLORWRITEENABLE_GREEN | D3DCOLORWRITEENABLE_BLUE | D3DCOLORWRITEENABLE_ALPHA) 138871953575
8299 <0x400293A0> IDirect3DTexture9::GetSurfaceLevel(0, 0x400292E8 --> 0x400293A8) 138871956940
8300 <0x40000070> IDirect3DDevice9::SetDepthStencilSurface(NULL) 138871958622
8301 <0x40000070> IDirect3DDevice9::SetRenderTarget(0x00000000, 0x400293A8) 138871961146
8302 <0x40000070> IDirect3DDevice9::SetViewport(0x400292F0) 138871964511
8303 <0x40000070> IDirect3DDevice9::SetPixelShaderConstantF(0, 0x40029230, 7) 138871966474
8304 <0x40000070> IDirect3DDevice9::SetSamplerState(0, D3DSAMP_MINFILTER, D3DTEXF_LINEAR) 138871970119
8305 <0x40000070> IDirect3DDevice9::SetVertexShader(0x400001B0) 138871973204
8309 <0x40000070> IDirect3DDevice9::SetPixelShader(0x40000540) 138871976569
8310 <0x40000070> IDirect3DDevice9::SetTexture(0, 0x40022B10) 138871982457
8311 <0x40000070> IDirect3DDevice9::SetFVF(D3DFVF_XYZW | D3DFVF_TEX3) 138871984700
8312 <0x40000070> IDirect3DDevice9::DrawPrimitiveUP(D3DPT_TRIANGLESTRIP, 2, 0x400292F8, 36) 138871986944 9429840
8313 <0x40000070> IDirect3DDevice9::SetTexture(0, NULL) 138881516895

For my capture I wasn't using msaa, so this translates into the following shader:

const char depth_matrix_program[] = {
"sampler samp0 : register(s0);\n"
"Texture2D Tex0 : register(t0);\n"
"uniform float4 cColMatrix[7] : register(c0);\n"
"void main(\n"
"out float4 ocol0 : SV_Target,\n"
" in float4 pos : SV_Position,\n"
" in float2 uv0 : TEXCOORD0){\n"
"float4 texcol = Tex0.Sample(samp0,uv0);\n"
"float4 EncodedDepth = frac((texcol.r * (16777215.0f/16777216.0f)) * float4(1.0f,256.0f,256.0f*256.0f,1.0f));\n"
"texcol = round(EncodedDepth * (16777216.0f/16777215.0f) * float4(255.0f,255.0f,255.0f,15.0f)) / float4(255.0f,255.0f,255.0f,15.0f);\n"
"ocol0 = float4(dot(texcol,cColMatrix[0]),dot(texcol,cColMatrix[1]),dot(texcol,cColMatrix[2]),dot(texcol,cColMatrix[3])) + cColMatrix[4];\n"
"}\n"
};

(16777215.0f/16777216.0f) = 0.999999940395355224609375 // (nearing 1, I assume this is to hack around the float precision issues but this gets discarded by the shader compiler)
1.0f, 256.0f, 256.0f*256.0f, 1.0f, // This is a shader way of upshifting (left shifting) values. (<<0, <<8, <<16, <<0)
// float4(255.0f,255.0f,255.0f,15.0f)) / float4(255.0f,255.0f,255.0f,15.0f)
float4(dot(texcol,cColMatrix[0]),
dot(texcol,cColMatrix[1]),
dot(texcol,cColMatrix[2]),
dot(texcol,cColMatrix[3])) +
cColMatrix[4]; // This whole oneliner is a shader matrix multiplication.

Here is the compiled version

//
// Generated by Microsoft (R) HLSL Shader Compiler 9.29.952.3111
//
// Parameters:
//
// float4 cColMatrix[7];
// sampler2D samp0;
//
//
// Registers:
//
// Name Reg Size
// ------------ ----- ----
// cColMatrix c0 5
// samp0 s0 1
//

// init
ps_3_0
def c5, 256, 15, 0.00392156886, 0.0666666701
def c6, 0.249999985, 63.9999962, 16383.999, 0

// declarations
dcl_texcoord2 v0
dcl_texcoord3 v1
dcl_2d s0

// sampling
texld r0, v0, s0
texld r1, v0.wzzw, s0
add r0.x, r0.x, r1.x
texld r1, v1, s0
add r0.x, r0.x, r1.x
texld r1, v1.wzzw, s0
add r0.x, r0.x, r1.x

// encode depth
mul r0, r0.x, c6.xyzx // shift the data up.
frc r0, r0  
mul r0, r0, c5.xxxy 
frc r1, r0
add r0, r0, -r1
mul r0, r0, c5.zzzw

// matrix mask.
dp4 r1.x, r0, c0
dp4 r1.y, r0, c1
dp4 r1.z, r0, c2
dp4 r1.w, r0, c3
add oC0, r1, c4

I will dump the intermediate textures that get used for this shader and get back.

Latest version without patch:

Dx9:
http://i.imgur.com/plBX3.jpg
Dx11:
http://i.imgur.com/svXHb.jpg

Latest version with patch (comment 21):
Dx9:
http://i.imgur.com/0hwDb.jpg
Dx11:
http://i.imgur.com/B7iFT.jpg

TCR:

Dx9:
http://i.imgur.com/Mlxy0.jpg
Dx11:
http://i.imgur.com/YAqN2.jpg

Dx9 looks spot on with comment 21, it is only with DX11 that an issue appears.

Masterg.

Judging from your screens the patch does not break any old behavior.

I would the Blue lines are outside the scope of this issue. (Maybe you want to open a new issue?)

The patch sofar looks pretty good to me, I've gotten hung up playing the actual game instead of debugging it :)

Here are a couple of other games I tested that do not seem to have been regressed by this change:
Zelda TP
Donkey Kong Returns
new Super Mario Bros.

Masterg, when I get to the same point you were at I'll try to debug that issue too. But I think it may be something temple specific, while this issue just bugged me to death.

On my todo list is to prove: that the old texture copy transform is broken.

There must be something wrong with the DOF in general and i hope this is the right place to post it.
Besides that i have the feeling, the effect is much smoother on the Wii compared to Dolphin with NATIVE 1x resolution, i noticed the following:
These Screenshots have been taken from the SS Intro. The scene on Dolphin is completely blurred, when the DOF should only be in the foreground.

Wii: http://i.imgur.com/SlRJL.jpg

Dolphin: http://i.imgur.com/xRTJ6.jpg

This issue occurs regardless what graphics plugin, settings, internal resolution or build you are using - even with the newest one posted on this page.
I hope you can figure out, what might be wrong.

Delroth if what you say is true then there is an easy way to test for possible regressions, see the issue list that games have for texcache rewrite(Issue 4635) and testing them would be pretty sufficient. Personally i tested plenty of games and haven't found an issue.

WMRPoker, that is one interesting observation that needs testing, did you use the build i posted or the values in comment 21 to test?

I tried the build you posted, the TCR and the WM+ emu ones John Peterson made - if i've got it right, he already implemented your code in the newest version (3.0-243).
Sidenote: I am no expert at all and wouldn't know how to use the values from #21.

WMRPoker, it seems that the problem you talk about has also some relation with the bloom offset issue with dolphin. Whatever fixes the bloom offset will fix that too, but i think it is a different issue altogether...

Ok i tried every game that had issues with texcache rewrite and even more and i couldn't spot any issues whatsoever caused by kamn.christian values:
colmat[1] = colmat[5] = colmat[9] = colmat[14] = 1.0f;
Imo it should be committed, it is very unlikely to cause any problems at all...

The only way to definitely know if it causes problems is to commit it in master or in a separate branch (although it seems to me that very few people usually test branches).

Or, you know, verify the new behavior against real HW or at least understand wtf is actually being done by replacing 10 with 9 ;)

Yep, you're right, ZSS might be fixed by replacing only third value 10 to 9, all others should be left by default to avoid additional problems with other games

OK, Kosta. Shall i delete my comments then?

WMRPoker, there are a lot of literally irrelevant spam that don't get deleted around here, imo in your case there is no need, so it is your call if you want to or not.

Okay, so let's drop the bomb...

As it seems right now the current efb2tex code is even more of an incredible hack than I thought before. Usually, EFB copies with a GX_CTF_* format can't directly be used as textures, but need to be loaded using a different format (GX_TF_*). Thus, it's not really determined how the copy target will "look" until the application has specified what format to use for the texture.
Now, the Dolphin implementation of efb2texture completely ignores this fact and just hardcodes a format for the efb copies, hoping that it's actually the format that will be used lateron. Obviously this causes lots of bugs and indeed given enough effort the efb2texture implementation could be made much more compatible (not perfect, but probably good enough to work with more games). However, that's not the whole story, yet:
In this particular case we have a Z16L target - and as far as I can see Dolphin hardcodes a texture format of IA8 (and that pretty badly btw). And I don't even know if that's the correct expectation in this case, but: We're most likely converting this stuff wrong.
Normal IA8 textures are represented internally using RGBA8 textures (in our D3D11 backend, that is) - the A channels are the same, the RGB and I channels have the same value. I.e. we'd have to store the upper 8 bits of the "lower 16 depth bits" as the RGB values (i.e. the same 8 bits three times) and the lower 8 bits of the "lower 16 depth bits" as the A values (not sure about the order, but you get the idea).
However, what we're doing right now is that we're storing the "lower 16 depth bits" as the A value and the "lower 24 [sic] depth bits" as the RGB bits. That's just plain wrong and partly a limitation of the depth matrix code and (most importantly) probably the cause of this issue.

This explanation isn't meant to be any useful for anyone who doesn't know the code, I just wanted to have it written down in case anyone comes around and wants to have a reasonable explanation of what's going on.

tl;dr: It's not simple to fix and whatever that colmat change is doing is just another hack on top of the existing bunch of other ones.

Neobrain, if it helped to track the source of the issues it is still good imo, since the ending results of all this might be even better, not only for this game but for others as well. 2 questions though:

1. Does that mean it needs a lot of work and rewriting of code?
2. Do you have any "plan" let's say of what might be needed to fix it properly and are you willing-or have time to try to do it?
   Tnx man.

1./2.: It's not a bad idea to hardcode stuff the way it was done, but a crapload of work will be needed to document the stuff (there's NO single mention anywhere what expectation is actually hardcoded). The proper fix would be an insane amount of work, but I guess it'll be enough to just implement the cases which are necessary to make games work. That'll take more time for testing, but shouldn't be too much work (given that anyone bothers to do it).

The amount of replaced code is probably small, it'll just add a bunch of new code... On top of our already crappy texcache code.

2. Maybe, maybe not. Depends on how much I feel like wasting my time looking at the mess of code which is our texture cache :/

"This explanation isn't meant to be any useful for anyone who doesn't know the code"

It might not be useful, but it is really interesting.

I call for comitting the hack, with a comment (and man are those scarse) and link to this issue.

//Hack: workaround for currently broken depthbuffer issue: http://code.google.com/p/dolphin-emu/issues/detail?id=5056

Make it a switchable hack, like projection hacks. Seems better than applying it to every games.

That would take more work and bog down the performance unless implemented correctly. People often think "just add an option" is a good idea, but in software design it often not a good choice to make.

Hmm, committing it for the time being would be nice for one reason (the same that i told delroth), it would prevent people of using and staying with an old and even more problematic build as a regular build. While i have commit rights though i am not really willing to do it guys, since i am mainly the ini database maintainer. As for the make it a toggling hack i don't think it is necessary, since it doesn't create any issues for other games and the logic behind of it is sound (texcache rewrite uses same values, yet the games reported to have issues with that branch don't have with this, nevermind other games were tested too with no issues).

For the sake of not breaking other games until next release, I'm all against committing the hack. No one can test all games out there and say for sure that none of them regressed.

kostamar: You can't just say commit it since it does not cause any issues, you don't know that.

As I do not think the battle about commiting or not should be fought out in a bug report:

Neo: it looks to me like removing TCR broke a bunch of games. This "hack" reintroduces DX11 TCR behavior of grabbing the GGGB components of the depth texture. (as was stated in the constant definitions)

To me it seems that you are not introducing new behavior, but merely reverting to the old.

I do not have commit rights nor did I come up with the fix, but looking at the current implementation of Z16L and how it is handeled plus debugging this in pix. I cannot see how the current implementation is supposed to work anyway.

Neo I challange you to find a game that is currently working with the current implementation :)

The current impl for Z16L is just broken altogether, but replacing it with something which is just a bad hack as well isn't much better.

Btw, could you check what format Zelda SS is using when that Z16L copy target is being applied as a texture?

TCR has not been removed, it was never merged in the first place (if we ignore 2 or 3 weeks between 3.0 release and migration to Git) because it caused a lot of issues in a lot of games.

The current EFB to Texture is broken and trying to apply random fixes hoping it will solve problems won't work. Using TCR behavior for Z16L will likely break other games using Z16L copies.

BTW, if I understand things correctly GGGB is not correct for Z16L -> IA8 either: I should be the lower part of Z16 (B) and A should be the upper part (G), giving BBBG instead of GGGB. Also, DX11 TCR does not convert to IA8 and does not do GGGB (it does GB00, see comment 28.

+1 to no_cluez, I definitely won't commit such a hack without understanding the consequences it has.

tommyhl2, i don't maintain the ini database for no reason you know ;-), i am among the most active testers of dolphin right now. I am 99.9% sure that it doesn't create any new issues. And my reason of wanting this to be committed as i said was this "It would prevent people of using and staying with an old and even more problematic build as a regular build".
Neobrain, you can commit it if you want and blame me if anything goes wrong for false information :-p.

Btw, could you check what format Zelda SS is using when that Z16L copy target is being applied as a texture?

Do you mean the internal Emulator format, or the D3D texture format?

kost: Not to get into a pissing match here, but to be 99% sure you would have to have tested every GC and Wii game known to exist with many different settings and scenarios. I highly doubt that you've done that in the last few minutes.

[SS]

tommyhl2.SS
"http://code.google.com/p/dolphin-emu/source/detail?r=8d9061ac39ee8841557ce461c48fdc2791ec8a20"
....Dude try to be repetitive in a positive manner next time, works better in the end, you will see...

Kost: Other than me being right in that issue and here, what does that issue have to do with this? Master is not the place to test half-cocked untested changes that are pure guesses(this is not SVN where you just commit stuff and cross your fingers, that's what a branch is for). Saying that you know for a fact that there aren't any games with issues because of this change is just plain ignorant and you know it. I'm with you in wanting Skyward to work well, but this is not the way.

Tommy since you were right and i was wrong, do you want me to revert that change? I have no problem really, i will also thank you in the commit for making me see how wrong i was. I might even not try to help any more with dolphin, since according to you making mistakes while trying to help around here is like a habit of mine, and maybe leaving the project would be better for everyone. Personally i am in. What say you?

There are still a lot of commits on master that break some things, and always will be.
I think that no one can be 100% sure of not breaking something even with the simplest commit on earth.
To be near 100% sure you need testing from a lot of users, but if you don't commit the changes you won't ever know if they really break anything.

Again, it's sad that general consensus here seems to be "test stuff by committing it and potentially fucking up master!" rather than "wait until people who know what they're doing have figured out what's wrong and how to fix it".

Anyway, let's all calm down and don't take stuff personally. Remember, it's the internet and stuff >_>

Point is, this is unlikely to be committed in its current state. Also please refocus the discussion on the issue itself or I'll restrict commenting to project members.

This issue was closed by revision 3d9c35f58e84.

This issue was closed by revision 3d9c35f58e84.

This issue was closed by revision 3d9c35f58e84.