AudioServer+Userland: Decouple client sample rates from device rate

[kleinesfilmroellchen]

Depends on #17656

This change was a long time in the making ever since we obtained sample rate awareness in the system. Now, each client has its own sample rate, accessible via new IPC APIs, and the device sample rate is only accessible via the management interface. AudioServer takes care of resampling client streams into the device sample rate. Therefore, the main improvement introduced with this commit is full responsiveness to sample rate changes; all open audio programs will continue to play at correct speed with the audio resampled to the new device rate.

The immediate benefits are manifold:

• Gets rid of the legacy hardware sample rate IPC message in the non-managing client
• Removes duplicate resampling and sample index rescaling code everywhere
• Avoids potential sample index scaling bugs in SoundPlayer (which have happened many times before) and fixes a sample index scaling bug in aplay
• Removes several FIXMEs
• Reduces amount of sample copying in all applications (especially Piano, where this is critical), improving performance
• Reduces number of resampling users, making future API changes (which will need to happen for correct resampling to be implemented) easier

I also threw in a simple race condition fix for Piano's audio player loop.

https://user-images.githubusercontent.com/28656157/223491224-eeb8efbc-a754-4c6a-b071-004409ee124d.mp4

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

[stale[bot]]

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

[gmta]

Looking at this PR and lots of other audio-related issues in the system at the moment. One of the aspects of these changes you describe as:

AudioServer takes care of resampling client streams into the device sample rate

This moves potential performance issues caused by resampling from the applications into the AudioServer. Wouldn't it be better if:

• Applications are able to query the AudioServer's main sample rate (i.e. the device sample rate)
• Applications can decide whether to adhere to the main sample rate (because of their own resampling logic, or availability of multiple audio streams, or... etc.) or to provide a different sample rate
• The audio client then transparently resamples the audio before sending it to AudioServer

?

I know that resampling is dead simple at the moment, but if we move towards higher quality resampling we might introduce a big performance impact without that showing up in the profiles of the applications themselves.

[kleinesfilmroellchen]

Wouldn't it be better if:

• Applications are able to query the AudioServer's main sample rate (i.e. the device sample rate)
• Applications can decide whether to adhere to the main sample rate (because of their own resampling logic, or availability of multiple audio streams, or... etc.) or to provide a different sample rate
• The audio client then transparently resamples the audio before sending it to AudioServer

?

I don't think this solves problems any better. The client can still be nice and ask for the hardware sample rate. We can keep that API in the player IPC interface for this purpose, though I originally intended to remove it.

Additionally, there's potential for optimization: Imagine that all clients use a sample rate that's not the hardware sample rate, but they all (or some of them) use the same sample rate, such as 44.1k or 48k, while the device is running at e.g. 192k for some reason. Doesn't seem unreasonable to me. In that case, the server, who is observing all incoming streams and their sample rates, can perform mixing per sample rate group and only resample once per group. Therefore, the amount of resampling is minimal; we only resample as often as we have total distinct sample mismatches, not for every client.

And finally, sample rate handling in the client always adds a lot of complexity to do it right entirely, since the client always needs to know about the current hardware sample rate and switch accordingly. If the client has an audio stream that can't be changed to another sample rate on the fly in the first place, what benefit does this provide?

[kleinesfilmroellchen]

Let me TL;DR this again:

• Optimization potential, as the server knows about all sample rates involved
• "Doing it right" is complicated and should therefore be centralized
• Fixing the problem at hand is simplest this way

[gmta]

I don't think this solves problems any better. The client can still be nice and ask for the hardware sample rate. We can keep that API in the player IPC interface for this purpose, though I originally intended to remove it.

If clients can query the hardware sample rate and/or be proactively updated about changes to it, we allow them to make smart choices where necessary.

Optimization potential, as the server knows about all sample rates involved

That's a fair point, and in practice we currently already have all these 44.1KHz clients talking to a 48KHz AudioServer. Although for each "group" you'd probably get individual mixing behaviors/artifacts and after resampling, we need to mix the results from these groups to get the final results.

[kleinesfilmroellchen]

You have to also consider that some clients bypass the "easy" playback infrastructure, such as Piano and SDL. Directly using the audio queue makes it much more complicated to react to resampling changes, and introduces extra logic, such as skipping resampling when the sample rates match to decrease latency.

All in all, I don't see how your points invalidate this change. At the very least, this fixes bugs and improves performance transparently. In future extension, this can simplify other performance improvements and multi-output audio routing.

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

[kleinesfilmroellchen]

@trflynn89 I performed a smoke test on Ladybird but didn't actually test browser audio itself. It seems like Qt already has an API to set the device sample rate which you were already using, so I hope my minimal changes are fine as-is.

[trflynn89]

@trflynn89 I performed a smoke test on Ladybird but didn't actually test browser audio itself. It seems like Qt already has an API to set the device sample rate which you were already using, so I hope my minimal changes are fine as-is.

I tested a few sites with these changes (e.g. bandcamp) and didn't encounter any issues