| Message ID | 20230702154838.722809-9-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | q800: add support for booting MacOS Classic - part 2 | expand |
> 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. > > Since not all audio backends guarantee an all-zero output when no data is > provided, explicitly generate at least one full output buffer of all-zero output > when the FIFO is disabled and continuously if the FIFO is empty. Otherwise some > audio backends such as Windows re-use their internal buffers causing the last > played sound to loop indefinitely. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/audio/asc.c | 43 ++++++++++++++++++++++++++++++++---------- > include/hw/audio/asc.h | 1 + > 2 files changed, 34 insertions(+), 10 deletions(-) > > diff --git a/hw/audio/asc.c b/hw/audio/asc.c > index ebcb8a97a6..f9bfae5168 100644 > --- a/hw/audio/asc.c > +++ b/hw/audio/asc.c > @@ -148,6 +148,20 @@ static uint8_t asc_fifo_get(ASCFIFOState *fs) > return val; > } Hi Mark, I don't understand why the flush_zero_samples variable is necessary at all. > > +static int generate_silence(ASCState *s, int maxsamples) > +{ > + uint8_t *buf = s->mixbuf; > + > + if (s->flush_zero_samples) { > + memset(buf, 0x80, maxsamples << s->shift); > + s->flush_zero_samples -= MIN(maxsamples, s->flush_zero_samples); > + > + return maxsamples; > + } > + > + return 0; > +} > + > static int generate_fifo(ASCState *s, int maxsamples) > { > uint8_t *buf = s->mixbuf; > @@ -156,18 +170,26 @@ static int generate_fifo(ASCState *s, int maxsamples) > limit = MIN(MAX(s->fifos[0].cnt, s->fifos[1].cnt), maxsamples); > > /* > - * If starting a new run with no FIFO data present, update the IRQ and > - * continue > + * MacOS (un)helpfully leaves the FIFO engine running even when it has > + * finished writing out samples. Since not all audio backends guarantee an > + * all-zero output when no data is provided, zero out the sample buffer > + * and then update the FIFO flags and IRQ as normal and continue > */ > - if (limit == 0 && s->fifos[0].int_status == 0 && > - s->fifos[1].int_status == 0) { > - s->fifos[0].int_status |= ASC_FIFO_STATUS_HALF_FULL | > - ASC_FIFO_STATUS_FULL_EMPTY; > - s->fifos[1].int_status |= ASC_FIFO_STATUS_HALF_FULL | > - ASC_FIFO_STATUS_FULL_EMPTY; > + if (limit == 0) { > + if (s->fifos[0].int_status == 0 && s->fifos[1].int_status == 0) { > + s->fifos[0].int_status |= ASC_FIFO_STATUS_HALF_FULL | > + ASC_FIFO_STATUS_FULL_EMPTY; > + s->fifos[1].int_status |= ASC_FIFO_STATUS_HALF_FULL | > + ASC_FIFO_STATUS_FULL_EMPTY; > + } > + > + if (s->flush_zero_samples == 0) { > + s->flush_zero_samples = s->samples; > + } At this point s->flush_zero_samples is != 0 and generate_silence() always generates maxsamples silent audio frames. > > + generate_silence(s, maxsamples); > asc_raise_irq(s); > - return 0; > + return maxsamples; > } > > while (count < limit) { > @@ -309,7 +331,7 @@ static void asc_out_cb(void *opaque, int free_b) > switch (s->regs[ASC_MODE] & 3) { > default: > /* Off */ This code will not be called for s->regs[ASC_MODE] & 3 == 0 because in asc_write() AUD_set_active_out(s->voice, 0) was called before. For s->regs[ASC_MODE] & 3 == 3 the code in asc_write() clears s->flush_zero_samples and generate_silence() always returns 0. The audio subsystem is running and expects new audio frames here. With best regards, Volker > - samples = 0; > + samples = generate_silence(s, samples); > break; > case 1: > /* FIFO mode */ > @@ -437,6 +459,7 @@ static void asc_write(void *opaque, hwaddr addr, uint64_t value, > asc_lower_irq(s); > if (value != 0) { > AUD_set_active_out(s->voice, 1); > + s->flush_zero_samples = 0; > } else { > AUD_set_active_out(s->voice, 0); > } > diff --git a/include/hw/audio/asc.h b/include/hw/audio/asc.h > index 41c6cba8fa..918f6ac582 100644 > --- a/include/hw/audio/asc.h > +++ b/include/hw/audio/asc.h > @@ -65,6 +65,7 @@ struct ASCState { > uint8_t *mixbuf; > int samples; > int shift; > + uint32_t flush_zero_samples; > > qemu_irq irq; >
On 07/07/2023 07:24, Volker Rümelin 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. >> >> Since not all audio backends guarantee an all-zero output when no data is >> provided, explicitly generate at least one full output buffer of all-zero output >> when the FIFO is disabled and continuously if the FIFO is empty. Otherwise some >> audio backends such as Windows re-use their internal buffers causing the last >> played sound to loop indefinitely. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/audio/asc.c | 43 ++++++++++++++++++++++++++++++++---------- >> include/hw/audio/asc.h | 1 + >> 2 files changed, 34 insertions(+), 10 deletions(-) >> >> diff --git a/hw/audio/asc.c b/hw/audio/asc.c >> index ebcb8a97a6..f9bfae5168 100644 >> --- a/hw/audio/asc.c >> +++ b/hw/audio/asc.c >> @@ -148,6 +148,20 @@ static uint8_t asc_fifo_get(ASCFIFOState *fs) >> return val; >> } > > Hi Mark, > > I don't understand why the flush_zero_samples variable is necessary at all. > >> +static int generate_silence(ASCState *s, int maxsamples) >> +{ >> + uint8_t *buf = s->mixbuf; >> + >> + if (s->flush_zero_samples) { >> + memset(buf, 0x80, maxsamples << s->shift); >> + s->flush_zero_samples -= MIN(maxsamples, s->flush_zero_samples); >> + >> + return maxsamples; >> + } >> + >> + return 0; >> +} >> + >> static int generate_fifo(ASCState *s, int maxsamples) >> { >> uint8_t *buf = s->mixbuf; >> @@ -156,18 +170,26 @@ static int generate_fifo(ASCState *s, int maxsamples) >> limit = MIN(MAX(s->fifos[0].cnt, s->fifos[1].cnt), maxsamples); >> /* >> - * If starting a new run with no FIFO data present, update the IRQ and >> - * continue >> + * MacOS (un)helpfully leaves the FIFO engine running even when it has >> + * finished writing out samples. Since not all audio backends guarantee an >> + * all-zero output when no data is provided, zero out the sample buffer >> + * and then update the FIFO flags and IRQ as normal and continue >> */ >> - if (limit == 0 && s->fifos[0].int_status == 0 && >> - s->fifos[1].int_status == 0) { >> - s->fifos[0].int_status |= ASC_FIFO_STATUS_HALF_FULL | >> - ASC_FIFO_STATUS_FULL_EMPTY; >> - s->fifos[1].int_status |= ASC_FIFO_STATUS_HALF_FULL | >> - ASC_FIFO_STATUS_FULL_EMPTY; >> + if (limit == 0) { >> + if (s->fifos[0].int_status == 0 && s->fifos[1].int_status == 0) { >> + s->fifos[0].int_status |= ASC_FIFO_STATUS_HALF_FULL | >> + ASC_FIFO_STATUS_FULL_EMPTY; >> + s->fifos[1].int_status |= ASC_FIFO_STATUS_HALF_FULL | >> + ASC_FIFO_STATUS_FULL_EMPTY; >> + } >> + >> + if (s->flush_zero_samples == 0) { >> + s->flush_zero_samples = s->samples; >> + } > > At this point s->flush_zero_samples is != 0 and generate_silence() always generates > maxsamples silent audio frames. > >> + generate_silence(s, maxsamples); >> asc_raise_irq(s); >> - return 0; >> + return maxsamples; >> } >> while (count < limit) { >> @@ -309,7 +331,7 @@ static void asc_out_cb(void *opaque, int free_b) >> switch (s->regs[ASC_MODE] & 3) { >> default: >> /* Off */ > > This code will not be called for s->regs[ASC_MODE] & 3 == 0 because in asc_write() > AUD_set_active_out(s->voice, 0) was called before. > > For s->regs[ASC_MODE] & 3 == 3 the code in asc_write() clears s->flush_zero_samples > and generate_silence() always returns 0. The audio subsystem is running and expects > new audio frames here. > > With best regards, > Volker Ooof indeed, thanks - I've been trying to tweak this logic to improve the results on Windows, but clearly missed this obvious flaw. Unfortunately at the moment I have visitors until the end of the week and so I don't have much of an opportunity to revisit this before next week. >> - samples = 0; >> + samples = generate_silence(s, samples); >> break; >> case 1: >> /* FIFO mode */ >> @@ -437,6 +459,7 @@ static void asc_write(void *opaque, hwaddr addr, uint64_t value, >> asc_lower_irq(s); >> if (value != 0) { >> AUD_set_active_out(s->voice, 1); >> + s->flush_zero_samples = 0; >> } else { >> AUD_set_active_out(s->voice, 0); >> } >> diff --git a/include/hw/audio/asc.h b/include/hw/audio/asc.h >> index 41c6cba8fa..918f6ac582 100644 >> --- a/include/hw/audio/asc.h >> +++ b/include/hw/audio/asc.h >> @@ -65,6 +65,7 @@ struct ASCState { >> uint8_t *mixbuf; >> int samples; >> int shift; >> + uint32_t flush_zero_samples; >> qemu_irq irq; ATB, Mark.
diff --git a/hw/audio/asc.c b/hw/audio/asc.c index ebcb8a97a6..f9bfae5168 100644 --- a/hw/audio/asc.c +++ b/hw/audio/asc.c @@ -148,6 +148,20 @@ static uint8_t asc_fifo_get(ASCFIFOState *fs) return val; } +static int generate_silence(ASCState *s, int maxsamples) +{ + uint8_t *buf = s->mixbuf; + + if (s->flush_zero_samples) { + memset(buf, 0x80, maxsamples << s->shift); + s->flush_zero_samples -= MIN(maxsamples, s->flush_zero_samples); + + return maxsamples; + } + + return 0; +} + static int generate_fifo(ASCState *s, int maxsamples) { uint8_t *buf = s->mixbuf; @@ -156,18 +170,26 @@ static int generate_fifo(ASCState *s, int maxsamples) limit = MIN(MAX(s->fifos[0].cnt, s->fifos[1].cnt), maxsamples); /* - * If starting a new run with no FIFO data present, update the IRQ and - * continue + * MacOS (un)helpfully leaves the FIFO engine running even when it has + * finished writing out samples. Since not all audio backends guarantee an + * all-zero output when no data is provided, zero out the sample buffer + * and then update the FIFO flags and IRQ as normal and continue */ - if (limit == 0 && s->fifos[0].int_status == 0 && - s->fifos[1].int_status == 0) { - s->fifos[0].int_status |= ASC_FIFO_STATUS_HALF_FULL | - ASC_FIFO_STATUS_FULL_EMPTY; - s->fifos[1].int_status |= ASC_FIFO_STATUS_HALF_FULL | - ASC_FIFO_STATUS_FULL_EMPTY; + if (limit == 0) { + if (s->fifos[0].int_status == 0 && s->fifos[1].int_status == 0) { + s->fifos[0].int_status |= ASC_FIFO_STATUS_HALF_FULL | + ASC_FIFO_STATUS_FULL_EMPTY; + s->fifos[1].int_status |= ASC_FIFO_STATUS_HALF_FULL | + ASC_FIFO_STATUS_FULL_EMPTY; + } + + if (s->flush_zero_samples == 0) { + s->flush_zero_samples = s->samples; + } + generate_silence(s, maxsamples); asc_raise_irq(s); - return 0; + return maxsamples; } while (count < limit) { @@ -309,7 +331,7 @@ static void asc_out_cb(void *opaque, int free_b) switch (s->regs[ASC_MODE] & 3) { default: /* Off */ - samples = 0; + samples = generate_silence(s, samples); break; case 1: /* FIFO mode */ @@ -437,6 +459,7 @@ static void asc_write(void *opaque, hwaddr addr, uint64_t value, asc_lower_irq(s); if (value != 0) { AUD_set_active_out(s->voice, 1); + s->flush_zero_samples = 0; } else { AUD_set_active_out(s->voice, 0); } diff --git a/include/hw/audio/asc.h b/include/hw/audio/asc.h index 41c6cba8fa..918f6ac582 100644 --- a/include/hw/audio/asc.h +++ b/include/hw/audio/asc.h @@ -65,6 +65,7 @@ struct ASCState { uint8_t *mixbuf; int samples; int shift; + uint32_t flush_zero_samples; qemu_irq irq;
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. Since not all audio backends guarantee an all-zero output when no data is provided, explicitly generate at least one full output buffer of all-zero output when the FIFO is disabled and continuously if the FIFO is empty. Otherwise some audio backends such as Windows re-use their internal buffers causing the last played sound to loop indefinitely. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/audio/asc.c | 43 ++++++++++++++++++++++++++++++++---------- include/hw/audio/asc.h | 1 + 2 files changed, 34 insertions(+), 10 deletions(-)