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

SPU: Logic re-write, remove spu-advanced toggle #562

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Aikku93
Copy link
Contributor

@Aikku93 Aikku93 commented Jul 10, 2022

This PR covers an almost complete re-write of all the SPU mixing logic (plus some miscellaneous bits and pieces related to it), which has been tested to achieve performance on-par with the "non-advanced" SPU logic from before my interpolation-related re-write.

Further improvements include:

  • Much better performance when using the dummy SPU
  • Fixing a bug I introduced in my interpolation re-write (ADPCM channels weren't restored correctly from savestates)
  • Fixing another bug I introduced where using capture-based echo effects would break in dual-SPU mode (this bug was present before my changes)
  • Perfect (cycle-accurate) synchronization with regards to the host sampling rate
  • Miscellaneous tweaks that resulted in better generated assembly and/or performance
  • Fix a long-standing edge-case bug related to capture (when switching to the dummy SPU core and setting dual-SPU mode simultaneously, the capture units were not generating silence, which would cause noise when switching from the dummy SPU core)
  • Fix an extremely obscure bug/oversight (which has probably never occurred) where capture units in one-shot mode would continue looping until manually disabled.
  • Capture is now fully emulated when using the dummy SPU core (this can be disabled via the ENABLE_DUMMY_SPU_CAPTURE toggle in SPU.cpp, in which case capture will only generate silence)
  • Overhead reduction when not mixing by increasing latency (eg. when updating the core SPU in dual-SPU mode, or using the dummy SPU core). This technically results in some loss of synchronicity for those cases, but should really only affect channel allocation and/or testing for channels to have ended, and synchronicity is immediately and completely restored when output is actually being generated.
  • Add support for Catmull-Rom interpolation on Linux (macOS still doesn't have the option)

I've tried my best to remove all references to the "advanced SPU logic" toggle, but have only really been able to test on Windows at the moment, so other platforms might not be functional at the moment (I believe the macOS build in particular needs some more attention).

Special thanks to Rise of Seren for better performance profiling than I could get on my computer.

@rofl0r
Copy link
Collaborator

rofl0r commented Jul 10, 2022

  • Fixing a bug I introduced in my interpolation re-write (ADPCM channels weren't restored correctly from savestates)
  • Fixing another bug I introduced where using capture-based echo effects would break in dual-SPU mode

it would be helpful if those 2 fixes were in separate commits, so someone reading git log can figure out the relation

@Aikku93
Copy link
Contributor Author

Aikku93 commented Jul 11, 2022

Alright, no problem. I'll attempt a rebase (apologies in advance if I mess something up; I'm still not all that comfortable invoking git directly).

@Jules-A
Copy link
Contributor

Jules-A commented Jul 11, 2022

Some FPS testing between builds from Github Actions before the Interpolation rework (48f5a82, testing master isn't fair since it had a few bugs) and this PR (477d4ec):

Before Interpolation Rework (48f5a82):
Xaudio-Sync-Adv-Cosine = 127
Xaudio-Sync-Basic-Cosine = 129
Xaudio-Sync-Basic-Linear = 131
Xaudio-Dual-Adv-Cosine = 124
Xaudio-Dual-Adv-Linear = 125
Xaudio-Dual-Basic-Linear = 135
Null-Sync-Adv-Cosine = 128
Null-Dual-Adv-Linear = 130
Null-Dual-Basic-Linear = 140
Null-Dual-Basic-Cosine = 139
SPU Rework (477d4ec):
Xaudio-Sync-Catmull = 133
Xaudio-Sync-Cosine = 134
Xaudio-Sync-Linear = 134
Xaudio-Dual-Catmull = 137
Xaudio-Dual-Cosine = 138
Xaudio-Dual-Linear = 137
Null-Sync-Catmull = 139
Null-Dual-Catmull = 140

Testing on my Win10 machine with FX8320 + RX570 with JIT 18, OGL Display+3d renderer, 15bit Color, rest default. Tested on Pokemon Heartgold inside a building with fps based on just 1 loop (since I tested so many this time) and by observing FPS based on the counter (can't get any tools to report correct results). All Sync testing with N. Didn't test DirectSound since it gives lower FPS than XAudio and probably only useful for XP or lower.
I know testing at such high fps probably isn't a good idea but it makes it easier to test.

@Aikku93
Copy link
Contributor Author

Aikku93 commented Jul 15, 2022

Before I mark this PR ready for review, I've got a couple of questions (mostly because I'm still trying to get used to contributing on projects instead of just working on my own ones).

So one of the changes was "Add support for Catmull-Rom interpolation on Linux (macOS still doesn't have the option)" (b6da15a). Should this be split out into a separate "Add Catmull-Rom support for Linux/macOS" PR/issue (and hopefully have someone look over what needs to be done to incorporate it properly), or is it fine to keep it here? Not having that interpolation option won't break anything, so I figure it might be better to do it as a new PR.

My other question was: I partially removed the advanced SPU option from the macOS code, but I know there's more places that I've missed. If the toggle is left in, then even though it won't actually do anything, the code won't break. Should I undo those changes and leave it for someone who actually knows what they're doing (or possibly even just undo the changes and raise it as an issue to be addressed at another time)?

@zeromus
Copy link
Contributor

zeromus commented Jul 15, 2022

For this project our policy is break it and let someone else fix it. We can't wait for everyone to do everything perfectly or else nobody would ever do anything.

@rogerman
Copy link
Collaborator

Just do it -- I'll fix it later.

@Aikku93: In the future, if you are working on unfamiliar code, then whenever you want to remove options and you see the flag variables controlled by accessor functions, it is better coding practice to hard code the getter function to YES/true or NO/false, and then comment out the flag variable assignment in the setter function. By doing this, you risk breaking absolutely nothing while still achieving your intent in removing the option.

@Aikku93
Copy link
Contributor Author

Aikku93 commented Jul 15, 2022

Alright, thanks for the help and tips. Appreciate it.

@Aikku93 Aikku93 marked this pull request as ready for review July 15, 2022 22:28
@rogerman
Copy link
Collaborator

@Aikku93: I'm currently reviewing the addition of the Catmull-Rom interpolation method and trying to get a handle on what it can do from the user's perspective so that I can write a proper tooltip for it. What's probably going to happen is that the Cocoa port's GUI is going to be updated to add Catmull-Rom at the same time as the Advanced SPU Logic option is removed.

@Aikku93
Copy link
Contributor Author

Aikku93 commented Jul 17, 2022

Hm, before this version ends up in master, would it serve any purpose to store SPUCHAN_PCM16B_SIZE and SPUCAPTURE_FIFO_SIZE as part of the SPU savestate (specifically, for loading the buffer data stored in the savestate)? That way, if those constants ever change in a different version, the savestate version won't need to change (as well as avoiding the need to include even more code in the savestate loading routine, since it's already getting a bit unwieldy from my changes).

The UI still presents the option, but does nothing
Enforce delay for channel playback and adjust capture delay prior to FIFO buffering.
@Aikku93
Copy link
Contributor Author

Aikku93 commented Oct 5, 2022

I've split up the single mega-commit into multiple ones, which should hopefully result in a clearer commit history. I've also more-or-less followed rogerman's advice re. the Cocoa port, and instead of removing all the associated code that I'm unfamiliar with, I've just commented out the line that actually writes to CommonSettings.spu_advanced.

Please let me know if there's still more commit refactoring to do. I think I'm starting to get the idea of how commits and PRs are supposed to work, but I'm still getting there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants