| Message ID | 20221218171539.11193-10-vr_qemu@t-online.de |
|---|---|
| State | New |
| Headers | show |
| Series | audio: more improvements | expand |
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? > 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? 23ms is usually a good trade off between low latency, CPU load and potential for audio dropouts. > 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. > } > > /* >
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. > 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. ./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 >> } >> >> /* >>
>>> 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. > > ./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 The correct command line is: ./qemu-system-x86_64 -device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev alsa,id=audio0,out.period-length=5000,out.dev=hw:PCH,,0 alsa: Requested period time 5000 was rejected, using 5804 > > >>> } >>> /* >>> >
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 > > >> } > >> > >> /* > >> > > >
Am 28.12.22 um 14:52 schrieb Christian Schoenebeck: > 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? Yes, that could be done. But that's not the point of my statement. My point was that nearly every audio backend uses an update period of 10ms and the update period of 10ms works well in the majority of cases. Changing the update period depending on the audio frontend would be possible, but at the moment I see no reason to work on this. >>> 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. It's not about optimization. The -audiodev alsa command line parameters out.buffer-length, in.buffer-length, out.period-length and in.period-lenght are specified in microseconds. I prefer the default values to use the same unit. I could add the unit in a comment after the values. I shouldn't have written anything about rounding. The value 5805E-6 * 44100 = 256.0005 is closer to 256 than 5804E-6 * 44100 = 255.9564. That's the only reason I selected 5805 /* us */. >> ./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 >> >>>> } >>>> >>>> /* >>>>
Am 28.12.22 um 14:52 schrieb Christian Schoenebeck: > 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? I think this should be the other way around. If period-length wasn't specified on the command line, it should be calculated from timer-period. With timer based playback or recording, the guest should be able to write or read new audio frames every timer-period microseconds. To have a high probability that the guest can write or read new frames every timer-period, the asynchronous updates of the audio backend have to be faster than timer-period e.g. 75-80% of timer-period. But that's a different patch. >>> 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. >>
On Friday, December 30, 2022 10:01:47 AM CET Volker Rümelin wrote: > Am 28.12.22 um 14:52 schrieb Christian Schoenebeck: > > 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? > > I think this should be the other way around. If period-length wasn't > specified on the command line, it should be calculated from > timer-period. With timer based playback or recording, the guest should > be able to write or read new audio frames every timer-period > microseconds. To have a high probability that the guest can write or > read new frames every timer-period, the asynchronous updates of the > audio backend have to be faster than timer-period e.g. 75-80% of > timer-period. But that's a different patch. Probably in both directions, depending on what the user supplied. I still have this feeling that this patch might just move the problem to other users (dropouts) until properly addressed, but anyway, for the time being: Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > >>> 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. > >>
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; } /*
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). 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. 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(-)