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

SW: Fix alignedWidth in TextureEncoder #9646

Merged

Conversation

PatrickFerry
Copy link
Contributor

This was causing issues in Software Renderer.

Attempting to solve https://bugs.dolphin-emu.org/issues/11487

@shuffle2
Copy link
Contributor

for which values of width, sBlkSize do you see this change producing different result from Common::AlignUp? That there is any difference suggests a kinda catastrophic bug in AlignUp

@shuffle2
Copy link
Contributor

It also seems like sBlkSize and tBlkSize are u16s that only store a power of 2 value, so the type of the args to SetSpans should be u16, and ENCODE_LOOP_BLOCKS should be changed to make t/s u32s?

@shuffle2
Copy link
Contributor

Oh, I missed that your replacement is (value + align) & ~(align-1) instead of the typical (value + align - 1) & ~(align-1). I guess this is because width is 1 less than the number of pixels of width

@PatrickFerry
Copy link
Contributor Author

PatrickFerry commented Apr 18, 2021

I would be happy for someone else to take over since I'm not sure what the alignedWidth value should be in this case.

width = 388
sBlkSize = 4

(Broken)
Common::AlignUp(width, sBlkSize) = 388

(Seems to work)
Common::AlignUp(width+1, sBlkSize) = 392

(Seems to work and was how this written before d7dc854)
(width + sBlkSize) & (~(sBlkSize - 1)) = 392

@PatrickFerry PatrickFerry marked this pull request as draft April 18, 2021 00:58
@shuffle2
Copy link
Contributor

shuffle2 commented Apr 18, 2021

Yea I think this PR is fine as-is, just took me a while to realize that d7dc854 caused that comment (width is 1 less than the number of pixels of width) to go out of sync with what the code was actually doing. (...right? 🙃)

@PatrickFerry
Copy link
Contributor Author

PatrickFerry commented Apr 18, 2021

Rewritten with alignup to align up to next multiple given that width is one more like the comment suggests

Common::AlignUp(width+1, sBlkSize);

@PatrickFerry PatrickFerry force-pushed the sw-textureencoder-alignedwidth branch from cee2425 to b1b4a1c Compare April 18, 2021 01:25
@PatrickFerry PatrickFerry marked this pull request as ready for review April 18, 2021 01:31
This was causing issues in Software Renderer. Look at bug 11487
@PatrickFerry PatrickFerry force-pushed the sw-textureencoder-alignedwidth branch from b1b4a1c to f6a4368 Compare April 19, 2021 01:07
Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Whoops, my bad 😅

@leoetlino leoetlino merged commit 1c6232e into dolphin-emu:master Apr 24, 2021
10 checks passed
@PatrickFerry PatrickFerry deleted the sw-textureencoder-alignedwidth branch April 24, 2021 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants