Re: [PATCH v2 08/20] asc: generate silence if FIFO empty but engine still running

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 14/9/23 09:56, Philippe Mathieu-Daudé wrote:
> 
> On 9/9/23 11:48, Mark Cave-Ayland wrote:
>> MacOS (un)helpfully leaves the FIFO engine running even when all the 
>> samples have
>> been written to the hardware, and expects the FIFO status flags and 
>> IRQ to be
>> updated continuously.
>>
>> There is an additional problem in that not all audio backends 
>> guarantee an
>> all-zero output when there is no FIFO data available, in particular 
>> the Windows
>> dsound backend which re-uses its internal circular buffer causing the 
>> last played
>> sound to loop indefinitely.
>>
>> Whilst this is effectively a bug in the Windows dsound backend, work 
>> around it
>> for now using a simple heuristic: if the FIFO remains empty for half a 
>> cycle
>> (~23ms) then continuously fill the generated buffer with empty silence.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/audio/asc.c         | 19 +++++++++++++++++++
>>   include/hw/audio/asc.h |  2 ++
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/hw/audio/asc.c b/hw/audio/asc.c
>> index 336ace0cd6..b01b285512 100644
>> --- a/hw/audio/asc.c
>> +++ b/hw/audio/asc.c
>> @@ -334,6 +334,21 @@ static void asc_out_cb(void *opaque, int free_b)
>>       }
>>       if (!generated) {
>> +        /* Workaround for audio underflow bug on Windows dsound 
>> backend */
>> +        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +        int silent_samples = muldiv64(now - s->fifo_empty_ns,
>> +                                      NANOSECONDS_PER_SECOND, ASC_FREQ);
>> +
>> +        if (silent_samples > ASC_FIFO_CYCLE_TIME / 2) {
>> +            /*
>> +             * No new FIFO data within half a cycle time (~23ms) so 
>> fill the
>> +             * entire available buffer with silence. This prevents an 
>> issue
>> +             * with the Windows dsound backend whereby the sound 
>> appears to
>> +             * loop because the FIFO has run out of data, and the driver
>> +             * reuses the stale content in its circular audio buffer.
>> +             */
>> +            AUD_write(s->voice, s->silentbuf, samples << s->shift);
>> +        }
>>           return;
>>       }
> 
> What about having audio_callback_fn returning a boolean, and using
> a flag in backends for that silence case? Roughtly:
> 
> -- >8 --
> diff --git a/audio/audio.h b/audio/audio.h
> index 01bdc567fb..4844771c92 100644
> --- a/audio/audio.h
> +++ b/audio/audio.h
> @@ -30,7 +30,7 @@
>   #include "hw/qdev-properties.h"
>   #include "hw/qdev-properties-system.h"
> 
> -typedef void (*audio_callback_fn) (void *opaque, int avail);
> +typedef bool (*audio_callback_fn) (void *opaque, int avail);
> 
>   #if HOST_BIG_ENDIAN
>   #define AUDIO_HOST_ENDIANNESS 1
> diff --git a/audio/audio.c b/audio/audio.c
> index 90c7c49d11..5b6e69fbd6 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1178,8 +1178,11 @@ static void audio_run_out (AudioState *s)
>                   if (free > sw->resample_buf.pos) {
>                       free = MIN(free, sw->resample_buf.size)
>                              - sw->resample_buf.pos;
> -                    sw->callback.fn(sw->callback.opaque,
> -                                    free * sw->info.bytes_per_frame);
> +                    if (!sw->callback.fn(sw->callback.opaque,
> +                                         free * sw->info.bytes_per_frame)
> +                            && unlikely(hw->silentbuf_required)) {
> +                        /* write silence ... */
> +                    }
>                   }
>               }
>           }
> ---

(Clarifying, not a blocker for this series, just wondering)