Re: [PATCH v2 07/20] audio: add Apple Sound Chip (ASC) emulation

["Volker Rümelin" <vr_qemu@t-online.de>]

Am 09.09.23 um 11:48 schrieb Mark Cave-Ayland:
> The Apple Sound Chip was primarily used by the Macintosh II to generate sound
> in hardware which was previously handled by the toolbox ROM with software
> interrupts.
>
> Implement both the standard ASC and also the enhanced ASC (EASC) functionality
> which is used in the Quadra 800.
>
> Note that whilst real ASC hardware uses AUDIO_FORMAT_S8, this implementation uses
> AUDIO_FORMAT_U8 instead because AUDIO_FORMAT_S8 is rarely used and not supported
> by some audio backends like PulseAudio and DirectSound when played directly with
> -audiodev out.mixing-engine=off.
>
> Co-developed-by: Laurent Vivier <laurent@vivier.eu>
> Co-developed-by: Volker Rümelin <vr_qemu@t-online.de>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  MAINTAINERS            |   2 +
>  hw/audio/Kconfig       |   3 +
>  hw/audio/asc.c         | 699 +++++++++++++++++++++++++++++++++++++++++
>  hw/audio/meson.build   |   1 +
>  hw/audio/trace-events  |  10 +
>  hw/m68k/Kconfig        |   1 +
>  include/hw/audio/asc.h |  84 +++++
>  7 files changed, 800 insertions(+)
>  create mode 100644 hw/audio/asc.c
>  create mode 100644 include/hw/audio/asc.h

Hi Mark,

the function generate_fifo() has four issues. Only the first one
is noticeable.

1. The calculation of the variable limit assumes generate_fifo()
generates one output sample from every input byte. This is correct
for the raw mode, but not for the CD-XA BRR mode. This mode
generates 28 output samples from 15 input bytes. This is the
reason for the stuttering end of a CD-XA BRR mode sound. Every
generate_fifo() call generates approximately only half of the
possible samples when the fifo bytes are running low.

2. generate_fifo() doesn't generate the last output sample from
a CD-XA BRR mode sound. The last sample is generated from internal
state and the code will not be called without at least one byte
in the fifo.

3. It's not necessary to wait for a complete 15 byte packet in
CD-XA BRR mode. Audio playback devices should write all
requested samples immediately if possible.

4. The saturation function in CD-XA BRR mode works with 16 bit
integers. It should saturate at +32767 and -32768.

Since I think a few lines of code explain the issues better
than my words, I've attached a patch below.

With best regards,
Volker

> +static int generate_fifo(ASCState *s, int maxsamples)
> +{
> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    uint8_t *buf = s->mixbuf;
> +    int i, limit, count = 0;
> +
> +    limit = MIN(MAX(s->fifos[0].cnt, s->fifos[1].cnt), maxsamples);
> +    while (count < limit) {
> +        uint8_t val;
> +        int16_t d, f0, f1;
> +        int32_t t;
> +        int shift, filter;
> +        bool hasdata = true;
> +
> +        for (i = 0; i < 2; i++) {
> +            ASCFIFOState *fs = &s->fifos[i];
> +
> +            switch (fs->extregs[ASC_EXTREGS_FIFOCTRL] & 0x83) {
> +            case 0x82:
> +                /*
> +                 * CD-XA BRR mode: exit if there isn't enough data in the FIFO
> +                 * for a complete 15 byte packet
> +                 */
> +                if (fs->xa_cnt == -1 && fs->cnt < 15) {
> +                    hasdata = false;
> +                    continue;
> +                }
> +
> +                if (fs->xa_cnt == -1) {
> +                    /* Start of packet, get flags */
> +                    fs->xa_flags = asc_fifo_get(fs);
> +                    fs->xa_cnt = 0;
> +                }
> +
> +                shift = fs->xa_flags & 0xf;
> +                filter = fs->xa_flags >> 4;
> +                f0 = (int8_t)fs->extregs[ASC_EXTREGS_CDXA_DECOMP_FILT +
> +                                 (filter << 1) + 1];
> +                f1 = (int8_t)fs->extregs[ASC_EXTREGS_CDXA_DECOMP_FILT +
> +                                 (filter << 1)];
> +                if ((fs->xa_cnt & 1) == 0) {
> +                    fs->xa_val = asc_fifo_get(fs);
> +                    d = (fs->xa_val & 0xf) << 12;
> +                } else {
> +                    d = (fs->xa_val & 0xf0) << 8;
> +                }
> +                t = (d >> shift) + (((fs->xa_last[0] * f0) +
> +                                     (fs->xa_last[1] * f1) + 32) >> 6);
> +                if (t < -32768) {
> +                    t = -32768;
> +                } else if (t > 32768) {
> +                    t = 32768;
> +                }
> +
> +                /*
> +                 * CD-XA BRR generates 16-bit signed output, so convert to
> +                 * 8-bit before writing to buffer. Does real hardware do the
> +                 * same?
> +                 */
> +                buf[count * 2 + i] = (uint8_t)(t / 256) ^ 0x80;
> +                fs->xa_cnt++;
> +
> +                fs->xa_last[1] = fs->xa_last[0];
> +                fs->xa_last[0] = (int16_t)t;
> +
> +                if (fs->xa_cnt == 28) {
> +                    /* End of packet */
> +                    fs->xa_cnt = -1;
> +                }
> +                break;
> +
> +            default:
> +                /* fallthrough */
> +            case 0x80:
> +                /* Raw mode */
> +                if (fs->cnt) {
> +                    val = asc_fifo_get(fs);
> +                } else {
> +                    val = 0x80;
> +                }
> +
> +                buf[count * 2 + i] = val;
> +                break;
> +            }
> +        }
> +
> +        if (!hasdata) {
> +            break;
> +        }
> +
> +        count++;
> +    }
> +
> +    /*
> +     * MacOS (un)helpfully leaves the FIFO engine running even when it has
> +     * finished writing out samples, but still expects the FIFO empty
> +     * interrupts to be generated for each FIFO cycle (without these interrupts
> +     * MacOS will freeze)
> +     */
> +    if (s->fifos[0].cnt == 0 && s->fifos[1].cnt == 0) {
> +        if (!s->fifo_empty_ns) {
> +            /* FIFO has completed first empty cycle */
> +            s->fifo_empty_ns = now;
> +        } else if (now > (s->fifo_empty_ns + ASC_FIFO_CYCLE_TIME)) {
> +            /* FIFO has completed entire cycle with no data */
> +            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;
> +            s->fifo_empty_ns = now;
> +            asc_raise_irq(s);
> +        }
> +    } else {
> +        /* FIFO contains data, reset empty time */
> +        s->fifo_empty_ns = 0;
> +    }
> +
> +    return count;
> +}
>

diff --git a/hw/audio/asc.c b/hw/audio/asc.c
index b01b285512..74988fef9c 100644
--- a/hw/audio/asc.c
+++ b/hw/audio/asc.c
@@ -155,31 +155,26 @@ static int generate_fifo(ASCState *s, int maxsamples)
 {
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     uint8_t *buf = s->mixbuf;
-    int i, limit, count = 0;
+    int i, wcount = 0;
 
-    limit = MIN(MAX(s->fifos[0].cnt, s->fifos[1].cnt), maxsamples);
-    while (count < limit) {
+    while (wcount < maxsamples) {
         uint8_t val;
         int16_t d, f0, f1;
         int32_t t;
         int shift, filter;
-        bool hasdata = true;
+        bool hasdata = false;
 
         for (i = 0; i < 2; i++) {
             ASCFIFOState *fs = &s->fifos[i];
 
             switch (fs->extregs[ASC_EXTREGS_FIFOCTRL] & 0x83) {
             case 0x82:
-                /*
-                 * CD-XA BRR mode: exit if there isn't enough data in
the FIFO
-                 * for a complete 15 byte packet
-                 */
-                if (fs->xa_cnt == -1 && fs->cnt < 15) {
-                    hasdata = false;
-                    continue;
-                }
-
+                /* CD-XA BRR mode: decompress 15 bytes into 28 16bit
samples */
                 if (fs->xa_cnt == -1) {
+                    if (!fs->cnt) {
+                        val = 0x80;
+                        break;
+                    }
                     /* Start of packet, get flags */
                     fs->xa_flags = asc_fifo_get(fs);
                     fs->xa_cnt = 0;
@@ -192,6 +187,10 @@ static int generate_fifo(ASCState *s, int maxsamples)
                 f1 = (int8_t)fs->extregs[ASC_EXTREGS_CDXA_DECOMP_FILT +
                                  (filter << 1)];
                 if ((fs->xa_cnt & 1) == 0) {
+                    if (!fs->cnt) {
+                        val = 0x80;
+                        break;
+                    }
                     fs->xa_val = asc_fifo_get(fs);
                     d = (fs->xa_val & 0xf) << 12;
                 } else {
@@ -201,8 +200,8 @@ static int generate_fifo(ASCState *s, int maxsamples)
                                      (fs->xa_last[1] * f1) + 32) >> 6);
                 if (t < -32768) {
                     t = -32768;
-                } else if (t > 32768) {
-                    t = 32768;
+                } else if (t > 32767) {
+                    t = 32767;
                 }
 
                 /*
@@ -210,7 +209,8 @@ static int generate_fifo(ASCState *s, int maxsamples)
                  * 8-bit before writing to buffer. Does real hardware
do the
                  * same?
                  */
-                buf[count * 2 + i] = (uint8_t)(t / 256) ^ 0x80;
+                val = (uint8_t)(t / 256) ^ 0x80;
+                hasdata = true;
                 fs->xa_cnt++;
 
                 fs->xa_last[1] = fs->xa_last[0];
@@ -228,20 +228,21 @@ static int generate_fifo(ASCState *s, int maxsamples)
                 /* Raw mode */
                 if (fs->cnt) {
                     val = asc_fifo_get(fs);
+                    hasdata = true;
                 } else {
                     val = 0x80;
                 }
-
-                buf[count * 2 + i] = val;
                 break;
             }
+
+            buf[wcount * 2 + i] = val;
         }
 
         if (!hasdata) {
             break;
         }
 
-        count++;
+        wcount++;
     }
 
     /*
@@ -268,7 +269,7 @@ static int generate_fifo(ASCState *s, int maxsamples)
         s->fifo_empty_ns = 0;
     }
 
-    return count;
+    return wcount;
 }
 
 static int generate_wavetable(ASCState *s, int maxsamples)
-- 
2.35.3