Always use a global mixer #6698
Conversation
|
This needs ample mobile / platform testing. Also failing tests show an actual issue. |
* Comment removed, because recycling the mixer is actually necessary in at least some cases (switching device means potentially changing sample rate and/or channel count). * Order of operations changed as well, so the failsave in the wasapi callback doesn't get triggered unnecessarily.
LastExceed
force-pushed
the
global-mixer
branch
from
afac9b0 to
aa50336
Compare
I tried building the game for my phone and failed (literally zero experience with mobile development). I also don't have access to any platform other than Windows 11 and Android 12, so I'm afraid I wouldn't be of much help here anyway
Turns out The last failing test is
No idea how this PR could possibly make a graphics test fail. Can't really look into it unfortunately, because platforms |
LastExceed
force-pushed
the
global-mixer
branch
from
aa50336 to
ed67607
Compare
| Bass.StreamFree(globalMixerHandle.Value.Value); | ||
| BassWasapi.Stop(); | ||
| BassWasapi.Free(); | ||
| freeGlobalMixer(); |
There was a problem hiding this comment.
having this in a method called freeWasapi (and only run on windows) feels wrong.
There was a problem hiding this comment.
I agree! But you're the one who put it there lol
Also this isn't a refactoring PR, so this is off-topic
There was a problem hiding this comment.
My review is attached to line 292. The global mixer was only used here previously, but now it's used everywhere, so calling a method named freeGlobalMixer from only the wasapi (platform scoped) method feels wrong.
Also, please do not self-resolve reviews, we leave that to the reviewer to make sure they are resolved to our content.
There was a problem hiding this comment.
My review is attached to line 292
It sure is. What about it?
from only the wasapi (platform scoped) method feels wrong
The callsite is platform scoped because so is the whole concept of switching outputs at the moment. If we were to implement another platform specific output, then I'm sure it would be called there too.
For the sake of the argument, I've added a commit that extracts it to the callsites of freeWasapi(). In my eyes, this looks worse than before:
- Its a bit copypasty
- init functions creating a mixer, but complementing free functions not destroying it, is inconsistent
I've thought a bit about how else to approach this, but frankly, the whole audio output subsystem (=this file plus about half of AudioManager.cs) is in dire need of a refactor, and that is way beyond the scope of this PR. I actually tried to do exactly that before this PR, but eventually recognized that establishing a consistent design will be a lot easier when output always functions the same way (i.e. always uses a global mixer), so here we are. I also contemplated including the refactor in this PR, but I much prefer to do one thing at a time.
It's your call of course. Personally I'd vote for dropping the last commit, but I don't mind keeping it, either.
please do not self-resolve reviews
Alright
There was a problem hiding this comment.
At least now the code is run in all cases, which would be my expectation based on its naming/function. Dropping the commit would return the discrepancy that the code is only run for wasapi even though the global mixer is to be used everywhere, which makes no sense without inline commentary.
2 participants
Closes #6697