[PATCH 00/12] audio: misc. improvements and bug fixes

[PATCH 00/12] audio: misc. improvements and bug fixes

[Volker Rümelin]

A series of audio improvements and fixes.

One note:

Patch 11/12 "audio: fix sw->buf size for audio recording":
If this patch is applied without the patch series "[PATCH 0/2] audio: 
prevent a class of guest-triggered aborts" at 
https://lists.nongnu.org/archive/html/qemu-devel/2022-09/msg02347.html, 
issue #71 triggers a QEMU abort. Patch 11/12 is nevertheless correct.

Volker Rümelin (12):
   audio: refactor code in audio_run_out()
   audio: fix GUS audio playback with out.mixing-engine=off
   audio: run downstream playback queue unconditionally
   alsaaudio: reduce playback latency
   audio: add more audio rate control functions
   spiceaudio: add a pcm_ops buffer_get_free function
   spiceaudio: update comment
   audio: swap audio_rate_get_bytes() function parameters
   audio: rename audio_sw_bytes_free()
   audio: refactor audio_get_avail()
   audio: fix sw->buf size for audio recording
   audio: prevent an integer overflow in resampling code

  audio/alsaaudio.c      |  38 ++++++++++++++-
  audio/audio.c          | 107 +++++++++++++++++++++++++++--------------
  audio/audio_int.h      |   4 +-
  audio/audio_template.h |   4 ++
  audio/dbusaudio.c      |   4 +-
  audio/noaudio.c        |   4 +-
  audio/rate_template.h  |  11 +++--
  audio/spiceaudio.c     |  19 ++++++--
  audio/wavaudio.c       |   2 +-
  9 files changed, 141 insertions(+), 52 deletions(-)

-- 
2.35.3

[PATCH 01/12] audio: refactor code in audio_run_out()

[Volker Rümelin]

Refactoring the code in audio_run_out() avoids code duplication
in the next patch. There's no functional change.

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

diff --git a/audio/audio.c b/audio/audio.c
index cfa4119c05..04f685fe24 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1121,8 +1121,12 @@ static void audio_run_out (AudioState *s)
     HWVoiceOut *hw = NULL;
     SWVoiceOut *sw;
 
-    if (!audio_get_pdo_out(s->dev)->mixing_engine) {
-        while ((hw = audio_pcm_hw_find_any_enabled_out(s, hw))) {
+    while ((hw = audio_pcm_hw_find_any_enabled_out(s, hw))) {
+        size_t played, live, prev_rpos;
+        size_t hw_free = audio_pcm_hw_get_free(hw);
+        int nb_live;
+
+        if (!audio_get_pdo_out(s->dev)->mixing_engine) {
             /* there is exactly 1 sw for each hw with no mixeng */
             sw = hw->sw_head.lh_first;
 
@@ -1137,14 +1141,9 @@ static void audio_run_out (AudioState *s)
             if (sw->active) {
                 sw->callback.fn(sw->callback.opaque, INT_MAX);
             }
-        }
-        return;
-    }
 
-    while ((hw = audio_pcm_hw_find_any_enabled_out(s, hw))) {
-        size_t played, live, prev_rpos;
-        size_t hw_free = audio_pcm_hw_get_free(hw);
-        int nb_live;
+            continue;
+        }
 
         for (sw = hw->sw_head.lh_first; sw; sw = sw->entries.le_next) {
             if (sw->active) {
-- 
2.35.3

Re: [PATCH 01/12] audio: refactor code in audio_run_out()

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

On Fri, Sep 23, 2022 at 10:43 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Refactoring the code in audio_run_out() avoids code duplication
> in the next patch. There's no functional change.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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



> ---
>  audio/audio.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index cfa4119c05..04f685fe24 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1121,8 +1121,12 @@ static void audio_run_out (AudioState *s)
>      HWVoiceOut *hw = NULL;
>      SWVoiceOut *sw;
>
> -    if (!audio_get_pdo_out(s->dev)->mixing_engine) {
> -        while ((hw = audio_pcm_hw_find_any_enabled_out(s, hw))) {
> +    while ((hw = audio_pcm_hw_find_any_enabled_out(s, hw))) {
> +        size_t played, live, prev_rpos;
> +        size_t hw_free = audio_pcm_hw_get_free(hw);
> +        int nb_live;
> +
> +        if (!audio_get_pdo_out(s->dev)->mixing_engine) {
>              /* there is exactly 1 sw for each hw with no mixeng */
>              sw = hw->sw_head.lh_first;
>
> @@ -1137,14 +1141,9 @@ static void audio_run_out (AudioState *s)
>              if (sw->active) {
>                  sw->callback.fn(sw->callback.opaque, INT_MAX);
>              }
> -        }
> -        return;
> -    }
>
> -    while ((hw = audio_pcm_hw_find_any_enabled_out(s, hw))) {
> -        size_t played, live, prev_rpos;
> -        size_t hw_free = audio_pcm_hw_get_free(hw);
> -        int nb_live;
> +            continue;
> +        }
>
>          for (sw = hw->sw_head.lh_first; sw; sw = sw->entries.le_next) {
>              if (sw->active) {
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2732 bytes --]

[PATCH 02/12] audio: fix GUS audio playback with out.mixing-engine=off

[Volker Rümelin]

Fix GUS audio playback with out.mixing-engine=off.

The GUS audio device needs to know the amount of samples to
produce in advance.

To reproduce start qemu with
-parallel none -device gus,audiodev=audio0
-audiodev pa,id=audio0,out.mixing-engine=off

and start the cartoon.exe demo in a FreeDOS guest. The demo file
is available on the download page of the GUSemu32 author.

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

diff --git a/audio/audio.c b/audio/audio.c
index 04f685fe24..343786243d 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1139,7 +1139,8 @@ static void audio_run_out (AudioState *s)
             }
 
             if (sw->active) {
-                sw->callback.fn(sw->callback.opaque, INT_MAX);
+                sw->callback.fn(sw->callback.opaque,
+                                hw_free * sw->info.bytes_per_frame);
             }
 
             continue;
-- 
2.35.3

Re: [PATCH 02/12] audio: fix GUS audio playback with out.mixing-engine=off

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Fri, Sep 23, 2022 at 10:39 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Fix GUS audio playback with out.mixing-engine=off.
>
> The GUS audio device needs to know the amount of samples to
> produce in advance.
>
> To reproduce start qemu with
> -parallel none -device gus,audiodev=audio0
> -audiodev pa,id=audio0,out.mixing-engine=off
>
> and start the cartoon.exe demo in a FreeDOS guest. The demo file
> is available on the download page of the GUSemu32 author.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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



> ---
>  audio/audio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 04f685fe24..343786243d 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1139,7 +1139,8 @@ static void audio_run_out (AudioState *s)
>              }
>
>              if (sw->active) {
> -                sw->callback.fn(sw->callback.opaque, INT_MAX);
> +                sw->callback.fn(sw->callback.opaque,
> +                                hw_free * sw->info.bytes_per_frame);
>              }
>
>              continue;
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2081 bytes --]

[PATCH 03/12] audio: run downstream playback queue unconditionally

[Volker Rümelin]

Run the downstream playback queue even if the emulated audio
device didn't write new samples. There still may be buffered
audio samples downstream.

This is for the -audiodev out.mixing-engine=off case. Commit
a8a98cfd42 ("audio: run downstream playback queue uncondition-
ally") fixed the out.mixing-engine=on case.

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

diff --git a/audio/audio.c b/audio/audio.c
index 343786243d..9e55834909 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1143,6 +1143,10 @@ static void audio_run_out (AudioState *s)
                                 hw_free * sw->info.bytes_per_frame);
             }
 
+            if (hw->pcm_ops->run_buffer_out) {
+                hw->pcm_ops->run_buffer_out(hw);
+            }
+
             continue;
         }
 
@@ -1501,10 +1505,6 @@ size_t audio_generic_write(HWVoiceOut *hw, void *buf, size_t size)
         }
     }
 
-    if (hw->pcm_ops->run_buffer_out) {
-        hw->pcm_ops->run_buffer_out(hw);
-    }
-
     return total;
 }
 
-- 
2.35.3

Re: [PATCH 03/12] audio: run downstream playback queue unconditionally

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]

On Fri, Sep 23, 2022 at 10:45 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Run the downstream playback queue even if the emulated audio
> device didn't write new samples. There still may be buffered
> audio samples downstream.
>
> This is for the -audiodev out.mixing-engine=off case. Commit
> a8a98cfd42 ("audio: run downstream playback queue uncondition-
> ally") fixed the out.mixing-engine=on case.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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


> ---
>  audio/audio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 343786243d..9e55834909 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1143,6 +1143,10 @@ static void audio_run_out (AudioState *s)
>                                  hw_free * sw->info.bytes_per_frame);
>              }
>
> +            if (hw->pcm_ops->run_buffer_out) {
> +                hw->pcm_ops->run_buffer_out(hw);
> +            }
> +
>              continue;
>          }
>
> @@ -1501,10 +1505,6 @@ size_t audio_generic_write(HWVoiceOut *hw, void
> *buf, size_t size)
>          }
>      }
>
> -    if (hw->pcm_ops->run_buffer_out) {
> -        hw->pcm_ops->run_buffer_out(hw);
> -    }
> -
>      return total;
>  }
>
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2298 bytes --]

[PATCH 04/12] alsaaudio: reduce playback latency

[Volker Rümelin]

Change the buffer_get_free pcm_ops function to report the free
ALSA playback buffer. The generic buffer becomes a temporary
buffer and is empty after a call to audio_run_out().

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

diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
index 4a61378cd7..7a2a94cd42 100644
--- a/audio/alsaaudio.c
+++ b/audio/alsaaudio.c
@@ -602,6 +602,42 @@ static int alsa_open(bool in, struct alsa_params_req *req,
     return -1;
 }
 
+static size_t alsa_buffer_get_free(HWVoiceOut *hw)
+{
+    ALSAVoiceOut *alsa = (ALSAVoiceOut *)hw;
+    snd_pcm_sframes_t avail;
+    size_t alsa_free, generic_free, generic_in_use;
+
+    avail = snd_pcm_avail_update(alsa->handle);
+    if (avail < 0) {
+        if (avail == -EPIPE) {
+            if (!alsa_recover(alsa->handle)) {
+                avail = snd_pcm_avail_update(alsa->handle);
+            }
+        }
+        if (avail < 0) {
+            alsa_logerr(avail,
+                        "Could not obtain number of available frames\n");
+            avail = 0;
+        }
+    }
+
+    alsa_free = avail * hw->info.bytes_per_frame;
+    generic_free = audio_generic_buffer_get_free(hw);
+    generic_in_use = hw->samples * hw->info.bytes_per_frame - generic_free;
+    if (generic_in_use) {
+        /*
+         * This code can only be reached in the unlikely case that
+         * snd_pcm_avail_update() returned a larger number of frames
+         * than snd_pcm_writei() could write. Make sure that all
+         * remaining bytes in the generic buffer can be written.
+         */
+        alsa_free = alsa_free > generic_in_use ? alsa_free - generic_in_use : 0;
+    }
+
+    return alsa_free;
+}
+
 static size_t alsa_write(HWVoiceOut *hw, void *buf, size_t len)
 {
     ALSAVoiceOut *alsa = (ALSAVoiceOut *) hw;
@@ -916,7 +952,7 @@ static struct audio_pcm_ops alsa_pcm_ops = {
     .init_out = alsa_init_out,
     .fini_out = alsa_fini_out,
     .write    = alsa_write,
-    .buffer_get_free = audio_generic_buffer_get_free,
+    .buffer_get_free = alsa_buffer_get_free,
     .run_buffer_out = audio_generic_run_buffer_out,
     .enable_out = alsa_enable_out,
 
-- 
2.35.3

Re: [PATCH 04/12] alsaaudio: reduce playback latency

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]

On Fri, Sep 23, 2022 at 10:51 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Change the buffer_get_free pcm_ops function to report the free
> ALSA playback buffer. The generic buffer becomes a temporary
> buffer and is empty after a call to audio_run_out().
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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


> ---
>  audio/alsaaudio.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
> index 4a61378cd7..7a2a94cd42 100644
> --- a/audio/alsaaudio.c
> +++ b/audio/alsaaudio.c
> @@ -602,6 +602,42 @@ static int alsa_open(bool in, struct alsa_params_req
> *req,
>      return -1;
>  }
>
> +static size_t alsa_buffer_get_free(HWVoiceOut *hw)
> +{
> +    ALSAVoiceOut *alsa = (ALSAVoiceOut *)hw;
> +    snd_pcm_sframes_t avail;
> +    size_t alsa_free, generic_free, generic_in_use;
> +
> +    avail = snd_pcm_avail_update(alsa->handle);
> +    if (avail < 0) {
> +        if (avail == -EPIPE) {
> +            if (!alsa_recover(alsa->handle)) {
> +                avail = snd_pcm_avail_update(alsa->handle);
> +            }
> +        }
> +        if (avail < 0) {
> +            alsa_logerr(avail,
> +                        "Could not obtain number of available frames\n");
> +            avail = 0;
> +        }
> +    }
> +
> +    alsa_free = avail * hw->info.bytes_per_frame;
> +    generic_free = audio_generic_buffer_get_free(hw);
> +    generic_in_use = hw->samples * hw->info.bytes_per_frame -
> generic_free;
> +    if (generic_in_use) {
> +        /*
> +         * This code can only be reached in the unlikely case that
> +         * snd_pcm_avail_update() returned a larger number of frames
> +         * than snd_pcm_writei() could write. Make sure that all
> +         * remaining bytes in the generic buffer can be written.
> +         */
> +        alsa_free = alsa_free > generic_in_use ? alsa_free -
> generic_in_use : 0;
> +    }
> +
> +    return alsa_free;
> +}
> +
>  static size_t alsa_write(HWVoiceOut *hw, void *buf, size_t len)
>  {
>      ALSAVoiceOut *alsa = (ALSAVoiceOut *) hw;
> @@ -916,7 +952,7 @@ static struct audio_pcm_ops alsa_pcm_ops = {
>      .init_out = alsa_init_out,
>      .fini_out = alsa_fini_out,
>      .write    = alsa_write,
> -    .buffer_get_free = audio_generic_buffer_get_free,
> +    .buffer_get_free = alsa_buffer_get_free,
>      .run_buffer_out = audio_generic_run_buffer_out,
>      .enable_out = alsa_enable_out,
>
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3671 bytes --]

[PATCH 05/12] audio: add more audio rate control functions

[Volker Rümelin]

The next patch needs two new rate control functions. The first
one returns the bytes needed at call time to maintain the
selected rate. The second one adjusts the bytes actually sent.

Split the audio_rate_get_bytes() function into these two
functions and reintroduce audio_rate_get_bytes().

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c     | 35 ++++++++++++++++++++++++-----------
 audio/audio_int.h |  2 ++
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 9e55834909..557538a7b7 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2250,26 +2250,39 @@ void audio_rate_start(RateCtl *rate)
     rate->start_ticks = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 }
 
-size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
-                            size_t bytes_avail)
+size_t audio_rate_peek_bytes(RateCtl *rate, struct audio_pcm_info *info)
 {
     int64_t now;
     int64_t ticks;
     int64_t bytes;
-    int64_t samples;
-    size_t ret;
+    int64_t frames;
 
     now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     ticks = now - rate->start_ticks;
     bytes = muldiv64(ticks, info->bytes_per_second, NANOSECONDS_PER_SECOND);
-    samples = (bytes - rate->bytes_sent) / info->bytes_per_frame;
-    if (samples < 0 || samples > 65536) {
-        AUD_log(NULL, "Resetting rate control (%" PRId64 " samples)\n", samples);
+    frames = (bytes - rate->bytes_sent) / info->bytes_per_frame;
+    if (frames < 0 || frames > 65536) {
+        AUD_log(NULL, "Resetting rate control (%" PRId64 " frames)\n", frames);
         audio_rate_start(rate);
-        samples = 0;
+        frames = 0;
     }
 
-    ret = MIN(samples * info->bytes_per_frame, bytes_avail);
-    rate->bytes_sent += ret;
-    return ret;
+    return frames * info->bytes_per_frame;
+}
+
+void audio_rate_add_bytes(RateCtl *rate, size_t bytes_used)
+{
+    rate->bytes_sent += bytes_used;
+}
+
+size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
+                            size_t bytes_avail)
+{
+    size_t bytes;
+
+    bytes = audio_rate_peek_bytes(rate, info);
+    bytes = MIN(bytes, bytes_avail);
+    audio_rate_add_bytes(rate, bytes);
+
+    return bytes;
 }
diff --git a/audio/audio_int.h b/audio/audio_int.h
index 2a6914d2aa..97e20e8429 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -263,6 +263,8 @@ typedef struct RateCtl {
 } RateCtl;
 
 void audio_rate_start(RateCtl *rate);
+size_t audio_rate_peek_bytes(RateCtl *rate, struct audio_pcm_info *info);
+void audio_rate_add_bytes(RateCtl *rate, size_t bytes_used);
 size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
                             size_t bytes_avail);
 
-- 
2.35.3

Re: [PATCH 05/12] audio: add more audio rate control functions

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

On Fri, Sep 23, 2022 at 10:43 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> The next patch needs two new rate control functions. The first
> one returns the bytes needed at call time to maintain the
> selected rate. The second one adjusts the bytes actually sent.
>
> Split the audio_rate_get_bytes() function into these two
> functions and reintroduce audio_rate_get_bytes().
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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



> ---
>  audio/audio.c     | 35 ++++++++++++++++++++++++-----------
>  audio/audio_int.h |  2 ++
>  2 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 9e55834909..557538a7b7 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -2250,26 +2250,39 @@ void audio_rate_start(RateCtl *rate)
>      rate->start_ticks = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  }
>
> -size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
> -                            size_t bytes_avail)
> +size_t audio_rate_peek_bytes(RateCtl *rate, struct audio_pcm_info *info)
>  {
>      int64_t now;
>      int64_t ticks;
>      int64_t bytes;
> -    int64_t samples;
> -    size_t ret;
> +    int64_t frames;
>
>      now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>      ticks = now - rate->start_ticks;
>      bytes = muldiv64(ticks, info->bytes_per_second,
> NANOSECONDS_PER_SECOND);
> -    samples = (bytes - rate->bytes_sent) / info->bytes_per_frame;
> -    if (samples < 0 || samples > 65536) {
> -        AUD_log(NULL, "Resetting rate control (%" PRId64 " samples)\n",
> samples);
> +    frames = (bytes - rate->bytes_sent) / info->bytes_per_frame;
> +    if (frames < 0 || frames > 65536) {
> +        AUD_log(NULL, "Resetting rate control (%" PRId64 " frames)\n",
> frames);
>          audio_rate_start(rate);
> -        samples = 0;
> +        frames = 0;
>      }
>
> -    ret = MIN(samples * info->bytes_per_frame, bytes_avail);
> -    rate->bytes_sent += ret;
> -    return ret;
> +    return frames * info->bytes_per_frame;
> +}
> +
> +void audio_rate_add_bytes(RateCtl *rate, size_t bytes_used)
> +{
> +    rate->bytes_sent += bytes_used;
> +}
> +
> +size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
> +                            size_t bytes_avail)
> +{
> +    size_t bytes;
> +
> +    bytes = audio_rate_peek_bytes(rate, info);
> +    bytes = MIN(bytes, bytes_avail);
> +    audio_rate_add_bytes(rate, bytes);
> +
> +    return bytes;
>  }
> diff --git a/audio/audio_int.h b/audio/audio_int.h
> index 2a6914d2aa..97e20e8429 100644
> --- a/audio/audio_int.h
> +++ b/audio/audio_int.h
> @@ -263,6 +263,8 @@ typedef struct RateCtl {
>  } RateCtl;
>
>  void audio_rate_start(RateCtl *rate);
> +size_t audio_rate_peek_bytes(RateCtl *rate, struct audio_pcm_info *info);
> +void audio_rate_add_bytes(RateCtl *rate, size_t bytes_used);
>  size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
>                              size_t bytes_avail);
>
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4234 bytes --]

[PATCH 06/12] spiceaudio: add a pcm_ops buffer_get_free function

[Volker Rümelin]

It seems there is a demand [1] for low latency playback over
SPICE. Add a pcm_ops buffer_get_free function to reduce the
playback latency. The mixing engine buffer becomes a temporary
buffer.

[1] https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg01644.html

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

diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
index a8d370fe6f..22892a7b9d 100644
--- a/audio/spiceaudio.c
+++ b/audio/spiceaudio.c
@@ -120,6 +120,13 @@ static void line_out_fini (HWVoiceOut *hw)
     spice_server_remove_interface (&out->sin.base);
 }
 
+static size_t line_out_get_free(HWVoiceOut *hw)
+{
+    SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
+
+    return audio_rate_peek_bytes(&out->rate, &hw->info);
+}
+
 static void *line_out_get_buffer(HWVoiceOut *hw, size_t *size)
 {
     SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
@@ -133,8 +140,6 @@ static void *line_out_get_buffer(HWVoiceOut *hw, size_t *size)
         *size = MIN((out->fsize - out->fpos) << 2, *size);
     }
 
-    *size = audio_rate_get_bytes(&hw->info, &out->rate, *size);
-
     return out->frame + out->fpos;
 }
 
@@ -142,6 +147,8 @@ static size_t line_out_put_buffer(HWVoiceOut *hw, void *buf, size_t size)
 {
     SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
 
+    audio_rate_add_bytes(&out->rate, size);
+
     if (buf) {
         assert(buf == out->frame + out->fpos && out->fpos <= out->fsize);
         out->fpos += size >> 2;
@@ -282,6 +289,7 @@ static struct audio_pcm_ops audio_callbacks = {
     .init_out = line_out_init,
     .fini_out = line_out_fini,
     .write    = audio_generic_write,
+    .buffer_get_free = line_out_get_free,
     .get_buffer_out = line_out_get_buffer,
     .put_buffer_out = line_out_put_buffer,
     .enable_out = line_out_enable,
-- 
2.35.3

Re: [PATCH 06/12] spiceaudio: add a pcm_ops buffer_get_free function

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

On Fri, Sep 23, 2022 at 10:58 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> It seems there is a demand [1] for low latency playback over
> SPICE. Add a pcm_ops buffer_get_free function to reduce the
> playback latency. The mixing engine buffer becomes a temporary
> buffer.
>
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg01644.html
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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



> ---
>  audio/spiceaudio.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
> index a8d370fe6f..22892a7b9d 100644
> --- a/audio/spiceaudio.c
> +++ b/audio/spiceaudio.c
> @@ -120,6 +120,13 @@ static void line_out_fini (HWVoiceOut *hw)
>      spice_server_remove_interface (&out->sin.base);
>  }
>
> +static size_t line_out_get_free(HWVoiceOut *hw)
> +{
> +    SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
> +
> +    return audio_rate_peek_bytes(&out->rate, &hw->info);
> +}
> +
>  static void *line_out_get_buffer(HWVoiceOut *hw, size_t *size)
>  {
>      SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
> @@ -133,8 +140,6 @@ static void *line_out_get_buffer(HWVoiceOut *hw,
> size_t *size)
>          *size = MIN((out->fsize - out->fpos) << 2, *size);
>      }
>
> -    *size = audio_rate_get_bytes(&hw->info, &out->rate, *size);
> -
>      return out->frame + out->fpos;
>  }
>
> @@ -142,6 +147,8 @@ static size_t line_out_put_buffer(HWVoiceOut *hw, void
> *buf, size_t size)
>  {
>      SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
>
> +    audio_rate_add_bytes(&out->rate, size);
> +
>      if (buf) {
>          assert(buf == out->frame + out->fpos && out->fpos <= out->fsize);
>          out->fpos += size >> 2;
> @@ -282,6 +289,7 @@ static struct audio_pcm_ops audio_callbacks = {
>      .init_out = line_out_init,
>      .fini_out = line_out_fini,
>      .write    = audio_generic_write,
> +    .buffer_get_free = line_out_get_free,
>      .get_buffer_out = line_out_get_buffer,
>      .put_buffer_out = line_out_put_buffer,
>      .enable_out = line_out_enable,
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3341 bytes --]

[PATCH 07/12] spiceaudio: update comment

[Volker Rümelin]

Replace a comment with a question with the answer.

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

diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
index 22892a7b9d..f52f3a8bbb 100644
--- a/audio/spiceaudio.c
+++ b/audio/spiceaudio.c
@@ -242,7 +242,10 @@ static size_t line_in_read(HWVoiceIn *hw, void *buf, size_t len)
     uint64_t to_read = audio_rate_get_bytes(&hw->info, &in->rate, len) >> 2;
     size_t ready = spice_server_record_get_samples(&in->sin, buf, to_read);
 
-    /* XXX: do we need this? */
+    /*
+     * If the client didn't send new frames, it most likely disconnected.
+     * Generate silence in this case to avoid a stalled audio stream.
+     */
     if (ready == 0) {
         memset(buf, 0, to_read << 2);
         ready = to_read;
-- 
2.35.3

Re: [PATCH 07/12] spiceaudio: update comment

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

On Fri, Sep 23, 2022 at 10:55 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Replace a comment with a question with the answer.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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



> ---
>  audio/spiceaudio.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
> index 22892a7b9d..f52f3a8bbb 100644
> --- a/audio/spiceaudio.c
> +++ b/audio/spiceaudio.c
> @@ -242,7 +242,10 @@ static size_t line_in_read(HWVoiceIn *hw, void *buf,
> size_t len)
>      uint64_t to_read = audio_rate_get_bytes(&hw->info, &in->rate, len) >>
> 2;
>      size_t ready = spice_server_record_get_samples(&in->sin, buf,
> to_read);
>
> -    /* XXX: do we need this? */
> +    /*
> +     * If the client didn't send new frames, it most likely disconnected.
> +     * Generate silence in this case to avoid a stalled audio stream.
> +     */
>      if (ready == 0) {
>          memset(buf, 0, to_read << 2);
>          ready = to_read;
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1908 bytes --]

[PATCH 08/12] audio: swap audio_rate_get_bytes() function parameters

[Volker Rümelin]

Swap the rate and info parameters of the audio_rate_get_bytes()
function to align the parameter order with the rest of the
audio_rate_*() functions.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c      | 2 +-
 audio/audio_int.h  | 2 +-
 audio/dbusaudio.c  | 4 ++--
 audio/noaudio.c    | 4 ++--
 audio/spiceaudio.c | 2 +-
 audio/wavaudio.c   | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 557538a7b7..233a86c440 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2275,7 +2275,7 @@ void audio_rate_add_bytes(RateCtl *rate, size_t bytes_used)
     rate->bytes_sent += bytes_used;
 }
 
-size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
+size_t audio_rate_get_bytes(RateCtl *rate, struct audio_pcm_info *info,
                             size_t bytes_avail)
 {
     size_t bytes;
diff --git a/audio/audio_int.h b/audio/audio_int.h
index 97e20e8429..e87ce014a0 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -265,7 +265,7 @@ typedef struct RateCtl {
 void audio_rate_start(RateCtl *rate);
 size_t audio_rate_peek_bytes(RateCtl *rate, struct audio_pcm_info *info);
 void audio_rate_add_bytes(RateCtl *rate, size_t bytes_used);
-size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
+size_t audio_rate_get_bytes(RateCtl *rate, struct audio_pcm_info *info,
                             size_t bytes_avail);
 
 static inline size_t audio_ring_dist(size_t dst, size_t src, size_t len)
diff --git a/audio/dbusaudio.c b/audio/dbusaudio.c
index a3d656d3b0..722df0355e 100644
--- a/audio/dbusaudio.c
+++ b/audio/dbusaudio.c
@@ -82,7 +82,7 @@ static void *dbus_get_buffer_out(HWVoiceOut *hw, size_t *size)
     }
 
     *size = MIN(vo->buf_size - vo->buf_pos, *size);
-    *size = audio_rate_get_bytes(&hw->info, &vo->rate, *size);
+    *size = audio_rate_get_bytes(&vo->rate, &hw->info, *size);
 
     return vo->buf + vo->buf_pos;
 
@@ -343,7 +343,7 @@ dbus_read(HWVoiceIn *hw, void *buf, size_t size)
 
     trace_dbus_audio_read(size);
 
-    /* size = audio_rate_get_bytes(&hw->info, &vo->rate, size); */
+    /* size = audio_rate_get_bytes(&vo->rate, &hw->info, size); */
 
     g_hash_table_iter_init(&iter, da->in_listeners);
     while (g_hash_table_iter_next(&iter, NULL, (void **)&listener)) {
diff --git a/audio/noaudio.c b/audio/noaudio.c
index 84a6bfbb1c..4fdee5adec 100644
--- a/audio/noaudio.c
+++ b/audio/noaudio.c
@@ -44,7 +44,7 @@ typedef struct NoVoiceIn {
 static size_t no_write(HWVoiceOut *hw, void *buf, size_t len)
 {
     NoVoiceOut *no = (NoVoiceOut *) hw;
-    return audio_rate_get_bytes(&hw->info, &no->rate, len);
+    return audio_rate_get_bytes(&no->rate, &hw->info, len);
 }
 
 static int no_init_out(HWVoiceOut *hw, struct audsettings *as, void *drv_opaque)
@@ -89,7 +89,7 @@ static void no_fini_in (HWVoiceIn *hw)
 static size_t no_read(HWVoiceIn *hw, void *buf, size_t size)
 {
     NoVoiceIn *no = (NoVoiceIn *) hw;
-    int64_t bytes = audio_rate_get_bytes(&hw->info, &no->rate, size);
+    int64_t bytes = audio_rate_get_bytes(&no->rate, &hw->info, size);
 
     audio_pcm_info_clear_buf(&hw->info, buf, bytes / hw->info.bytes_per_frame);
     return bytes;
diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
index f52f3a8bbb..d17ef1a25e 100644
--- a/audio/spiceaudio.c
+++ b/audio/spiceaudio.c
@@ -239,7 +239,7 @@ static void line_in_fini (HWVoiceIn *hw)
 static size_t line_in_read(HWVoiceIn *hw, void *buf, size_t len)
 {
     SpiceVoiceIn *in = container_of (hw, SpiceVoiceIn, hw);
-    uint64_t to_read = audio_rate_get_bytes(&hw->info, &in->rate, len) >> 2;
+    uint64_t to_read = audio_rate_get_bytes(&in->rate, &hw->info, len) >> 2;
     size_t ready = spice_server_record_get_samples(&in->sin, buf, to_read);
 
     /*
diff --git a/audio/wavaudio.c b/audio/wavaudio.c
index ac666335c7..3e1d84db83 100644
--- a/audio/wavaudio.c
+++ b/audio/wavaudio.c
@@ -42,7 +42,7 @@ typedef struct WAVVoiceOut {
 static size_t wav_write_out(HWVoiceOut *hw, void *buf, size_t len)
 {
     WAVVoiceOut *wav = (WAVVoiceOut *) hw;
-    int64_t bytes = audio_rate_get_bytes(&hw->info, &wav->rate, len);
+    int64_t bytes = audio_rate_get_bytes(&wav->rate, &hw->info, len);
     assert(bytes % hw->info.bytes_per_frame == 0);
 
     if (bytes && fwrite(buf, bytes, 1, wav->f) != 1) {
-- 
2.35.3

Re: [PATCH 08/12] audio: swap audio_rate_get_bytes() function parameters

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 4870 bytes --]

On Fri, Sep 23, 2022 at 10:58 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Swap the rate and info parameters of the audio_rate_get_bytes()
> function to align the parameter order with the rest of the
> audio_rate_*() functions.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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



> ---
>  audio/audio.c      | 2 +-
>  audio/audio_int.h  | 2 +-
>  audio/dbusaudio.c  | 4 ++--
>  audio/noaudio.c    | 4 ++--
>  audio/spiceaudio.c | 2 +-
>  audio/wavaudio.c   | 2 +-
>  6 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 557538a7b7..233a86c440 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -2275,7 +2275,7 @@ void audio_rate_add_bytes(RateCtl *rate, size_t
> bytes_used)
>      rate->bytes_sent += bytes_used;
>  }
>
> -size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
> +size_t audio_rate_get_bytes(RateCtl *rate, struct audio_pcm_info *info,
>                              size_t bytes_avail)
>  {
>      size_t bytes;
> diff --git a/audio/audio_int.h b/audio/audio_int.h
> index 97e20e8429..e87ce014a0 100644
> --- a/audio/audio_int.h
> +++ b/audio/audio_int.h
> @@ -265,7 +265,7 @@ typedef struct RateCtl {
>  void audio_rate_start(RateCtl *rate);
>  size_t audio_rate_peek_bytes(RateCtl *rate, struct audio_pcm_info *info);
>  void audio_rate_add_bytes(RateCtl *rate, size_t bytes_used);
> -size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
> +size_t audio_rate_get_bytes(RateCtl *rate, struct audio_pcm_info *info,
>                              size_t bytes_avail);
>
>  static inline size_t audio_ring_dist(size_t dst, size_t src, size_t len)
> diff --git a/audio/dbusaudio.c b/audio/dbusaudio.c
> index a3d656d3b0..722df0355e 100644
> --- a/audio/dbusaudio.c
> +++ b/audio/dbusaudio.c
> @@ -82,7 +82,7 @@ static void *dbus_get_buffer_out(HWVoiceOut *hw, size_t
> *size)
>      }
>
>      *size = MIN(vo->buf_size - vo->buf_pos, *size);
> -    *size = audio_rate_get_bytes(&hw->info, &vo->rate, *size);
> +    *size = audio_rate_get_bytes(&vo->rate, &hw->info, *size);
>
>      return vo->buf + vo->buf_pos;
>
> @@ -343,7 +343,7 @@ dbus_read(HWVoiceIn *hw, void *buf, size_t size)
>
>      trace_dbus_audio_read(size);
>
> -    /* size = audio_rate_get_bytes(&hw->info, &vo->rate, size); */
> +    /* size = audio_rate_get_bytes(&vo->rate, &hw->info, size); */
>
>      g_hash_table_iter_init(&iter, da->in_listeners);
>      while (g_hash_table_iter_next(&iter, NULL, (void **)&listener)) {
> diff --git a/audio/noaudio.c b/audio/noaudio.c
> index 84a6bfbb1c..4fdee5adec 100644
> --- a/audio/noaudio.c
> +++ b/audio/noaudio.c
> @@ -44,7 +44,7 @@ typedef struct NoVoiceIn {
>  static size_t no_write(HWVoiceOut *hw, void *buf, size_t len)
>  {
>      NoVoiceOut *no = (NoVoiceOut *) hw;
> -    return audio_rate_get_bytes(&hw->info, &no->rate, len);
> +    return audio_rate_get_bytes(&no->rate, &hw->info, len);
>  }
>
>  static int no_init_out(HWVoiceOut *hw, struct audsettings *as, void
> *drv_opaque)
> @@ -89,7 +89,7 @@ static void no_fini_in (HWVoiceIn *hw)
>  static size_t no_read(HWVoiceIn *hw, void *buf, size_t size)
>  {
>      NoVoiceIn *no = (NoVoiceIn *) hw;
> -    int64_t bytes = audio_rate_get_bytes(&hw->info, &no->rate, size);
> +    int64_t bytes = audio_rate_get_bytes(&no->rate, &hw->info, size);
>
>      audio_pcm_info_clear_buf(&hw->info, buf, bytes /
> hw->info.bytes_per_frame);
>      return bytes;
> diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
> index f52f3a8bbb..d17ef1a25e 100644
> --- a/audio/spiceaudio.c
> +++ b/audio/spiceaudio.c
> @@ -239,7 +239,7 @@ static void line_in_fini (HWVoiceIn *hw)
>  static size_t line_in_read(HWVoiceIn *hw, void *buf, size_t len)
>  {
>      SpiceVoiceIn *in = container_of (hw, SpiceVoiceIn, hw);
> -    uint64_t to_read = audio_rate_get_bytes(&hw->info, &in->rate, len) >>
> 2;
> +    uint64_t to_read = audio_rate_get_bytes(&in->rate, &hw->info, len) >>
> 2;
>      size_t ready = spice_server_record_get_samples(&in->sin, buf,
> to_read);
>
>      /*
> diff --git a/audio/wavaudio.c b/audio/wavaudio.c
> index ac666335c7..3e1d84db83 100644
> --- a/audio/wavaudio.c
> +++ b/audio/wavaudio.c
> @@ -42,7 +42,7 @@ typedef struct WAVVoiceOut {
>  static size_t wav_write_out(HWVoiceOut *hw, void *buf, size_t len)
>  {
>      WAVVoiceOut *wav = (WAVVoiceOut *) hw;
> -    int64_t bytes = audio_rate_get_bytes(&hw->info, &wav->rate, len);
> +    int64_t bytes = audio_rate_get_bytes(&wav->rate, &hw->info, len);
>      assert(bytes % hw->info.bytes_per_frame == 0);
>
>      if (bytes && fwrite(buf, bytes, 1, wav->f) != 1) {
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 6126 bytes --]

[PATCH 09/12] audio: rename audio_sw_bytes_free()

[Volker Rümelin]

Rename and refactor audio_sw_bytes_free(). This function is not
limited to calculate the free audio buffer size. The renamed
function returns the number of frames instead of bytes.

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

diff --git a/audio/audio.c b/audio/audio.c
index 233a86c440..dd66b745e5 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1010,9 +1010,16 @@ static size_t audio_get_avail (SWVoiceIn *sw)
     return (((int64_t) live << 32) / sw->ratio) * sw->info.bytes_per_frame;
 }
 
-static size_t audio_sw_bytes_free(SWVoiceOut *sw, size_t free)
+/**
+ * audio_frontend_frames_out() - returns the number of frames needed to
+ * get frames_out frames after resampling
+ *
+ * @sw: audio playback frontend
+ * @frames_out: number of frames
+ */
+static size_t audio_frontend_frames_out(SWVoiceOut *sw, size_t frames_out)
 {
-    return (((int64_t)free << 32) / sw->ratio) * sw->info.bytes_per_frame;
+    return ((int64_t)frames_out << 32) / sw->ratio;
 }
 
 static size_t audio_get_free(SWVoiceOut *sw)
@@ -1034,8 +1041,8 @@ static size_t audio_get_free(SWVoiceOut *sw)
     dead = sw->hw->mix_buf->size - live;
 
 #ifdef DEBUG_OUT
-    dolog("%s: get_free live %zu dead %zu sw_bytes %zu\n",
-          SW_NAME(sw), live, dead, audio_sw_bytes_free(sw, dead));
+    dolog("%s: get_free live %zu dead %zu frontend frames %zu\n",
+          SW_NAME(sw), live, dead, audio_frontend_frames_out(sw, dead));
 #endif
 
     return dead;
@@ -1156,13 +1163,14 @@ static void audio_run_out (AudioState *s)
                 size_t free;
 
                 if (hw_free > sw->total_hw_samples_mixed) {
-                    free = audio_sw_bytes_free(sw,
+                    free = audio_frontend_frames_out(sw,
                         MIN(sw_free, hw_free - sw->total_hw_samples_mixed));
                 } else {
                     free = 0;
                 }
                 if (free > 0) {
-                    sw->callback.fn(sw->callback.opaque, free);
+                    sw->callback.fn(sw->callback.opaque,
+                                    free * sw->info.bytes_per_frame);
                 }
             }
         }
-- 
2.35.3

Re: [PATCH 09/12] audio: rename audio_sw_bytes_free()

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]

On Fri, Sep 23, 2022 at 10:38 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Rename and refactor audio_sw_bytes_free(). This function is not
> limited to calculate the free audio buffer size. The renamed
> function returns the number of frames instead of bytes.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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



> ---
>  audio/audio.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 233a86c440..dd66b745e5 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1010,9 +1010,16 @@ static size_t audio_get_avail (SWVoiceIn *sw)
>      return (((int64_t) live << 32) / sw->ratio) *
> sw->info.bytes_per_frame;
>  }
>
> -static size_t audio_sw_bytes_free(SWVoiceOut *sw, size_t free)
> +/**
> + * audio_frontend_frames_out() - returns the number of frames needed to
> + * get frames_out frames after resampling
> + *
> + * @sw: audio playback frontend
> + * @frames_out: number of frames
> + */
> +static size_t audio_frontend_frames_out(SWVoiceOut *sw, size_t frames_out)
>  {
> -    return (((int64_t)free << 32) / sw->ratio) * sw->info.bytes_per_frame;
> +    return ((int64_t)frames_out << 32) / sw->ratio;
>  }
>
>  static size_t audio_get_free(SWVoiceOut *sw)
> @@ -1034,8 +1041,8 @@ static size_t audio_get_free(SWVoiceOut *sw)
>      dead = sw->hw->mix_buf->size - live;
>
>  #ifdef DEBUG_OUT
> -    dolog("%s: get_free live %zu dead %zu sw_bytes %zu\n",
> -          SW_NAME(sw), live, dead, audio_sw_bytes_free(sw, dead));
> +    dolog("%s: get_free live %zu dead %zu frontend frames %zu\n",
> +          SW_NAME(sw), live, dead, audio_frontend_frames_out(sw, dead));
>  #endif
>
>      return dead;
> @@ -1156,13 +1163,14 @@ static void audio_run_out (AudioState *s)
>                  size_t free;
>
>                  if (hw_free > sw->total_hw_samples_mixed) {
> -                    free = audio_sw_bytes_free(sw,
> +                    free = audio_frontend_frames_out(sw,
>                          MIN(sw_free, hw_free -
> sw->total_hw_samples_mixed));
>                  } else {
>                      free = 0;
>                  }
>                  if (free > 0) {
> -                    sw->callback.fn(sw->callback.opaque, free);
> +                    sw->callback.fn(sw->callback.opaque,
> +                                    free * sw->info.bytes_per_frame);
>                  }
>              }
>          }
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3678 bytes --]

[PATCH 10/12] audio: refactor audio_get_avail()

[Volker Rümelin]

Split out the code in audio_get_avail() that calculates the
buffer size that the audio frontend can read. This is similar
to the code changes in audio_get_free().

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

diff --git a/audio/audio.c b/audio/audio.c
index dd66b745e5..ba0c62b120 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -986,6 +986,18 @@ void AUD_set_active_in (SWVoiceIn *sw, int on)
     }
 }
 
+/**
+ * audio_frontend_frames_in() - returns the number of frames the resampling
+ * code generates from frames_in frames
+ *
+ * @sw: audio recording frontend
+ * @frames_in: number of frames
+ */
+static size_t audio_frontend_frames_in(SWVoiceIn *sw, size_t frames_in)
+{
+    return ((int64_t)frames_in << 32) / sw->ratio;
+}
+
 static size_t audio_get_avail (SWVoiceIn *sw)
 {
     size_t live;
@@ -1002,12 +1014,12 @@ static size_t audio_get_avail (SWVoiceIn *sw)
     }
 
     ldebug (
-        "%s: get_avail live %zu ret %" PRId64 "\n",
+        "%s: get_avail live %zu frontend frames %zu\n",
         SW_NAME (sw),
-        live, (((int64_t) live << 32) / sw->ratio) * sw->info.bytes_per_frame
+        live, audio_frontend_frames_in(sw, live)
         );
 
-    return (((int64_t) live << 32) / sw->ratio) * sw->info.bytes_per_frame;
+    return live;
 }
 
 /**
@@ -1309,11 +1321,13 @@ static void audio_run_in (AudioState *s)
             sw->total_hw_samples_acquired -= min;
 
             if (sw->active) {
+                size_t sw_avail = audio_get_avail(sw);
                 size_t avail;
 
-                avail = audio_get_avail (sw);
+                avail = audio_frontend_frames_in(sw, sw_avail);
                 if (avail > 0) {
-                    sw->callback.fn (sw->callback.opaque, avail);
+                    sw->callback.fn(sw->callback.opaque,
+                                    avail * sw->info.bytes_per_frame);
                 }
             }
         }
-- 
2.35.3

Re: [PATCH 10/12] audio: refactor audio_get_avail()

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2410 bytes --]

On Fri, Sep 23, 2022 at 10:51 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Split out the code in audio_get_avail() that calculates the
> buffer size that the audio frontend can read. This is similar
> to the code changes in audio_get_free().
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

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



> ---
>  audio/audio.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index dd66b745e5..ba0c62b120 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -986,6 +986,18 @@ void AUD_set_active_in (SWVoiceIn *sw, int on)
>      }
>  }
>
> +/**
> + * audio_frontend_frames_in() - returns the number of frames the
> resampling
> + * code generates from frames_in frames
> + *
> + * @sw: audio recording frontend
> + * @frames_in: number of frames
> + */
> +static size_t audio_frontend_frames_in(SWVoiceIn *sw, size_t frames_in)
> +{
> +    return ((int64_t)frames_in << 32) / sw->ratio;
> +}
> +
>  static size_t audio_get_avail (SWVoiceIn *sw)
>  {
>      size_t live;
> @@ -1002,12 +1014,12 @@ static size_t audio_get_avail (SWVoiceIn *sw)
>      }
>
>      ldebug (
> -        "%s: get_avail live %zu ret %" PRId64 "\n",
> +        "%s: get_avail live %zu frontend frames %zu\n",
>          SW_NAME (sw),
> -        live, (((int64_t) live << 32) / sw->ratio) *
> sw->info.bytes_per_frame
> +        live, audio_frontend_frames_in(sw, live)
>          );
>
> -    return (((int64_t) live << 32) / sw->ratio) *
> sw->info.bytes_per_frame;
> +    return live;
>  }
>
>  /**
> @@ -1309,11 +1321,13 @@ static void audio_run_in (AudioState *s)
>              sw->total_hw_samples_acquired -= min;
>
>              if (sw->active) {
> +                size_t sw_avail = audio_get_avail(sw);
>                  size_t avail;
>
> -                avail = audio_get_avail (sw);
> +                avail = audio_frontend_frames_in(sw, sw_avail);
>                  if (avail > 0) {
> -                    sw->callback.fn (sw->callback.opaque, avail);
> +                    sw->callback.fn(sw->callback.opaque,
> +                                    avail * sw->info.bytes_per_frame);
>                  }
>              }
>          }
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3452 bytes --]

[PATCH 11/12] audio: fix sw->buf size for audio recording

[Volker Rümelin]

The calculation of the buffer size needed to store audio samples
after resampling is wrong for audio recording. For audio recording
sw->ratio is calculated as

sw->ratio = frontend sample rate / backend sample rate.

From this follows

frontend samples = frontend sample rate / backend sample rate
 * backend samples
frontend samples = sw->ratio * backend samples

In 2 of 3 places in the audio recording code where sw->ratio
is used in a calculation to get the number of frontend frames,
the calculation is wrong. Fix this. The 3rd formula in
audio_pcm_sw_read() is correct.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/71
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c          | 2 +-
 audio/audio_template.h | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/audio/audio.c b/audio/audio.c
index ba0c62b120..60c7472d37 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -995,7 +995,7 @@ void AUD_set_active_in (SWVoiceIn *sw, int on)
  */
 static size_t audio_frontend_frames_in(SWVoiceIn *sw, size_t frames_in)
 {
-    return ((int64_t)frames_in << 32) / sw->ratio;
+    return (int64_t)frames_in * sw->ratio >> 32;
 }
 
 static size_t audio_get_avail (SWVoiceIn *sw)
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7192b19e73..6a0337ac6b 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -112,7 +112,11 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE) (SW *sw)
         return 0;
     }
 
+#ifdef DAC
     samples = ((int64_t) sw->HWBUF->size << 32) / sw->ratio;
+#else
+    samples = (int64_t)sw->HWBUF->size * sw->ratio >> 32;
+#endif
 
     sw->buf = audio_calloc(__func__, samples, sizeof(struct st_sample));
     if (!sw->buf) {
-- 
2.35.3

Re: [PATCH 11/12] audio: fix sw->buf size for audio recording

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]

On Fri, Sep 23, 2022 at 10:48 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> The calculation of the buffer size needed to store audio samples
> after resampling is wrong for audio recording. For audio recording
> sw->ratio is calculated as
>
> sw->ratio = frontend sample rate / backend sample rate.
>
> From this follows
>
> frontend samples = frontend sample rate / backend sample rate
>  * backend samples
> frontend samples = sw->ratio * backend samples
>
> In 2 of 3 places in the audio recording code where sw->ratio
> is used in a calculation to get the number of frontend frames,
> the calculation is wrong. Fix this. The 3rd formula in
> audio_pcm_sw_read() is correct.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/71
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

Would you mind adding the test to qtest?

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



> ---
>  audio/audio.c          | 2 +-
>  audio/audio_template.h | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index ba0c62b120..60c7472d37 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -995,7 +995,7 @@ void AUD_set_active_in (SWVoiceIn *sw, int on)
>   */
>  static size_t audio_frontend_frames_in(SWVoiceIn *sw, size_t frames_in)
>  {
> -    return ((int64_t)frames_in << 32) / sw->ratio;
> +    return (int64_t)frames_in * sw->ratio >> 32;
>  }
>
>  static size_t audio_get_avail (SWVoiceIn *sw)
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index 7192b19e73..6a0337ac6b 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -112,7 +112,11 @@ static int glue (audio_pcm_sw_alloc_resources_, TYPE)
> (SW *sw)
>          return 0;
>      }
>
> +#ifdef DAC
>      samples = ((int64_t) sw->HWBUF->size << 32) / sw->ratio;
> +#else
> +    samples = (int64_t)sw->HWBUF->size * sw->ratio >> 32;
> +#endif
>
>      sw->buf = audio_calloc(__func__, samples, sizeof(struct st_sample));
>      if (!sw->buf) {
> --
> 2.35.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3183 bytes --]

Re: [PATCH 11/12] audio: fix sw->buf size for audio recording

[Volker Rümelin]

Am 27.09.22 um 13:54 schrieb Marc-André Lureau:

>
> On Fri, Sep 23, 2022 at 10:48 PM Volker Rümelin <vr_qemu@t-online.de> 
> wrote:
>
>     The calculation of the buffer size needed to store audio samples
>     after resampling is wrong for audio recording. For audio recording
>     sw->ratio is calculated as
>
>     sw->ratio = frontend sample rate / backend sample rate.
>
>     >From this follows
>
>     frontend samples = frontend sample rate / backend sample rate
>      * backend samples
>     frontend samples = sw->ratio * backend samples
>
>     In 2 of 3 places in the audio recording code where sw->ratio
>     is used in a calculation to get the number of frontend frames,
>     the calculation is wrong. Fix this. The 3rd formula in
>     audio_pcm_sw_read() is correct.
>
>     Resolves: https://gitlab.com/qemu-project/qemu/-/issues/71
>     Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>
>
> Would you mind adding the test to qtest?
>
> lgtm
> Acked-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>

Hi Marc-André,

I will give it a try. But it will be a separate patch, because the test 
from issue #71 now checks for the error at 
https://lists.nongnu.org/archive/html/qemu-devel/2022-09/msg02347.html 
and not the one from issue #71.

With best regards,
Volker

[PATCH 12/12] audio: prevent an integer overflow in resampling code

[Volker Rümelin]

There are corner cases where rate->opos can overflow. For
example, if QEMU is started with -audiodev pa,id=audio0,
out.frequency=11025 -device ich9-intel-hda -device hda-duplex,
audiodev=audio0 and the guest plays audio with a sampling
frequency of 44100Hz, rate->opos will overflow after 27.05h
and the audio stream will be silent for a long time.

To prevent a rate->opos and also a rate->ipos overflow, both
are wrapped around after a short time. The wrap around point
rate->ipos >= 0x10001 is an arbitrarily selected value and can
be any small value, 0 and 1 included.

The comment that an ipos overflow will result in an infinite
loop has been removed, because in this case the resampling code
only generates no more output samples and the audio stream stalls.
However, there is no infinite loop.

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

diff --git a/audio/rate_template.h b/audio/rate_template.h
index f94c940c61..b432719ebb 100644
--- a/audio/rate_template.h
+++ b/audio/rate_template.h
@@ -72,11 +72,6 @@ void NAME (void *opaque, struct st_sample *ibuf, struct st_sample *obuf,
             ilast = *ibuf++;
             rate->ipos++;
 
-            /* if ipos overflow, there is  a infinite loop */
-            if (rate->ipos == 0xffffffff) {
-                rate->ipos = 1;
-                rate->opos = rate->opos & 0xffffffff;
-            }
             /* See if we finished the input buffer yet */
             if (ibuf >= iend) {
                 goto the_end;
@@ -85,6 +80,12 @@ void NAME (void *opaque, struct st_sample *ibuf, struct st_sample *obuf,
 
         icur = *ibuf;
 
+        /* wrap ipos and opos around long before they overflow */
+        if (rate->ipos >= 0x10001) {
+            rate->ipos = 1;
+            rate->opos &= 0xffffffff;
+        }
+
         /* interpolate */
 #ifdef FLOAT_MIXENG
 #ifdef RECIPROCAL
-- 
2.35.3

Re: [PATCH 00/12] audio: misc. improvements and bug fixes

[Gerd Hoffmann]

On Fri, Sep 23, 2022 at 08:34:58PM +0200, Volker Rümelin wrote:
> A series of audio improvements and fixes.
> 
> One note:
> 
> Patch 11/12 "audio: fix sw->buf size for audio recording":
> If this patch is applied without the patch series "[PATCH 0/2] audio:
> prevent a class of guest-triggered aborts" at
> https://lists.nongnu.org/archive/html/qemu-devel/2022-09/msg02347.html,
> issue #71 triggers a QEMU abort. Patch 11/12 is nevertheless correct.

Added to patch queue.

thanks,
  Gerd