| Message ID | 20230909094827.33871-9-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | q800: add support for booting MacOS Classic - part 2 | expand |
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 ... */ + } } } } ---
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)
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. With best regards, Volker > -- >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 ... */ > + } > } > } > } > ---
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.
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>
On 25/09/2023 18:19, Laurent Vivier wrote: > 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? Basically when the FIFO is left running constantly (i.e. the frontend is always generating samples regardless of whether there is data), we are relying on undefined behaviour when there is underflow in these cases. It just so happens that Windows is one of the more popular platforms for Mac emulation and that happens to use a circular buffer which is why the audio artifacts exist - but side-effects can potentially occur on any audio backend. > Anyway, code looks good: > > Reviewed-by: Laurent Vivier <laurent@vivier.eu> Thanks! ATB, Mark.
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;
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(+)