Repository navigation
Add AlsaSettings for opening an ALSA PCM by address - #644
whatisyourname0 wants to merge 7 commits into
Conversation
|
Thanks for this PR! It seems to add two (or more?) different things, though: a new ALSA setting and a whole new testing framework. I think both are valuable, but they should be discussed separately. Regarding tests, there are already some discussions in #14 and #131, and there has been a very recent PR that also comes with its own test framework: #645. |
|
Thanks for the review! I agree that adding I'll update this PR to focus on the ALSA PCM-address support and its documentation, removing the new test suite, test dependencies, and test-related CI changes from the diff. I'll preserve the regression tests on a separate branch and keep the validation results and reproduction instructions available for reviewing the feature. Thanks also for pointing me to #14, #131, and #645. I'll follow up on testing separately, building on those discussions before proposing a shared approach. |
|
Done in 8e0aeba: this PR now contains only the ALSA PCM-address support and its documentation. The tests and reproduction instructions are preserved on |
|
Thanks for the update! I like the
I think an explicit sentinel would be better, as it would make the implementation more straightforward and it would be closer to PortAudio's C API. Maybe something like DEVICE_FROM_EXTRA_SETTINGS = _lib.paUseHostApiSpecificDeviceSpecificationMaybe you can come up with a better name? Your example would then look something like this: with sd.RawInputStream(
device=sd.DEVICE_FROM_EXTRA_SETTINGS,
samplerate=48000,
channels=2,
dtype='int16',
blocksize=1024,
latency=4096 / 48000,
extra_settings=sd.AlsaSettings(device_string='null'),
) as stream:
data, overflowed = stream.read(1024)
print(len(data), overflowed)
It is kinda annoying, but I guess we don't really have a choice, right? However, I would also prefer the handling of this to be less host-API-specific. Whenever |
|
Thanks for the review, and that makes sense. Your proposed name communicates the intent clearly than mine. I'll also rework the shared parameter handling around whether device information is available, removing the I'll update the examples and the separately maintained regression tests to cover the changed behavior. |
|
Updated and pushed in ffcda44: device selection now uses |
mgeier
left a comment
There was a problem hiding this comment.
Here's a first round of review ... I must postpone the rest because I couldn't manage to run this yet, I always get a sounddevice.PortAudioError: Error querying device -2. Is there anything special I need to do for this to work?
| Notes | ||
| ----- | ||
| The stream's device attribute reports PortAudio's special value | ||
| ``-2`` when `DEVICE_FROM_EXTRA_SETTINGS` is selected. | ||
| PortAudio rejects these settings with a regular ALSA device index. | ||
| For direct output with ``mapping=[1]`` in `play()` or `playrec()`, | ||
| use `OutputStream` with explicit channels instead; automatic | ||
| stereo detection requires enumerated device information. |
There was a problem hiding this comment.
I think these notes also belong into the device_string section?
There was a problem hiding this comment.
Moved these notes into the device_string section as well.
There was a problem hiding this comment.
You can still put it into a note directive, if you like.
There was a problem hiding this comment.
Thanks, I moved those details into a note.
|
Thanks for trying this. I reproduced the same error when This minimal example works locally on Linux with the current PR head: import sounddevice as sd
with sd.RawInputStream(
device=sd.DEVICE_FROM_EXTRA_SETTINGS,
extra_settings=sd.AlsaSettings('null'),
samplerate=48000,
channels=2,
dtype='int16',
blocksize=1024,
latency=4096 / 48000,
) as stream:
data, overflowed = stream.read(1024)
print(len(data), overflowed)If this exact example also fails on your setup, could you share the full traceback and your PortAudio version? That should help identify which device-information lookup is being reached. Thanks also for the inline review comments. I'll address those separately. |
|
Thanks, that was it! As I mentioned in #644 (comment), you should check for failure (namely for a This way, we would get meaningful error messages for missing |
|
Thanks for confirming! I misunderstood that part of your earlier suggestion and only propagated the original error. I'll make a change that catches |
mgeier
left a comment
There was a problem hiding this comment.
Here's the rest of my review, let me know if anything is unclear!
| if not isinstance(device_string, str): | ||
| raise TypeError('device_string must be a string') | ||
| if not device_string or '\0' in device_string: | ||
| raise ValueError('device_string must be nonempty and contain no NUL') |
There was a problem hiding this comment.
I think those errors are not really necessary:
| if not isinstance(device_string, str): | |
| raise TypeError('device_string must be a string') | |
| if not device_string or '\0' in device_string: | |
| raise ValueError('device_string must be nonempty and contain no NUL') |
There was a problem hiding this comment.
I removed the explicit type and empty-string checks. I retained the embedded-NUL check because C would silently interpret 'null\0different-device' as 'null', opening a different PCM than the supplied address. I added a regression test for that truncation.
There was a problem hiding this comment.
I think this is a quite unrealistic edge case, and I suggest we either document it or check it, but not both.
There was a problem hiding this comment.
Hmm, if that's the case, I kept the check and removed the duplicate at the following commit
| if (isinstance(channels, bool) or | ||
| not isinstance(channels, numbers.Integral) or channels < 1): | ||
| raise ValueError('A positive channel count is required') |
There was a problem hiding this comment.
Why would you check this?
We are nowhere else checking for positivity or non-bool-ness. I think the behavior is reasonable enough without this check.
There was a problem hiding this comment.
I removed the type and boolean checks, but found a problem when removing the positivity check: with PortAudio 19.6.0 on Linux ARM64, opening the direct 'null' PCM with channels=0 aborts the Python process inside PortAudio.
I therefore retained a minimal non-positive channel-count check for DEVICE_FROM_EXTRA_SETTINGS, after the normal CFFI conversion. The regression tests run these cases in subprocesses so a native abort is reported as a test failure.
There was a problem hiding this comment.
Is this CPU-specific?
I can reproduce it on x86_64.
This seems to be a bug in PortAudio. Would you like to check if that has already been reported?
There was a problem hiding this comment.
ARM64 was my test environment (Macbook M4 Pro). Thanks for checking x86_64 too.
Besides, I couldn’t find an existing PortAudio issue for this validation. Maybe I should check it out at the PortAudio.
| if channels is None or samplerate is None or latency in ('low', 'high'): | ||
| try: | ||
| info = query_devices(device) | ||
| except PortAudioError as error: | ||
| required = [] | ||
| if channels is None: | ||
| required.append('channels') | ||
| if samplerate is None: | ||
| required.append('samplerate') | ||
| if latency in ('low', 'high'): | ||
| required.append('numeric latency') | ||
| raise ValueError( | ||
| 'Cannot determine {} stream parameters from device information; ' | ||
| 'specify {} explicitly or through defaults'.format( | ||
| kind, ', '.join(required))) from error | ||
| if channels is None: | ||
| channels = info['max_' + kind + '_channels'] | ||
| if samplerate is None: | ||
| samplerate = info['default_samplerate'] | ||
| if latency in ('low', 'high'): | ||
| latency = info['default_' + latency + '_' + kind + '_latency'] |
There was a problem hiding this comment.
Now the if-conditions appear 3 times ... I think 2 times is enough. Maybe something like this (error messages can be improved):
| if channels is None or samplerate is None or latency in ('low', 'high'): | |
| try: | |
| info = query_devices(device) | |
| except PortAudioError as error: | |
| required = [] | |
| if channels is None: | |
| required.append('channels') | |
| if samplerate is None: | |
| required.append('samplerate') | |
| if latency in ('low', 'high'): | |
| required.append('numeric latency') | |
| raise ValueError( | |
| 'Cannot determine {} stream parameters from device information; ' | |
| 'specify {} explicitly or through defaults'.format( | |
| kind, ', '.join(required))) from error | |
| if channels is None: | |
| channels = info['max_' + kind + '_channels'] | |
| if samplerate is None: | |
| samplerate = info['default_samplerate'] | |
| if latency in ('low', 'high'): | |
| latency = info['default_' + latency + '_' + kind + '_latency'] | |
| if channels is None or samplerate is None or latency in ('low', 'high'): | |
| try: | |
| info = query_devices(device) | |
| error = None | |
| except PortAudioError as e: | |
| info = {} | |
| error = e | |
| if channels is None: | |
| if error is None: | |
| channels = info['max_' + kind + '_channels'] | |
| else: | |
| raise TypeError('channels=None is not supported') from error | |
| if samplerate is None: | |
| if error is None: | |
| samplerate = info['default_samplerate'] | |
| else: | |
| raise TypeError('samplerate=None is not supported') from error | |
| if latency in ('low', 'high'): | |
| if error is None: | |
| latency = info['default_' + latency + '_' + kind + '_latency'] | |
| else: | |
| raise TypeError("latency='low'/'high' is not supported") from error |
What do you think about that?
There was a problem hiding this comment.
Thanks, this is clearer. I adopted that structure, raising TypeError for the first unresolved parameter and preserving the original PortAudioError as the cause. The messages indicate which value needs to be supplied explicitly or through defaults.
There was a problem hiding this comment.
explicitly or through defaults
Where does this formulation come from? LLM?
I find it hard to understand.
|
Thanks for the detailed review! I’ve pushed the changes in 7641867 and replied to the individual comments. I kept two small checks for the native abort and address-truncation cases described above. I also updated the separate regression-test branch. All 133 tests pass on Linux ARM64 with Python 3.14.7 and PortAudio 19.6.0, including 74 baseline comparisons. Type checking and the documentation build pass too. These tests use ALSA null/file PCMs; they don’t establish physical-device or other-platform compatibility. |
| # Keep the settings and their C buffers alive for the stream. | ||
| self._extra_settings = iextra, oextra |
There was a problem hiding this comment.
I don't think returning the extra settings from _get_stream_parameters() is necessary.
Instead, you can just keep alive the extra_settings the user passed in.
The only downside is that if a pair of extra settings are given, both are kept alive, even in input-only and output-only streams. But users will probably not create a pair when they only need a single settings object.
There was a problem hiding this comment.
Now that I'm thinking about it ... why does it even need to be kept alive?
I assume the device string is only accessed in Pa_OpenStream and never again?
There was a problem hiding this comment.
Removed the extra return value and, following your later comment, the stream-owned references too at the following commit.
Thanks for pointing out!
There was a problem hiding this comment.
Checked the PortAudio 19.6.0 and current upstream branch, and neither retains the string after opening.
Removing the references and verified that blocking and callback I/O work after the settings are collected is the right behavior in this time, I think.
| device, channels, sampleformat, latency, | ||
| extra_settings._streaminfo if extra_settings else _ffi.NULL)) | ||
| return parameters, dtype, samplesize, samplerate | ||
| # PortAudio's ALSA direct-device path asserts on non-positive channels. |
There was a problem hiding this comment.
This comment is hard to understand, because "assert" has very different behavior in C and Python.
Also, it would be good to mention that this works around a bug in PortAudio, and ideally an issue link should be provided.
There was a problem hiding this comment.
Thanks for pointing it out, as I'm pretty confused how they behaveonf those both language) I clarified that tihs works around a PortAudio C assertion that can abort the process, and linked to your reproduction.
| if channels is None or samplerate is None or latency in ('low', 'high'): | ||
| try: | ||
| info = query_devices(device) | ||
| except PortAudioError as error: | ||
| required = [] | ||
| if channels is None: | ||
| required.append('channels') | ||
| if samplerate is None: | ||
| required.append('samplerate') | ||
| if latency in ('low', 'high'): | ||
| required.append('numeric latency') | ||
| raise ValueError( | ||
| 'Cannot determine {} stream parameters from device information; ' | ||
| 'specify {} explicitly or through defaults'.format( | ||
| kind, ', '.join(required))) from error | ||
| if channels is None: | ||
| channels = info['max_' + kind + '_channels'] | ||
| if samplerate is None: | ||
| samplerate = info['default_samplerate'] | ||
| if latency in ('low', 'high'): | ||
| latency = info['default_' + latency + '_' + kind + '_latency'] |
There was a problem hiding this comment.
explicitly or through defaults
Where does this formulation come from? LLM?
I find it hard to understand.
Yes to be honest, that wording came from LLM-assisted slop drafting, because of usage of my LLM to convert & double check my English expression. My apologies for the slops :-( Anyway, I replaced it with direct instructions like “Set channels to an integer.” from the following commit. |
This adds
AlsaSettings(device_string)so an application can open an ALSA PCM address through PortAudio'sPaAlsaStreamInfo.deviceString. Select it explicitly withdevice=sd.DEVICE_FROM_EXTRA_SETTINGS, corresponding topaUseHostApiSpecificDeviceSpecification.This implements the ALSA device-string part of #4 through the existing platform-specific settings interface. It does not resolve all of #4; #85 proposes a broader host-API interface.
Replace
'null'with a hardware PCM address or a named ALSA configuration. A PCM address is not a persistent hardware identifier.Behavior
AlsaSettingsalone does not override device selection. Set the sentinel in the call or insd.default.device. PortAudio rejects these settings with a regular ALSA device index.AlsaSettingstype checks. If lookup fails, it raisesTypeErrornaming the first unresolved parameter and chains the originalPortAudioError.sd.default.channelsandsd.default.samplerate. Latency must be numeric;'low'and'high'need device information unavailable for the sentinel. Format checks do not use latency.-2instream.device.deviceStringafterward, so streams do not keep the settings alive until close.mapping=[1]inplay(), oroutput_mapping=[1]inplayrec(), needs enumerated device information for automatic stereo detection. UseOutputStreamorStream, respectively, with the channel count specified.Compatibility and validation
Feature revision:
e9a9f0d.The separate regression branch at
adfdf2bdiffers from that feature revision only by six files undertests/. The feature PR adds no test dependencies, CI steps or lockfile changes.All 204 tests passed, without skips, on Linux ARM64 and CPython 3.14.7 against each of:
1ca0eee, which reports 19.7.0-devel.The suite includes 125 identical public API calls against base
626fcb0and the candidate in separate processes using the same synthetic ALSA configuration. It covers index/name/default selection, blocking and callback input/output/duplex, configured and device-derived defaults, format checks andplay()/rec()/playrec().114 outcomes match exactly. Eleven cases assert three intentional differences:
TypeError, with the originalPortAudioErroras the cause. Code catching onlyPortAudioErrorneeds to account for this changed failure path.The remaining 79 cases cover direct-device behavior, errors, exact captured bytes, reopen/recovery and settings lifetime. Lifetime tests check ownership during native calls and exercise blocking/callback I/O after collection. Pyright, warning-as-error Sphinx HTML build, focused Ruff checks and
git diff --checkalso passed.See the test README for reproduction commands. Set
SOUNDDEVICE_BASELINE_PATHto the built base source to include the 125 differential cases; without it those cases are explicitly skipped.Limits
These runs use isolated ALSA plug/null/file PCMs. They do not establish physical-device behavior, hotplug, real-time performance, negotiated hardware buffer sizes, or native Windows/macOS compatibility. The final revision has not been rerun across the earlier Python-version matrix. PortAudio initialization may still enumerate devices; no performance improvement or automatic hardware-identity resolution is claimed.