Hi,
On 2/23/26 16:47, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The audio_pcm_info structure stored three fields (bits, is_signed,
> is_float) that were always derived from the AudioFormat enum. This
> redundancy meant the same information was represented twice, with no
> type-level guarantee that they stayed in sync.
>
> Replace these fields with a single AudioFormat field, and add helper
> functions to extract the derived properties when needed:
> - audio_format_bits()
> - audio_format_is_signed()
> - audio_format_is_float()
>
> This improves type safety by making AudioFormat the single source of
> truth, eliminating the possibility of inconsistent state between the
> format enum and its derived boolean/integer representations.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> ---
> audio/audio_int.h | 4 +-
> audio/audio_template.h | 12 +--
> include/qemu/audio.h | 49 +++++++++
> audio/audio-mixeng-be.c | 218 ++++++++++++----------------------------
> audio/dbusaudio.c | 12 +--
> audio/coreaudio.m | 2 +-
> 6 files changed, 126 insertions(+), 171 deletions(-)
>
> diff --git a/audio/audio_int.h b/audio/audio_int.h
> index 5334c4baad2..dd5f2220d75 100644
> --- a/audio/audio_int.h
> +++ b/audio/audio_int.h
> @@ -45,9 +45,7 @@ struct audio_callback {
> };
>
> struct audio_pcm_info {
> - int bits;
> - bool is_signed;
> - bool is_float;
> + AudioFormat af;
> int freq;
> int nchannels;
> int bytes_per_frame;
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index 08d60422589..3da91a4782c 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -173,7 +173,7 @@ static int glue (audio_pcm_sw_init_, TYPE) (
> sw->empty = true;
> #endif
>
> - if (sw->info.is_float) {
> + if (audio_format_is_float(hw->info.af)) {
> #ifdef DAC
> sw->conv = mixeng_conv_float[sw->info.nchannels == 2]
> [sw->info.swap_endianness];
> @@ -188,9 +188,9 @@ static int glue (audio_pcm_sw_init_, TYPE) (
> sw->clip = mixeng_clip
> #endif
> [sw->info.nchannels == 2]
> - [sw->info.is_signed]
> + [audio_format_is_signed(hw->info.af)]
> [sw->info.swap_endianness]
> - [audio_bits_to_index(sw->info.bits)];
> + [audio_format_to_index(hw->info.af)];
> }
>
> sw->name = g_strdup (name);
> @@ -300,7 +300,7 @@ static HW *glue(audio_pcm_hw_add_new_, TYPE)(AudioMixengBackend *s,
> goto err1;
> }
>
> - if (hw->info.is_float) {
> + if (audio_format_is_float(hw->info.af)) {
> #ifdef DAC
> hw->clip = mixeng_clip_float[hw->info.nchannels == 2]
> [hw->info.swap_endianness];
> @@ -315,9 +315,9 @@ static HW *glue(audio_pcm_hw_add_new_, TYPE)(AudioMixengBackend *s,
> hw->conv = mixeng_conv
> #endif
> [hw->info.nchannels == 2]
> - [hw->info.is_signed]
> + [audio_format_is_signed(hw->info.af)]
> [hw->info.swap_endianness]
> - [audio_bits_to_index(hw->info.bits)];
> + [audio_format_to_index(hw->info.af)];
> }
>
> glue(audio_pcm_hw_alloc_resources_, TYPE)(hw);
> diff --git a/include/qemu/audio.h b/include/qemu/audio.h
> index b6b6ee9b560..9e4566d60aa 100644
> --- a/include/qemu/audio.h
> +++ b/include/qemu/audio.h
> @@ -185,6 +185,55 @@ bool audio_be_set_dbus_server(AudioBackend *be,
>
> const char *audio_application_name(void);
>
> +static inline int audio_format_bits(AudioFormat fmt)
> +{
> + switch (fmt) {
> + case AUDIO_FORMAT_S8:
> + case AUDIO_FORMAT_U8:
> + return 8;
> +
> + case AUDIO_FORMAT_S16:
> + case AUDIO_FORMAT_U16:
> + return 16;
> +
> + case AUDIO_FORMAT_F32:
> + case AUDIO_FORMAT_S32:
> + case AUDIO_FORMAT_U32:
> + return 32;
> +
> + case AUDIO_FORMAT__MAX:
> + break;
> + }
> +
> + g_assert_not_reached();
> +}
> +
> +static inline bool audio_format_is_float(AudioFormat fmt)
> +{
> + return fmt == AUDIO_FORMAT_F32;
> +}
> +
> +static inline bool audio_format_is_signed(AudioFormat fmt)
> +{
> + switch (fmt) {
> + case AUDIO_FORMAT_S8:
> + case AUDIO_FORMAT_S16:
> + case AUDIO_FORMAT_S32:
> + case AUDIO_FORMAT_F32:
> + return true;
> +
> + case AUDIO_FORMAT_U8:
> + case AUDIO_FORMAT_U16:
> + case AUDIO_FORMAT_U32:
> + return false;
> +
> + case AUDIO_FORMAT__MAX:
> + break;
> + }
> +
> + g_assert_not_reached();
> +}
> +
> #define DEFINE_AUDIO_PROPERTIES(_s, _f) \
> DEFINE_PROP_AUDIODEV("audiodev", _s, _f)
>
> diff --git a/audio/audio-mixeng-be.c b/audio/audio-mixeng-be.c
> index d02cfe3c509..931d5b206f2 100644
> --- a/audio/audio-mixeng-be.c
> +++ b/audio/audio-mixeng-be.c
> @@ -62,23 +62,28 @@ int audio_bug (const char *funcname, int cond)
> return cond;
> }
>
> -static inline int audio_bits_to_index (int bits)
> +/*
> + * Convert audio format to mixeng_clip index. Used by audio_pcm_sw_init_ and
> + * audio_mixeng_backend_add_capture()
> + */
> +static int audio_format_to_index(AudioFormat af)
> {
> - switch (bits) {
> - case 8:
> + switch (af) {
> + case AUDIO_FORMAT_U8:
> + case AUDIO_FORMAT_S8:
> return 0;
> -
> - case 16:
> + case AUDIO_FORMAT_U16:
> + case AUDIO_FORMAT_S16:
> return 1;
> -
> - case 32:
> + case AUDIO_FORMAT_U32:
> + case AUDIO_FORMAT_S32:
> return 2;
> -
> - default:
> - audio_bug ("bits_to_index", 1);
> - AUD_log (NULL, "invalid bits %d\n", bits);
> - return 0;
> + case AUDIO_FORMAT_F32:
> + case AUDIO_FORMAT__MAX:
> + break;
> }
> +
> + g_assert_not_reached();
> }
>
> void AUD_vlog (const char *cap, const char *fmt, va_list ap)
> @@ -172,141 +177,68 @@ static int audio_validate_settings (const struct audsettings *as)
>
> static int audio_pcm_info_eq (struct audio_pcm_info *info, const struct audsettings *as)
> {
> - int bits = 8;
> - bool is_signed = false, is_float = false;
> -
> - switch (as->fmt) {
> - case AUDIO_FORMAT_S8:
> - is_signed = true;
> - /* fall through */
> - case AUDIO_FORMAT_U8:
> - break;
> -
> - case AUDIO_FORMAT_S16:
> - is_signed = true;
> - /* fall through */
> - case AUDIO_FORMAT_U16:
> - bits = 16;
> - break;
> -
> - case AUDIO_FORMAT_F32:
> - is_float = true;
> - /* fall through */
> - case AUDIO_FORMAT_S32:
> - is_signed = true;
> - /* fall through */
> - case AUDIO_FORMAT_U32:
> - bits = 32;
> - break;
> -
> - default:
> - abort();
> - }
> - return info->freq == as->freq
> + return info->af == as->fmt
> + && info->freq == as->freq
> && info->nchannels == as->nchannels
> - && info->is_signed == is_signed
> - && info->is_float == is_float
> - && info->bits == bits
> && info->swap_endianness == (as->endianness != HOST_BIG_ENDIAN);
> }
>
> void audio_pcm_init_info (struct audio_pcm_info *info, const struct audsettings *as)
> {
> - int bits = 8, mul;
> - bool is_signed = false, is_float = false;
> -
> - switch (as->fmt) {
> - case AUDIO_FORMAT_S8:
> - is_signed = true;
> - /* fall through */
> - case AUDIO_FORMAT_U8:
> - mul = 1;
> - break;
> -
> - case AUDIO_FORMAT_S16:
> - is_signed = true;
> - /* fall through */
> - case AUDIO_FORMAT_U16:
> - bits = 16;
> - mul = 2;
> - break;
> -
> - case AUDIO_FORMAT_F32:
> - is_float = true;
> - /* fall through */
> - case AUDIO_FORMAT_S32:
> - is_signed = true;
> - /* fall through */
> - case AUDIO_FORMAT_U32:
> - bits = 32;
> - mul = 4;
> - break;
> -
> - default:
> - abort();
> - }
> -
> + info->af = as->fmt;
> info->freq = as->freq;
> - info->bits = bits;
> - info->is_signed = is_signed;
> - info->is_float = is_float;
> info->nchannels = as->nchannels;
> - info->bytes_per_frame = as->nchannels * mul;
> + info->bytes_per_frame = as->nchannels * audio_format_bits(as->fmt) / 8;
> info->bytes_per_second = info->freq * info->bytes_per_frame;
> info->swap_endianness = (as->endianness != HOST_BIG_ENDIAN);
> }
>
> -void audio_pcm_info_clear_buf (struct audio_pcm_info *info, void *buf, int len)
> +void audio_pcm_info_clear_buf(struct audio_pcm_info *info, void *buf, int len)
> {
> if (!len) {
> return;
> }
>
> - if (info->is_signed || info->is_float) {
> - memset(buf, 0x00, len * info->bytes_per_frame);
> - } else {
> - switch (info->bits) {
> - case 8:
> - memset(buf, 0x80, len * info->bytes_per_frame);
> - break;
> -
> - case 16:
> - {
> - int i;
> - uint16_t *p = buf;
> - short s = INT16_MAX;
> -
> - if (info->swap_endianness) {
> - s = bswap16 (s);
> - }
> -
> - for (i = 0; i < len * info->nchannels; i++) {
> - p[i] = s;
> - }
> - }
> - break;
> + switch (info->af) {
> + case AUDIO_FORMAT_U8:
> + memset(buf, 0x80, len * info->bytes_per_frame);
> + break;
> + case AUDIO_FORMAT_U16: {
> + int i;
> + uint16_t *p = buf;
> + short s = INT16_MAX;
>
> - case 32:
> - {
> - int i;
> - uint32_t *p = buf;
> - int32_t s = INT32_MAX;
> + if (info->swap_endianness) {
> + s = bswap16(s);
> + }
>
> - if (info->swap_endianness) {
> - s = bswap32 (s);
> - }
> + for (i = 0; i < len * info->nchannels; i++) {
> + p[i] = s;
> + }
> + break;
> + }
> + case AUDIO_FORMAT_U32: {
> + int i;
> + uint32_t *p = buf;
> + int32_t s = INT32_MAX;
>
> - for (i = 0; i < len * info->nchannels; i++) {
> - p[i] = s;
> - }
> - }
> - break;
> + if (info->swap_endianness) {
> + s = bswap32(s);
> + }
>
> - default:
> - AUD_log (NULL, "audio_pcm_info_clear_buf: invalid bits %d\n",
> - info->bits);
> - break;
> + for (i = 0; i < len * info->nchannels; i++) {
> + p[i] = s;
> }
> + break;
> + }
> + case AUDIO_FORMAT_S8:
> + case AUDIO_FORMAT_S16:
> + case AUDIO_FORMAT_S32:
> + case AUDIO_FORMAT_F32:
> + memset(buf, 0x00, len * info->bytes_per_frame);
> + break;
> + case AUDIO_FORMAT__MAX:
> + g_assert_not_reached();
> }
> }
>
> @@ -719,8 +651,8 @@ static size_t audio_pcm_sw_write(SWVoiceOut *sw, void *buf, size_t buf_len)
> #ifdef DEBUG_AUDIO
> static void audio_pcm_print_info (const char *cap, struct audio_pcm_info *info)
> {
> - dolog("%s: bits %d, sign %d, float %d, freq %d, nchan %d\n",
> - cap, info->bits, info->is_signed, info->is_float, info->freq,
> + dolog("%s: %s, freq %d, nchan %d\n",
> + cap, AudioFormat_str(info->af), info->freq,
> info->nchannels);
> }
> #endif
> @@ -1759,15 +1691,15 @@ static CaptureVoiceOut *audio_mixeng_backend_add_capture(
>
> cap->buf = g_malloc0_n(hw->mix_buf.size, hw->info.bytes_per_frame);
>
> - if (hw->info.is_float) {
> + if (audio_format_is_float(hw->info.af)) {
> hw->clip = mixeng_clip_float[hw->info.nchannels == 2]
> [hw->info.swap_endianness];
> } else {
> hw->clip = mixeng_clip
> [hw->info.nchannels == 2]
> - [hw->info.is_signed]
> + [audio_format_is_signed(hw->info.af)]
> [hw->info.swap_endianness]
> - [audio_bits_to_index(hw->info.bits)];
> + [audio_format_to_index(hw->info.af)];
> }
>
> QLIST_INSERT_HEAD (&s->cap_head, cap, entries);
> @@ -1869,29 +1801,6 @@ audsettings audiodev_to_audsettings(AudiodevPerDirectionOptions *pdo)
> };
> }
>
> -int audioformat_bytes_per_sample(AudioFormat fmt)
> -{
> - switch (fmt) {
> - case AUDIO_FORMAT_U8:
> - case AUDIO_FORMAT_S8:
> - return 1;
> -
> - case AUDIO_FORMAT_U16:
> - case AUDIO_FORMAT_S16:
> - return 2;
> -
> - case AUDIO_FORMAT_U32:
> - case AUDIO_FORMAT_S32:
> - case AUDIO_FORMAT_F32:
> - return 4;
> -
> - case AUDIO_FORMAT__MAX:
> - ;
> - }
> - abort();
> -}
> -
> -
> /* frames = freq * usec / 1e6 */
> int audio_buffer_frames(AudiodevPerDirectionOptions *pdo,
> audsettings *as, int def_usecs)
> @@ -1914,8 +1823,7 @@ int audio_buffer_samples(AudiodevPerDirectionOptions *pdo,
> int audio_buffer_bytes(AudiodevPerDirectionOptions *pdo,
> audsettings *as, int def_usecs)
> {
> - return audio_buffer_samples(pdo, as, def_usecs) *
> - audioformat_bytes_per_sample(as->fmt);
> + return audio_buffer_samples(pdo, as, def_usecs) * audio_format_bits(as->fmt) / 8;
> }
>
> void audio_rate_start(RateCtl *rate)
> diff --git a/audio/dbusaudio.c b/audio/dbusaudio.c
> index 5f65c043c16..b212b3d6022 100644
> --- a/audio/dbusaudio.c
> +++ b/audio/dbusaudio.c
> @@ -147,9 +147,9 @@ dbus_init_out_listener(QemuDBusDisplay1AudioOutListener *listener,
> qemu_dbus_display1_audio_out_listener_call_init(
> listener,
> (uintptr_t)hw,
> - hw->info.bits,
> - hw->info.is_signed,
> - hw->info.is_float,
> + audio_format_bits(hw->info.af),
> + audio_format_is_signed(hw->info.af),
> + audio_format_is_float(hw->info.af),
> hw->info.freq,
> hw->info.nchannels,
> hw->info.bytes_per_frame,
> @@ -273,9 +273,9 @@ dbus_init_in_listener(QemuDBusDisplay1AudioInListener *listener, HWVoiceIn *hw)
> qemu_dbus_display1_audio_in_listener_call_init(
> listener,
> (uintptr_t)hw,
> - hw->info.bits,
> - hw->info.is_signed,
> - hw->info.is_float,
> + audio_format_bits(hw->info.af),
> + audio_format_is_signed(hw->info.af),
> + audio_format_is_float(hw->info.af),
> hw->info.freq,
> hw->info.nchannels,
> hw->info.bytes_per_frame,
> diff --git a/audio/coreaudio.m b/audio/coreaudio.m
> index 0c0e8583c89..1f4b3d9d467 100644
> --- a/audio/coreaudio.m
> +++ b/audio/coreaudio.m
> @@ -359,7 +359,7 @@ static OSStatus init_out_device(coreaudioVoiceOut *core)
> AudioValueRange frameRange;
>
> AudioStreamBasicDescription streamBasicDescription = {
> - .mBitsPerChannel = core->hw.info.bits,
> + .mBitsPerChannel = audio_format_bits(core->hw.info.af),
> .mBytesPerFrame = core->hw.info.bytes_per_frame,
> .mBytesPerPacket = core->hw.info.bytes_per_frame,
> .mChannelsPerFrame = core->hw.info.nchannels,
Audio (driver=pipewire,model=virtio) is broken/distorted on staging tree
and I bisected it to this patch. Can't see anything obviously wrong in
the code, any ideas where the problem could be?
--
Best regards,
Dmitry