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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed latency setting and cleaned-up OpenAL backend #5631

Merged
merged 5 commits into from Jun 27, 2017

Conversation

@LAGonauta
Copy link
Contributor

LAGonauta commented Jun 16, 2017

Before these changes each unit of latency was actually 5ms, with a minimum latency of ~10 ms. If it was set to 4 ms on the UI, the actual latency was 10 ms + 5 ms (for each buffer, 256 samples at 48kHz) * 4 ms = 30 ms.
Now 30 ms on the UI means 30 ms on the backend.

Problem: People that had "low" values on their latency setting may think that the latency is now higher (even though it can be lower as I also commented out a Wait() event that did more harm than good) and they will need to increase their values to 20 or 30 ms.
I changed the setting in the INI from "Latency" to "AudioLatency" so everyone should have a good default.
Instead of commenting out the Wait() event (which I soon discovered caused high CPU usage, duh), I swapped it out for a std::sleep_for. Works a lot better now, only causes buffer underruns when the latency is set too low.

I also renamed all variables to the current coding standard, renamed some variables to better represent what they really are, removed a redundant conversion from short to floating point when playing back stereo and swapped all C-style arrays to std::array.

This PR is partially based on PR# 5191 and PR# 5235. Some changes I believe are particularly important (such as the removal of the redundant conversion from short to float) so I want to move it from PR# 5235 which will only be merged when I manage to discover why FreeSurround causes glitches on Ubuntu 16.04.

EDIT:

Just did some experiments with this PR and OpenAL Soft. If we set the latency to 15 ms and reduce the period size in its config file to 128 we get the same audio latency as XAudio2 馃槉

I also investigated why commenting out the Wait() event allows lower latencies.
After we call Wait() the thread unpauses only when SoundStream.Update() is called and samples are pushed into the mixer by SendAIBuffer(), but for some reason this doesn't work very well with OpenAL and fixed sized buffers. I made some changes on another branch where OpenAL receives and buffers all available samples at once hoping to lower its latency. The latency continued the same but works really well with Wait().

Luckily as OpenAL is in sync with the mixer (as it plays samples at the same rate it receives them) changing Wait() to sleep_for brings only advantages.

@LAGonauta LAGonauta force-pushed the LAGonauta:openal-real-latency branch 2 times, most recently from 42668f8 to 92e63a3 Jun 16, 2017
@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Jun 17, 2017

I've ran into at least 2 people who've had issues with the new OpenAL latency, so I'd be happy with anything that makes it easier to use.

@MerryMage

This comment has been minimized.

Copy link
Member

MerryMage commented Jun 17, 2017

Two comments:

  • Add "ms" to the UI after the latency textbox.
  • Perhaps rename the INI setting from "Latency" to "AudioLatency" due to the change in semantics of the given value.
@LAGonauta LAGonauta force-pushed the LAGonauta:openal-real-latency branch from 92e63a3 to 81a7607 Jun 17, 2017
@LAGonauta

This comment has been minimized.

Copy link
Contributor Author

LAGonauta commented Jun 17, 2017

@MerryMage Good idea, now everyone gets a good default for a start.

@LAGonauta LAGonauta force-pushed the LAGonauta:openal-real-latency branch from 81a7607 to 2bf73fc Jun 17, 2017
@ligfx

This comment has been minimized.

Copy link
Contributor

ligfx commented Jun 18, 2017

Looks good to me. Thanks for fixing the unit-less "Latency" option, that's bugged me for so long!

Could we lower the default latency? 30ms seems high to me, other backends are between about 5ms to 10ms.

@LAGonauta LAGonauta force-pushed the LAGonauta:openal-real-latency branch from 2bf73fc to d528098 Jun 18, 2017
@LAGonauta

This comment has been minimized.

Copy link
Contributor Author

LAGonauta commented Jun 18, 2017

Just reduced the default from 30 ms to 20 ms (latency between 14 and 20 ms). I could reduce it further to 10ms (latency between 7 and 10 ms), but it only works well if we edit OpenAL Soft's configuration file and set period_size to 128 samples (the default is 1024).

Of course with a X-Fi we can set it to 6ms and it works fine (latency between 4 to 6 ms).

@LAGonauta LAGonauta force-pushed the LAGonauta:openal-real-latency branch from d528098 to e108f82 Jun 18, 2017
@ligfx
ligfx approved these changes Jun 18, 2017
Copy link
Contributor

shuffle2 left a comment

some nitpicks...

ALuint uiBuffers[OAL_MAX_BUFFERS];
ALuint uiSource;
ALfloat fVolume;
std::vector<short> realtime_buffer;

This comment has been minimized.

Copy link
@shuffle2

shuffle2 Jun 18, 2017

Contributor

should probably use m_ prefix everywhere if you're going to use it one place?

This comment has been minimized.

Copy link
@LAGonauta

LAGonauta Jun 19, 2017

Author Contributor

I added the "m_" prefix to all global but private variables. Should I also add it to all local variables?

@@ -25,84 +25,84 @@
bool OpenALStream::Start()
{
m_run_thread.Set();
bool bReturn = false;
bool b_return = false;

This comment has been minimized.

Copy link
@shuffle2

shuffle2 Jun 18, 2017

Contributor

b prefix was used here as hungarian notation. IMO this should be renamd to something like success or w/e. Sorry for nitpick :(

This comment has been minimized.

Copy link
@LAGonauta

LAGonauta Jun 19, 2017

Author Contributor

Good idea, I just did not have a better name 馃槉

@LAGonauta LAGonauta force-pushed the LAGonauta:openal-real-latency branch from e108f82 to 7aeae51 Jun 19, 2017
LAGonauta added 5 commits Jun 17, 2017
Before these changes each value of latency were actually 5ms, with a
minimum latency of ~10 ms. If it was set to 4 ms on the UI, the actual
latency was 10 + 5 * 4 = 30 ms.
Now 30 ms on the UI means 30 ms on the backend.
It seems to make no difference besides allowing lower latencies and more
stability on hardware OpenAL cards. Maybe the Wait() call waits for too
long, causing buffers underruns.
Also changed C-Style casts to static_cast
@LAGonauta LAGonauta force-pushed the LAGonauta:openal-real-latency branch from 7aeae51 to db7bb3b Jun 27, 2017
@LAGonauta

This comment has been minimized.

Copy link
Contributor Author

LAGonauta commented Jun 27, 2017

Just rebased this PR with the OpenAL changes by ligfx

@shuffle2 shuffle2 merged commit ce4d514 into dolphin-emu:master Jun 27, 2017
10 checks passed
10 checks passed
default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
@LAGonauta LAGonauta deleted the LAGonauta:openal-real-latency branch Jun 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can鈥檛 perform that action at this time.