[PATCH 00/11] audio: more improvements

[PATCH 00/11] audio: more improvements

[Volker Rümelin]

A few patches from my audio patch queue.

Patches 1 - 2:
If a guest selects an unsupported sample rate, an error message is 
currently shown. The first patch takes care to suppress the error 
message and reports with the qemu_log_mask(LOG_UNIMP, ...) function that 
this is not supported. The second patch is needed because there are two 
code paths to reach the qemu_log_mask() function in the 
audio_pcm_sw_alloc_resources_* functions. The second path prints an 
additional error message up to now.

For more background information:
https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg04940.html

Patches 3 - 4:
General improvements.

Patches 5 - 9:
These patches remove the audio_calloc() function. The GLib g_new0 macro 
is a better replacement for audio_calloc() and we have one less 
audio_bug() function call site. There's one exception where g_malloc0() 
fits better.

Patches 10 - 11:
Audio playback and recording with the ALSA audio backend currently 
doesn't work well with the default audio settings.

Volker Rümelin (11):
   audio: log unimplemented audio device sample rates
   audio: don't show unnecessary error messages
   audio: rename hardware store to backend
   audio: remove unused #define AUDIO_STRINGIFY
   audio/mixeng: use g_new0() instead of audio_calloc()
   audio/alsaaudio: use g_new0() instead of audio_calloc()
   audio/audio_template: use g_malloc0() to replace audio_calloc()
   audio/audio_template: use g_new0() to replace audio_calloc()
   audio: remove audio_calloc() function
   alsaaudio: change default playback settings
   alsaaudio: reintroduce default recording settings

  audio/alsaaudio.c      | 27 ++++++++----------------
  audio/audio.c          | 26 +----------------------
  audio/audio_int.h      |  4 ----
  audio/audio_template.h | 48 ++++++++++++++++++++----------------------
  audio/mixeng.c         |  7 +-----
  5 files changed, 34 insertions(+), 78 deletions(-)

-- 
2.35.3

[PATCH 01/11] audio: log unimplemented audio device sample rates

[Volker Rümelin]

Some emulated audio devices allow guests to select very low
sample rates that the audio subsystem doesn't support. The lowest
supported sample rate depends on the audio backend used and in
most cases can be changed with various -audiodev arguments. Until
now, the audio_bug function emits an error message similar to the
following error message

A bug was just triggered in audio_calloc
Save all your work and restart without audio
I am sorry
Context:
audio_pcm_sw_alloc_resources_out passed invalid arguments to
 audio_calloc
nmemb=0 size=16 (len=0)
audio: Could not allocate buffer for `ac97.po' (0 samples)

and the audio subsystem continues without sound for the affected
device.

The fact that the selected sample rate is not supported is not a
guest error. Instead of displaying an error message, the missing
audio support is now logged. Simply continuing without sound is
correct, since the audio stream won't transport anything
reasonable at such high resample ratios anyway.

The AUD_open_* functions return NULL like before. The opened
audio device will not be registered in the audio subsystem and
consequently the audio frontend callback functions will not be
called. The AUD_read and AUD_write functions return early in this
case. This is necessary because, for example, the Sound Blaster 16
emulation calls AUD_write from the DMA callback function.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c          |  1 +
 audio/audio_template.h | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/audio/audio.c b/audio/audio.c
index d849a94a81..f6b420688d 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -31,6 +31,7 @@
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-audio.h"
 #include "qemu/cutils.h"
+#include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/help_option.h"
 #include "sysemu/sysemu.h"
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 720a32e57e..bfa94b4d22 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -115,6 +115,19 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
 #else
     samples = (int64_t)sw->HWBUF->size * sw->ratio >> 32;
 #endif
+    if (samples == 0) {
+        HW *hw = sw->hw;
+        size_t f_fe_min;
+
+        /* f_fe_min = ceil(1 [frames] * f_be [Hz] / size_be [frames]) */
+        f_fe_min = (hw->info.freq + HWBUF->size - 1) / HWBUF->size;
+        qemu_log_mask(LOG_UNIMP,
+                      AUDIO_CAP ": The guest selected a " NAME " sample rate"
+                      " of %d Hz for %s. Only sample rates >= %zu Hz are"
+                      " supported.\n",
+                      sw->info.freq, sw->name, f_fe_min);
+        return -1;
+    }
 
     sw->buf = audio_calloc(__func__, samples, sizeof(struct st_sample));
     if (!sw->buf) {
-- 
2.35.3

[PATCH 02/11] audio: don't show unnecessary error messages

[Volker Rümelin]

Let the audio_pcm_create_voice_pair_* functions handle error
reporting. This avoids an additional error message in case
the guest selected an unimplemented sample rate.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio_template.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/audio/audio_template.h b/audio/audio_template.h
index bfa94b4d22..ee320a2e3f 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -421,6 +421,7 @@ static SW *glue(audio_pcm_create_voice_pair_, TYPE)(
 
     hw = glue(audio_pcm_hw_add_, TYPE)(s, &hw_as);
     if (!hw) {
+        dolog("Could not create a backend for voice `%s'\n", sw_name);
         goto err2;
     }
 
@@ -520,7 +521,6 @@ SW *glue (AUD_open_, TYPE) (
     } else {
         sw = glue(audio_pcm_create_voice_pair_, TYPE)(s, name, as);
         if (!sw) {
-            dolog ("Failed to create voice `%s'\n", name);
             return NULL;
         }
     }
-- 
2.35.3

[PATCH 03/11] audio: rename hardware store to backend

[Volker Rümelin]

Use a consistent friendly name for the HWVoiceOut and HWVoiceIn
structures.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio_template.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/audio/audio_template.h b/audio/audio_template.h
index ee320a2e3f..ac744d3484 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -509,8 +509,8 @@ SW *glue (AUD_open_, TYPE) (
         HW *hw = sw->hw;
 
         if (!hw) {
-            dolog ("Internal logic error voice `%s' has no hardware store\n",
-                   SW_NAME (sw));
+            dolog("Internal logic error: voice `%s' has no backend\n",
+                  SW_NAME(sw));
             goto fail;
         }
 
-- 
2.35.3

[PATCH 04/11] audio: remove unused #define AUDIO_STRINGIFY

[Volker Rümelin]

Remove the unused #define AUDIO_STRINGIFY. It was last used before
commit 470bcabd8f ("audio: Replace AUDIO_FUNC with __func__").

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio_int.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/audio/audio_int.h b/audio/audio_int.h
index e87ce014a0..4632cdf9cc 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -294,9 +294,6 @@ static inline size_t audio_ring_posb(size_t pos, size_t dist, size_t len)
 #define ldebug(fmt, ...) (void)0
 #endif
 
-#define AUDIO_STRINGIFY_(n) #n
-#define AUDIO_STRINGIFY(n) AUDIO_STRINGIFY_(n)
-
 typedef struct AudiodevListEntry {
     Audiodev *dev;
     QSIMPLEQ_ENTRY(AudiodevListEntry) next;
-- 
2.35.3

[PATCH 05/11] audio/mixeng: use g_new0() instead of audio_calloc()

[Volker Rümelin]

Replace audio_calloc() with the equivalent g_new0().

With a n_structs argument of 1, g_new0() never returns NULL.
Also remove the unnecessary NULL checks.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c          | 5 -----
 audio/audio_template.h | 6 +-----
 audio/mixeng.c         | 7 +------
 3 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index f6b420688d..ac5434a13c 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -507,11 +507,6 @@ static int audio_attach_capture (HWVoiceOut *hw)
         sw->ratio = ((int64_t) hw_cap->info.freq << 32) / sw->info.freq;
         sw->vol = nominal_volume;
         sw->rate = st_rate_start (sw->info.freq, hw_cap->info.freq);
-        if (!sw->rate) {
-            dolog ("Could not start rate conversion for `%s'\n", SW_NAME (sw));
-            g_free (sw);
-            return -1;
-        }
         QLIST_INSERT_HEAD (&hw_cap->sw_head, sw, entries);
         QLIST_INSERT_HEAD (&hw->cap_head, sc, entries);
 #ifdef DEBUG_CAPTURE
diff --git a/audio/audio_template.h b/audio/audio_template.h
index ac744d3484..d343a1dcb3 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -141,11 +141,7 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
 #else
     sw->rate = st_rate_start (sw->hw->info.freq, sw->info.freq);
 #endif
-    if (!sw->rate) {
-        g_free (sw->buf);
-        sw->buf = NULL;
-        return -1;
-    }
+
     return 0;
 }
 
diff --git a/audio/mixeng.c b/audio/mixeng.c
index 100a306d6f..fe454e0725 100644
--- a/audio/mixeng.c
+++ b/audio/mixeng.c
@@ -414,12 +414,7 @@ struct rate {
  */
 void *st_rate_start (int inrate, int outrate)
 {
-    struct rate *rate = audio_calloc(__func__, 1, sizeof(*rate));
-
-    if (!rate) {
-        dolog ("Could not allocate resampler (%zu bytes)\n", sizeof (*rate));
-        return NULL;
-    }
+    struct rate *rate = g_new0(struct rate, 1);
 
     rate->opos = 0;
 
-- 
2.35.3

[PATCH 06/11] audio/alsaaudio: use g_new0() instead of audio_calloc()

[Volker Rümelin]

Replace audio_calloc() with the equivalent g_new0().

The value of the g_new0() argument count is >= 1, which means
g_new0() will never return NULL. Also remove the unnecessary
NULL check.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/alsaaudio.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
index 714bfb6453..5f50dfa0bf 100644
--- a/audio/alsaaudio.c
+++ b/audio/alsaaudio.c
@@ -222,11 +222,7 @@ static int alsa_poll_helper (snd_pcm_t *handle, struct pollhlp *hlp, int mask)
         return -1;
     }
 
-    pfds = audio_calloc ("alsa_poll_helper", count, sizeof (*pfds));
-    if (!pfds) {
-        dolog ("Could not initialize poll mode\n");
-        return -1;
-    }
+    pfds = g_new0(struct pollfd, count);
 
     err = snd_pcm_poll_descriptors (handle, pfds, count);
     if (err < 0) {
-- 
2.35.3

[PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

[Volker Rümelin]

Use g_malloc0() as a direct replacement for audio_calloc().

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio_template.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/audio/audio_template.h b/audio/audio_template.h
index d343a1dcb3..5f51ef26b2 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_, TYPE)(AudioState *s,
         return NULL;
     }
 
-    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
+    hw = g_malloc0(glue(drv->voice_size_, TYPE));
     if (!hw) {
         dolog ("Can not allocate voice `%s' size %d\n",
                drv->name, glue (drv->voice_size_, TYPE));
-- 
2.35.3

[PATCH 08/11] audio/audio_template: use g_new0() to replace audio_calloc()

[Volker Rümelin]

Replace audio_calloc() with the equivalent g_new0().

With a n_structs argument >= 1, g_new0() never returns NULL.
Also remove the unnecessary NULL checks.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio_template.h | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/audio/audio_template.h b/audio/audio_template.h
index 5f51ef26b2..9c600448fb 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -129,12 +129,7 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
         return -1;
     }
 
-    sw->buf = audio_calloc(__func__, samples, sizeof(struct st_sample));
-    if (!sw->buf) {
-        dolog ("Could not allocate buffer for `%s' (%d samples)\n",
-               SW_NAME (sw), samples);
-        return -1;
-    }
+    sw->buf = g_new0(st_sample, samples);
 
 #ifdef DAC
     sw->rate = st_rate_start (sw->info.freq, sw->hw->info.freq);
@@ -407,34 +402,28 @@ static SW *glue(audio_pcm_create_voice_pair_, TYPE)(
         hw_as = *as;
     }
 
-    sw = audio_calloc(__func__, 1, sizeof(*sw));
-    if (!sw) {
-        dolog ("Could not allocate soft voice `%s' (%zu bytes)\n",
-               sw_name ? sw_name : "unknown", sizeof (*sw));
-        goto err1;
-    }
+    sw = g_new0(SW, 1);
     sw->s = s;
 
     hw = glue(audio_pcm_hw_add_, TYPE)(s, &hw_as);
     if (!hw) {
         dolog("Could not create a backend for voice `%s'\n", sw_name);
-        goto err2;
+        goto err1;
     }
 
     glue (audio_pcm_hw_add_sw_, TYPE) (hw, sw);
 
     if (glue (audio_pcm_sw_init_, TYPE) (sw, hw, sw_name, as)) {
-        goto err3;
+        goto err2;
     }
 
     return sw;
 
-err3:
+err2:
     glue (audio_pcm_hw_del_sw_, TYPE) (sw);
     glue (audio_pcm_hw_gc_, TYPE) (&hw);
-err2:
-    g_free (sw);
 err1:
+    g_free(sw);
     return NULL;
 }
 
-- 
2.35.3

[PATCH 09/11] audio: remove audio_calloc() function

[Volker Rümelin]

Now that the last call site of audio_calloc() was removed, remove
the unused audio_calloc() function.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c     | 20 --------------------
 audio/audio_int.h |  1 -
 2 files changed, 21 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index ac5434a13c..fb0d4a2cac 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -147,26 +147,6 @@ static inline int audio_bits_to_index (int bits)
     }
 }
 
-void *audio_calloc (const char *funcname, int nmemb, size_t size)
-{
-    int cond;
-    size_t len;
-
-    len = nmemb * size;
-    cond = !nmemb || !size;
-    cond |= nmemb < 0;
-    cond |= len < size;
-
-    if (audio_bug ("audio_calloc", cond)) {
-        AUD_log (NULL, "%s passed invalid arguments to audio_calloc\n",
-                 funcname);
-        AUD_log (NULL, "nmemb=%d size=%zu (len=%zu)\n", nmemb, size, len);
-        return NULL;
-    }
-
-    return g_malloc0 (len);
-}
-
 void AUD_vlog (const char *cap, const char *fmt, va_list ap)
 {
     if (cap) {
diff --git a/audio/audio_int.h b/audio/audio_int.h
index 4632cdf9cc..9d04be9128 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -251,7 +251,6 @@ void audio_pcm_init_info (struct audio_pcm_info *info, struct audsettings *as);
 void audio_pcm_info_clear_buf (struct audio_pcm_info *info, void *buf, int len);
 
 int audio_bug (const char *funcname, int cond);
-void *audio_calloc (const char *funcname, int nmemb, size_t size);
 
 void audio_run(AudioState *s, const char *msg);
 
-- 
2.35.3

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

[Volker Rümelin]

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(-)

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;
     }
 
     /*
-- 
2.35.3

[PATCH 11/11] alsaaudio: reintroduce default recording settings

[Volker Rümelin]

Audio recording with ALSA default settings currently doesn't
work. The debug log shows updates every 0.75s and 1.5s.

audio: Elapsed since last alsa run (running): 0.743030
audio: Elapsed since last alsa run (running): 1.486048
audio: Elapsed since last alsa run (running): 0.743008
audio: Elapsed since last alsa run (running): 1.485878
audio: Elapsed since last alsa run (running): 1.486040
audio: Elapsed since last alsa run (running): 1.485886

The time between updates should be in the 10ms range. Audio
recording with ALSA has the same timing contraints as playback.
Reintroduce the default recording settings and use the same
default settings for recording as for playback.

The term "reintroduce" is correct because commit a93f328177
("alsaaudio: port to -audiodev config") removed the default
settings for recording.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/alsaaudio.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
index 0cc982e61f..057571dd1e 100644
--- a/audio/alsaaudio.c
+++ b/audio/alsaaudio.c
@@ -923,15 +923,13 @@ static void *alsa_audio_init(Audiodev *dev)
         dev->u.alsa.out->buffer_length = 92880;
     }
 
-    /*
-     * OptsVisitor sets unspecified optional fields to zero, but do not depend
-     * on it...
-     */
     if (!dev->u.alsa.in->has_period_length) {
-        dev->u.alsa.in->period_length = 0;
+        /* 256 frames assuming 44100Hz */
+        dev->u.alsa.in->period_length = 5805;
     }
     if (!dev->u.alsa.in->has_buffer_length) {
-        dev->u.alsa.in->buffer_length = 0;
+        /* 4096 frames assuming 44100Hz */
+        dev->u.alsa.in->buffer_length = 92880;
     }
 
     return dev;
-- 
2.35.3

Re: [PATCH 02/11] audio: don't show unnecessary error messages

[Philippe Mathieu-Daudé]

On 18/12/22 18:15, Volker Rümelin wrote:
> Let the audio_pcm_create_voice_pair_* functions handle error
> reporting. This avoids an additional error message in case
> the guest selected an unimplemented sample rate.
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/audio_template.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 06/11] audio/alsaaudio: use g_new0() instead of audio_calloc()

[Philippe Mathieu-Daudé]

On 18/12/22 18:15, Volker Rümelin wrote:
> Replace audio_calloc() with the equivalent g_new0().
> 
> The value of the g_new0() argument count is >= 1, which means
> g_new0() will never return NULL. Also remove the unnecessary
> NULL check.

Yes, since commit 7267c0947d (Sat Aug 20 22:09:37 2011)...

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/alsaaudio.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

[Philippe Mathieu-Daudé]

On 18/12/22 18:15, Volker Rümelin wrote:
> Use g_malloc0() as a direct replacement for audio_calloc().
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/audio_template.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index d343a1dcb3..5f51ef26b2 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_, TYPE)(AudioState *s,
>           return NULL;
>       }
>   
> -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
>       if (!hw) {

g_malloc0() can't fail. Either you want g_try_malloc0() or
remove the error path.

>           dolog ("Can not allocate voice `%s' size %d\n",
>                  drv->name, glue (drv->voice_size_, TYPE));

Re: [PATCH 09/11] audio: remove audio_calloc() function

[Philippe Mathieu-Daudé]

On 18/12/22 18:15, Volker Rümelin wrote:
> Now that the last call site of audio_calloc() was removed, remove
> the unused audio_calloc() function.
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/audio.c     | 20 --------------------
>   audio/audio_int.h |  1 -
>   2 files changed, 21 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 04/11] audio: remove unused #define AUDIO_STRINGIFY

[Philippe Mathieu-Daudé]

On 18/12/22 18:15, Volker Rümelin wrote:
> Remove the unused #define AUDIO_STRINGIFY. It was last used before
> commit 470bcabd8f ("audio: Replace AUDIO_FUNC with __func__").
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/audio_int.h | 3 ---
>   1 file changed, 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

[Volker Rümelin]

Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
> On 18/12/22 18:15, Volker Rümelin wrote:
>> Use g_malloc0() as a direct replacement for audio_calloc().
>>
>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>> ---
>>   audio/audio_template.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/audio/audio_template.h b/audio/audio_template.h
>> index d343a1dcb3..5f51ef26b2 100644
>> --- a/audio/audio_template.h
>> +++ b/audio/audio_template.h
>> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_, 
>> TYPE)(AudioState *s,
>>           return NULL;
>>       }
>>   -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
>> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
>>       if (!hw) {
>
> g_malloc0() can't fail. Either you want g_try_malloc0() or
> remove the error path.
>

g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the 
code is correct.

>>           dolog ("Can not allocate voice `%s' size %d\n",
>>                  drv->name, glue (drv->voice_size_, TYPE));
>

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

[Christian Schoenebeck]

On Sunday, December 18, 2022 6:39:00 PM CET Volker Rümelin wrote:
> Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
> > On 18/12/22 18:15, Volker Rümelin wrote:
> >> Use g_malloc0() as a direct replacement for audio_calloc().
> >>
> >> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> >> ---
> >>   audio/audio_template.h | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/audio/audio_template.h b/audio/audio_template.h
> >> index d343a1dcb3..5f51ef26b2 100644
> >> --- a/audio/audio_template.h
> >> +++ b/audio/audio_template.h
> >> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_, 
> >> TYPE)(AudioState *s,
> >>           return NULL;
> >>       }
> >>   -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
> >> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
> >>       if (!hw) {
> >
> > g_malloc0() can't fail. Either you want g_try_malloc0() or
> > remove the error path.
> >
> 
> g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the 
> code is correct.

Correct, that's the only case these glib functions return NULL. And AFAICS
this can be zero with CoreAudio or wav.

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

> 
> >>           dolog ("Can not allocate voice `%s' size %d\n",
> >>                  drv->name, glue (drv->voice_size_, TYPE));
> >
> 
> 
> 

Re: [PATCH 01/11] audio: log unimplemented audio device sample rates

[Christian Schoenebeck]

On Sunday, December 18, 2022 6:15:29 PM CET Volker Rümelin wrote:
> Some emulated audio devices allow guests to select very low
> sample rates that the audio subsystem doesn't support. The lowest
> supported sample rate depends on the audio backend used and in
> most cases can be changed with various -audiodev arguments. Until
> now, the audio_bug function emits an error message similar to the
> following error message
> 
> A bug was just triggered in audio_calloc
> Save all your work and restart without audio
> I am sorry
> Context:
> audio_pcm_sw_alloc_resources_out passed invalid arguments to
>  audio_calloc
> nmemb=0 size=16 (len=0)
> audio: Could not allocate buffer for `ac97.po' (0 samples)
> 
> and the audio subsystem continues without sound for the affected
> device.
> 
> The fact that the selected sample rate is not supported is not a
> guest error. Instead of displaying an error message, the missing
> audio support is now logged. Simply continuing without sound is
> correct, since the audio stream won't transport anything
> reasonable at such high resample ratios anyway.
> 
> The AUD_open_* functions return NULL like before. The opened
> audio device will not be registered in the audio subsystem and
> consequently the audio frontend callback functions will not be
> called. The AUD_read and AUD_write functions return early in this
> case. This is necessary because, for example, the Sound Blaster 16
> emulation calls AUD_write from the DMA callback function.
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  audio/audio.c          |  1 +
>  audio/audio_template.h | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index d849a94a81..f6b420688d 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -31,6 +31,7 @@
>  #include "qapi/qobject-input-visitor.h"
>  #include "qapi/qapi-visit-audio.h"
>  #include "qemu/cutils.h"
> +#include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "qemu/help_option.h"
>  #include "sysemu/sysemu.h"
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index 720a32e57e..bfa94b4d22 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -115,6 +115,19 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
>  #else
>      samples = (int64_t)sw->HWBUF->size * sw->ratio >> 32;
>  #endif
> +    if (samples == 0) {
> +        HW *hw = sw->hw;
> +        size_t f_fe_min;
> +
> +        /* f_fe_min = ceil(1 [frames] * f_be [Hz] / size_be [frames]) */
> +        f_fe_min = (hw->info.freq + HWBUF->size - 1) / HWBUF->size;
> +        qemu_log_mask(LOG_UNIMP,
> +                      AUDIO_CAP ": The guest selected a " NAME " sample rate"
> +                      " of %d Hz for %s. Only sample rates >= %zu Hz are"
> +                      " supported.\n",
> +                      sw->info.freq, sw->name, f_fe_min);
> +        return -1;

You probably want to `sw->buf = NULL;` before returning here, or adjust the
condition for the error message below.

The other thing that puzzles me, in error case these template functions return
-1, which would then be feed to g_malloc*()?

> +    }
>  
>      sw->buf = audio_calloc(__func__, samples, sizeof(struct st_sample));
>      if (!sw->buf) {
> 

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

[Philippe Mathieu-Daudé]

On 18/12/22 21:05, Christian Schoenebeck wrote:
> On Sunday, December 18, 2022 6:39:00 PM CET Volker Rümelin wrote:
>> Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
>>> On 18/12/22 18:15, Volker Rümelin wrote:
>>>> Use g_malloc0() as a direct replacement for audio_calloc().
>>>>
>>>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>>>> ---
>>>>    audio/audio_template.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/audio/audio_template.h b/audio/audio_template.h
>>>> index d343a1dcb3..5f51ef26b2 100644
>>>> --- a/audio/audio_template.h
>>>> +++ b/audio/audio_template.h
>>>> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_,
>>>> TYPE)(AudioState *s,
>>>>            return NULL;
>>>>        }
>>>>    -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
>>>> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
>>>>        if (!hw) {
>>>
>>> g_malloc0() can't fail. Either you want g_try_malloc0() or
>>> remove the error path.
>>>
>>
>> g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the
>> code is correct.
> 
> Correct, that's the only case these glib functions return NULL. And AFAICS
> this can be zero with CoreAudio or wav.

Oh I forgot the '0' case, my bad.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 05/11] audio/mixeng: use g_new0() instead of audio_calloc()

[Richard Henderson]

On 12/18/22 09:15, Volker Rümelin wrote:
> Replace audio_calloc() with the equivalent g_new0().
> 
> With a n_structs argument of 1, g_new0() never returns NULL.
> Also remove the unnecessary NULL checks.
> 
> Signed-off-by: Volker Rümelin<vr_qemu@t-online.de>
> ---
>   audio/audio.c          | 5 -----
>   audio/audio_template.h | 6 +-----
>   audio/mixeng.c         | 7 +------
>   3 files changed, 2 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 06/11] audio/alsaaudio: use g_new0() instead of audio_calloc()

[Richard Henderson]

On 12/18/22 09:15, Volker Rümelin wrote:
> Replace audio_calloc() with the equivalent g_new0().
> 
> The value of the g_new0() argument count is >= 1, which means
> g_new0() will never return NULL. Also remove the unnecessary
> NULL check.
> 
> Signed-off-by: Volker Rümelin<vr_qemu@t-online.de>
> ---
>   audio/alsaaudio.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 08/11] audio/audio_template: use g_new0() to replace audio_calloc()

[Richard Henderson]

On 12/18/22 09:15, Volker Rümelin wrote:
> Replace audio_calloc() with the equivalent g_new0().
> 
> With a n_structs argument >= 1, g_new0() never returns NULL.
> Also remove the unnecessary NULL checks.
> 
> Signed-off-by: Volker Rümelin<vr_qemu@t-online.de>
> ---
>   audio/audio_template.h | 23 ++++++-----------------
>   1 file changed, 6 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 01/11] audio: log unimplemented audio device sample rates

[Volker Rümelin]

Am 18.12.22 um 21:26 schrieb Christian Schoenebeck:
> On Sunday, December 18, 2022 6:15:29 PM CET Volker Rümelin wrote:
>> Some emulated audio devices allow guests to select very low
>> sample rates that the audio subsystem doesn't support. The lowest
>> supported sample rate depends on the audio backend used and in
>> most cases can be changed with various -audiodev arguments. Until
>> now, the audio_bug function emits an error message similar to the
>> following error message
>>
>> A bug was just triggered in audio_calloc
>> Save all your work and restart without audio
>> I am sorry
>> Context:
>> audio_pcm_sw_alloc_resources_out passed invalid arguments to
>>   audio_calloc
>> nmemb=0 size=16 (len=0)
>> audio: Could not allocate buffer for `ac97.po' (0 samples)
>>
>> and the audio subsystem continues without sound for the affected
>> device.
>>
>> The fact that the selected sample rate is not supported is not a
>> guest error. Instead of displaying an error message, the missing
>> audio support is now logged. Simply continuing without sound is
>> correct, since the audio stream won't transport anything
>> reasonable at such high resample ratios anyway.
>>
>> The AUD_open_* functions return NULL like before. The opened
>> audio device will not be registered in the audio subsystem and
>> consequently the audio frontend callback functions will not be
>> called. The AUD_read and AUD_write functions return early in this
>> case. This is necessary because, for example, the Sound Blaster 16
>> emulation calls AUD_write from the DMA callback function.
>>
>> Signed-off-by: Volker Rümelin<vr_qemu@t-online.de>
>> ---
>>   audio/audio.c          |  1 +
>>   audio/audio_template.h | 13 +++++++++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/audio/audio.c b/audio/audio.c
>> index d849a94a81..f6b420688d 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
>> @@ -31,6 +31,7 @@
>>   #include "qapi/qobject-input-visitor.h"
>>   #include "qapi/qapi-visit-audio.h"
>>   #include "qemu/cutils.h"
>> +#include "qemu/log.h"
>>   #include "qemu/module.h"
>>   #include "qemu/help_option.h"
>>   #include "sysemu/sysemu.h"
>> diff --git a/audio/audio_template.h b/audio/audio_template.h
>> index 720a32e57e..bfa94b4d22 100644
>> --- a/audio/audio_template.h
>> +++ b/audio/audio_template.h
>> @@ -115,6 +115,19 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
>>   #else
>>       samples = (int64_t)sw->HWBUF->size * sw->ratio >> 32;
>>   #endif
>> +    if (samples == 0) {
>> +        HW *hw = sw->hw;
>> +        size_t f_fe_min;
>> +
>> +        /* f_fe_min = ceil(1 [frames] * f_be [Hz] / size_be [frames]) */
>> +        f_fe_min = (hw->info.freq + HWBUF->size - 1) / HWBUF->size;
>> +        qemu_log_mask(LOG_UNIMP,
>> +                      AUDIO_CAP ": The guest selected a " NAME " sample rate"
>> +                      " of %d Hz for %s. Only sample rates >= %zu Hz are"
>> +                      " supported.\n",
>> +                      sw->info.freq, sw->name, f_fe_min);
>> +        return -1;
> You probably want to `sw->buf = NULL;` before returning here, or adjust the
> condition for the error message below.

sw->buf is always NULL here. In the audio_pcm_create_voice_pair_*() 
functions we have sw = audio_calloc(__func__, 1, sizeof(*sw)) (after 
patch 08/11 sw = g_new0(SW, 1)) and the audio_pcm_sw_free_resources_*() 
functions also set sw->buf = NULL after freeing sw->buf.

> The other thing that puzzles me, in error case these template functions return
> -1, which would then be feed to g_malloc*()?

Sorry, I can't see where -1 would be fed to g_malloc*().

On error the audio_pcm_sw_alloc_resources_*() functions return error 
code -1, and that error code propagates up to the AUD_open_*() functions 
or the audio_pcm_create_voice_pair_*() functions which return NULL.

>> +    }
>>   
>>       sw->buf = audio_calloc(__func__, samples, sizeof(struct st_sample));
>>       if (!sw->buf) {
>>

Re: [PATCH 01/11] audio: log unimplemented audio device sample rates

[Christian Schoenebeck]

On Monday, December 19, 2022 8:22:25 AM CET Volker Rümelin wrote:
> Am 18.12.22 um 21:26 schrieb Christian Schoenebeck:
> > On Sunday, December 18, 2022 6:15:29 PM CET Volker Rümelin wrote:
> >> Some emulated audio devices allow guests to select very low
> >> sample rates that the audio subsystem doesn't support. The lowest
> >> supported sample rate depends on the audio backend used and in
> >> most cases can be changed with various -audiodev arguments. Until
> >> now, the audio_bug function emits an error message similar to the
> >> following error message
> >>
> >> A bug was just triggered in audio_calloc
> >> Save all your work and restart without audio
> >> I am sorry
> >> Context:
> >> audio_pcm_sw_alloc_resources_out passed invalid arguments to
> >>   audio_calloc
> >> nmemb=0 size=16 (len=0)
> >> audio: Could not allocate buffer for `ac97.po' (0 samples)
> >>
> >> and the audio subsystem continues without sound for the affected
> >> device.
> >>
> >> The fact that the selected sample rate is not supported is not a
> >> guest error. Instead of displaying an error message, the missing
> >> audio support is now logged. Simply continuing without sound is
> >> correct, since the audio stream won't transport anything
> >> reasonable at such high resample ratios anyway.
> >>
> >> The AUD_open_* functions return NULL like before. The opened
> >> audio device will not be registered in the audio subsystem and
> >> consequently the audio frontend callback functions will not be
> >> called. The AUD_read and AUD_write functions return early in this
> >> case. This is necessary because, for example, the Sound Blaster 16
> >> emulation calls AUD_write from the DMA callback function.
> >>
> >> Signed-off-by: Volker Rümelin<vr_qemu@t-online.de>
> >> ---
> >>   audio/audio.c          |  1 +
> >>   audio/audio_template.h | 13 +++++++++++++
> >>   2 files changed, 14 insertions(+)
> >>
> >> diff --git a/audio/audio.c b/audio/audio.c
> >> index d849a94a81..f6b420688d 100644
> >> --- a/audio/audio.c
> >> +++ b/audio/audio.c
> >> @@ -31,6 +31,7 @@
> >>   #include "qapi/qobject-input-visitor.h"
> >>   #include "qapi/qapi-visit-audio.h"
> >>   #include "qemu/cutils.h"
> >> +#include "qemu/log.h"
> >>   #include "qemu/module.h"
> >>   #include "qemu/help_option.h"
> >>   #include "sysemu/sysemu.h"
> >> diff --git a/audio/audio_template.h b/audio/audio_template.h
> >> index 720a32e57e..bfa94b4d22 100644
> >> --- a/audio/audio_template.h
> >> +++ b/audio/audio_template.h
> >> @@ -115,6 +115,19 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
> >>   #else
> >>       samples = (int64_t)sw->HWBUF->size * sw->ratio >> 32;
> >>   #endif
> >> +    if (samples == 0) {
> >> +        HW *hw = sw->hw;
> >> +        size_t f_fe_min;
> >> +
> >> +        /* f_fe_min = ceil(1 [frames] * f_be [Hz] / size_be [frames]) */
> >> +        f_fe_min = (hw->info.freq + HWBUF->size - 1) / HWBUF->size;
> >> +        qemu_log_mask(LOG_UNIMP,
> >> +                      AUDIO_CAP ": The guest selected a " NAME " sample rate"
> >> +                      " of %d Hz for %s. Only sample rates >= %zu Hz are"
> >> +                      " supported.\n",
> >> +                      sw->info.freq, sw->name, f_fe_min);
> >> +        return -1;
> > You probably want to `sw->buf = NULL;` before returning here, or adjust the
> > condition for the error message below.
> 
> sw->buf is always NULL here. In the audio_pcm_create_voice_pair_*() 
> functions we have sw = audio_calloc(__func__, 1, sizeof(*sw)) (after 
> patch 08/11 sw = g_new0(SW, 1)) and the audio_pcm_sw_free_resources_*() 
> functions also set sw->buf = NULL after freeing sw->buf.

OK

> > The other thing that puzzles me, in error case these template functions return
> > -1, which would then be feed to g_malloc*()?
> 
> Sorry, I can't see where -1 would be fed to g_malloc*().
> 
> On error the audio_pcm_sw_alloc_resources_*() functions return error 
> code -1, and that error code propagates up to the AUD_open_*() functions 
> or the audio_pcm_create_voice_pair_*() functions which return NULL.

I thought about patch 7 where you do:

-    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
+    hw = g_malloc0(glue(drv->voice_size_, TYPE));

But I just realized that it is using audio_pcm_hw_add_new_, which not returns
a negative value anywhere, so fine as well:

Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

> 
> >> +    }
> >>   
> >>       sw->buf = audio_calloc(__func__, samples, sizeof(struct st_sample));
> >>       if (!sw->buf) {
> >>
> 
> 
> 

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

[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?

> 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.

>      }
>  
>      /*
> 

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

[Volker Rümelin]

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

>>       }
>>   
>>       /*
>>

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

[Volker Rümelin]

>>> 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

>
>
>>>       }
>>>         /*
>>>
>

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

[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?

> > 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
> 
> >>       }
> >>   
> >>       /*
> >>
> 
> 
> 

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

[Volker Rümelin]

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
>>
>>>>        }
>>>>    
>>>>        /*
>>>>

Re: [PATCH 03/11] audio: rename hardware store to backend

[Thomas Huth]

On 18/12/2022 18.15, Volker Rümelin wrote:
> Use a consistent friendly name for the HWVoiceOut and HWVoiceIn
> structures.
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/audio_template.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index ee320a2e3f..ac744d3484 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -509,8 +509,8 @@ SW *glue (AUD_open_, TYPE) (
>           HW *hw = sw->hw;
>   
>           if (!hw) {
> -            dolog ("Internal logic error voice `%s' has no hardware store\n",
> -                   SW_NAME (sw));
> +            dolog("Internal logic error: voice `%s' has no backend\n",
> +                  SW_NAME(sw));
>               goto fail;
>           }
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 04/11] audio: remove unused #define AUDIO_STRINGIFY

[Thomas Huth]

On 18/12/2022 18.15, Volker Rümelin wrote:
> Remove the unused #define AUDIO_STRINGIFY. It was last used before
> commit 470bcabd8f ("audio: Replace AUDIO_FUNC with __func__").
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/audio_int.h | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/audio/audio_int.h b/audio/audio_int.h
> index e87ce014a0..4632cdf9cc 100644
> --- a/audio/audio_int.h
> +++ b/audio/audio_int.h
> @@ -294,9 +294,6 @@ static inline size_t audio_ring_posb(size_t pos, size_t dist, size_t len)
>   #define ldebug(fmt, ...) (void)0
>   #endif
>   
> -#define AUDIO_STRINGIFY_(n) #n
> -#define AUDIO_STRINGIFY(n) AUDIO_STRINGIFY_(n)
> -
>   typedef struct AudiodevListEntry {
>       Audiodev *dev;
>       QSIMPLEQ_ENTRY(AudiodevListEntry) next;

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Volker Rümelin]

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.
>>

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

[Christian Schoenebeck]

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.
> >>

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

[Daniel P. Berrangé]

On Sun, Dec 18, 2022 at 06:39:00PM +0100, Volker Rümelin wrote:
> Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
> > On 18/12/22 18:15, Volker Rümelin wrote:
> > > Use g_malloc0() as a direct replacement for audio_calloc().
> > > 
> > > Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> > > ---
> > >   audio/audio_template.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/audio/audio_template.h b/audio/audio_template.h
> > > index d343a1dcb3..5f51ef26b2 100644
> > > --- a/audio/audio_template.h
> > > +++ b/audio/audio_template.h
> > > @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_,
> > > TYPE)(AudioState *s,
> > >           return NULL;
> > >       }
> > >   -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
> > > +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
> > >       if (!hw) {
> > 
> > g_malloc0() can't fail. Either you want g_try_malloc0() or
> > remove the error path.
> > 
> 
> g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the code
> is correct.

IMHO relying on that is rather misleading to people reviewing the code
though. As seen by Philippe's reply, people generally don't expect that
g_malloc0 can return NULL, and it is not at all obvious that we are
intentionally expecting 0 to be passed as a size.

Please make this explicit by removing and if (!hw) check after
g_malloc, and adding a check before g_malloc

   if (audio_bug(__func__, glue(drv->voice_size_, TYPE) == 0)) {
       dolog (...)


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 08/11] audio/audio_template: use g_new0() to replace audio_calloc()

[Daniel P. Berrangé]

On Sun, Dec 18, 2022 at 06:15:36PM +0100, Volker Rümelin wrote:
> Replace audio_calloc() with the equivalent g_new0().
> 
> With a n_structs argument >= 1, g_new0() never returns NULL.
> Also remove the unnecessary NULL checks.
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  audio/audio_template.h | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index 5f51ef26b2..9c600448fb 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -129,12 +129,7 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
>          return -1;
>      }
>  
> -    sw->buf = audio_calloc(__func__, samples, sizeof(struct st_sample));
> -    if (!sw->buf) {
> -        dolog ("Could not allocate buffer for `%s' (%d samples)\n",
> -               SW_NAME (sw), samples);
> -        return -1;
> -    }
> +    sw->buf = g_new0(st_sample, samples);

"samples" is a signed integer, and audio_calloc would check for
it being negative and raise an error. It would also check for
samples being zero. I think we still need both these checks,
as a negative value of samples when cast to size_t would be a
huge size.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 08/11] audio/audio_template: use g_new0() to replace audio_calloc()

[Volker Rümelin]

Am 16.01.23 um 10:03 schrieb Daniel P. Berrangé:
> On Sun, Dec 18, 2022 at 06:15:36PM +0100, Volker Rümelin wrote:
>> Replace audio_calloc() with the equivalent g_new0().
>>
>> With a n_structs argument >= 1, g_new0() never returns NULL.
>> Also remove the unnecessary NULL checks.
>>
>> Signed-off-by: Volker Rümelin<vr_qemu@t-online.de>
>> ---
>>   audio/audio_template.h | 23 ++++++-----------------
>>   1 file changed, 6 insertions(+), 17 deletions(-)
>>
>> diff --git a/audio/audio_template.h b/audio/audio_template.h
>> index 5f51ef26b2..9c600448fb 100644
>> --- a/audio/audio_template.h
>> +++ b/audio/audio_template.h
>> @@ -129,12 +129,7 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
>>           return -1;
>>       }
>>   
>> -    sw->buf = audio_calloc(__func__, samples, sizeof(struct st_sample));
>> -    if (!sw->buf) {
>> -        dolog ("Could not allocate buffer for `%s' (%d samples)\n",
>> -               SW_NAME (sw), samples);
>> -        return -1;
>> -    }
>> +    sw->buf = g_new0(st_sample, samples);
> "samples" is a signed integer, and audio_calloc would check for
> it being negative and raise an error. It would also check for
> samples being zero. I think we still need both these checks,
> as a negative value of samples when cast to size_t would be a
> huge size.

Hi Daniel,

patch 01/11 ("audio: log unimplemented audio device sample rates") takes 
care of samples == 0. But you are right, I didn't consider samples < 0. 
I will send a version 2 patch within the next few days.

With best regards,
Volker

> With regards,
> Daniel

Re: [PATCH 07/11] audio/audio_template: use g_malloc0() to replace audio_calloc()

[Volker Rümelin]

Am 16.01.23 um 09:58 schrieb Daniel P. Berrangé:
> On Sun, Dec 18, 2022 at 06:39:00PM +0100, Volker Rümelin wrote:
>> Am 18.12.22 um 18:26 schrieb Philippe Mathieu-Daudé:
>>> On 18/12/22 18:15, Volker Rümelin wrote:
>>>> Use g_malloc0() as a direct replacement for audio_calloc().
>>>>
>>>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>>>> ---
>>>>    audio/audio_template.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/audio/audio_template.h b/audio/audio_template.h
>>>> index d343a1dcb3..5f51ef26b2 100644
>>>> --- a/audio/audio_template.h
>>>> +++ b/audio/audio_template.h
>>>> @@ -273,7 +273,7 @@ static HW *glue(audio_pcm_hw_add_new_,
>>>> TYPE)(AudioState *s,
>>>>            return NULL;
>>>>        }
>>>>    -    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
>>>> +    hw = g_malloc0(glue(drv->voice_size_, TYPE));
>>>>        if (!hw) {
>>> g_malloc0() can't fail. Either you want g_try_malloc0() or
>>> remove the error path.
>>>
>> g_malloc0() returns NULL if drv->voice_size_(out|in) is 0. I think the code
>> is correct.
> IMHO relying on that is rather misleading to people reviewing the code
> though. As seen by Philippe's reply, people generally don't expect that
> g_malloc0 can return NULL, and it is not at all obvious that we are
> intentionally expecting 0 to be passed as a size.
>
> Please make this explicit by removing and if (!hw) check after
> g_malloc, and adding a check before g_malloc
>
>     if (audio_bug(__func__, glue(drv->voice_size_, TYPE) == 0)) {
>         dolog (...)

I'll change it.

With best regards,
Volker

>
> With regards,
> Daniel