Fix copy filter clamping #10222
Fix copy filter clamping #10222
Conversation
This fixes horizontal lines in the bloom effect of Spyro: A Hero's Tail, which is a regression caused by PR dolphin-emu#10204 Screenshot of regression: https://user-images.githubusercontent.com/138484/142030503-90fcd8d5-63d3-4820-874a-72e9be0c4768.png Fixed: https://user-images.githubusercontent.com/138484/142031598-b85ff55c-1302-4e4d-bcb2-57848974056b.png Spyro uses an 640x80 pixel sub-buffer within the EFB to calculate it's bloom effects, which it places below the main 640x448 buffer. EFB layout: https://user-images.githubusercontent.com/138484/142030573-e933b6ae-c37e-4be6-86d4-0bc779b92535.png Note: Colors are wrong because the main color buffer uses RGBA6, while the bloom is calculated in RGB8 This allows it to do bloom without backing up part of the EFB to main memory, as most games do. But, since some of the sub-buffers used in the bloom effect are taller than 80 pixels, they need to be sliced up into smaller sub, sub buffers which get combined later when copied to main memory. At one point, a 320x224 buffer is broken up into 320x80, 320x64 and 320x80 slices. These are copied out with the copy filter set to a vertical blur. Because there was an off-by-one errror in the clamping coordinates, the bottom line of the color buffer would be blurred into the top of each slice. Final combined EFB copy: https://user-images.githubusercontent.com/138484/142031360-2c076839-7c96-4b3b-a093-d899d0a2c7ae.png Fixed version: https://user-images.githubusercontent.com/138484/142031370-72e41a35-3b3e-4662-a483-79203e357ecc.png Before dolphin-emu#10204 the copy filter wasn't enabled for efb copies, and most other games don't do this type of slicing. FIFO CI shows that a few other games are effected, it's always just a minor difference to the top line where there was previously a slight hint of garbage.
phire
force-pushed
the
fix-copy-filter-clamping
branch
from
5f4f49d
to
7128bef
Compare
| uniforms.clamp_top = clamp_top ? framebuffer_rect.top * rcp_efb_height : 0.0f; | ||
| uniforms.clamp_bottom = clamp_bottom ? framebuffer_rect.bottom * rcp_efb_height : 1.0f; | ||
| int bottom_coord = framebuffer_rect.bottom - 1; | ||
| uniforms.clamp_bottom = clamp_bottom ? bottom_coord * rcp_efb_height : 1.0f; |
There was a problem hiding this comment.
This is the corresponding code in the software renderer (which only exists in EncodeXFB; the equivalent for EFBs doesn't handle clamping or the copy filter currently):
dolphin/Source/Core/VideoBackends/Software/EfbInterface.cpp
Lines 587 to 612 in 0b81640
Is it also affected? Given that y_prev can be 0 and y_next can be EFB_HEIGHT or source_rect.bottom I suspect it is... but I'm not 100% certain. Since it's not implemented for EFBs at all currently this isn't too pressing.
I also need to question the 1.0f in this code, though; shouldn't it be (EFB_HEIGHT - 1) * rcp_efb_height? Or does that not matter because the texture is clamped elsewhere as well?
There was a problem hiding this comment.
Looks like the same bug. The for loop iterates from top to bottom - 1, yet it clamps at bottom.
As for the 1.0f... I'm not sure, but I suspect our whole coordinate system might be off-by-one here.
There was a problem hiding this comment.
I've fixed videosoftware.
I'll have to look more deeply into the UV coordinates later.
There was a problem hiding this comment.
The fifoci dffs for software are most visible in mii-channel and nhl-slap; I took a look at mii-channel and it has clamping enabled, so the garbage data going away is correct (it's not an out-of-bounds read that the comment warns about).
There was a problem hiding this comment.
As I previously guessed, the texture is clamped elsewhere: either GetLinearSamplerState or GetPointSamplerState is used:
dolphin/Source/Core/VideoCommon/TextureCacheBase.cpp
Lines 2698 to 2699 in 0b81640
and those clamp u and v:
dolphin/Source/Core/VideoCommon/RenderState.cpp
Lines 357 to 358 in 0b81640
dolphin/Source/Core/VideoCommon/RenderState.cpp
Lines 372 to 373 in 0b81640
So, it won't go out of bounds, though whether this results in things being off slightly I'm less sure. With linear filtering I vaguely think it might be correct to use 1.0f but I'm iffy on that.
There was a problem hiding this comment.
Yeah, the texture is clamped.
If the coordinate space is off-by-one then it should still produce the correct result for point-sampled copies, but will produce a slightly wrong result for linear sampling.
|
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
This fixes horizontal lines in the bloom effect of Spyro: A Hero's Tail which is a regression caused by PR #10204.
Screenshot of regression
Fixed
Spyro uses an 640x80 pixel sub-buffer within the EFB to calculate
it's bloom effects, which it places below the main 640x448 buffer.
EFB layout
Note: Colors are wrong because the main color buffer uses RGBA6, while the bloom is calculated in RGB8
This allows it to do bloom without backing up part of the EFB to
main memory, as most games do.
But, since some of the sub-buffers used in the bloom effect are taller
than 80 pixels, they need to be sliced up into smaller sub, sub buffers
which get combined later when copied to main memory.
At one point, a 320x224 buffer is broken up into 320x80, 320x64 and
320x80 slices. These are copied out with the copy filter set to a
vertical blur.
Because there was an off-by-one errror in the clamping coordinates,
the bottom line of the color buffer would be blurred into
the top of each slice.
Final combined EFB copy
Fixed version
Before #10204 the copy filter wasn't enabled for efb copies, and most
other games don't do this type of slicing.
FIFO CI shows that a few other games are effected, it's always just a minor difference to the top line where there was previously a slight hint of garbage.