Re: [PATCH 10/11] alsaaudio: change default playback settings

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Monday, December 26, 2022 4:08:37 PM CET Volker Rümelin wrote:
> Am 21.12.22 um 12:03 schrieb Christian Schoenebeck:
> > On Sunday, December 18, 2022 6:15:38 PM CET Volker Rümelin wrote:
> >> The currently used default playback settings in the ALSA audio
> >> backend are a bit unfortunate. With a few emulated audio devices,
> >> audio playback does not work properly. Here is a short part of
> >> the debug log while audio is playing (elapsed time in seconds).
> > Which emulated devices are these?
> 
> The hda device and sb16. When I wrote this patch two months ago ac97 
> also had occasional dropouts, but at the moment ac97 works without issues.
> 
> >> audio: Elapsed since last alsa run (running): 0.046244
> >> audio: Elapsed since last alsa run (running): 0.023137
> >> audio: Elapsed since last alsa run (running): 0.023170
> >> audio: Elapsed since last alsa run (running): 0.023650
> >> audio: Elapsed since last alsa run (running): 0.060802
> >> audio: Elapsed since last alsa run (running): 0.031931
> >>
> >> For some audio devices the time of more than 23ms between updates
> >> is too long.
> >>
> >> Set the period time to 5.8ms so that the maximum time between
> >> two updates typically does not exceed 11ms. This roughly matches
> >> the 10ms period time when doing playback with the audio timer.
> >> After this patch the debug log looks like this.
> > And what about dynamically adapting that value instead of reducing period time
> > for everyone by default?
> 
> It seems this would be only needed for the ALSA backend. All other 
> backends with the exception of OSS are fine with a 10ms period, and the 
> ALSA audio backend also uses 10ms with -audiodev 
> alsa,out.try-poll=off,in.try-poll=off.

OK, but all it would need was adjusting dev->timer_period appropriately either
in audio_validate_opts() [audio/audio.c, line 2126] to handle it generalized
or at the end of alsa_audio_init() [audio/alsaaudio.c, line 944] if
specifically for ALSA only, no?

> > 23ms is usually a good trade off between low latency, CPU load and potential
> > for audio dropouts.
> 
> Quite often it's longer than 23ms. For the rest of the audio backends a 
> timer period of 10ms was selected as a good trade off between CPU load 
> and audio dropouts. But you are right, this patch increases the CPU load.
> 
> On my system the CPU load is increased by 0.9%. This was measured with a 
> Linux guest using rhythmbox for audio playback. The guest was configured 
> to use pulseaudio as sound server. The measurement was done with top -b 
> -d 10 -n 14 over a period of two minutes. The first and last measurement 
> was dropped. The average QEMU CPU load was 10.7% with and 9.8% without 
> this patch.
> 
> I would prefer a system with a 0.9% increased CPU load where audio just 
> works over a system where you have to fine tune audio parameters.
> 
> >> audio: Elapsed since last alsa run (running): 0.011919
> >> audio: Elapsed since last alsa run (running): 0.005788
> >> audio: Elapsed since last alsa run (running): 0.005995
> >> audio: Elapsed since last alsa run (running): 0.011069
> >> audio: Elapsed since last alsa run (running): 0.005901
> >> audio: Elapsed since last alsa run (running): 0.006084
> >>
> >> Signed-off-by: Volker Rümelin<vr_qemu@t-online.de>
> >> ---
> >>   audio/alsaaudio.c | 11 ++++-------
> >>   1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
> >> index 5f50dfa0bf..0cc982e61f 100644
> >> --- a/audio/alsaaudio.c
> >> +++ b/audio/alsaaudio.c
> >> @@ -913,17 +913,14 @@ static void *alsa_audio_init(Audiodev *dev)
> >>       alsa_init_per_direction(aopts->in);
> >>       alsa_init_per_direction(aopts->out);
> >>   
> >> -    /*
> >> -     * need to define them, as otherwise alsa produces no sound
> >> -     * doesn't set has_* so alsa_open can identify it wasn't set by the user
> >> -     */
> >> +    /* don't set has_* so alsa_open can identify it wasn't set by the user */
> >>       if (!dev->u.alsa.out->has_period_length) {
> >> -        /* 1024 frames assuming 44100Hz */
> >> -        dev->u.alsa.out->period_length = 1024 * 1000000 / 44100;
> >> +        /* 256 frames assuming 44100Hz */
> >> +        dev->u.alsa.out->period_length = 5805;
> >>       }
> >>       if (!dev->u.alsa.out->has_buffer_length) {
> >>           /* 4096 frames assuming 44100Hz */
> >> -        dev->u.alsa.out->buffer_length = 4096ll * 1000000 / 44100;
> >> +        dev->u.alsa.out->buffer_length = 92880;
> > Not a big fan of magic numbers, as it makes code less readable.
> 
> I can't see how this can be improved. The buffer length is unchanged. I 
> just evaluated the constant expression to have a time in microseconds 
> like the rest of the audio backends. And libasound tells me to use 
> 5804us for the period length which I rounded up to 5805us. I would 
> prefer a period length of 5000us.

Probably nitpicking as the preceding comment indicates the numbers, but maybe
simply like this?

dev->u.alsa.out->period_length = ceil(256.0 * 1000000.0 / 44100.0);
...
dev->u.alsa.out->buffer_length = ceil(4096.0 * 1000000.0 / 44100.0);

I mean these are number literals passed to a built-in function, so the
compiler should automatically evaluate this constant expression at compile
time, so it should end up in the binary with the same constant value as you
did directly in code, at least if optimization was turned on.

> ./qemu-system-x86_64 -device ich9-intel-hda -device 
> hda-duplex,audiodev=audio0 -audiodev 
> alsa,id=audio0,out.period-length=5000,out.dev=PCH,,0
> alsa: Requested period time 5000 was rejected, using 5804
> 
> >>       }
> >>   
> >>       /*
> >>
> 
> 
>