[PATCH v2 00/11] audio: more improvements

[PATCH v2 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(). 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.

v2: Address Daniel's comments

[PATCH v2 07/11] audio/audio_template: use g_malloc0() to replace 
audio_calloc()
The n_bytes argument of g_malloc0() is now always > 0 at this call site.

[PATCH v2 08/11] audio/audio_template: use g_new0() to replace 
audio_calloc()
Check samples for negative values. It's possible to have negative 
samples due to overflows or unsigned to signed conversions. Patch 01/11 
takes care of samples == 0.

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      |  8 ++---
  audio/audio_template.h | 68 ++++++++++++++++++++++--------------------
  audio/mixeng.c         |  7 +----
  5 files changed, 48 insertions(+), 88 deletions(-)

-- 
2.35.3

[PATCH v2 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.

Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
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 v2 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 v2 03/11] audio: rename hardware store to backend

[Volker Rümelin]

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

Reviewed-by: Thomas Huth <thuth@redhat.com>
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

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

[Philippe Mathieu-Daudé]

On 21/1/23 10:47, Volker Rümelin wrote:
> Use a consistent friendly name for the HWVoiceOut and HWVoiceIn
> structures.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   audio/audio_template.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

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

[PATCH v2 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__").

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
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 v2 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.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
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 v2 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
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 v2 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().

Since the type of the parameter n_bytes of the function g_malloc0()
is unsigned, the type of the variables voice_size_out and
voice_size_in has been changed to size_t. This means that the
function argument no longer has to be checked for negative values.

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

diff --git a/audio/audio_int.h b/audio/audio_int.h
index 4632cdf9cc..ce2d6bf92c 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -151,8 +151,8 @@ struct audio_driver {
     int can_be_default;
     int max_voices_out;
     int max_voices_in;
-    int voice_size_out;
-    int voice_size_in;
+    size_t voice_size_out;
+    size_t voice_size_in;
     QLIST_ENTRY(audio_driver) next;
 };
 
diff --git a/audio/audio_template.h b/audio/audio_template.h
index d343a1dcb3..6b7d1fea83 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -40,7 +40,7 @@ static void glue(audio_init_nb_voices_, TYPE)(AudioState *s,
                                               struct audio_driver *drv)
 {
     int max_voices = glue (drv->max_voices_, TYPE);
-    int voice_size = glue (drv->voice_size_, TYPE);
+    size_t voice_size = glue(drv->voice_size_, TYPE);
 
     if (glue (s->nb_hw_voices_, TYPE) > max_voices) {
         if (!max_voices) {
@@ -63,8 +63,8 @@ static void glue(audio_init_nb_voices_, TYPE)(AudioState *s,
     }
 
     if (audio_bug(__func__, voice_size && !max_voices)) {
-        dolog ("drv=`%s' voice_size=%d max_voices=0\n",
-               drv->name, voice_size);
+        dolog("drv=`%s' voice_size=%zu max_voices=0\n",
+              drv->name, voice_size);
     }
 }
 
@@ -273,13 +273,11 @@ static HW *glue(audio_pcm_hw_add_new_, TYPE)(AudioState *s,
         return NULL;
     }
 
-    hw = audio_calloc(__func__, 1, glue(drv->voice_size_, TYPE));
-    if (!hw) {
-        dolog ("Can not allocate voice `%s' size %d\n",
-               drv->name, glue (drv->voice_size_, TYPE));
-        return NULL;
-    }
-
+    /*
+     * Since glue(s->nb_hw_voices_, TYPE) is != 0, glue(drv->voice_size_, TYPE)
+     * is guaranteed to be != 0. See the audio_init_nb_voices_* functions.
+     */
+    hw = g_malloc0(glue(drv->voice_size_, TYPE));
     hw->s = s;
     hw->pcm_ops = drv->pcm_ops;
 
-- 
2.35.3

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

[Daniel P. Berrangé]

On Sat, Jan 21, 2023 at 10:47:31AM +0100, Volker Rümelin wrote:
> Use g_malloc0() as a direct replacement for audio_calloc().
> 
> Since the type of the parameter n_bytes of the function g_malloc0()
> is unsigned, the type of the variables voice_size_out and
> voice_size_in has been changed to size_t. This means that the
> function argument no longer has to be checked for negative values.
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  audio/audio_int.h      |  4 ++--
>  audio/audio_template.h | 18 ++++++++----------
>  2 files changed, 10 insertions(+), 12 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH v2 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 | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/audio/audio_template.h b/audio/audio_template.h
index 6b7d1fea83..ba010d7e21 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -115,6 +115,12 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
 #else
     samples = (int64_t)sw->HWBUF->size * sw->ratio >> 32;
 #endif
+    if (audio_bug(__func__, samples < 0)) {
+        dolog("Can not allocate buffer for `%s' (%d samples)\n",
+              SW_NAME(sw), samples);
+        return -1;
+    }
+
     if (samples == 0) {
         HW *hw = sw->hw;
         size_t f_fe_min;
@@ -129,12 +135,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);
@@ -405,34 +406,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

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

[Daniel P. Berrangé]

On Sat, Jan 21, 2023 at 10:47:32AM +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 | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH v2 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 ce2d6bf92c..5028f2354a 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 v2 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

Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
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

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

[Philippe Mathieu-Daudé]

On 21/1/23 10:47, 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).
> 
> 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
> 
> Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> 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;

Please use DIV_ROUND_UP():

     DIV_ROUND_UP(1000000ul << 8, 44100);

Or

     DIV_ROUND_UP(512 * 1000000ul, 44100);

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

Ditto:

     DIV_ROUND_UP(1000000ul << 12, 44100);

>       }
>   
>       /*

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

[Volker Rümelin]

Am 23.01.23 um 08:43 schrieb Philippe Mathieu-Daudé:
> On 21/1/23 10:47, 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).
>>
>> 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
>>
>> Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>> 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;
>
> Please use DIV_ROUND_UP():
>
>     DIV_ROUND_UP(1000000ul << 8, 44100);
>
> Or
>
>     DIV_ROUND_UP(512 * 1000000ul, 44100);

Hi,

the corresponding -audiodev alsa command line parameters 
out.buffer-length, in.buffer-length, out.period-length and 
in.period-lenght are specified in microseconds. I prefer that the 
default values use the same unit. I believe that specifying buffer 
lengths in microseconds makes more sense than specifying buffer lengths 
in audio frames.

With best regards,
Volker

>
>
>>       }
>>       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;
>
> Ditto:
>
>     DIV_ROUND_UP(1000000ul << 12, 44100);
>
>>       }
>>         /*
>

[PATCH v2 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 v2 11/11] alsaaudio: reintroduce default recording settings

[Philippe Mathieu-Daudé]

On 21/1/23 10:47, Volker Rümelin wrote:
> 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(-)

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

Please use DIV_ROUND_UP(). Maybe worth adding definitions?

Re: [PATCH v2 00/11] audio: more improvements

[Marc-André Lureau]

Hi

On Sat, Jan 21, 2023 at 1:47 PM Volker Rümelin <vr_qemu@t-online.de> wrote:
>
> 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(). 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.
>
> v2: Address Daniel's comments
>
> [PATCH v2 07/11] audio/audio_template: use g_malloc0() to replace
> audio_calloc()
> The n_bytes argument of g_malloc0() is now always > 0 at this call site.
>
> [PATCH v2 08/11] audio/audio_template: use g_new0() to replace
> audio_calloc()
> Check samples for negative values. It's possible to have negative
> samples due to overflows or unsigned to signed conversions. Patch 01/11
> takes care of samples == 0.
>
> 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

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH v2 00/11] audio: more improvements

[Volker Rümelin]

Ping. Is there anything left to do to get this patch series merged for 
8.0? All patches are reviewed.

With best regards,
Volker

> 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(). 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.
>
> v2: Address Daniel's comments
>
> [PATCH v2 07/11] audio/audio_template: use g_malloc0() to replace 
> audio_calloc()
> The n_bytes argument of g_malloc0() is now always > 0 at this call site.
>
> [PATCH v2 08/11] audio/audio_template: use g_new0() to replace 
> audio_calloc()
> Check samples for negative values. It's possible to have negative 
> samples due to overflows or unsigned to signed conversions. Patch 
> 01/11 takes care of samples == 0.
>
> 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      |  8 ++---
>  audio/audio_template.h | 68 ++++++++++++++++++++++--------------------
>  audio/mixeng.c         |  7 +----
>  5 files changed, 48 insertions(+), 88 deletions(-)
>