From 571ff40704529aa9992293a9fb6aa32c6c602eef Mon Sep 17 00:00:00 2001 From: Daniel Nouri Date: Sat, 4 Oct 2025 16:31:01 +0200 Subject: [PATCH 01/23] alsa: Fix IEC958 digital output not unmuted on path activation When selecting an HDMI/DisplayPort (IEC958) output path, the hardware mute switch remains in kernel default state (muted), causing no audio output despite correct software routing. Root cause: pa_alsa_path_select() only sets mute switches when mute_during_activation is enabled. No mixer paths enable this setting, making the switch configuration code unreachable for IEC958 paths. Solution: Always set mute switches to match device mute status after path activation, regardless of mute_during_activation setting. Testing: Added test-alsa-path-select tool to verify the fix. - Loads mixer path and calls pa_alsa_path_select() - Verifies switch states match expected values - Tested on AMD Radeon HDMI and Realtek ALC257 analog Manual verification: - Before: IEC958 switch OFF, no audio - After: IEC958 switch set correctly, audio works This bug was inherited from PulseAudio's ALSA mixer path code where HDMI path configurations lack IEC958 unmute sections. Fixes: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/3261 See-Also: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/issues/562 See-Also: https://github.com/alsa-project/alsa-ucm-conf/commit/33be660e4b1e75c19d5332556c3d2636dd3344bf See-Also: https://bugs.launchpad.net/hundredpapercuts/+bug/681996 --- spa/plugins/alsa/acp/alsa-mixer.c | 24 +- spa/plugins/alsa/acp/meson.build | 9 + spa/plugins/alsa/acp/test-alsa-path-select.c | 348 +++++++++++++++++++ 3 files changed, 374 insertions(+), 7 deletions(-) create mode 100644 spa/plugins/alsa/acp/test-alsa-path-select.c diff --git a/spa/plugins/alsa/acp/alsa-mixer.c b/spa/plugins/alsa/acp/alsa-mixer.c index 1f494ee0b..64088177f 100644 --- a/spa/plugins/alsa/acp/alsa-mixer.c +++ b/spa/plugins/alsa/acp/alsa-mixer.c @@ -1479,13 +1479,23 @@ int pa_alsa_path_select(pa_alsa_path *p, pa_alsa_setting *s, snd_mixer_t *m, boo if (s) setting_select(s, m); - /* Finally restore hw mute to the device mute status. */ - if (p->mute_during_activation) { - PA_LLIST_FOREACH(e, p->elements) { - if (e->switch_use == PA_ALSA_SWITCH_MUTE) { - if (element_set_switch(e, m, !device_is_muted) < 0) - return -1; - } + /* Set hw mute switches to match device mute status. + * + * When mute_during_activation is enabled, switches are temporarily muted + * during path setup to avoid pops/clicks, and this restores them. + * + * When mute_during_activation is disabled (default), switches may still be + * in their kernel default state (often muted for digital outputs like IEC958). + * We must explicitly unmute them if the device isn't muted. + * + * This ensures HDMI/DisplayPort audio (IEC958) works correctly, as these + * digital output switches default to muted in ALSA drivers but are not + * automatically enabled when the path is selected. + */ + PA_LLIST_FOREACH(e, p->elements) { + if (e->switch_use == PA_ALSA_SWITCH_MUTE) { + if (element_set_switch(e, m, !device_is_muted) < 0) + return -1; } } diff --git a/spa/plugins/alsa/acp/meson.build b/spa/plugins/alsa/acp/meson.build index 0ec97e2b4..f5adfa193 100644 --- a/spa/plugins/alsa/acp/meson.build +++ b/spa/plugins/alsa/acp/meson.build @@ -20,3 +20,12 @@ acp_lib = static_library( dependencies : [ spa_dep, alsa_dep, mathlib, ] ) acp_dep = declare_dependency(link_with: acp_lib) + +executable('test-alsa-path-select', + [ 'test-alsa-path-select.c' ], + c_args : acp_c_args, + include_directories : [configinc, includes_inc ], + dependencies : [ spa_dep, alsa_dep, mathlib, ], + link_with : [ acp_lib ], + install : false, +) diff --git a/spa/plugins/alsa/acp/test-alsa-path-select.c b/spa/plugins/alsa/acp/test-alsa-path-select.c new file mode 100644 index 000000000..f30662d50 --- /dev/null +++ b/spa/plugins/alsa/acp/test-alsa-path-select.c @@ -0,0 +1,348 @@ +/* ALSA path select test tool */ +/* SPDX-FileCopyrightText: Copyright © 2025 Daniel Nouri */ +/* SPDX-License-Identifier: MIT */ + +/* + * Tests pa_alsa_path_select() to verify mute switches are correctly set + * based on device mute status, regardless of mute_during_activation setting. + * + * Regression test for HDMI/DisplayPort audio bug where IEC958 switches + * remain in kernel default state (OFF) because pa_alsa_path_select() only + * set switches when mute_during_activation=true, but no paths enable this. + * + * Test verification: + * Before fix: IEC958 switch stays OFF after path select → test FAILS + * After fix: IEC958 switch set correctly based on device state → test PASSES + * + * Usage: + * test-alsa-path-select -c [-p ] [-m] [-v] + * + * Examples: + * test-alsa-path-select -c 1 # IEC958 unmuted (expect ON) + * test-alsa-path-select -c 1 -m # IEC958 muted (expect OFF) + * test-alsa-path-select -c 0 -p analog-output # Regression test + * + * Exit codes: + * 0 = Test passed + * 1 = Test failed + * 2 = Setup error + */ + +#include +#include +#include +#include +#include +#include +#include + +#include "alsa-mixer.h" +#include "alsa-util.h" + +#define DEFAULT_PATHS_DIR "/usr/share/alsa-card-profile/mixer/paths" + +struct test_config { + int card; + const char *path_name; + const char *paths_dir; + bool device_muted; + bool verbose; +}; + +static void show_help(const char *name) +{ + printf("Usage: %s [options]\n", name); + printf("\n"); + printf("Test tool for pa_alsa_path_select() - verifies IEC958 unmute fix\n"); + printf("\n"); + printf("Options:\n"); + printf(" -c, --card ALSA card number (required)\n"); + printf(" -p, --path Mixer path name (default: iec958-stereo-output)\n"); + printf(" -d, --paths-dir Path to mixer path configs (default: %s)\n", DEFAULT_PATHS_DIR); + printf(" -m, --muted Test with device muted (default: unmuted)\n"); + printf(" -v, --verbose Verbose output\n"); + printf(" -h, --help Show this help\n"); + printf("\n"); + printf("Examples:\n"); + printf(" # Test IEC958 unmute on card 1\n"); + printf(" %s -c 1\n", name); + printf("\n"); + printf(" # Test analog output\n"); + printf(" %s -c 0 -p analog-output\n", name); + printf("\n"); + printf(" # Test with device muted\n"); + printf(" %s -c 1 -m\n", name); + printf("\n"); + printf("Exit codes:\n"); + printf(" 0 - Test passed (switches set correctly)\n"); + printf(" 1 - Test failed (switches not set as expected)\n"); + printf(" 2 - Setup error (path not found, hardware error, etc.)\n"); +} + +static int parse_args(int argc, char *argv[], struct test_config *config) +{ + static const struct option long_options[] = { + { "card", required_argument, NULL, 'c' }, + { "path", required_argument, NULL, 'p' }, + { "paths-dir", required_argument, NULL, 'd' }, + { "muted", no_argument, NULL, 'm' }, + { "verbose", no_argument, NULL, 'v' }, + { "help", no_argument, NULL, 'h' }, + { NULL, 0, NULL, 0 } + }; + + int c; + config->card = -1; + config->path_name = "iec958-stereo-output.conf"; + config->paths_dir = DEFAULT_PATHS_DIR; + config->device_muted = false; + config->verbose = false; + + while ((c = getopt_long(argc, argv, "c:p:d:mvh", long_options, NULL)) != -1) { + switch (c) { + case 'c': + config->card = atoi(optarg); + break; + case 'p': + config->path_name = optarg; + break; + case 'd': + config->paths_dir = optarg; + break; + case 'm': + config->device_muted = true; + break; + case 'v': + config->verbose = true; + break; + case 'h': + show_help(argv[0]); + return -1; + default: + show_help(argv[0]); + return -2; + } + } + + if (config->card < 0) { + fprintf(stderr, "Error: Card number is required (-c )\n\n"); + show_help(argv[0]); + return -2; + } + + return 0; +} + +static int get_switch_state(snd_mixer_t *mixer, const char *name, int index, bool *state) +{ + snd_mixer_elem_t *elem; + snd_mixer_selem_id_t *sid; + int val; + + snd_mixer_selem_id_alloca(&sid); + snd_mixer_selem_id_set_name(sid, name); + snd_mixer_selem_id_set_index(sid, index); + + elem = snd_mixer_find_selem(mixer, sid); + if (!elem) { + fprintf(stderr, "Warning: Element '%s',%d not found\n", name, index); + return -ENOENT; + } + + if (!snd_mixer_selem_has_playback_switch(elem)) { + fprintf(stderr, "Warning: Element '%s',%d has no playback switch\n", name, index); + return -EINVAL; + } + + if (snd_mixer_selem_get_playback_switch(elem, SND_MIXER_SCHN_MONO, &val) < 0) { + if (snd_mixer_selem_get_playback_switch(elem, SND_MIXER_SCHN_FRONT_LEFT, &val) < 0) { + fprintf(stderr, "Error: Cannot read switch state for '%s',%d\n", name, index); + return -EIO; + } + } + + *state = val ? true : false; + return 0; +} + +int main(int argc, char *argv[]) +{ + struct test_config config; + pa_alsa_path *path = NULL; + snd_mixer_t *mixer = NULL; + char card_name[64]; + int ret = 2; // Setup error by default + int err; + + /* Parse arguments */ + err = parse_args(argc, argv, &config); + if (err < 0) { + return err == -1 ? 0 : 2; // -1 is help, -2 is error + } + + printf("Testing pa_alsa_path_select() with:\n"); + printf(" Card: %d\n", config.card); + printf(" Path: %s\n", config.path_name); + printf(" Paths dir: %s\n", config.paths_dir); + printf(" Device muted: %s\n", config.device_muted ? "yes" : "no"); + printf("\n"); + + /* Load mixer path */ + path = pa_alsa_path_new(config.paths_dir, config.path_name, PA_ALSA_DIRECTION_OUTPUT); + if (!path) { + fprintf(stderr, "Error: Failed to load path '%s' from %s\n", + config.path_name, config.paths_dir); + fprintf(stderr, "Make sure the path file exists: %s/%s.conf\n", + config.paths_dir, config.path_name); + goto cleanup; + } + + /* Count elements BEFORE probing */ + int element_count_before = 0; + pa_alsa_element *e_temp; + PA_LLIST_FOREACH(e_temp, path->elements) { + element_count_before++; + } + printf("Elements in path BEFORE probing: %d\n", element_count_before); + + if (config.verbose && element_count_before > 0) { + printf("Loaded path: %s\n", path->name ? path->name : config.path_name); + printf(" mute_during_activation: %s\n", path->mute_during_activation ? "yes" : "no"); + printf(" Elements:\n"); + + PA_LLIST_FOREACH(e_temp, path->elements) { + const char *switch_use; + switch (e_temp->switch_use) { + case PA_ALSA_SWITCH_MUTE: switch_use = "mute"; break; + case PA_ALSA_SWITCH_ON: switch_use = "on"; break; + case PA_ALSA_SWITCH_OFF: switch_use = "off"; break; + default: switch_use = "ignore"; break; + } + printf(" - %s (index %d): switch=%s\n", + e_temp->alsa_id.name, e_temp->alsa_id.index, switch_use); + } + printf("\n"); + } + + /* Open ALSA mixer */ + snprintf(card_name, sizeof(card_name), "hw:%d", config.card); + + if (snd_mixer_open(&mixer, 0) < 0) { + fprintf(stderr, "Error: Failed to open mixer\n"); + goto cleanup; + } + + if (snd_mixer_attach(mixer, card_name) < 0) { + fprintf(stderr, "Error: Failed to attach mixer to %s\n", card_name); + fprintf(stderr, "Make sure card %d exists (check with: aplay -l)\n", config.card); + goto cleanup; + } + + if (snd_mixer_selem_register(mixer, NULL, NULL) < 0) { + fprintf(stderr, "Error: Failed to register mixer\n"); + goto cleanup; + } + + if (snd_mixer_load(mixer) < 0) { + fprintf(stderr, "Error: Failed to load mixer elements\n"); + goto cleanup; + } + + /* Probe the path to ensure elements are available */ + printf("Probing path...\n"); + if (pa_alsa_path_probe(path, NULL, mixer, false) < 0) { + fprintf(stderr, "Error: Path probe failed - required elements not found on card\n"); + goto cleanup; + } + printf("Path probe: OK\n\n"); + + /* Record switch states before activation */ + printf("Switch states BEFORE pa_alsa_path_select():\n"); + pa_alsa_element *e; + int mute_switch_count = 0; + PA_LLIST_FOREACH(e, path->elements) { + if (e->switch_use == PA_ALSA_SWITCH_MUTE) { + bool state; + mute_switch_count++; + if (get_switch_state(mixer, e->alsa_id.name, e->alsa_id.index, &state) == 0) { + printf(" %s,%d = %s\n", e->alsa_id.name, e->alsa_id.index, + state ? "ON" : "OFF"); + } + } + } + + if (mute_switch_count == 0) { + fprintf(stderr, "Warning: No mute switches found in path\n"); + printf("This test requires a path with switch=mute elements\n"); + ret = 2; + goto cleanup; + } + + /* Call pa_alsa_path_select() - THE FUNCTION UNDER TEST */ + printf("\nCalling pa_alsa_path_select(path, NULL, mixer, device_is_muted=%s)...\n", + config.device_muted ? "true" : "false"); + + err = pa_alsa_path_select(path, NULL, mixer, config.device_muted); + if (err < 0) { + fprintf(stderr, "Error: pa_alsa_path_select() failed with error %d\n", err); + ret = 2; + goto cleanup; + } + printf("pa_alsa_path_select(): OK\n\n"); + + /* Verify switch states after activation */ + printf("Switch states AFTER pa_alsa_path_select():\n"); + bool all_correct = true; + bool expected_state = !config.device_muted; // unmuted device = switches ON + + PA_LLIST_FOREACH(e, path->elements) { + if (e->switch_use == PA_ALSA_SWITCH_MUTE) { + bool actual_state; + if (get_switch_state(mixer, e->alsa_id.name, e->alsa_id.index, &actual_state) == 0) { + printf(" %s,%d = %s", e->alsa_id.name, e->alsa_id.index, + actual_state ? "ON" : "OFF"); + + if (actual_state == expected_state) { + printf(" ✓\n"); + } else { + printf(" ✗ (expected %s)\n", expected_state ? "ON" : "OFF"); + all_correct = false; + } + } + } + } + + /* Determine test result */ + printf("\n"); + printf("========================================\n"); + if (all_correct) { + printf("TEST PASSED\n"); + printf("All mute switches correctly set to %s\n", expected_state ? "ON" : "OFF"); + if (!path->mute_during_activation) { + printf("\nThis proves the fix works:\n"); + printf("- Path has mute_during_activation=false (default)\n"); + printf("- Switches were still set correctly\n"); + printf("- Bug would have left them in kernel default state\n"); + } + ret = 0; + } else { + printf("TEST FAILED\n"); + printf("One or more switches NOT set correctly\n"); + printf("Expected: %s, but some switches were %s\n", + expected_state ? "ON (unmuted)" : "OFF (muted)", + expected_state ? "OFF" : "ON"); + printf("\nThis indicates the bug is still present:\n"); + printf("- Switches not set when mute_during_activation=false\n"); + printf("- IEC958 fix may not be applied\n"); + ret = 1; + } + printf("========================================\n"); + +cleanup: + if (mixer) + snd_mixer_close(mixer); + if (path) + pa_alsa_path_free(path); + + return ret; +} From 5c3def51a429bdebbcbd862cdabe3eb2dc3ff9f1 Mon Sep 17 00:00:00 2001 From: Daniel Nouri Date: Sun, 5 Oct 2025 00:21:24 +0200 Subject: [PATCH 02/23] refactor: Remove redundant ret assignments in test-alsa-path-select The variable ret is initialized to 2 at function entry. Error paths that want this default value can simply goto cleanup without reassigning it. --- spa/plugins/alsa/acp/test-alsa-path-select.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/spa/plugins/alsa/acp/test-alsa-path-select.c b/spa/plugins/alsa/acp/test-alsa-path-select.c index f30662d50..e9bf4b3cc 100644 --- a/spa/plugins/alsa/acp/test-alsa-path-select.c +++ b/spa/plugins/alsa/acp/test-alsa-path-select.c @@ -274,7 +274,6 @@ int main(int argc, char *argv[]) if (mute_switch_count == 0) { fprintf(stderr, "Warning: No mute switches found in path\n"); printf("This test requires a path with switch=mute elements\n"); - ret = 2; goto cleanup; } @@ -285,7 +284,6 @@ int main(int argc, char *argv[]) err = pa_alsa_path_select(path, NULL, mixer, config.device_muted); if (err < 0) { fprintf(stderr, "Error: pa_alsa_path_select() failed with error %d\n", err); - ret = 2; goto cleanup; } printf("pa_alsa_path_select(): OK\n\n"); From 87d34335f33d4a8db8a359c5c55dcbdee3e02fbe Mon Sep 17 00:00:00 2001 From: Daniel Nouri Date: Sun, 5 Oct 2025 21:16:23 +0200 Subject: [PATCH 03/23] refactor: Remove test-alsa-path-select tool Not run by meson test and requires specific ALSA hardware. Fix verification already completed manually. --- spa/plugins/alsa/acp/meson.build | 9 - spa/plugins/alsa/acp/test-alsa-path-select.c | 346 ------------------- 2 files changed, 355 deletions(-) delete mode 100644 spa/plugins/alsa/acp/test-alsa-path-select.c diff --git a/spa/plugins/alsa/acp/meson.build b/spa/plugins/alsa/acp/meson.build index f5adfa193..0ec97e2b4 100644 --- a/spa/plugins/alsa/acp/meson.build +++ b/spa/plugins/alsa/acp/meson.build @@ -20,12 +20,3 @@ acp_lib = static_library( dependencies : [ spa_dep, alsa_dep, mathlib, ] ) acp_dep = declare_dependency(link_with: acp_lib) - -executable('test-alsa-path-select', - [ 'test-alsa-path-select.c' ], - c_args : acp_c_args, - include_directories : [configinc, includes_inc ], - dependencies : [ spa_dep, alsa_dep, mathlib, ], - link_with : [ acp_lib ], - install : false, -) diff --git a/spa/plugins/alsa/acp/test-alsa-path-select.c b/spa/plugins/alsa/acp/test-alsa-path-select.c deleted file mode 100644 index e9bf4b3cc..000000000 --- a/spa/plugins/alsa/acp/test-alsa-path-select.c +++ /dev/null @@ -1,346 +0,0 @@ -/* ALSA path select test tool */ -/* SPDX-FileCopyrightText: Copyright © 2025 Daniel Nouri */ -/* SPDX-License-Identifier: MIT */ - -/* - * Tests pa_alsa_path_select() to verify mute switches are correctly set - * based on device mute status, regardless of mute_during_activation setting. - * - * Regression test for HDMI/DisplayPort audio bug where IEC958 switches - * remain in kernel default state (OFF) because pa_alsa_path_select() only - * set switches when mute_during_activation=true, but no paths enable this. - * - * Test verification: - * Before fix: IEC958 switch stays OFF after path select → test FAILS - * After fix: IEC958 switch set correctly based on device state → test PASSES - * - * Usage: - * test-alsa-path-select -c [-p ] [-m] [-v] - * - * Examples: - * test-alsa-path-select -c 1 # IEC958 unmuted (expect ON) - * test-alsa-path-select -c 1 -m # IEC958 muted (expect OFF) - * test-alsa-path-select -c 0 -p analog-output # Regression test - * - * Exit codes: - * 0 = Test passed - * 1 = Test failed - * 2 = Setup error - */ - -#include -#include -#include -#include -#include -#include -#include - -#include "alsa-mixer.h" -#include "alsa-util.h" - -#define DEFAULT_PATHS_DIR "/usr/share/alsa-card-profile/mixer/paths" - -struct test_config { - int card; - const char *path_name; - const char *paths_dir; - bool device_muted; - bool verbose; -}; - -static void show_help(const char *name) -{ - printf("Usage: %s [options]\n", name); - printf("\n"); - printf("Test tool for pa_alsa_path_select() - verifies IEC958 unmute fix\n"); - printf("\n"); - printf("Options:\n"); - printf(" -c, --card ALSA card number (required)\n"); - printf(" -p, --path Mixer path name (default: iec958-stereo-output)\n"); - printf(" -d, --paths-dir Path to mixer path configs (default: %s)\n", DEFAULT_PATHS_DIR); - printf(" -m, --muted Test with device muted (default: unmuted)\n"); - printf(" -v, --verbose Verbose output\n"); - printf(" -h, --help Show this help\n"); - printf("\n"); - printf("Examples:\n"); - printf(" # Test IEC958 unmute on card 1\n"); - printf(" %s -c 1\n", name); - printf("\n"); - printf(" # Test analog output\n"); - printf(" %s -c 0 -p analog-output\n", name); - printf("\n"); - printf(" # Test with device muted\n"); - printf(" %s -c 1 -m\n", name); - printf("\n"); - printf("Exit codes:\n"); - printf(" 0 - Test passed (switches set correctly)\n"); - printf(" 1 - Test failed (switches not set as expected)\n"); - printf(" 2 - Setup error (path not found, hardware error, etc.)\n"); -} - -static int parse_args(int argc, char *argv[], struct test_config *config) -{ - static const struct option long_options[] = { - { "card", required_argument, NULL, 'c' }, - { "path", required_argument, NULL, 'p' }, - { "paths-dir", required_argument, NULL, 'd' }, - { "muted", no_argument, NULL, 'm' }, - { "verbose", no_argument, NULL, 'v' }, - { "help", no_argument, NULL, 'h' }, - { NULL, 0, NULL, 0 } - }; - - int c; - config->card = -1; - config->path_name = "iec958-stereo-output.conf"; - config->paths_dir = DEFAULT_PATHS_DIR; - config->device_muted = false; - config->verbose = false; - - while ((c = getopt_long(argc, argv, "c:p:d:mvh", long_options, NULL)) != -1) { - switch (c) { - case 'c': - config->card = atoi(optarg); - break; - case 'p': - config->path_name = optarg; - break; - case 'd': - config->paths_dir = optarg; - break; - case 'm': - config->device_muted = true; - break; - case 'v': - config->verbose = true; - break; - case 'h': - show_help(argv[0]); - return -1; - default: - show_help(argv[0]); - return -2; - } - } - - if (config->card < 0) { - fprintf(stderr, "Error: Card number is required (-c )\n\n"); - show_help(argv[0]); - return -2; - } - - return 0; -} - -static int get_switch_state(snd_mixer_t *mixer, const char *name, int index, bool *state) -{ - snd_mixer_elem_t *elem; - snd_mixer_selem_id_t *sid; - int val; - - snd_mixer_selem_id_alloca(&sid); - snd_mixer_selem_id_set_name(sid, name); - snd_mixer_selem_id_set_index(sid, index); - - elem = snd_mixer_find_selem(mixer, sid); - if (!elem) { - fprintf(stderr, "Warning: Element '%s',%d not found\n", name, index); - return -ENOENT; - } - - if (!snd_mixer_selem_has_playback_switch(elem)) { - fprintf(stderr, "Warning: Element '%s',%d has no playback switch\n", name, index); - return -EINVAL; - } - - if (snd_mixer_selem_get_playback_switch(elem, SND_MIXER_SCHN_MONO, &val) < 0) { - if (snd_mixer_selem_get_playback_switch(elem, SND_MIXER_SCHN_FRONT_LEFT, &val) < 0) { - fprintf(stderr, "Error: Cannot read switch state for '%s',%d\n", name, index); - return -EIO; - } - } - - *state = val ? true : false; - return 0; -} - -int main(int argc, char *argv[]) -{ - struct test_config config; - pa_alsa_path *path = NULL; - snd_mixer_t *mixer = NULL; - char card_name[64]; - int ret = 2; // Setup error by default - int err; - - /* Parse arguments */ - err = parse_args(argc, argv, &config); - if (err < 0) { - return err == -1 ? 0 : 2; // -1 is help, -2 is error - } - - printf("Testing pa_alsa_path_select() with:\n"); - printf(" Card: %d\n", config.card); - printf(" Path: %s\n", config.path_name); - printf(" Paths dir: %s\n", config.paths_dir); - printf(" Device muted: %s\n", config.device_muted ? "yes" : "no"); - printf("\n"); - - /* Load mixer path */ - path = pa_alsa_path_new(config.paths_dir, config.path_name, PA_ALSA_DIRECTION_OUTPUT); - if (!path) { - fprintf(stderr, "Error: Failed to load path '%s' from %s\n", - config.path_name, config.paths_dir); - fprintf(stderr, "Make sure the path file exists: %s/%s.conf\n", - config.paths_dir, config.path_name); - goto cleanup; - } - - /* Count elements BEFORE probing */ - int element_count_before = 0; - pa_alsa_element *e_temp; - PA_LLIST_FOREACH(e_temp, path->elements) { - element_count_before++; - } - printf("Elements in path BEFORE probing: %d\n", element_count_before); - - if (config.verbose && element_count_before > 0) { - printf("Loaded path: %s\n", path->name ? path->name : config.path_name); - printf(" mute_during_activation: %s\n", path->mute_during_activation ? "yes" : "no"); - printf(" Elements:\n"); - - PA_LLIST_FOREACH(e_temp, path->elements) { - const char *switch_use; - switch (e_temp->switch_use) { - case PA_ALSA_SWITCH_MUTE: switch_use = "mute"; break; - case PA_ALSA_SWITCH_ON: switch_use = "on"; break; - case PA_ALSA_SWITCH_OFF: switch_use = "off"; break; - default: switch_use = "ignore"; break; - } - printf(" - %s (index %d): switch=%s\n", - e_temp->alsa_id.name, e_temp->alsa_id.index, switch_use); - } - printf("\n"); - } - - /* Open ALSA mixer */ - snprintf(card_name, sizeof(card_name), "hw:%d", config.card); - - if (snd_mixer_open(&mixer, 0) < 0) { - fprintf(stderr, "Error: Failed to open mixer\n"); - goto cleanup; - } - - if (snd_mixer_attach(mixer, card_name) < 0) { - fprintf(stderr, "Error: Failed to attach mixer to %s\n", card_name); - fprintf(stderr, "Make sure card %d exists (check with: aplay -l)\n", config.card); - goto cleanup; - } - - if (snd_mixer_selem_register(mixer, NULL, NULL) < 0) { - fprintf(stderr, "Error: Failed to register mixer\n"); - goto cleanup; - } - - if (snd_mixer_load(mixer) < 0) { - fprintf(stderr, "Error: Failed to load mixer elements\n"); - goto cleanup; - } - - /* Probe the path to ensure elements are available */ - printf("Probing path...\n"); - if (pa_alsa_path_probe(path, NULL, mixer, false) < 0) { - fprintf(stderr, "Error: Path probe failed - required elements not found on card\n"); - goto cleanup; - } - printf("Path probe: OK\n\n"); - - /* Record switch states before activation */ - printf("Switch states BEFORE pa_alsa_path_select():\n"); - pa_alsa_element *e; - int mute_switch_count = 0; - PA_LLIST_FOREACH(e, path->elements) { - if (e->switch_use == PA_ALSA_SWITCH_MUTE) { - bool state; - mute_switch_count++; - if (get_switch_state(mixer, e->alsa_id.name, e->alsa_id.index, &state) == 0) { - printf(" %s,%d = %s\n", e->alsa_id.name, e->alsa_id.index, - state ? "ON" : "OFF"); - } - } - } - - if (mute_switch_count == 0) { - fprintf(stderr, "Warning: No mute switches found in path\n"); - printf("This test requires a path with switch=mute elements\n"); - goto cleanup; - } - - /* Call pa_alsa_path_select() - THE FUNCTION UNDER TEST */ - printf("\nCalling pa_alsa_path_select(path, NULL, mixer, device_is_muted=%s)...\n", - config.device_muted ? "true" : "false"); - - err = pa_alsa_path_select(path, NULL, mixer, config.device_muted); - if (err < 0) { - fprintf(stderr, "Error: pa_alsa_path_select() failed with error %d\n", err); - goto cleanup; - } - printf("pa_alsa_path_select(): OK\n\n"); - - /* Verify switch states after activation */ - printf("Switch states AFTER pa_alsa_path_select():\n"); - bool all_correct = true; - bool expected_state = !config.device_muted; // unmuted device = switches ON - - PA_LLIST_FOREACH(e, path->elements) { - if (e->switch_use == PA_ALSA_SWITCH_MUTE) { - bool actual_state; - if (get_switch_state(mixer, e->alsa_id.name, e->alsa_id.index, &actual_state) == 0) { - printf(" %s,%d = %s", e->alsa_id.name, e->alsa_id.index, - actual_state ? "ON" : "OFF"); - - if (actual_state == expected_state) { - printf(" ✓\n"); - } else { - printf(" ✗ (expected %s)\n", expected_state ? "ON" : "OFF"); - all_correct = false; - } - } - } - } - - /* Determine test result */ - printf("\n"); - printf("========================================\n"); - if (all_correct) { - printf("TEST PASSED\n"); - printf("All mute switches correctly set to %s\n", expected_state ? "ON" : "OFF"); - if (!path->mute_during_activation) { - printf("\nThis proves the fix works:\n"); - printf("- Path has mute_during_activation=false (default)\n"); - printf("- Switches were still set correctly\n"); - printf("- Bug would have left them in kernel default state\n"); - } - ret = 0; - } else { - printf("TEST FAILED\n"); - printf("One or more switches NOT set correctly\n"); - printf("Expected: %s, but some switches were %s\n", - expected_state ? "ON (unmuted)" : "OFF (muted)", - expected_state ? "OFF" : "ON"); - printf("\nThis indicates the bug is still present:\n"); - printf("- Switches not set when mute_during_activation=false\n"); - printf("- IEC958 fix may not be applied\n"); - ret = 1; - } - printf("========================================\n"); - -cleanup: - if (mixer) - snd_mixer_close(mixer); - if (path) - pa_alsa_path_free(path); - - return ret; -} From b7a2fcf27e34dff2806521ffe7cff5d20b401450 Mon Sep 17 00:00:00 2001 From: Daniel Nouri Date: Mon, 6 Oct 2025 08:17:06 +0200 Subject: [PATCH 04/23] alsa: Enable IEC958 switches on device activation IEC958 (S/PDIF, HDMI, DisplayPort) switches default to muted in ALSA drivers, causing no audio output on digital devices. While UCM configurations and mixer paths can handle IEC958 unmuting, several scenarios lack coverage: - Pro-audio profiles (bypass UCM and mixer paths by design) - Devices without UCM configurations - Devices with incomplete mixer path definitions - Cards with multiple HDMI/DP outputs (indexed switches) This ensures IEC958 switches are enabled during device activation and port changes. The implementation uses the device mixer when available, falls back to the card mixer for pro-audio profiles, and enables all IEC958 switches regardless of index. Safe for all configurations: the operation is idempotent and provides defense-in-depth even when UCM or mixer paths handle it correctly. Tested on AMD Rembrandt GPU with 3 HDMI outputs in pro-audio mode. --- spa/plugins/alsa/acp/acp.c | 57 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/spa/plugins/alsa/acp/acp.c b/spa/plugins/alsa/acp/acp.c index 7fc749a7e..276b6c553 100644 --- a/spa/plugins/alsa/acp/acp.c +++ b/spa/plugins/alsa/acp/acp.c @@ -1615,6 +1615,59 @@ static int device_disable(pa_card *impl, pa_alsa_mapping *mapping, pa_alsa_devic return 0; } +/* Synchronize IEC958 digital output/input switch states. + * + * IEC958 switches default to muted in ALSA drivers. Cards with multiple + * HDMI/DP outputs have indexed switches (IEC958,0 IEC958,1 etc). We enable + * all switches since we cannot reliably map device numbers to indices. + */ +static void sync_iec958_controls(pa_alsa_device *d) +{ + snd_mixer_t *mixer_handle; + snd_mixer_elem_t *elem; + pa_card *impl; + int r; + + mixer_handle = d->mixer_handle; + + /* Pro-audio profiles don't have per-device mixers, use card mixer */ + if (!mixer_handle) { + impl = d->card; + if (!impl || impl->card.index == ACP_INVALID_INDEX) + return; + + mixer_handle = pa_alsa_open_mixer(impl->ucm.mixers, impl->card.index, true); + if (!mixer_handle) + return; + } + + /* Enable all IEC958 switches */ + for (elem = snd_mixer_first_elem(mixer_handle); elem; + elem = snd_mixer_elem_next(elem)) { + + if (snd_mixer_elem_get_type(elem) != SND_MIXER_ELEM_SIMPLE) + continue; + + const char *name = snd_mixer_selem_get_name(elem); + if (!name || !pa_startswith(name, "IEC958")) + continue; + + if (snd_mixer_selem_has_playback_switch(elem)) { + r = snd_mixer_selem_set_playback_switch_all(elem, 1); + if (r < 0) + pa_log_warn("Failed to enable IEC958 playback switch: %s", + pa_alsa_strerror(r)); + } + + if (snd_mixer_selem_has_capture_switch(elem)) { + r = snd_mixer_selem_set_capture_switch_all(elem, 1); + if (r < 0) + pa_log_warn("Failed to enable IEC958 capture switch: %s", + pa_alsa_strerror(r)); + } + } +} + static int device_enable(pa_card *impl, pa_alsa_mapping *mapping, pa_alsa_device *dev) { const char *mod_name; @@ -1681,6 +1734,9 @@ static int device_enable(pa_card *impl, pa_alsa_mapping *mapping, pa_alsa_device } } + /* Enable IEC958 switches for digital outputs */ + sync_iec958_controls(dev); + return 0; } @@ -2179,6 +2235,7 @@ int acp_device_set_port(struct acp_device *dev, uint32_t port_index, uint32_t fl pa_sink_suspend(s, false, PA_SUSPEND_UNAVAILABLE); #endif } + sync_iec958_controls(d); if (impl->events && impl->events->port_changed) impl->events->port_changed(impl->user_data, old ? old->port.index : 0, p->port.index); From 1cd056aa1eb3a773e430c64882b7b38007dcb5b8 Mon Sep 17 00:00:00 2001 From: Daniel Nouri Date: Mon, 6 Oct 2025 09:06:01 +0200 Subject: [PATCH 05/23] revert: Remove mixer path IEC958 fix Reverts alsa-mixer.c changes from 32a3ffc74. The comprehensive ACP layer fix (bdb82be4e) handles all scenarios including pro-audio profiles that bypass mixer paths, making this approach redundant. --- spa/plugins/alsa/acp/alsa-mixer.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/spa/plugins/alsa/acp/alsa-mixer.c b/spa/plugins/alsa/acp/alsa-mixer.c index 64088177f..1f494ee0b 100644 --- a/spa/plugins/alsa/acp/alsa-mixer.c +++ b/spa/plugins/alsa/acp/alsa-mixer.c @@ -1479,23 +1479,13 @@ int pa_alsa_path_select(pa_alsa_path *p, pa_alsa_setting *s, snd_mixer_t *m, boo if (s) setting_select(s, m); - /* Set hw mute switches to match device mute status. - * - * When mute_during_activation is enabled, switches are temporarily muted - * during path setup to avoid pops/clicks, and this restores them. - * - * When mute_during_activation is disabled (default), switches may still be - * in their kernel default state (often muted for digital outputs like IEC958). - * We must explicitly unmute them if the device isn't muted. - * - * This ensures HDMI/DisplayPort audio (IEC958) works correctly, as these - * digital output switches default to muted in ALSA drivers but are not - * automatically enabled when the path is selected. - */ - PA_LLIST_FOREACH(e, p->elements) { - if (e->switch_use == PA_ALSA_SWITCH_MUTE) { - if (element_set_switch(e, m, !device_is_muted) < 0) - return -1; + /* Finally restore hw mute to the device mute status. */ + if (p->mute_during_activation) { + PA_LLIST_FOREACH(e, p->elements) { + if (e->switch_use == PA_ALSA_SWITCH_MUTE) { + if (element_set_switch(e, m, !device_is_muted) < 0) + return -1; + } } } From f7c3d379698e0c8b0eefec6cbab10a0b04f0bf8a Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Fri, 24 Oct 2025 12:46:38 +0200 Subject: [PATCH 06/23] fmt-ops: allocate shaper memory dynamically It is based on the number of channels so allocate it dynamically. --- spa/plugins/audioconvert/fmt-ops.c | 6 ++++-- spa/plugins/audioconvert/fmt-ops.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/spa/plugins/audioconvert/fmt-ops.c b/spa/plugins/audioconvert/fmt-ops.c index 3296c220b..3fc2c5f0a 100644 --- a/spa/plugins/audioconvert/fmt-ops.c +++ b/spa/plugins/audioconvert/fmt-ops.c @@ -551,7 +551,7 @@ int convert_init(struct convert *conv) const struct dither_info *dinfo; const struct noise_info *ninfo; const struct clear_info *cinfo; - uint32_t i, conv_flags, data_size[3]; + uint32_t i, conv_flags, data_size[4]; /* we generate int32 bits of random values. With this scale * factor, we bring this in the [-1.0, 1.0] range */ @@ -615,15 +615,17 @@ int convert_init(struct convert *conv) data_size[0] = SPA_ROUND_UP(conv->noise_size * sizeof(float), FMT_OPS_MAX_ALIGN); data_size[1] = SPA_ROUND_UP(RANDOM_SIZE * sizeof(uint32_t), FMT_OPS_MAX_ALIGN); data_size[2] = SPA_ROUND_UP(RANDOM_SIZE * sizeof(int32_t), FMT_OPS_MAX_ALIGN); + data_size[3] = SPA_ROUND_UP(conv->n_channels * sizeof(struct shaper), FMT_OPS_MAX_ALIGN); conv->data = calloc(FMT_OPS_MAX_ALIGN + - data_size[0] + data_size[1] + data_size[2], 1); + data_size[0] + data_size[1] + data_size[2] + data_size[3], 1); if (conv->data == NULL) return -errno; conv->noise = SPA_PTR_ALIGN(conv->data, FMT_OPS_MAX_ALIGN, float); conv->random = SPA_PTROFF(conv->noise, data_size[0], uint32_t); conv->prev = SPA_PTROFF(conv->random, data_size[1], int32_t); + conv->shaper = SPA_PTROFF(conv->prev, data_size[2], struct shaper); for (i = 0; i < RANDOM_SIZE; i++) conv->random[i] = random(); diff --git a/spa/plugins/audioconvert/fmt-ops.h b/spa/plugins/audioconvert/fmt-ops.h index 9f4655c22..f738e3858 100644 --- a/spa/plugins/audioconvert/fmt-ops.h +++ b/spa/plugins/audioconvert/fmt-ops.h @@ -236,7 +236,7 @@ struct convert { uint32_t noise_size; const float *ns; uint32_t n_ns; - struct shaper shaper[64]; + struct shaper *shaper; void (*update_noise) (struct convert *conv, float *noise, uint32_t n_samples); void (*process) (struct convert *conv, void * SPA_RESTRICT dst[], const void * SPA_RESTRICT src[], From c4244a6cf3e042af5c2c344ba0a52836ec45aac8 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Fri, 24 Oct 2025 17:00:11 +0200 Subject: [PATCH 07/23] string: use spa_strbuf instead of snprintf magic --- spa/plugins/alsa/acp/acp.c | 19 ++++++++------- spa/plugins/alsa/acp/channelmap.h | 5 +--- spa/plugins/alsa/alsa-acp-device.c | 9 +++++--- spa/plugins/alsa/alsa-pcm.c | 37 ++++++++++++++---------------- src/pipewire/settings.c | 19 +++++++-------- 5 files changed, 41 insertions(+), 48 deletions(-) diff --git a/spa/plugins/alsa/acp/acp.c b/spa/plugins/alsa/acp/acp.c index 364d71eb0..f6f03a5ff 100644 --- a/spa/plugins/alsa/acp/acp.c +++ b/spa/plugins/alsa/acp/acp.c @@ -245,7 +245,7 @@ static void init_device(pa_card *impl, pa_alsa_device *dev, pa_alsa_direction_t pa_proplist_update(dev->proplist, PA_UPDATE_REPLACE, m->input_proplist); } if (m->split) { - char pos[2048]; + char pos[PA_CHANNELS_MAX*8]; struct spa_strbuf b; int i; @@ -1142,8 +1142,9 @@ static int hdmi_eld_changed(snd_mixer_elem_t *melem, unsigned int mask) pa_proplist_unset(p->proplist, ACP_KEY_AUDIO_POSITION_DETECTED); } else { uint32_t positions[eld.lpcm_channels]; - char position[64]; - int i = 0, pos = 0; + char position[eld.lpcm_channels * 8]; + struct spa_strbuf b; + int i = 0; if (eld.speakers & 0x01) { positions[i++] = ACP_CHANNEL_FL; @@ -1172,16 +1173,14 @@ static int hdmi_eld_changed(snd_mixer_elem_t *melem, unsigned int mask) if (eld.speakers & 0x10) { positions[i++] = ACP_CHANNEL_RC; } - while (i < eld.lpcm_channels) positions[i++] = ACP_CHANNEL_UNKNOWN; - for (i = 0, pos = 0; i < eld.lpcm_channels; i++) { - pos += snprintf(&position[pos], sizeof(position) - pos, "%s,", channel_names[positions[i]]); - } - - /* Overwrite trailing , */ - position[pos - 1] = 0; + spa_strbuf_init(&b, position, sizeof(position)); + spa_strbuf_append(&b, "["); + for (i = 0; i < eld.lpcm_channels; i++) + spa_strbuf_append(&b, "%s%s", i ? "," : "", channel_names[positions[i]]); + spa_strbuf_append(&b, "]"); changed |= (old_position == NULL) || (!spa_streq(old_position, position)); pa_proplist_sets(p->proplist, ACP_KEY_AUDIO_POSITION_DETECTED, position); diff --git a/spa/plugins/alsa/acp/channelmap.h b/spa/plugins/alsa/acp/channelmap.h index 2d4b54444..bb8f3f5f3 100644 --- a/spa/plugins/alsa/acp/channelmap.h +++ b/spa/plugins/alsa/acp/channelmap.h @@ -451,7 +451,6 @@ static inline int pa_channel_map_equal(const pa_channel_map *a, const pa_channel static inline char* pa_channel_map_snprint(char *s, size_t l, const pa_channel_map *map) { unsigned channel; - bool first = true; char *e; if (!pa_channel_map_valid(map)) { pa_snprintf(s, l, "%s", _("(invalid)")); @@ -460,12 +459,10 @@ static inline char* pa_channel_map_snprint(char *s, size_t l, const pa_channel_m *(e = s) = 0; for (channel = 0; channel < map->channels && l > 1; channel++) { l -= pa_snprintf(e, l, "%s%s", - first ? "" : ",", + channel == 0 ? "" : ",", pa_channel_position_to_string(map->map[channel])); e = strchr(e, 0); - first = false; } - return s; } diff --git a/spa/plugins/alsa/alsa-acp-device.c b/spa/plugins/alsa/alsa-acp-device.c index 6ab41c8d2..44342a7a3 100644 --- a/spa/plugins/alsa/alsa-acp-device.c +++ b/spa/plugins/alsa/alsa-acp-device.c @@ -156,12 +156,13 @@ static int emit_node(struct impl *this, struct acp_device *dev) const struct acp_dict_item *it; uint32_t n_items, i; char device_name[128], path[210], channels[16], ch[12], routes[16]; - char card_index[16], card_name[64], *p; + char card_index[16], card_name[64]; char positions[MAX_CHANNELS * 12]; char codecs[512]; struct spa_device_object_info info; struct acp_card *card = this->card; const char *stream, *card_id, *bus; + struct spa_strbuf b; info = SPA_DEVICE_OBJECT_INFO_INIT(); info.type = SPA_TYPE_INTERFACE_Node; @@ -200,11 +201,13 @@ static int emit_node(struct impl *this, struct acp_device *dev) snprintf(channels, sizeof(channels), "%d", dev->format.channels); items[n_items++] = SPA_DICT_ITEM_INIT(SPA_KEY_AUDIO_CHANNELS, channels); - p = positions; + spa_strbuf_init(&b, positions, sizeof(positions)); + spa_strbuf_append(&b, "["); for (i = 0; i < dev->format.channels; i++) { - p += snprintf(p, 12, "%s%s", i == 0 ? "" : ",", + spa_strbuf_append(&b, "%s%s", i == 0 ? " " : ", ", acp_channel_str(ch, sizeof(ch), dev->format.map[i])); } + spa_strbuf_append(&b, " ]"); items[n_items++] = SPA_DICT_ITEM_INIT(SPA_KEY_AUDIO_POSITION, positions); if (dev->n_codecs > 0) { diff --git a/spa/plugins/alsa/alsa-pcm.c b/spa/plugins/alsa/alsa-pcm.c index 676214685..309b2d91d 100644 --- a/spa/plugins/alsa/alsa-pcm.c +++ b/spa/plugins/alsa/alsa-pcm.c @@ -240,36 +240,33 @@ static int alsa_set_param(struct state *state, const char *k, const char *s) static int position_to_string(struct channel_map *map, char *val, size_t len) { - uint32_t i, o = 0; + uint32_t i; char pos[8]; - int r; - o += snprintf(val, len, "[ "); + struct spa_strbuf b; + + spa_strbuf_init(&b, val, len); + spa_strbuf_append(&b, "["); for (i = 0; i < map->n_pos; i++) { - r = snprintf(val+o, len-o, "%s%s", i == 0 ? "" : ", ", + spa_strbuf_append(&b, "%s%s", i == 0 ? " " : ", ", spa_type_audio_channel_make_short_name(map->pos[i], pos, sizeof(pos), "UNK")); - if (r < 0 || o + r >= len) - return -ENOSPC; - o += r; } - if (len > o) - o += snprintf(val+o, len-o, " ]"); + if (spa_strbuf_append(&b, " ]") < 2) + return -ENOSPC; return 0; } static int uint32_array_to_string(uint32_t *vals, uint32_t n_vals, char *val, size_t len) { - uint32_t i, o = 0; - int r; - o += snprintf(val, len, "[ "); - for (i = 0; i < n_vals; i++) { - r = snprintf(val+o, len-o, "%s%d", i == 0 ? "" : ", ", vals[i]); - if (r < 0 || o + r >= len) - return -ENOSPC; - o += r; - } - if (len > o) - o += snprintf(val+o, len-o, " ]"); + uint32_t i; + struct spa_strbuf b; + + spa_strbuf_init(&b, val, len); + spa_strbuf_append(&b, "["); + for (i = 0; i < n_vals; i++) + spa_strbuf_append(&b, "%s%d", i == 0 ? " " : ", ", vals[i]); + if (spa_strbuf_append(&b, " ]") < 2) + return -ENOSPC; return 0; } diff --git a/src/pipewire/settings.c b/src/pipewire/settings.c index 597e686a9..74d4cfcb2 100644 --- a/src/pipewire/settings.c +++ b/src/pipewire/settings.c @@ -245,24 +245,21 @@ void pw_settings_init(struct pw_context *this) static void expose_settings(struct pw_context *context, struct pw_impl_metadata *metadata) { struct settings *s = &context->settings; - uint32_t i, o; - char rates[MAX_RATES*16] = ""; + uint32_t i; + char rates[MAX_RATES*16]; + struct spa_strbuf b; pw_impl_metadata_set_propertyf(metadata, PW_ID_CORE, "log.level", "", "%d", s->log_level); pw_impl_metadata_set_propertyf(metadata, PW_ID_CORE, "clock.rate", "", "%d", s->clock_rate); - for (i = 0, o = 0; i < s->n_clock_rates; i++) { - int r = snprintf(rates+o, sizeof(rates)-o, "%s%d", i == 0 ? "" : ", ", + + spa_strbuf_init(&b, rates, sizeof(rates)); + for (i = 0; i < s->n_clock_rates; i++) + spa_strbuf_append(&b, "%s%d", i == 0 ? "" : ", ", s->clock_rates[i]); - if (r < 0 || o + r >= (int)sizeof(rates)) { - snprintf(rates, sizeof(rates), "%d", s->clock_rate); - break; - } - o += r; - } if (s->n_clock_rates == 0) - snprintf(rates, sizeof(rates), "%d", s->clock_rate); + spa_strbuf_append(&b, "%d", s->clock_rate); pw_impl_metadata_set_propertyf(metadata, PW_ID_CORE, "clock.allowed-rates", "", "[ %s ]", rates); From c8d4de5e77a016d94824e4240c851b5d032c9946 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Fri, 24 Oct 2025 17:00:42 +0200 Subject: [PATCH 08/23] acp: bump max channels to 128 --- spa/plugins/alsa/acp/channelmap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spa/plugins/alsa/acp/channelmap.h b/spa/plugins/alsa/acp/channelmap.h index bb8f3f5f3..72e59fdc6 100644 --- a/spa/plugins/alsa/acp/channelmap.h +++ b/spa/plugins/alsa/acp/channelmap.h @@ -27,7 +27,7 @@ extern "C" { #endif -#define PA_CHANNELS_MAX 64 +#define PA_CHANNELS_MAX 128 #define PA_CHANNEL_MAP_SNPRINT_MAX (PA_CHANNELS_MAX * 32) From 88c65932d8d8a002ae9e2d42ca2a25b06a804a19 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Fri, 24 Oct 2025 17:16:03 +0200 Subject: [PATCH 09/23] acp: use global max channels if defined --- spa/plugins/alsa/acp/channelmap.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/spa/plugins/alsa/acp/channelmap.h b/spa/plugins/alsa/acp/channelmap.h index 72e59fdc6..2b13d2528 100644 --- a/spa/plugins/alsa/acp/channelmap.h +++ b/spa/plugins/alsa/acp/channelmap.h @@ -27,7 +27,11 @@ extern "C" { #endif -#define PA_CHANNELS_MAX 128 +#ifdef SPA_AUDIO_MAX_CHANNELS +#define PA_CHANNELS_MAX ((int)SPA_AUDIO_MAX_CHANNELS) +#else +#define PA_CHANNELS_MAX 64 +#endif #define PA_CHANNEL_MAP_SNPRINT_MAX (PA_CHANNELS_MAX * 32) From 9f1a149876c56ec07ae74b10e9c4ef8a97084bf7 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Wed, 22 Oct 2025 19:32:22 +0300 Subject: [PATCH 10/23] ci: add file package, for coverity Try to fix coverity by adding missing 'file' package to container --- .gitlab-ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 9cfde68d0..448bfa06e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -38,7 +38,7 @@ include: .fedora: variables: # Update this tag when you want to trigger a rebuild - FDO_DISTRIBUTION_TAG: '2025-10-15.0' + FDO_DISTRIBUTION_TAG: '2025-10-22.0' FDO_DISTRIBUTION_VERSION: '42' FDO_DISTRIBUTION_PACKAGES: >- alsa-lib-devel @@ -48,6 +48,7 @@ include: dbus-devel doxygen fdk-aac-free-devel + file findutils gcc gcc-c++ From 93495d3a754bc7492cfb27fd551180f0be9b6932 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sun, 26 Oct 2025 15:38:17 +0200 Subject: [PATCH 11/23] spa: param: infer raw audio channels from position if unset The behavior before b8eeb2db45adfb was that spa_audio_info_raw_update() always sets audio.channels when audio.position is updated. The new behavior does not set audio.channels when parsing audio.position. This breaks things e.g. when combine-sink and loopback nodes are created with only audio.position specified. Restore the previous behavior. --- spa/include/spa/param/audio/raw-json.h | 1 + 1 file changed, 1 insertion(+) diff --git a/spa/include/spa/param/audio/raw-json.h b/spa/include/spa/param/audio/raw-json.h index c33a9310f..e83c4495b 100644 --- a/spa/include/spa/param/audio/raw-json.h +++ b/spa/include/spa/param/audio/raw-json.h @@ -78,6 +78,7 @@ spa_audio_info_raw_ext_update(struct spa_audio_info_raw *info, size_t size, max_position, &v) > 0) { if (v > max_position) return -ECHRNG; + info->channels = v; SPA_FLAG_CLEAR(info->flags, SPA_AUDIO_FLAG_UNPOSITIONED); } } From 3febf09b85c23a6cf69fbba1d250b85820abd76a Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 25 Oct 2025 13:22:32 +0300 Subject: [PATCH 12/23] alsa: fix typoed braces in condition + assign --- spa/plugins/alsa/alsa-pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spa/plugins/alsa/alsa-pcm.c b/spa/plugins/alsa/alsa-pcm.c index 309b2d91d..4f2fde8b6 100644 --- a/spa/plugins/alsa/alsa-pcm.c +++ b/spa/plugins/alsa/alsa-pcm.c @@ -773,7 +773,7 @@ static void bind_ctl_event(struct spa_source *source) snd_ctl_elem_id_alloca(&bound_id); snd_ctl_elem_value_alloca(&old_value); - while ((err = snd_ctl_read(state->ctl, ev) > 0)) { + while ((err = snd_ctl_read(state->ctl, ev)) > 0) { bool changed = false; if (snd_ctl_event_get_type(ev) != SND_CTL_EVENT_ELEM) From fe2c62b9b163ff824647cf7c0ec9d8369f836a0e Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 25 Oct 2025 13:59:47 +0300 Subject: [PATCH 13/23] meson.build: set project cc flags also for native builds Use the build flags also for all native build targets. Avoids spurious warnings in spa-json-dump --- meson.build | 4 ++-- spa/plugins/audioconvert/meson.build | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/meson.build b/meson.build index e81ce8214..e82ba03b3 100644 --- a/meson.build +++ b/meson.build @@ -118,8 +118,8 @@ cc_flags = common_flags + [ '-DSPA_AUDIO_MAX_CHANNELS=128u', ] add_project_arguments(cc.get_supported_arguments(cc_flags), language: 'c') - -cc_flags_native = cc_native.get_supported_arguments(cc_flags) +add_project_arguments(cc_native.get_supported_arguments(cc_flags), + language: 'c', native: true) have_cpp = add_languages('cpp', native: false, required : false) diff --git a/spa/plugins/audioconvert/meson.build b/spa/plugins/audioconvert/meson.build index 394bc11eb..64379c845 100644 --- a/spa/plugins/audioconvert/meson.build +++ b/spa/plugins/audioconvert/meson.build @@ -125,7 +125,7 @@ sparesampledumpcoeffs_sources = [ sparesampledumpcoeffs = executable( 'spa-resample-dump-coeffs', sparesampledumpcoeffs_sources, - c_args : [ cc_flags_native, '-DRESAMPLE_DISABLE_PRECOMP' ], + c_args : [ '-DRESAMPLE_DISABLE_PRECOMP' ], dependencies : [ spa_dep, mathlib_native ], install : false, native : true, From b0e308e0dce9c78a3ee71eab0cd9a2a8f959d47b Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 25 Oct 2025 14:27:10 +0300 Subject: [PATCH 14/23] spa: examples: fix getopt usage + typos in adapter-control --- spa/examples/adapter-control.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spa/examples/adapter-control.c b/spa/examples/adapter-control.c index 6aa36dfaa..0a48f2fd4 100644 --- a/spa/examples/adapter-control.c +++ b/spa/examples/adapter-control.c @@ -578,7 +578,7 @@ static int make_nodes(struct data *data) SPA_PARAM_PORT_CONFIG_direction, SPA_POD_Id(SPA_DIRECTION_OUTPUT), SPA_PARAM_PORT_CONFIG_mode, SPA_POD_Id(SPA_PARAM_PORT_CONFIG_MODE_dsp), SPA_PARAM_PORT_CONFIG_format, SPA_POD_Pod(param)); - if ((res = spa_node_set_param(data->source_node, SPA_PARAM_PortConfig, 0, param) < 0)) { + if ((res = spa_node_set_param(data->source_node, SPA_PARAM_PortConfig, 0, param)) < 0) { printf("can't setup source node %d\n", res); return res; } @@ -647,7 +647,7 @@ static int make_nodes(struct data *data) SPA_PARAM_PORT_CONFIG_format, SPA_POD_Pod(param)); - if ((res = spa_node_set_param(data->sink_node, SPA_PARAM_PortConfig, 0, param) < 0)) { + if ((res = spa_node_set_param(data->sink_node, SPA_PARAM_PortConfig, 0, param)) < 0) { printf("can't setup sink node %d\n", res); return res; } @@ -987,7 +987,7 @@ int main(int argc, char *argv[]) setlocale(LC_ALL, ""); - while ((c = getopt_long(argc, argv, "hdmstiac:", long_options, NULL)) != -1) { + while ((c = getopt_long(argc, argv, "hd:m:s:t:i:a:c:", long_options, NULL)) != -1) { switch (c) { case 'h': show_help(&data, argv[0], false); From 68dc45cc62a6c79caca5e711faac2727e3b49154 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 25 Oct 2025 13:49:46 +0300 Subject: [PATCH 15/23] audioconvert: simplify volume ramp generation Don't use floating point accumulators, interpolate from sample position. --- spa/plugins/audioconvert/audioconvert.c | 82 ++++++++----------------- 1 file changed, 27 insertions(+), 55 deletions(-) diff --git a/spa/plugins/audioconvert/audioconvert.c b/spa/plugins/audioconvert/audioconvert.c index e8c95b44a..f201522f8 100644 --- a/spa/plugins/audioconvert/audioconvert.c +++ b/spa/plugins/audioconvert/audioconvert.c @@ -1538,8 +1538,6 @@ static int get_ramp_samples(struct impl *this, struct volume_ramp_params *vrp) samples = (vrp->volume_ramp_time * vrp->rate) / 1000; spa_log_info(this->log, "volume ramp samples calculated from time is %d", samples); } - if (!samples) - samples = -1; return samples; } @@ -1550,12 +1548,10 @@ static int get_ramp_step_samples(struct impl *this, struct volume_ramp_params *v if (vrp->volume_ramp_step_samples) samples = vrp->volume_ramp_step_samples; else if (vrp->volume_ramp_step_time) { - /* convert the step time which is in nano seconds to seconds */ - samples = (vrp->volume_ramp_step_time/1000) * (vrp->rate/1000); + /* convert the step time which is in nano seconds to seconds, round up */ + samples = SPA_MAX(1u, vrp->volume_ramp_step_time/1000) * (vrp->rate/1000); spa_log_debug(this->log, "volume ramp step samples calculated from time is %d", samples); } - if (!samples) - samples = -1; return samples; } @@ -1568,76 +1564,52 @@ static float get_volume_at_scale(struct volume_ramp_params *vrp, float value) return 0.0; } -static struct spa_pod *generate_ramp_up_seq(struct impl *this, struct volume_ramp_params *vrp, +static struct spa_pod *generate_ramp_seq(struct impl *this, struct volume_ramp_params *vrp, void *buffer, size_t size) { struct spa_pod_dynamic_builder b; struct spa_pod_frame f[1]; - float start = vrp->start, end = vrp->end, volume_accum = start; - int ramp_samples = get_ramp_samples(this, vrp); - int ramp_step_samples = get_ramp_step_samples(this, vrp); - float volume_step = ((end - start) / (ramp_samples / ramp_step_samples)); - uint32_t volume_offs = 0; + float start = vrp->start, end = vrp->end; + int samples = get_ramp_samples(this, vrp); + int step = get_ramp_step_samples(this, vrp); + int offs = 0; + + if (samples < 0 || step < 0 || (samples > 0 && step == 0)) + return NULL; spa_pod_dynamic_builder_init(&b, buffer, size, 4096); spa_pod_builder_push_sequence(&b.b, &f[0], 0); - spa_log_info(this->log, "generating ramp up sequence from %f to %f with a" - " step value %f at scale %d", start, end, volume_step, vrp->scale); - do { - float vas = get_volume_at_scale(vrp, volume_accum); - spa_log_trace(this->log, "volume accum %f", vas); - spa_pod_builder_control(&b.b, volume_offs, SPA_CONTROL_Properties); - spa_pod_builder_add_object(&b.b, - SPA_TYPE_OBJECT_Props, 0, - SPA_PROP_volume, SPA_POD_Float(vas)); - volume_accum += volume_step; - volume_offs += ramp_step_samples; - } while (volume_accum < end); - return spa_pod_builder_pop(&b.b, &f[0]); -} + spa_log_info(this->log, "generating ramp sequence from %f to %f with " + "step %d/%d at scale %d", start, end, step, samples, vrp->scale); -static struct spa_pod *generate_ramp_down_seq(struct impl *this, struct volume_ramp_params *vrp, - void *buffer, size_t size) -{ - struct spa_pod_dynamic_builder b; - struct spa_pod_frame f[1]; - int ramp_samples = get_ramp_samples(this, vrp); - int ramp_step_samples = get_ramp_step_samples(this, vrp); - float start = vrp->start, end = vrp->end, volume_accum = start; - float volume_step = ((start - end) / (ramp_samples / ramp_step_samples)); - uint32_t volume_offs = 0; + while (1) { + float pos = (samples == 0) ? end : + SPA_CLAMP(start + (end - start) * offs / samples, + SPA_MIN(start, end), SPA_MAX(start, end)); + float vas = get_volume_at_scale(vrp, pos); - spa_pod_dynamic_builder_init(&b, buffer, size, 4096); - - spa_pod_builder_push_sequence(&b.b, &f[0], 0); - spa_log_info(this->log, "generating ramp down sequence from %f to %f with a" - " step value %f at scale %d", start, end, volume_step, vrp->scale); - do { - float vas = get_volume_at_scale(vrp, volume_accum); - spa_log_trace(this->log, "volume accum %f", vas); - spa_pod_builder_control(&b.b, volume_offs, SPA_CONTROL_Properties); + spa_log_trace(this->log, "volume %d accum %f", offs, vas); + spa_pod_builder_control(&b.b, offs, SPA_CONTROL_Properties); spa_pod_builder_add_object(&b.b, SPA_TYPE_OBJECT_Props, 0, SPA_PROP_volume, SPA_POD_Float(vas)); - volume_accum -= volume_step; - volume_offs += ramp_step_samples; - } while (volume_accum > end); + if (offs >= samples) + break; + + offs = SPA_MIN(samples, offs + step); + } + return spa_pod_builder_pop(&b.b, &f[0]); } static void generate_volume_ramp(struct impl *this, struct volume_ramp_params *vrp, void *buffer, size_t size) { - void *sequence = NULL; - if (vrp->start == vrp->end) - spa_log_error(this->log, "no change in volume, cannot ramp volume"); - else if (vrp->end > vrp->start) - sequence = generate_ramp_up_seq(this, vrp, buffer, size); - else - sequence = generate_ramp_down_seq(this, vrp, buffer, size); + void *sequence; + sequence = generate_ramp_seq(this, vrp, buffer, size); if (!sequence) spa_log_error(this->log, "unable to generate sequence"); From 3d08c0557f99d21c7fba9dd532ee14d683e440e2 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 25 Oct 2025 15:14:42 +0300 Subject: [PATCH 16/23] properties: fix assign + conditional expression --- src/pipewire/properties.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pipewire/properties.c b/src/pipewire/properties.c index de81088b2..ac0aac0d0 100644 --- a/src/pipewire/properties.c +++ b/src/pipewire/properties.c @@ -252,7 +252,8 @@ static int update_string(struct pw_properties *props, const char *str, size_t si continue; } /* item changed or added, apply changes later */ - if ((errno = -add_item(&changes, key, false, val, true) < 0)) { + if ((res = add_item(&changes, key, false, val, true)) < 0) { + errno = -res; it[0].state = SPA_JSON_ERROR_FLAG; break; } From 8a23b13798e52ea39dc2eb92dd134bae95d6c6a4 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sun, 26 Oct 2025 17:44:03 +0200 Subject: [PATCH 17/23] spa: param: pass correct struct size to spa_format_audio_raw_ext_parse/build --- spa/include/spa/param/audio/format-utils.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spa/include/spa/param/audio/format-utils.h b/spa/include/spa/param/audio/format-utils.h index 07f5d94e3..86ea84f75 100644 --- a/spa/include/spa/param/audio/format-utils.h +++ b/spa/include/spa/param/audio/format-utils.h @@ -59,7 +59,8 @@ spa_format_audio_ext_parse(const struct spa_pod *format, struct spa_audio_info * switch (info->media_subtype) { case SPA_MEDIA_SUBTYPE_raw: - return spa_format_audio_raw_ext_parse(format, &info->info.raw, size); + return spa_format_audio_raw_ext_parse(format, &info->info.raw, + size - offsetof(struct spa_audio_info, info.raw)); case SPA_MEDIA_SUBTYPE_dsp: return spa_format_audio_dsp_parse(format, &info->info.dsp); case SPA_MEDIA_SUBTYPE_iec958: @@ -110,7 +111,8 @@ spa_format_audio_ext_build(struct spa_pod_builder *builder, uint32_t id, { switch (info->media_subtype) { case SPA_MEDIA_SUBTYPE_raw: - return spa_format_audio_raw_ext_build(builder, id, &info->info.raw, size); + return spa_format_audio_raw_ext_build(builder, id, &info->info.raw, + size - offsetof(struct spa_audio_info, info.raw)); case SPA_MEDIA_SUBTYPE_dsp: return spa_format_audio_dsp_build(builder, id, &info->info.dsp); case SPA_MEDIA_SUBTYPE_iec958: From c6d0b364ab0657a66687a761b602100f823b8957 Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sun, 26 Oct 2025 17:51:01 +0200 Subject: [PATCH 18/23] spa: param: add size checks for spa_audio_info* structs In the API that take struct size for spa_audio_info*, also check the struct size. --- spa/include/spa/param/audio/format-utils.h | 51 +++++++- spa/include/spa/param/audio/format.h | 2 + spa/include/spa/param/audio/raw-json.h | 7 ++ spa/include/spa/param/audio/raw-utils.h | 8 ++ spa/include/spa/param/audio/raw.h | 2 + test/meson.build | 1 + test/test-spa-format.c | 129 +++++++++++++++++++++ 7 files changed, 197 insertions(+), 3 deletions(-) create mode 100644 test/test-spa-format.c diff --git a/spa/include/spa/param/audio/format-utils.h b/spa/include/spa/param/audio/format-utils.h index 86ea84f75..24d06dd1e 100644 --- a/spa/include/spa/param/audio/format-utils.h +++ b/spa/include/spa/param/audio/format-utils.h @@ -46,18 +46,58 @@ extern "C" { #endif #endif +SPA_API_AUDIO_FORMAT_UTILS bool +spa_format_audio_ext_valid_size(uint32_t media_subtype, size_t size) +{ + switch (media_subtype) { + case SPA_MEDIA_SUBTYPE_raw: + return size >= offsetof(struct spa_audio_info, info.raw) && + SPA_AUDIO_INFO_RAW_VALID_SIZE(size - offsetof(struct spa_audio_info, info.raw)); + +#define _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(format) \ + case SPA_MEDIA_SUBTYPE_ ## format: \ + return size >= offsetof(struct spa_audio_info, info.format) + sizeof(struct spa_audio_info_ ## format); + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(dsp) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(iec958) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(dsd) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(mp3) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(aac) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(vorbis) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(wma) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(ra) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(amr) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(alac) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(flac) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(ape) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(ac3) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(eac3) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(truehd) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(dts) + _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE(mpegh) +#undef _SPA_FORMAT_AUDIO_EXT_VALID_SIZE_CASE + } + return false; +} + SPA_API_AUDIO_FORMAT_UTILS int spa_format_audio_ext_parse(const struct spa_pod *format, struct spa_audio_info *info, size_t size) { int res; + uint32_t media_type, media_subtype; - if ((res = spa_format_parse(format, &info->media_type, &info->media_subtype)) < 0) + if ((res = spa_format_parse(format, &media_type, &media_subtype)) < 0) return res; - if (info->media_type != SPA_MEDIA_TYPE_audio) + if (media_type != SPA_MEDIA_TYPE_audio) return -EINVAL; - switch (info->media_subtype) { + if (!spa_format_audio_ext_valid_size(media_subtype, size)) + return -EINVAL; + + info->media_type = media_type; + info->media_subtype = media_subtype; + + switch (media_subtype) { case SPA_MEDIA_SUBTYPE_raw: return spa_format_audio_raw_ext_parse(format, &info->info.raw, size - offsetof(struct spa_audio_info, info.raw)); @@ -109,6 +149,11 @@ SPA_API_AUDIO_FORMAT_UTILS struct spa_pod * spa_format_audio_ext_build(struct spa_pod_builder *builder, uint32_t id, const struct spa_audio_info *info, size_t size) { + if (!spa_format_audio_ext_valid_size(info->media_subtype, size)) { + errno = EINVAL; + return NULL; + } + switch (info->media_subtype) { case SPA_MEDIA_SUBTYPE_raw: return spa_format_audio_raw_ext_build(builder, id, &info->info.raw, diff --git a/spa/include/spa/param/audio/format.h b/spa/include/spa/param/audio/format.h index ac9b10dda..6e7a71f6c 100644 --- a/spa/include/spa/param/audio/format.h +++ b/spa/include/spa/param/audio/format.h @@ -59,6 +59,8 @@ struct spa_audio_info { struct spa_audio_info_dts dts; struct spa_audio_info_mpegh mpegh; } info; + + /* padding follows here when info has flexible size */ }; /** diff --git a/spa/include/spa/param/audio/raw-json.h b/spa/include/spa/param/audio/raw-json.h index e83c4495b..38fcb449c 100644 --- a/spa/include/spa/param/audio/raw-json.h +++ b/spa/include/spa/param/audio/raw-json.h @@ -60,6 +60,10 @@ spa_audio_info_raw_ext_update(struct spa_audio_info_raw *info, size_t size, { uint32_t v; uint32_t max_position = SPA_AUDIO_INFO_RAW_MAX_POSITION(size); + + if (!SPA_AUDIO_INFO_RAW_VALID_SIZE(size)) + return -EINVAL; + if (spa_streq(key, SPA_KEY_AUDIO_FORMAT)) { if (force || info->format == 0) info->format = (enum spa_audio_format)spa_type_audio_format_from_short_name(val); @@ -100,6 +104,9 @@ spa_audio_info_raw_ext_init_dict_keys_va(struct spa_audio_info_raw *info, size_t { int res; + if (!SPA_AUDIO_INFO_RAW_VALID_SIZE(size)) + return -EINVAL; + memset(info, 0, size); SPA_FLAG_SET(info->flags, SPA_AUDIO_FLAG_UNPOSITIONED); if (dict) { diff --git a/spa/include/spa/param/audio/raw-utils.h b/spa/include/spa/param/audio/raw-utils.h index ce5e61e67..3bb94eaa3 100644 --- a/spa/include/spa/param/audio/raw-utils.h +++ b/spa/include/spa/param/audio/raw-utils.h @@ -35,6 +35,9 @@ spa_format_audio_raw_ext_parse(const struct spa_pod *format, struct spa_audio_in int res; uint32_t max_position = SPA_AUDIO_INFO_RAW_MAX_POSITION(size); + if (!SPA_AUDIO_INFO_RAW_VALID_SIZE(size)) + return -EINVAL; + info->flags = 0; res = spa_pod_parse_object(format, SPA_TYPE_OBJECT_Format, NULL, @@ -64,6 +67,11 @@ spa_format_audio_raw_ext_build(struct spa_pod_builder *builder, uint32_t id, struct spa_pod_frame f; uint32_t max_position = SPA_AUDIO_INFO_RAW_MAX_POSITION(size); + if (!SPA_AUDIO_INFO_RAW_VALID_SIZE(size)) { + errno = EINVAL; + return NULL; + } + spa_pod_builder_push_object(builder, &f, SPA_TYPE_OBJECT_Format, id); spa_pod_builder_add(builder, SPA_FORMAT_mediaType, SPA_POD_Id(SPA_MEDIA_TYPE_audio), diff --git a/spa/include/spa/param/audio/raw.h b/spa/include/spa/param/audio/raw.h index 392b65daa..bcc0a122d 100644 --- a/spa/include/spa/param/audio/raw.h +++ b/spa/include/spa/param/audio/raw.h @@ -293,6 +293,8 @@ struct spa_audio_info_raw { #define SPA_AUDIO_INFO_RAW_MAX_POSITION(size) (((size)-offsetof(struct spa_audio_info_raw,position))/sizeof(uint32_t)) +#define SPA_AUDIO_INFO_RAW_VALID_SIZE(size) ((size) >= offsetof(struct spa_audio_info_raw, position)) + #define SPA_KEY_AUDIO_FORMAT "audio.format" /**< an audio format as string, * Ex. "S16LE" */ diff --git a/test/meson.build b/test/meson.build index ad658bcc6..5e38db383 100644 --- a/test/meson.build +++ b/test/meson.build @@ -112,6 +112,7 @@ test('test-spa', executable('test-spa', 'test-spa-buffer.c', 'test-spa-control.c', + 'test-spa-format.c', 'test-spa-json.c', 'test-spa-utils.c', 'test-spa-log.c', diff --git a/test/test-spa-format.c b/test/test-spa-format.c new file mode 100644 index 000000000..7cbec7690 --- /dev/null +++ b/test/test-spa-format.c @@ -0,0 +1,129 @@ +/* Simple Plugin API */ +/* SPDX-FileCopyrightText: Copyright © 2025 Pauli Virtanen */ +/* SPDX-License-Identifier: MIT */ + +#include +#include + +#include "pwtest.h" + +PWTEST(audio_format_sizes) +{ + union { + uint8_t buf[1024]; + struct spa_audio_info align; + } data; + struct spa_audio_info info; + size_t i; + + memset(&info, 0xf3, sizeof(info)); + info.media_type = SPA_MEDIA_TYPE_audio; + info.media_subtype = SPA_MEDIA_SUBTYPE_raw; + info.info.raw.channels = 5; + info.info.raw.format = SPA_AUDIO_FORMAT_F32P; + info.info.raw.rate = 12345; + info.info.raw.flags = 0; + info.info.raw.position[0] = 1; + info.info.raw.position[1] = 2; + info.info.raw.position[2] = 3; + info.info.raw.position[3] = 4; + info.info.raw.position[4] = 5; + + for (i = 0; i < sizeof(data.buf); ++i) { + struct spa_pod *pod; + uint8_t buf[4096]; + struct spa_pod_builder b; + + spa_pod_builder_init(&b, buf, sizeof(buf)); + memcpy(data.buf, &info, sizeof(info)); + + pod = spa_format_audio_ext_build(&b, 123, (void *)data.buf, i); + if (i < offsetof(struct spa_audio_info, info.raw) + + offsetof(struct spa_audio_info_raw, position)) + pwtest_bool_true(!pod); + else + pwtest_bool_true(pod); + } + + for (i = 0; i < sizeof(data.buf); ++i) { + struct spa_pod *pod; + uint8_t buf[4096]; + struct spa_pod_builder b; + int ret; + + spa_pod_builder_init(&b, buf, sizeof(buf)); + pod = spa_format_audio_ext_build(&b, 123, &info, sizeof(info)); + pwtest_bool_true(pod); + + memset(data.buf, 0xf3, sizeof(data.buf)); + + ret = spa_format_audio_ext_parse(pod, (void *)data.buf, i); + if (i < offsetof(struct spa_audio_info, info.raw) + + offsetof(struct spa_audio_info_raw, position) + + info.info.raw.channels*sizeof(uint32_t)) { + for (size_t j = i; j < sizeof(data.buf); ++j) + pwtest_int_eq(data.buf[j], 0xf3); + pwtest_int_lt(ret, 0); + } else { + pwtest_int_ge(ret, 0); + pwtest_bool_true(memcmp(data.buf, &info, SPA_MIN(i, sizeof(info))) == 0); + } + } + + memset(&info, 0xf3, sizeof(info)); + info.media_type = SPA_MEDIA_TYPE_audio; + info.media_subtype = SPA_MEDIA_SUBTYPE_aac; + info.info.aac.rate = 12345; + info.info.aac.channels = 6; + info.info.aac.bitrate = 54321; + info.info.aac.stream_format = SPA_AUDIO_AAC_STREAM_FORMAT_MP4LATM; + + for (i = 0; i < sizeof(data.buf); ++i) { + struct spa_pod *pod; + uint8_t buf[4096]; + struct spa_pod_builder b; + + spa_pod_builder_init(&b, buf, sizeof(buf)); + memcpy(data.buf, &info, sizeof(info)); + + pod = spa_format_audio_ext_build(&b, 123, (void *)data.buf, i); + if (i < offsetof(struct spa_audio_info, info.raw) + + sizeof(struct spa_audio_info_aac)) + pwtest_bool_true(!pod); + else + pwtest_bool_true(pod); + } + + for (i = 0; i < sizeof(data.buf); ++i) { + struct spa_pod *pod; + uint8_t buf[4096]; + struct spa_pod_builder b; + int ret; + + spa_pod_builder_init(&b, buf, sizeof(buf)); + pod = spa_format_audio_ext_build(&b, 123, &info, sizeof(info)); + pwtest_bool_true(pod); + + memset(data.buf, 0xf3, sizeof(data.buf)); + + ret = spa_format_audio_ext_parse(pod, (void *)data.buf, i); + if (i < offsetof(struct spa_audio_info, info.raw) + + sizeof(struct spa_audio_info_aac)) { + for (size_t j = i; j < sizeof(data.buf); ++j) + pwtest_int_eq(data.buf[j], 0xf3); + pwtest_int_lt(ret, 0); + } else { + pwtest_int_ge(ret, 0); + pwtest_bool_true(memcmp(data.buf, &info, SPA_MIN(i, sizeof(info))) == 0); + } + } + + return PWTEST_PASS; +} + +PWTEST_SUITE(spa_format) +{ + pwtest_add(audio_format_sizes, PWTEST_NOARG); + + return PWTEST_PASS; +} From 614186a59076e8c857eaf7d94702b20e5f5030e9 Mon Sep 17 00:00:00 2001 From: Jonas Holmberg Date: Wed, 22 Oct 2025 13:50:24 +0200 Subject: [PATCH 19/23] module-echo-cancel: Sync capture and sink buffers Call process() when capture and sink ringbuffers contain data from the same graph cycle and only process the latest block from them to avoid adding latency that can accumulate if one of the streams gets more than one buffer before the other gets its first buffer when starting up. --- src/modules/module-echo-cancel.c | 92 +++++++++++++++++++++++++------- 1 file changed, 74 insertions(+), 18 deletions(-) diff --git a/src/modules/module-echo-cancel.c b/src/modules/module-echo-cancel.c index 997506e6f..037509c35 100644 --- a/src/modules/module-echo-cancel.c +++ b/src/modules/module-echo-cancel.c @@ -229,8 +229,10 @@ struct impl { struct spa_audio_aec *aec; uint32_t aec_blocksize; - unsigned int capture_ready:1; - unsigned int sink_ready:1; + struct spa_io_position *capture_position; + struct spa_io_position *sink_position; + uint32_t capture_cycle; + uint32_t sink_cycle; unsigned int do_disconnect:1; @@ -309,11 +311,17 @@ static void process(struct impl *impl) struct spa_data *dd; uint32_t i, size; uint32_t rindex, pindex, oindex, pdindex, avail; + int32_t pavail, pdavail; size = impl->aec_blocksize; - /* First read a block from the playback and capture ring buffers */ - spa_ringbuffer_get_read_index(&impl->rec_ring, &rindex); + /* First read a block from the capture ring buffer */ + avail = spa_ringbuffer_get_read_index(&impl->rec_ring, &rindex); + while (avail > size) { + /* drop samples from previous graph cycles */ + spa_ringbuffer_read_update(&impl->rec_ring, rindex + size); + avail = spa_ringbuffer_get_read_index(&impl->rec_ring, &rindex); + } for (i = 0; i < impl->rec_info.channels; i++) { /* captured samples, with echo from sink */ @@ -331,19 +339,30 @@ static void process(struct impl *impl) out[i] = &out_buf[i][0]; } - spa_ringbuffer_get_read_index(&impl->play_ring, &pindex); - spa_ringbuffer_get_read_index(&impl->play_delayed_ring, &pdindex); + pavail = spa_ringbuffer_get_read_index(&impl->play_ring, &pindex); + pdavail = spa_ringbuffer_get_read_index(&impl->play_delayed_ring, &pdindex); if (impl->playback != NULL && (pout = pw_stream_dequeue_buffer(impl->playback)) == NULL) { pw_log_debug("out of playback buffers: %m"); /* playback stream may not yet be in streaming state, drop play * data to avoid introducing additional playback latency */ - spa_ringbuffer_read_update(&impl->play_ring, pindex + size); - spa_ringbuffer_read_update(&impl->play_delayed_ring, pdindex + size); + spa_ringbuffer_read_update(&impl->play_ring, pindex + pavail); + spa_ringbuffer_read_update(&impl->play_delayed_ring, pdindex + pdavail); goto done; } + while (pavail > size) { + /* drop samples from previous graph cycles */ + spa_ringbuffer_read_update(&impl->play_ring, pindex + size); + pavail = spa_ringbuffer_get_read_index(&impl->play_ring, &pindex); + } + while (pdavail > size) { + /* drop samples from previous graph cycles */ + spa_ringbuffer_read_update(&impl->play_delayed_ring, pdindex + size); + pdavail = spa_ringbuffer_get_read_index(&impl->play_delayed_ring, &pdindex); + } + for (i = 0; i < impl->play_info.channels; i++) { /* echo from sink */ play[i] = &play_buf[i][0]; @@ -454,8 +473,8 @@ static void process(struct impl *impl) } done: - impl->sink_ready = false; - impl->capture_ready = false; + impl->capture_cycle = 0; + impl->sink_cycle = 0; } static void reset_buffers(struct impl *impl) @@ -479,8 +498,8 @@ static void reset_buffers(struct impl *impl) spa_ringbuffer_get_read_index(&impl->play_ring, &index); spa_ringbuffer_read_update(&impl->play_ring, index + (sizeof(float) * (impl->buffer_delay))); - impl->sink_ready = false; - impl->capture_ready = false; + impl->capture_cycle = 0; + impl->sink_cycle = 0; } static void capture_destroy(void *d) @@ -546,8 +565,11 @@ static void capture_process(void *data) spa_ringbuffer_write_update(&impl->rec_ring, index + size); if (avail + size >= impl->aec_blocksize) { - impl->capture_ready = true; - if (impl->sink_ready) + if (impl->capture_position) + impl->capture_cycle = impl->capture_position->clock.cycle; + else + pw_log_warn("no capture position"); + if (impl->capture_cycle == impl->sink_cycle) process(impl); } @@ -740,12 +762,26 @@ static void input_param_changed(void *data, uint32_t id, const struct spa_pod* p } } +static void capture_io_changed(void *data, uint32_t id, void *area, uint32_t size) +{ + struct impl *impl = data; + + switch (id) { + case SPA_IO_Position: + impl->capture_position = area; + break; + default: + break; + } +} + static const struct pw_stream_events capture_events = { PW_VERSION_STREAM_EVENTS, .destroy = capture_destroy, .state_changed = capture_state_changed, .process = capture_process, - .param_changed = input_param_changed + .param_changed = input_param_changed, + .io_changed = capture_io_changed }; static void source_destroy(void *d) @@ -930,10 +966,15 @@ static void sink_process(void *data) SPA_PTROFF(d->data, offs, void), size); } spa_ringbuffer_write_update(&impl->play_ring, index + size); + spa_ringbuffer_get_write_index(&impl->play_delayed_ring, &index); + spa_ringbuffer_write_update(&impl->play_delayed_ring, index + size); if (avail + size >= impl->aec_blocksize) { - impl->sink_ready = true; - if (impl->capture_ready) + if (impl->sink_position) + impl->sink_cycle = impl->sink_position->clock.cycle; + else + pw_log_warn("no sink position"); + if (impl->capture_cycle == impl->sink_cycle) process(impl); } @@ -955,12 +996,27 @@ static const struct pw_stream_events playback_events = { .state_changed = playback_state_changed, .param_changed = output_param_changed }; + +static void sink_io_changed(void *data, uint32_t id, void *area, uint32_t size) +{ + struct impl *impl = data; + + switch (id) { + case SPA_IO_Position: + impl->sink_position = area; + break; + default: + break; + } +} + static const struct pw_stream_events sink_events = { PW_VERSION_STREAM_EVENTS, .destroy = sink_destroy, .process = sink_process, .state_changed = sink_state_changed, - .param_changed = output_param_changed + .param_changed = output_param_changed, + .io_changed = sink_io_changed }; #define MAX_PARAMS 512u From 0276bb5b063d8b34a55b1292c4a01c1f7b5c9cc4 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Mon, 27 Oct 2025 11:43:04 +0100 Subject: [PATCH 20/23] modules: ringbuffer avail is signed --- src/modules/module-echo-cancel.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/modules/module-echo-cancel.c b/src/modules/module-echo-cancel.c index 037509c35..a4cdd2435 100644 --- a/src/modules/module-echo-cancel.c +++ b/src/modules/module-echo-cancel.c @@ -309,15 +309,15 @@ static void process(struct impl *impl) const float *play_delayed[impl->play_info.channels]; float *out[impl->out_info.channels]; struct spa_data *dd; - uint32_t i, size; - uint32_t rindex, pindex, oindex, pdindex, avail; - int32_t pavail, pdavail; + uint32_t i; + uint32_t rindex, pindex, oindex, pdindex, size; + int32_t avail, pavail, pdavail; size = impl->aec_blocksize; /* First read a block from the capture ring buffer */ avail = spa_ringbuffer_get_read_index(&impl->rec_ring, &rindex); - while (avail > size) { + while (avail > (int32_t)size) { /* drop samples from previous graph cycles */ spa_ringbuffer_read_update(&impl->rec_ring, rindex + size); avail = spa_ringbuffer_get_read_index(&impl->rec_ring, &rindex); @@ -352,12 +352,12 @@ static void process(struct impl *impl) goto done; } - while (pavail > size) { + while (pavail > (int32_t)size) { /* drop samples from previous graph cycles */ spa_ringbuffer_read_update(&impl->play_ring, pindex + size); pavail = spa_ringbuffer_get_read_index(&impl->play_ring, &pindex); } - while (pdavail > size) { + while (pdavail > (int32_t)size) { /* drop samples from previous graph cycles */ spa_ringbuffer_read_update(&impl->play_delayed_ring, pdindex + size); pdavail = spa_ringbuffer_get_read_index(&impl->play_delayed_ring, &pdindex); @@ -450,7 +450,7 @@ static void process(struct impl *impl) * available on the source */ avail = spa_ringbuffer_get_read_index(&impl->out_ring, &oindex); - while (avail >= size) { + while (avail >= (int32_t)size) { if ((cout = pw_stream_dequeue_buffer(impl->source)) != NULL) { for (i = 0; i < impl->out_info.channels; i++) { dd = &cout->buffer->datas[i]; From 94d0d8bc095b001f36f4e27c23d22c7ce4aca8ca Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Mon, 27 Oct 2025 13:32:03 +0100 Subject: [PATCH 21/23] spa: add spa_json_init_relax spa_json_init assumes that we start in an object and always requires a key/value pair. If the last part is a key, it returns and error and does not want to return the key value. This causes problems when parsing AUX0,AUX1,AUX2 or any relaxed array withand odd number of elements. Make a new spa_json_init_relax that takes the type of the container we're assuming we're in and set the state of the parser to array when we are parsing a relaxed array. Fixes #4944 --- spa/include/spa/utils/json-core.h | 9 +++++++++ spa/include/spa/utils/json.h | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/spa/include/spa/utils/json-core.h b/spa/include/spa/utils/json-core.h index 800763571..5616bffe1 100644 --- a/spa/include/spa/utils/json-core.h +++ b/spa/include/spa/utils/json-core.h @@ -54,6 +54,15 @@ SPA_API_JSON void spa_json_init(struct spa_json * iter, const char *data, size_t { *iter = SPA_JSON_INIT(data, size); } + +#define SPA_JSON_INIT_RELAX(type,data,size) \ + ((struct spa_json) { (data), (data)+(size), NULL, (uint32_t)((type) == '[' ? 0x10 : 0x0), 0 }) + +SPA_API_JSON void spa_json_init_relax(struct spa_json * iter, char type, const char *data, size_t size) +{ + *iter = SPA_JSON_INIT_RELAX(type, data, size); +} + #define SPA_JSON_ENTER(iter) ((struct spa_json) { (iter)->cur, (iter)->end, (iter), (iter)->state & 0xff0, 0 }) SPA_API_JSON void spa_json_enter(struct spa_json * iter, struct spa_json * sub) diff --git a/spa/include/spa/utils/json.h b/spa/include/spa/utils/json.h index c8030345e..212637dab 100644 --- a/spa/include/spa/utils/json.h +++ b/spa/include/spa/utils/json.h @@ -105,7 +105,7 @@ SPA_API_JSON_UTILS int spa_json_begin_container(struct spa_json * iter, spa_json_init(iter, data, size); res = spa_json_enter_container(iter, iter, type); if (res == -EPROTO && relax) - spa_json_init(iter, data, size); + spa_json_init_relax(iter, type, data, size); else if (res <= 0) return res; return 1; From 23c449af5d6afc8dde0685d61e6ed11d89b09000 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Mon, 27 Oct 2025 14:20:25 +0100 Subject: [PATCH 22/23] test: add test for an array with odd number of items We have to use the relax version to get the expected container type correct. --- test/test-spa-json.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/test-spa-json.c b/test/test-spa-json.c index 66ef2eeac..0c3c46f59 100644 --- a/test/test-spa-json.c +++ b/test/test-spa-json.c @@ -609,7 +609,7 @@ static void test_array(const char *str, const char * const vals[]) spa_json_init(&it[0], str, strlen(str)); if (spa_json_enter_array(&it[0], &it[1]) <= 0) - spa_json_init(&it[1], str, strlen(str)); + spa_json_init_relax(&it[1], '[', str, strlen(str)); for (i = 0; vals[i]; i++) { pwtest_int_gt(spa_json_get_string(&it[1], val, sizeof(val)), 0); pwtest_str_eq(val, vals[i]); @@ -624,6 +624,7 @@ PWTEST(json_array) test_array("[FL FR]", (const char *[]){ "FL", "FR", NULL }); test_array("FL FR", (const char *[]){ "FL", "FR", NULL }); test_array("[ FL FR ]", (const char *[]){ "FL", "FR", NULL }); + test_array("FL FR FC", (const char *[]){ "FL", "FR", "FC", NULL }); return PWTEST_PASS; } From 76a31a47c2cf063abc70c29f42e60fb0fa03dee5 Mon Sep 17 00:00:00 2001 From: Jonas Holmberg Date: Mon, 27 Oct 2025 14:37:03 +0100 Subject: [PATCH 23/23] module-echo-cancel: Avoid discontinuity Keep the samples in the ringbuffer that are needed the next cycle to avoid discontinuity when the aec blocksize is not equal to or divisible by quantum. --- src/modules/module-echo-cancel.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/modules/module-echo-cancel.c b/src/modules/module-echo-cancel.c index a4cdd2435..98efa35c5 100644 --- a/src/modules/module-echo-cancel.c +++ b/src/modules/module-echo-cancel.c @@ -317,10 +317,15 @@ static void process(struct impl *impl) /* First read a block from the capture ring buffer */ avail = spa_ringbuffer_get_read_index(&impl->rec_ring, &rindex); - while (avail > (int32_t)size) { - /* drop samples from previous graph cycles */ + while (avail >= (int32_t)size * 2) { + /* drop samples that are not needed this or next cycle. Note + * that samples are kept in the ringbuffer until next cycle if + * size is not equal to or divisible by quantum, to avoid + * discontinuity */ + pw_log_debug("avail %d", avail); spa_ringbuffer_read_update(&impl->rec_ring, rindex + size); avail = spa_ringbuffer_get_read_index(&impl->rec_ring, &rindex); + pw_log_debug("new avail %d, size %u", avail, size); } for (i = 0; i < impl->rec_info.channels; i++) { @@ -352,15 +357,19 @@ static void process(struct impl *impl) goto done; } - while (pavail > (int32_t)size) { - /* drop samples from previous graph cycles */ - spa_ringbuffer_read_update(&impl->play_ring, pindex + size); + if (pavail > avail) { + /* drop too old samples from previous graph cycles */ + pw_log_debug("pavail %d, dropping %d", pavail, pavail - avail); + spa_ringbuffer_read_update(&impl->play_ring, pindex + pavail - avail); pavail = spa_ringbuffer_get_read_index(&impl->play_ring, &pindex); + pw_log_debug("new pavail %d, avail %d", pavail, avail); } - while (pdavail > (int32_t)size) { - /* drop samples from previous graph cycles */ - spa_ringbuffer_read_update(&impl->play_delayed_ring, pdindex + size); + if (pdavail > avail) { + /* drop too old samples from previous graph cycles */ + pw_log_debug("pdavail %d, dropping %d", pdavail, pdavail - avail); + spa_ringbuffer_read_update(&impl->play_delayed_ring, pdindex + pdavail - avail); pdavail = spa_ringbuffer_get_read_index(&impl->play_delayed_ring, &pdindex); + pw_log_debug("new pdavail %d, avail %d", pdavail, avail); } for (i = 0; i < impl->play_info.channels; i++) {