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

[Christian Schoenebeck]

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

OK

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

I thought about patch 7 where you do:

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

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

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

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