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

Reduce gradual audio desyncing in dumps and apply the correct sample rate for GameCube audio #10762

Merged

Conversation

CasualPokePlayer
Copy link
Contributor

@CasualPokePlayer CasualPokePlayer commented Jun 17, 2022

This PR aims to help resolve issue 12951. Its current status right now should reduce the gradual desync, but not completely eliminate it. Eliminating it would be a bit complicated here, as either audio dumps would need to be reworked into some format that accepts non-integer sample rates (but not really, since everything playing back the audio would round it down anyways), or resampling the audio to an integer sample rate (probably the most "simple" here).

@CasualPokePlayer CasualPokePlayer force-pushed the fix_slow_audio_desyncs branch from d818385 to 43616a4 Compare June 17, 2022 08:57
@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Jun 17, 2022

Your 54000000 * 2 dividend is quite a ways towards the int32 limit, you might need to update a few of these variables to be int64.

@CasualPokePlayer
Copy link
Contributor Author

Your 54000000 * 2 dividend is quite a ways towards the int32 limit, you might need to update a few of these variables to be int64.

The dividend is a u64 in the first place, integer promotion should handle issues of that limit.

@CasualPokePlayer
Copy link
Contributor Author

Not sure what to do now. Seems like resampling is the only solution here, nothing seems to support a non-integer sample rate, and the resampling code used in the Mixer is a bit hard to follow.

@JosJuice
Copy link
Member

I think I would prefer to just output without resampling and set a rounded sample rate in the WAV header, at least for now. I don't think everyone wants the resampling, and it is possible to fix the incorrect sample rate that's specified in the WAV header in post.

@CasualPokePlayer CasualPokePlayer force-pushed the fix_slow_audio_desyncs branch from 4007e26 to ffcf639 Compare June 18, 2022 08:36
@CasualPokePlayer
Copy link
Contributor Author

CasualPokePlayer commented Jun 18, 2022

Alright, reverted those hacky resampling commits and just have the other fixes. This should at least make the GC sample rate "accurate" I guess. Resampling can come at some later point in time.

@CasualPokePlayer CasualPokePlayer marked this pull request as ready for review June 18, 2022 08:38
@CasualPokePlayer CasualPokePlayer changed the title Fix gradual audio desyncing in dumps Reduce gradual audio desyncing in dumps and apply the correct sample rate for GameCube audio Jun 18, 2022
Source/Core/Core/HW/AudioInterface.h Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/AudioCommon/Mixer.h Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/SystemTimers.cpp Show resolved Hide resolved
Source/Core/Core/HW/AudioInterface.cpp Show resolved Hide resolved
Source/Core/AudioCommon/WaveFile.cpp Outdated Show resolved Hide resolved
@CasualPokePlayer CasualPokePlayer force-pushed the fix_slow_audio_desyncs branch from ffcf639 to c4c5f05 Compare June 18, 2022 09:17
@CasualPokePlayer CasualPokePlayer force-pushed the fix_slow_audio_desyncs branch from 2ebf20e to 167efed Compare July 2, 2022 05:57
@CasualPokePlayer
Copy link
Contributor Author

CasualPokePlayer commented Jul 2, 2022

Looking through audio formats again and realizing there are formats that support non-integer sample rates, one namely being the aiff format. Perhaps this could be used as a solution here (possibly just given as a separate option if desired).

EDIT: Eh, this doesn't seem to be a good solution anyways, everything that takes in an aiff just rounds down the sample rate anyways! The reduction in audio desyncing is at least good enough I guess.

@Filoppi
Copy link
Contributor

Filoppi commented Jul 2, 2022

@CasualPokePlayer #8983 this PR introduced floating point sampling rate support for the audio backend. Dumps were still approximated to the closest integer as it didn't really mattered IMO. I'm still trying to get back and finish it given the only pending thing was finishing the improved audio stretching code. It might be outdated and require some merging, but I think most of the changes would still apply. This is the first time in two years I have some time off work so I might finally go back and finish it.

@CasualPokePlayer
Copy link
Contributor Author

CasualPokePlayer commented Jul 2, 2022

The old "appoximates" just do not work here, as it causes issues with the actual audio timing code (rounding issues causes it to be off from its reported rate), which causes very noticable audio desyncs in dumps within at least an hour. To quote from the relevant issue

https://github.com/dolphin-emu/dolphin/blob/d32b13fb36e6616f7bb4a5a395c34ae2f2ada32b/Source/Core/Core/HW/SystemTimers.cpp#L113
This is the relevant timing code. It expects a constant number of cycles per sample, doing math with the CPU clock rate and such to figure that out. The divisions do not divide evenly, causing rounding errors and the actual output sample rate to be around 32024.2hz/48061.7hz.

The solution this PR opted for is to use dividends and divisors (I'm opting for fixing the dividend here, but maybe it should be variable) to represent the sample rate. Floats are not an option here as timing code should not be reliant on them (mainly performance/determinism concerns), if a float is used it must be only for actual audio output / resampling (which is a separate thread anyways).

The changes in this PR still have audio resampled for the backend (as it was previously doing) and that resampler supported input floats anyways, but at least its original input sample rate will be accurate (and not have errors like reported 32029 -> actual 32024.2, or even worse for 48KHz audio). The wav dumper still has a rounding issue since it doesnt support a float for a sample rate, but the error is much lower now (less than 1 hz).

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Jul 2, 2022

Yeah, this PR is kinda orthogonal to any audio outputting changes. This actually affects the timing.

@CasualPokePlayer CasualPokePlayer force-pushed the fix_slow_audio_desyncs branch from 6c1cc43 to 89efecd Compare July 3, 2022 08:27
@CasualPokePlayer CasualPokePlayer changed the title Reduce gradual audio desyncing in dumps and apply the correct sample rate for GameCube audio Add AIFF Audio Dumping Optin, eliminate gradual audio desyncing in dumps when using AIFF (and reduce it for WAV), and apply the correct sample rate for GameCube audio Jul 3, 2022
@JosJuice
Copy link
Member

JosJuice commented Jul 3, 2022

Could we do this AIFF stuff in a separate PR afterwards, for ease of reviewing?

@CasualPokePlayer CasualPokePlayer force-pushed the fix_slow_audio_desyncs branch from 89efecd to 167efed Compare July 3, 2022 08:41
@CasualPokePlayer
Copy link
Contributor Author

Fair enough

@CasualPokePlayer CasualPokePlayer changed the title Add AIFF Audio Dumping Optin, eliminate gradual audio desyncing in dumps when using AIFF (and reduce it for WAV), and apply the correct sample rate for GameCube audio Reduce gradual audio desyncing in dumps and apply the correct sample rate for GameCube audio Jul 3, 2022
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss left a comment

Choose a reason for hiding this comment

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

Some minor stuff but this looks correct to me.

Source/Core/AudioCommon/Mixer.cpp Outdated Show resolved Hide resolved
Source/Core/AudioCommon/Mixer.cpp Outdated Show resolved Hide resolved
Source/Core/AudioCommon/WaveFile.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/AudioInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/AudioInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
…for that sample rate, with it using a fixed dividend of 54000000 * 2.

This should reduce (but not completely eliminate) gradual audio desyncs in dumps. This also allows for accurate sample rates for the GameCube.
Completely eliminating gradual audio desyncs will require resampling to an integer sample rate, as nothing seems to support a non-integer sample rate.
@CasualPokePlayer CasualPokePlayer force-pushed the fix_slow_audio_desyncs branch from 167efed to 4234b25 Compare July 3, 2022 22:07
@AdmiralCurtiss AdmiralCurtiss merged commit de3d134 into dolphin-emu:master Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants