On 27/01/2026 18:24, [email protected] wrote:
From: Marc-André Lureau <[email protected]>
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 <[email protected]>
---
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..a2fbc286eb1 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)
Does the inline make a difference here? My understanding is that the
compiler treats this as a weak hint these days, and generally does the
right thing by itself.
+{
+ 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;
+ }
I'm not sure that AUDIO_FORMAT__MAX is a valid choice here - can you
drop the explicit case for AUDIO_FORMAT__MAX and then simply have a
default that calls g_assert_not_reached() instead?
+ g_assert_not_reached();
+}
+
+static inline bool audio_format_is_float(AudioFormat fmt)
Same comment here re: inline.
+{
+ return fmt == AUDIO_FORMAT_F32;
+}
+
+static inline bool audio_format_is_signed(AudioFormat fmt)
And here.
+{
+ 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:
Same comment here re: 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 a0e542754e5..146026d0b39 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:
Same comment re: 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);
This doesn't look right - isn't this the signed version?
+ 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;
+ }
I think this is signed too?
+ 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;
}
And also here.
+ 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);
... and this the unsigned version? I think they've been swapped.
+ break;
+ case AUDIO_FORMAT__MAX:
+ g_assert_not_reached();
This looks more like I would expect, although possibly replace the case
with a default? I expect that another assert() would have tripped by now
with real usage if AUDIO_FORMAT__MAX were specified, but I think it
would be good to be consistent.
}
}
@@ -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);
More dolog. Presumably this gets converted to error_report() later?
}
#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 e284542b2dd..72d6194033b 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 40d7986b1d7..08bab353831 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,
ATB,
Mark.
