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

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

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

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

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

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

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

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

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

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

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

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

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