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

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

On 16/9/23 10:19, Volker Rümelin wrote:
> Am 14.09.23 um 09:56 schrieb Philippe Mathieu-Daudé:
>>
>> 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:
>>
> 
> Hi Philippe,
> 
> I don't think there will be many audio devices that need this feature in
> the future. It's probably better to keep the code in hw/audio/asc.c
> 
> I plan to change the buffer underflow behavior of the DirectSound
> backend after these patches are commited. I already have a patch
> available. This doesn't mean this patch is unnecessary after my patch.
> It is a mistake when audio devices simply stop writing audio samples
> without changing the status of the audio playback stream to deactivated.
> At the moment the ASC device can't deactivate the audio stream.

OK, thanks for clarifying.