Commit 90320051ea99 ("spiceaudio: add a pcm_ops buffer_get_free
function") caused to emit messages saying "Resetting rate control"
frequently when the guest generates no frames.

audio_rate_peek_bytes() resets the rate control when frames < 0 ||
frames > 65536 where frames is the rate-limited number of frames.
Resetting when frames < 0 is sensible as the number simply doesn't make
sense.

There is a problem when frames > 65536. It implies the guest stopped
generating frames for a while so it makes sense to reset the rate
control when the guest resumed generating frames. However, the
commit mentioned earlier broke this assumption by letting spiceaudio
call audio_rate_peek_bytes() whether the guest is generating frames or
not.

Reset the rate control in audio_rate_add_bytes(), which is called only
when actually adding frames, according to the previous call to
audio_rate_peek_bytes() to avoid frequent rate control resets even when
the guest generates no frame.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 audio/audio_int.h |  1 +
 audio/audio.c     | 14 ++++++++------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/audio/audio_int.h b/audio/audio_int.h
index 2d079d00a259..f78ca05f92fb 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -255,6 +255,7 @@ const char *audio_application_name(void);
 typedef struct RateCtl {
     int64_t start_ticks;
     int64_t bytes_sent;
+    int64_t peeked_frames;
 } RateCtl;
 
 void audio_rate_start(RateCtl *rate);
diff --git a/audio/audio.c b/audio/audio.c
index 41ee11aaad6d..049d9d07aa58 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2274,17 +2274,19 @@ size_t audio_rate_peek_bytes(RateCtl *rate, struct audio_pcm_info *info)
     ticks = now - rate->start_ticks;
     bytes = muldiv64(ticks, info->bytes_per_second, NANOSECONDS_PER_SECOND);
     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);
-        frames = 0;
-    }
+    rate->peeked_frames = frames;
 
-    return frames * info->bytes_per_frame;
+    return frames < 0 ? 0 : frames * info->bytes_per_frame;
 }
 
 void audio_rate_add_bytes(RateCtl *rate, size_t bytes_used)
 {
+    if (rate->peeked_frames < 0 || rate->peeked_frames > 65536) {
+        AUD_log(NULL, "Resetting rate control (%" PRId64 " frames)\n",
+                rate->peeked_frames);
+        audio_rate_start(rate);
+    }
+
     rate->bytes_sent += bytes_used;
 }
 

---
base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
change-id: 20250317-rate-308c5c79f50f

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

On Mon, Mar 17, 2025 at 9:07 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Commit 90320051ea99 ("spiceaudio: add a pcm_ops buffer_get_free
> function") caused to emit messages saying "Resetting rate control"
> frequently when the guest generates no frames.
>
> audio_rate_peek_bytes() resets the rate control when frames < 0 ||
> frames > 65536 where frames is the rate-limited number of frames.
> Resetting when frames < 0 is sensible as the number simply doesn't make
> sense.
>
> There is a problem when frames > 65536. It implies the guest stopped
> generating frames for a while so it makes sense to reset the rate
> control when the guest resumed generating frames. However, the
> commit mentioned earlier broke this assumption by letting spiceaudio
> call audio_rate_peek_bytes() whether the guest is generating frames or
> not.
>
> Reset the rate control in audio_rate_add_bytes(), which is called only
> when actually adding frames, according to the previous call to
> audio_rate_peek_bytes() to avoid frequent rate control resets even when
> the guest generates no frame.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

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

> ---
>  audio/audio_int.h |  1 +
>  audio/audio.c     | 14 ++++++++------
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/audio/audio_int.h b/audio/audio_int.h
> index 2d079d00a259..f78ca05f92fb 100644
> --- a/audio/audio_int.h
> +++ b/audio/audio_int.h
> @@ -255,6 +255,7 @@ const char *audio_application_name(void);
>  typedef struct RateCtl {
>      int64_t start_ticks;
>      int64_t bytes_sent;
> +    int64_t peeked_frames;
>  } RateCtl;
>
>  void audio_rate_start(RateCtl *rate);
> diff --git a/audio/audio.c b/audio/audio.c
> index 41ee11aaad6d..049d9d07aa58 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -2274,17 +2274,19 @@ size_t audio_rate_peek_bytes(RateCtl *rate, struct audio_pcm_info *info)
>      ticks = now - rate->start_ticks;
>      bytes = muldiv64(ticks, info->bytes_per_second, NANOSECONDS_PER_SECOND);
>      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);
> -        frames = 0;
> -    }
> +    rate->peeked_frames = frames;
>
> -    return frames * info->bytes_per_frame;
> +    return frames < 0 ? 0 : frames * info->bytes_per_frame;
>  }
>
>  void audio_rate_add_bytes(RateCtl *rate, size_t bytes_used)
>  {
> +    if (rate->peeked_frames < 0 || rate->peeked_frames > 65536) {
> +        AUD_log(NULL, "Resetting rate control (%" PRId64 " frames)\n",
> +                rate->peeked_frames);
> +        audio_rate_start(rate);
> +    }
> +
>      rate->bytes_sent += bytes_used;
>  }
>
>
> ---
> base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
> change-id: 20250317-rate-308c5c79f50f
>
> Best regards,
> --
> Akihiko Odaki <akihiko.odaki@daynix.com>
>
>


-- 
Marc-André Lureau