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

[Volker Rümelin]

Am 20.09.23 um 17:39 schrieb Mark Cave-Ayland:
> On 14/09/2023 08:06, Volker Rümelin wrote:
>
>> 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
>
> Hi Volker,
>
> Thanks for detailed explanation above - everything makes sense based
> upon previous discussions. My only question is in comment 3 where you
> say "Audio playback devices should write all requested samples
> immediately if possible": can you clarify does this mean that the
> supplied length to the audio callback represents a *required* rather
> than a *maximum* number of samples?
>

Hi Mark,

the supplied length is the number of bytes needed to fill the downstream
audio buffer completely or as fast as possible. An incompletely filled
buffer increases the probability of dropouts. The supplied length is
also the maximum number of bytes AUD_write() will write.

So it's better to write as many bytes as possible, but if you can't it
won't cause audio problems immediately in most cases.

With best regards,
Volker

> Regardless of this I've had some time to test on Windows this
> afternoon, and I can confirm that your changes fix the outstanding
> audio issues. I've squashed your changes locally and I'll included
> them in the next revision of the series, although I'll likely wait a
> bit first to see if any more reviews are forthcoming.
>
>
> Many thanks,
>
> Mark.
>