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

[Laurent Vivier <laurent@vivier.eu>]

Le 09/09/2023 à 11:48, Mark Cave-Ayland a écrit :
> 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;
>       }
>   
> @@ -611,6 +626,7 @@ static void asc_unrealize(DeviceState *dev)
>       ASCState *s = ASC(dev);
>   
>       g_free(s->mixbuf);
> +    g_free(s->silentbuf);
>   
>       AUD_remove_card(&s->card);
>   }
> @@ -633,6 +649,9 @@ static void asc_realize(DeviceState *dev, Error **errp)
>       s->samples = AUD_get_buffer_size_out(s->voice) >> s->shift;
>       s->mixbuf = g_malloc0(s->samples << s->shift);
>   
> +    s->silentbuf = g_malloc0(s->samples << s->shift);
> +    memset(s->silentbuf, 0x80, s->samples << s->shift);
> +
>       /* Add easc registers if required */
>       if (s->type == ASC_TYPE_EASC) {
>           memory_region_add_subregion(&s->asc, ASC_EXTREG_OFFSET,
> diff --git a/include/hw/audio/asc.h b/include/hw/audio/asc.h
> index d9412815c3..4741f92c46 100644
> --- a/include/hw/audio/asc.h
> +++ b/include/hw/audio/asc.h
> @@ -68,6 +68,8 @@ struct ASCState {
>       int samples;
>       int shift;
>   
> +    uint8_t *silentbuf;
> +
>       /* Time when we were last able to generate samples */
>       int64_t fifo_empty_ns;
>   

If it's specific to Windows why not using "#if defined(CONFIG_WIN32) && 
defined(CONFIG_AUDIO_DSOUND)" to clearly identify this piece of code as specific to a windows bug 
with dsound?

Anyway, code looks good:

Reviewed-by: Laurent Vivier <laurent@vivier.eu>