-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core Audio for interfaces with large #'s of channels #83
Conversation
This is a good time to decide how host specific functions will be added to the API. ASIO has some nice functions I would also like to integrate. Initially though I would think that we wouldn't want to add these functions to the global namespace since they won't be available for all APIs. |
For the two API changes in my current commits, here are two alternative implementations that cross my mind:
If either or both of these two ideas don't sound too ugly, I'm happy to conjure a first draft so that you'd have something concrete to ponder. |
To help with brainstorming, what are the ASIO functions you'd like to integrate? |
Most of all I'd like to get GetAvailableBufferSizes added. But GetInputChannelName/GetOutputChannelName could also be useful. |
Hmm, and Core Audio offers similar data via kAudioDevicePropertyBufferFrameSizeRange, which is exposed via Perhaps could this data also be returned by
where the dict value contains some "iterable" and doesn't have to be a list or tuple. (An app is free to fee this iterable to On Core Audio, I think |
I wouldn't get too attached to the idea of exposing host api buffer sizes as some kind of common functionality. Portaudio is pretty generic at the stream level so the feature set across apis is pretty damn varied. For one thing, ALSA doesn't have any feature like this - which is the only real api that it is supported in Linux. Another example is PaWasapi's GetFramesPerHostBuffer which is for a similiar use case as GetAvailableBufferSizes but is more limited. I agree It would be nice if the set and get interface for similar features were functionally identical (same input variables, same output variables) but I'm not sure how much value it would provide given the lack of similarity between apis. One other thing to keep in mind in this discussion: not all host specific settings are stream specific. For example Alsa has a few settings that apply to all created streams. In these cases it might make sense to have these options settable/gettable through the Settings class as staticmethods. This is just my two cents anyway, I'm just a contributor :P. |
I'm open to changes in the host API specific settings, including abandoning the current I'm reluctant to add one or more top-level functions that are only available on a subset of host APIs. Some notes about platform-specific features are mentioned in #4. I think it would be nice if we could find an API that also allows for some ALSA features, namely See also #55 which was brought up by @hiccup7. Probably we can find something that avoids this issue? @dholl About "channel map" and "channel selectors": I'm not sure if it makes sense to make those compatible, since they are explicitly platform specific. I've exposed the |
sounddevice.py
Outdated
|
||
>>> maccore_in = sd.MacCoreSettings(channel_selectors=[8, 9]) | ||
>>> sd.default.extra_settings = maccore_in, maccore_out | ||
>>> sd.playrec(..., channels=1, ...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this example. You set channels_selectors=[8,9] but then specify channels=1 to playrec? Wouldn't you want to pass channels=2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb typo. Yep, should be channels=2.
sounddevice.py
Outdated
if rate_conv_quality not in conversion_dict: | ||
raise ValueError('rate_conv_quality must be one of {!r}'.format(list(conversion_dict.keys()))) | ||
|
||
if channel_selectors is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of type checking seems like overkill. Do you truly require a list or a tuple? Why not just do something similar to what's done in the AsioSettings constructor - check that it's not a single int and be done with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsioSettings' input verification is so loose that I question the value of having it at all:
if isinstance(channel_selectors, int):
raise TypeError('channel_selectors must be a list or tuple')
For example, if someone passes in some other invalid data type such as a string, dict, OrderedDict, etc..., then the informative TypeError would never be raised, and error detection would fall to whatever _ffi.new('int[]', channel_selectors)
will throw.
Is there some performance concern for the additional input type checking? If so, I could wrap it in an if __debug__:
check so that it only gets invoked as often as the other existing assert's...
On the whole, I'm fine with simplifying or stripping out this input checking. Is there a different example that strikes a better balance between overkill and insufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for the isinstance()
check you are mentioning is the Python Zen mantra: "Errors should never pass silently".
Passing an int
would cause ffi.new()
to allocate that number of int
s, which is not what we want, and this error would happen silently.
All other wrong types will raise an error, and that's the important thing.
I agree that the error message generated by CFFI is not ideal, especially since it says "[...] cannot be interpreted as an integer".
I would say this is a (minor) bug in CFFI and should be changed there, but at least it's raising a TypeError
, which is good.
Is there some performance concern for the additional input type checking?
Nope. Except in the audio callback, it's never about performance.
This check we are discussing here is purely about wrong user input raising an error immediately.
In general, I try to embrace duck typing whenever reasonable. Therefore, I try to avoid isinstance()
checks as much as possible.
Also, I'm not trying to catch each single exception and re-raise it with a different exception type or message. I think this just produces too much boilerplate code (and works against duck typing).
But sure, we always have to draw the line somewhere, and I'm not saying I did it right everywhere, and I'm open to changes that add or remove error checking code.
Is there a different example that strikes a better balance between overkill and insufficient?
I don't know.
But each type check is killing a duck, so they should be used sparingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update: the CFFI bug was fixed: https://bitbucket.org/cffi/cffi/issues/363/incomplete-error-message-in-ffinew
sounddevice.py
Outdated
# settings such as AsioSettings or WasapiSettings and not allow | ||
# any accidental reliance on positional parameters. | ||
|
||
if not isinstance(channels, int): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the _StreamBase constructor should be taking care of these kinds of checks. If you really want to be sure channels is valid, then modify the _StreamBase constructor so this function doesn't get called until after the stream object is opened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. This check on channels
doesn't need to be here. I'll remove it if there's consensus that adding a MacCoreSettings struct is preferable to replacing all 3 *Settings structs with dict.
sounddevice.py
Outdated
#print('Debug: kind={} max_channels={} channel_selectors={!r} this_channelmap={!r}'.format(kind, max_channels, self._channel_selectors, this_channelmap)) | ||
|
||
cache_key = kind,device,channels | ||
if cache_key in self._cache_cffi_streaminfo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify why and how you're using this cache? From what I can see this function will never be called more than once, (at Stream instantiation time) therefore the cache will only ever contain one entry.
Or maybe this is a mistake and you mean to declare the cache at the class level (not the instance level)? If so, I'm not sure how much benefit having a cache of MacCoreStreamInfo objects provides. I'm of the mindset that If you're going to add complexity, however minor, it needs to be justified by some objective advantages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "cache" is just in case someone reuses the same object for opening multiple, different devices, so that I don't accidentally permit a CFFI-allocated struct to be freed before the caller really is done with it. Is there some guarantee that a library user cannot pass the same instance to different devices?
One use case for feeding the same MacCoreSettings struct to multiple devices would be if a caller wants to specify change_dev_params or allow_rate_conv. But yeah, in the case of a caller also specifying channel_selectors, they're likely going to be using separate objects for separate devices to specify separate mappings.
In any case, I purposefully kept this cache only at the instance level so that memory will be freed as soon as the instance itself is freed, similar to AsioSettings and WasapiSettings.
I only bothered with this MacCoreSettings class to present the same API as the AsioSettings and WasapiSettings. In my own opinion, I'd rather do away with all 3 of these API-specific settings classes in favor of having the caller pass in a simple dict as I mentioned above. Then the CFFI (_streaminfo) structs could be allocated during _get_stream_parameters and freed as soon as RawStream/Stream/whatever is done with them. I'm game to throw together a draft of this idea if folks are interested in reviewing it further.
I'll update the pull request to incorporate the feedback thus far... :) |
Ah, regarding some way to expose hostapi-specific functions without polluting the global namespace, how about returning a dict (or named tuple?) of callables from query_hostapis? For example, the returned dict could have a key called 'api' which is a collection of whatever specific functions exist for just that hostapi. On my system, hostapi index 0 corresponds to Core Audio, so I might daydream of some usage like so: Assuming
On someone else's system, hostapi_id 0 might be ASIO, so perhaps that namespace might look like:
An application may of course need to check Perhaps these functions are just straight 1:1 mappings to the underlying PortAudio functions? (i.e., no other object-oriented whatever applied such as asio.device[0].get_available_buffer_sizes?) Keeping it simple without any further abstraction would ease the initial implementation. So, would this mechanism of providing API access from query_hostapis be palatable? |
I like this idea! The detour over alsa = query_hostapis('alsa')
if alsa:
alsa.set_something('blah') Something like this could probably address the concerns from #55? wasapi = query_hostapis('wasapi')
if wasapi:
wasapi.exclusive = True I don't know if the above example would work, since I don't know if the "exclusive" setting is supposed to be global or per-stream or per-device or ... The different host-API-specific features affect different parts, some are global functions, others are applied to a stream and others are applied separately to the input and output settings when creating a stream (and probably there are still other scenarios which I missed). @dholl Can you please provide a list of host-API-specific features that you think can be handled with this (probably in a separate issue)? |
@mgeier Regarding
So the function would be defined as:
|
OK, I just simplified If I get rid of [For now with this PR, I'm holding back on further ideas for refactoring the *Settings classes...] |
Thanks @dholl!
Yes, definitely, that's a much simpler and less intrusive change. |
sounddevice.py
Outdated
unsigned long version; /**structure version */ | ||
unsigned long flags; /** flags to modify behaviour */ | ||
SInt32 const * channelMap; /** Channel map for HAL channel mapping , if not needed, use NULL;*/ | ||
unsigned long channelMapSize; /** Channel map size for HAL channel mapping , if not needed, use 0;*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the rest of the cdef
, I've removed those inline comments and moved them to the actual documentation wherever it made sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK! (fixing now)
sounddevice.py
Outdated
device. *channel_map* is a list of integers specifying the | ||
(zero-based) channel numbers to use. Note that | ||
*channel_map* is treated differently between input and | ||
output channels. See the example below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation on this is surprisingly hard to find, but what about adding this link?
https://app.assembla.com/spaces/portaudio/git/source/master/src/hostapi/coreaudio/notes.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might as well add a sentence about how it works for output channels. Something like "Unused output channels can be marked with -1
"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reminding me about the link! That's how I got my first draft of code running. I'll include it in the doc string and update with hopefully more helpful verbiage. (The updated doc string will likely need your critique again.)
sounddevice.py
Outdated
raise ValueError('rate_conv_quality must be one of {!r}'.format(list(conversion_dict.keys()))) | ||
|
||
if channel_map is not None: | ||
if not all(isinstance(ch, int) for ch in channel_map): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is wrong in several ways.
You are already assuming an iterable by iterating over it, so the check doesn't catch non-iterables.
If you strictly want to accept an iterable (and not only, e.g. a sequence), you should only iterate over it once!
You are iterating once for the type check and later you are passing it on to CFFI, which iterates again.
Checking for a list of int
is IMHO not necessary, since CFFI does that anyway.
You should check for a single int though, as I mentioned somewhere else before.
You should probably also check for an empty list, since this causes a strange error:
||PaMacCore (AUHAL)|| Error on line 1589: err='-50', msg=Unknown Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
sounddevice.py
Outdated
>>> # For this example device, max_output_channels == 94. | ||
>>> # Now overwrite any -1's for channels we'll use for output: | ||
>>> for ndx in range(len(output_channels)): | ||
>>> ch_map_out[output_channels[ndx]]=ndx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code seems overly complicated.
Why not just provide a list literal with some concrete values, e.g. [-1, -1, 0, 1]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do you really have to specify all channels up to the maximum number if you don't want to use them?
I tried it and it didn't seem to be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find much information on the matter either. The closest notes I found were also at https://app.assembla.com/spaces/portaudio/git/source/master/src/hostapi/coreaudio/notes.txt which does state to include the extra -1's for unused channels at the end.
I'm wary to cut it any shorter if it accidentally works today, but isn't guaranteed to continue working in the future. And the memory cost of a few dozen or even hundred int's isn't too much, considering the memory may be freed after the open is finished, right?
But all this complexity with the -1 for unused channels is what drove my initial hunch to include this array book-keeping on behalf of the caller. <shrug/> I'm still fine with leaving it out though at the expense of more words in the doc string and more platform-specific annoyances for those lost souls (like myself...) on Core Audio systems. :)
Anyhow, since the current MacCoreSettings/CoreAudioSettings class punts all responsibility for array formatting to the caller, it can be left up to application code on how paranoid (or not) they want to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, and I'm including shorter 6-channel examples in the discussion text, largely borrowed from https://app.assembla.com/spaces/portaudio/git/source/master/src/hostapi/coreaudio/notes.txt, because you're right, having a concrete short example should get the point across a bit more easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, we shouldn't rely on potentially accidental behavior. And I agree that we should let the users decide if they want to be paranoid or not.
I like your 6-channel example. It is simple enough and it doesn't ignore trailing unused channels.
But why do you need two examples, one in the description text and one in the code example?
What can be shown with 94 channels that cannot be shown with 6?
Can't you combine them into one code example?
Also, I don't really like the extensive use of comments in the code example.
I would interrupt the code lines with normal text paragraphs, or is there anything speaking against that?
That's what I have done in other places of the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
sounddevice.py
Outdated
(zero-based) channel numbers to use. Note that | ||
*channel_map* is treated differently between input and | ||
output channels. See the example below. | ||
change_dev_params : bool, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really necessary to abbreviate this? What about change_device_parameters
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll make 'em longer then. The 72-column line wrapping was getting annoying, so that's why I started getting stingy with names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that can be annoying. And in some cases it might be worth making an exception. But I think here it isn't.
sounddevice.py
Outdated
but might disrupt the device if other programs are using it, | ||
even when you are just Querying the device. ``False`` is | ||
the default. | ||
prevent_rate_conv : bool, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. If you want it to be shorter, you could use prevent_conversion
.
I think in the rest of the API I tried to mimic the original PortAudio names, so why not fail_if_conversion_required
?
It's not pretty, but at least it's not our fault!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
sounddevice.py
Outdated
In combination with the above flag, ``True`` causes the | ||
stream opening to fail, unless the exact sample rates are | ||
supported by the device. | ||
rate_conv_quality : string, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here, IMHO no abbreviation necessary.
What about conversion_quality
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
sounddevice.py
Outdated
|
||
Example | ||
------- | ||
Input from 'Mic 9' and 'Mic 10' on RME Fireface UFX+, and send |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really necessary to mention the name of the device here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your call. I only included it to give a concrete example, along with channel names such as 'Mic 9' and 'Analog 3'.
OK, I just stripped out the hardware names. (commits will be coming later today.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concrete examples are nice, but this just looked suspiciously like advertisement ...
sounddevice.py
Outdated
@@ -2349,6 +2376,97 @@ def __init__(self, channel_selectors): | |||
channelSelectors=self._selectors)) | |||
|
|||
|
|||
class MacCoreSettings(object): | |||
|
|||
def __init__(self, channel_map=None, change_dev_params=False, prevent_rate_conv=False, rate_conv_quality='Max'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
sounddevice.py
Outdated
'Low': _lib.paMacCoreConversionQualityLow , | ||
'Medium': _lib.paMacCoreConversionQualityMedium, | ||
'High': _lib.paMacCoreConversionQualityHigh , | ||
'Max': _lib.paMacCoreConversionQualityMax , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the aligned names, but I wouldn't align the commas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
sounddevice.py
Outdated
|
||
Parameters | ||
---------- | ||
channel_map : array of int, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about "sequence of int"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
sounddevice.py
Outdated
In combination with the above flag, ``True`` causes the | ||
stream opening to fail, unless the exact sample rates are | ||
supported by the device. | ||
conversion_quality : string, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
str
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, even better:
{'min', 'low', 'medium', 'high', 'max'}, optional
I prefer lowercase names here.
And wouldn't it be better to match case-insensitively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! (and I just included .lower() in the dict lookups)
Any tips on line wrapping? The 'p' in "optional" is now at column 72.
.oO( Ah, the bane of having long descriptive variable names like 'conversion_quality'. I personally do enjoy long descriptive names, much to the chagrin of coworkers, but then again, I also have no personal limit on line lengths. But when writing for others, I'm flexible. ;) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not a fan of long names, but in this case I think it is better to follow the sub-optimal PortAudio naming instead of coming up with our own names which may have problems on their own. This way we can at least blame it on PortAudio.
I am very anal about line lengths, but that doesn't mean I would never accept exceptions.
In this case I would make an exception.
@dholl Thanks a lot for the changes! The MacCore vs. CoreAudio usage is indeed inconsistent. Here I would go for CoreAudio, because that's the actual name of the thing. Thanks for changing it! |
sounddevice.py
Outdated
>>> | ||
>>> sd.playrec(..., channels=2, extra_settings=(mc_in, mc_out)) | ||
""" | ||
# Check inputs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems a bit lost, is it really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! Removed.
sounddevice.py
Outdated
} | ||
|
||
if channel_map is not None: | ||
if isinstance(channel_map, int): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really have to be inside the "not None" block.
See below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
sounddevice.py
Outdated
if isinstance(channel_map, int): | ||
raise TypeError('channel_map must be a list or tuple') | ||
if isinstance(channel_map, (list, tuple)) \ | ||
and len(channel_map)==0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a complicated way to say not channel_map
.
What about:
if channel_map is not None and not channel_map:
raise ...
It's still a bit awkward, isn't it?
I think it's better to check the length after CFFI is done with it, i.e. close to the end of the function:
if len(self._channel_map) == 0:
raise ...
(That's probably even a bug in PortAudio, shouldn't they raise a meaningful error when that happens?)
Still too strange?
What about completely ignoring an empty channel_map
and not raising an error?
An empty map would just mean "default mapping" ...
This would reduce the implementation to
if channel_map:
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a bit of digging, and I think it should work when using _lib.PaMacCore_SetupChannelMap(self._streaminfo, _ffi.NULL, 0)
, which should clear the property value, see https://developer.apple.com/reference/audiotoolbox/1440371-audiounitsetproperty?language=objc.
I guess the error is happening because we use an empty int[]
array instead of NULL
?
I currently don't have access to a Mac, so I can't check myself.
I assume error number -50
means kAudio_ParamError
, as defined in CoreAudioTypes.h
, which is not handled in PaMacCore_SetError()
, defined in https://app.assembla.com/spaces/portaudio/git/source/master/src/hostapi/coreaudio/pa_mac_core_utilities.c.
Anyway, I think passing channel_map=[]
could be understood as clearing the property and therefore using the default mapping, right?
I'm also OK with raising an error, which is probably the safer option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great questions. In case of specifying input channels, I could perceive channel_map=[]
as indicating "No input channels" in a general sense, because the input list is only a list of desired channels (but why would they want no channels???). However for output channels, channel_map=[]
could lead to potentially undefined behavior if we assume that the length of channel_map should match max_output_channels and all unused channels must contain -1. (imho, weird that Core Audio decides on this seemingly different interpretation between in's and out's...)
Maybe for now, I'll err on the side of raising an error for empty lists and tuples. I think I found a middle-ground that fits within 72 columns. How's this:
# Minimal checking on channel_map to catch errors that might
# otherwise go unnoticed:
if isinstance(channel_map, int):
raise TypeError('channel_map must be a list or tuple')
if isinstance(channel_map, (list, tuple)) and not channel_map:
raise TypeError('channel_map must not be empty')
Is the comment all right in there? This is a subtle flag to myself that we're performing very permissive input checking to allow as many pythonic ducklings as humanly imaginable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should stay on the safe side and raise an error.
And yes, the comment makes sense, since it isn't obvious at first glance why we would check for int
specifically.
I still don't like the list
and tuple
check. It may not make a practical difference now, it is rather a philosophical question.
And theoretically, the CFFI call might allow more iterables in the future, and then your check will not detect if those other iterables are empty.
I still think that the best way to handle this is to check for int
before the call to _ffi.new()
and check for len(...) == 0
afterwards.
This is getting quite ideological, so feel free to refuse my suggestion.
sounddevice.py
Outdated
try: | ||
self._flags = conversion_dict[conversion_quality] | ||
except KeyError: | ||
raise ValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line break isn't necessary, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The break had to happen somewhere to fit within 72 cols.
except KeyError:
raise ValueError(
'conversion_quality must be one of {0!r}'.format(
list(conversion_dict.keys())))
My original inclination (without col limits) is:
except KeyError:
raise ValueError('conversion_quality must be one of {0!r}'.format(list(conversion_dict.keys())))
because in my myopic world perspective, when I see a raise ValueError... at the start of the line, I personally don't care how long it is since the rest is "uninsteresting error handling", never mind that error handling is crucial. :)
I'm open for any ideas on where to split that line. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is 79 columns, 72 is only suggested for docstrings.
I was thinking of
raise ValueError('conversion_quality must be one of {0!r}'.format(
list(conversion_dict.keys())))
Or does this violate any guidelines?
I just realized that .keys()
is unnecessary, so it would be
raise ValueError('conversion_quality must be one of {0!r}'.format(
list(conversion_dict)))
Oh, and the !r
isn't necessary either, right?
raise ValueError('conversion_quality must be one of {0}'.format(
list(conversion_dict)))
And a single replacement without specific formatting at the end of the string is kinda strange, right?
Yet another option would be
raise ValueError('conversion_quality must be one of ' +
repr(list(conversion_dict)))
sounddevice.py
Outdated
raise TypeError('channel_map must be a list or tuple') | ||
if isinstance(channel_map, (list, tuple)) \ | ||
and len(channel_map)==0: | ||
raise TypeError('channel_map may not be empty') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
sounddevice.py
Outdated
output channels. | ||
|
||
For input devices, *channel_map* is a list of integers | ||
specifying the (zero-based) channel numbers to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some repetition here, this is basically the same as two sentences before.
Can this be avoided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Is it still coherent if I remove the first sentence? (will include in next commit)
sounddevice.py
Outdated
channels. | ||
|
||
See the example below. For additional information, see | ||
https://app.assembla.com/spaces/portaudio/git/source/master/src/hostapi/coreaudio/notes.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URLs can actually be broken, but the syntax admittedly is a bit obscure.
See the example below. For additional information, see
`<https://app.assembla.com/spaces/portaudio/git/source/
master/src/hostapi/coreaudio/notes.txt>`_.
Something like this would also be possible:
See the example below. For additional information, see the
`PortAudio documentation`__.
__ https://app.assembla.com/spaces/portaudio/git/source/
master/src/hostapi/coreaudio/notes.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
sounddevice.py
Outdated
If ``True``, allows PortAudio to change things like the | ||
device's frame size, which allows for much lower latency, | ||
but might disrupt the device if other programs are using it, | ||
even when you are just Querying the device. ``False`` is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
querying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, Thanks!
sounddevice.py
Outdated
------- | ||
This example assumes a device having 6 input and 6 output | ||
channels. Input is from the second and fourth channels, and | ||
output will be sent to the third and fifth: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be clear already, but to make it even more obvious, you could write something like "... and the two output channels will be sent to the third and fifth output channel of the device, respectively".
Or would this be more confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, OK I took another pass at it. :)
sounddevice.py
Outdated
>>> ch_map_in = [1, 3] | ||
>>> ca_in = sd.CoreAudioSettings(channel_map=ch_map_in) | ||
>>> ch_map_out = [-1, -1, 0, -1, 1, -1] | ||
>>> ca_out = sd.CoreAudioSettings(channel_map=ch_map_out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about avoiding the profusion of variable names and compressing those lines?
>>> import sounddevice as sd
>>> ca_in = sd.CoreAudioSettings(channel_map=[1, 3])
>>> ca_out = sd.CoreAudioSettings(channel_map=[-1, -1, 0, -1, 1, -1])
>>> sd.playrec(..., channels=2, extra_settings=(ca_in, ca_out))
BTW, for code examples I don't follow the 72 columns limit of docstrings. The code itself should be limited to 79 columns, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
sounddevice.py
Outdated
>>> ch_map_out = [-1, -1, 0, -1, 1, -1] | ||
>>> ca_out = sd.CoreAudioSettings(channel_map=ch_map_out) | ||
>>> sd.playrec(..., channels=2, extra_settings=(ca_in, ca_out)) | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood correctly, the NumPy docstring style suggests to end each docstring with a blank line. At least that's what I've done everywhere else ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK!
sounddevice.py
Outdated
In combination with the above flag, ``True`` causes the | ||
stream opening to fail, unless the exact sample rates are | ||
supported by the device. | ||
conversion_quality : string, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not a fan of long names, but in this case I think it is better to follow the sub-optimal PortAudio naming instead of coming up with our own names which may have problems on their own. This way we can at least blame it on PortAudio.
I am very anal about line lengths, but that doesn't mean I would never accept exceptions.
In this case I would make an exception.
sounddevice.py
Outdated
|
||
try: | ||
self._flags = conversion_dict[conversion_quality.lower()] | ||
except KeyError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could make a little tweak here and use except (KeyError, AttributeError):
, which would also raise our error message on non-string input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
sounddevice.py
Outdated
if isinstance(channel_map, int): | ||
raise TypeError('channel_map must be a list or tuple') | ||
if isinstance(channel_map, (list, tuple)) \ | ||
and len(channel_map)==0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should stay on the safe side and raise an error.
And yes, the comment makes sense, since it isn't obvious at first glance why we would check for int
specifically.
I still don't like the list
and tuple
check. It may not make a practical difference now, it is rather a philosophical question.
And theoretically, the CFFI call might allow more iterables in the future, and then your check will not detect if those other iterables are empty.
I still think that the best way to handle this is to check for int
before the call to _ffi.new()
and check for len(...) == 0
afterwards.
This is getting quite ideological, so feel free to refuse my suggestion.
sounddevice.py
Outdated
} | ||
|
||
# Minimal checking on channel_map to catch errors that might otherwise | ||
# go unnotice: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnoticed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doh.... thanks!
sounddevice.py
Outdated
try: | ||
self._flags = conversion_dict[conversion_quality] | ||
except KeyError: | ||
raise ValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is 79 columns, 72 is only suggested for docstrings.
I was thinking of
raise ValueError('conversion_quality must be one of {0!r}'.format(
list(conversion_dict.keys())))
Or does this violate any guidelines?
I just realized that .keys()
is unnecessary, so it would be
raise ValueError('conversion_quality must be one of {0!r}'.format(
list(conversion_dict)))
Oh, and the !r
isn't necessary either, right?
raise ValueError('conversion_quality must be one of {0}'.format(
list(conversion_dict)))
And a single replacement without specific formatting at the end of the string is kinda strange, right?
Yet another option would be
raise ValueError('conversion_quality must be one of ' +
repr(list(conversion_dict)))
sounddevice.py
Outdated
|
||
def __init__(self, channel_map=None, change_device_parameters=False, | ||
fail_if_conversion_required=False, | ||
conversion_quality='max'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second line break is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
sounddevice.py
Outdated
raise ValueError( | ||
'conversion_quality must be one of {0!r}'.format( | ||
list(conversion_dict.keys()))) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it!
ca8b1eb
to
71d767d
Compare
@mgeier So, next round of edits? I like the short, concise nature that the code is being distilled down to, error checks and all. :) Also, do you have any preference on if I should squash the commits down into 1? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, next round of edits?
That's the spirit!
I was hoping this would be the last round, but I found another PEP8 violation.
The other comment is just me trying to improve my English ...
I also like how it looks now, thanks for your work on it! It's a simple thing, so the code should be simple, too.
Also, do you have any preference on if I should squash the commits down into 1?
Yes, if you don't mind I'd prefer to have this as a single commit in the end, since it adds one thing, it doesn't really make sense to split this into several commits, right?
But there is still one question open: does this stuff actually work?
I tried the channel_map
a bit, and the output mapping seems to work. I assume the input works, too, but I didn't fully check this (it gave me one channel, but I didn't check if it was the one I asked for).
How can I check if change_device_parameters
does anything?
How can I provoke a failure with fail_if_conversion_required
?
I think I'm too lazy to investigate if conversion_quality
works, did you check this?
sounddevice.py
Outdated
------- | ||
This example assumes a device having 6 input and 6 output | ||
channels. Input is from the second and fourth channels, and two | ||
output channels will be sent to the third and fifth outputs of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I'm not a native speaker, so I'm likely to be wrong, but shouldn't that be "output"?
Same for "channel" in the line above?
Or are both options possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are possible. :) As a native speaker, English is an annoying beast:
In this case, I'm using "outputs" as a noun and "output" as adjective. But to be very annoying, I could change up the text a little to also use it as a verb:
This example assumes a device having 6 input and 6 output
channels. Input is from the second and fourth channels, and
two channels will be output to the third and fifth outputs of
the device, respectively:
In the above text, the first "output" is used as an adjective to modify the following word "channels". The second "output" is being used as a verb (replacing the word "sent"), and the third "output" is a noun to refer to the physical device outputs.
Some amusing background: https://en.wikipedia.org/wiki/Buffalo_buffalo_Buffalo_buffalo_buffalo_buffalo_Buffalo_buffalo
A guiding principle is to eliminate repetitive phrases, and thus I was trying hard to not say "output channels" more than once.
I think the current text is probably fine enough for technical literature, though it won't win poetry awards. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the English lesson, very much appreciated!
I would like to better understand the singular vs. plural angle, which I'm still having problems with.
To use another example (that cannot be used as adjective or verb AFAIK):
"They sent their second and fourth children to college."
"They sent their second and fourth child to college."
Which one is correct? Are both correct?
In German (which is my native language) this would call for the singular, plural would be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined toward the first option: "second and fourth children". In this wording, it is clear that there is more than 1 child that went to college.
If you wanted to use the singular form "child", then you'd need to include it twice, like so:
"They sent their second child and fourth child to college."
But then this starts to sound a little repetitive with using "child" twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, very interesting!
sounddevice.py
Outdated
if channel_map is not None: | ||
# this array must be kept alive! | ||
self._channel_map = _ffi.new('SInt32[]', channel_map) | ||
if len(self._channel_map)==0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8 suggests (and I very much endorse) spaces around ==
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! Now with more pep!
So I don't see a get_channel_name function any more. Isn't this one of the main things you wanted to implement? Or are we planning to discuss host specific APIs in another issue? |
@tgarc AFAIU, @dholl will make further suggestions in new PRs, see #83 (comment). I think it's good to get this |
@tgarc Yep! I dropped Prelude: Example usage: import sounddevice as sd
# sd.hostapis is a namedtuple, so it supports
# the usual variety of access mechanisms:
# sd.hostapis[ndx].name
# sd.hostapis.coreaudio.name
# getattr(sd.hostapis, 'coreaudio').name
for hostapi in sd.hostapis:
print hostapi.name # prints "Core Audio" - may this change with future i8n efforts in PortAudio?
print hostapi.devices # usual list of devices...
print hostapi.default_input_device
print hostapi.default_output_device
print hostapi.apiname # prints "coreaudio" - this will be constant regardless of locale.
print repr(hostapi.api)
if hasattr(hostapi.api, 'get_input_channel_name'):
# Both 'coreaudio' and 'asio' support api.get_input_channel_name(device, chan)
for device in hostapi.devices:
num_ch = sd.query_devices(device)['max_input_channels']
for ch in range(num_ch):
ch_name = hostapi.api.get_input_channel_name(device, ch)
print('device {} ch {} input name {!r}'.format(device, ch, ch_name))
if hasattr(hostapi.api, 'get_output_channel_name'):
# Both 'coreaudio' and 'asio' support api.get_output_channel_name(device, chan)
for device in hostapi.devices:
num_ch = sd.query_devices(device)['max_output_channels']
for ch in range(num_ch):
ch_name = hostapi.api.get_output_channel_name(device, ch)
print('device {} ch {} output name {!r}'.format(device, ch, ch_name))
# This is another named tuple, containing the functions and
# CoreAudioSettings class, so we might move *Settings classes
# out of the global namespace: AsioSettings, CoreAudioSettings,
# WasapiSettings. @mgeier As requested, here's a list of host-API-specific features that I think can be handled, and have created an initial draft in my asio:
.api.Settings (AsioSettings)
.api.get_available_buffer_sizes
.api.get_input_channel_name
.api.get_output_channel_name
.api.set_stream_sample_rate
coreaudio:
.api.Settings (CoreAudioSettings)
.api.get_input_channel_name
.api.get_output_channel_name
.api.get_buffer_size_range
alsa:
.api.enable_realtime_scheduling
.api.get_stream_input_card
.api.get_stream_output_card
.api.set_num_periods
.api.set_retries_busy
wasapi:
.api.Settings (WasapiSettings)
.api.get_frames_per_host_buffer Note: In my branch, these .api.* names are presently available via both chan_name = query_hostapis(name="Core Audio")['api'].get_input_channel_name(device, channel)
chan_name = hostapis.coreaudio.api.get_input_channel_name(device, channel) At the moment, I've only tested Core Audio, and I'd love for other folks to test the others if they already have a hardware setup. I'm open to handing out contributor access to this branch in my fork to expedite the work flow. okay, now back to the current PR to finish |
@dholl Okay, I'm looking forward to discussing this! |
0e72c5b
to
a350aeb
Compare
Squashed into 1 commit! I also shortened the example text a bit more:
|
I hope so. ;)
I tested with 2 mic's plugged into different ports on a UFX+, and it does let me switch.
When I use it, it changes
Now, run at 96 kHz without
And list devices again:
Now we'll do the same run again, but this time, my
And when we re-run
I also notice subtle pops from the speakers when I'm changing the sampling rate with
Trying 8 kHz without
Note the default sample rate is still 96000:
(Note: Core Audio returns empty strings for channel names of builtin inputs and outputs. But on a UFX+, I do get useful strings such as 'Mic 9' and whatnot.) So now. with
But now the default_sample_rate is 44.1 kHz:
So running one last time to test
So that's good that we see an error. However, running again without
This unintuitive enforcement of
So from a user application standpoint, if it wants
Ha! Me too... I wasn't in the mood today for running any spectral analysis such as identifying the phase/magnitude response or intermodulation distortion. Though, I could conceivable do something with getting a UFX+ to loop back my mac's output to software. For now, without having a paying contract in place ;) I've only run my tool with each of the 5 conversion algorithms, and they each sound too similar to my untrained ears at conversion from 44.1 kHz to 8 kHz. So, all this verified is that the underlying Port Audio and Core Audio layers are not vomiting.
Hmm, looking at my CPU load of coreaudiod, the load doesn't seem to vary much between max, high, medium, low, or min. All are around 25-27% |
Thanks for the detailed writeup! I couldn't reproduce the change of default samplerate with my quick tests, but I could get the strange error messages and the proper exception as you described. The different CPU load of different quality settings is probably only visible in extreme cases, I'm going to assume that the selection works. I guess most people will use the default |
These 3 commits access PA's Core Audio API from pa_mac_core.h, and provide a "get_channel_name" function for querying names of individual device channels along with MacCoreSettings (for use with extra_settings) to expose additional Core Audio capabilities. (See doc string in MacCoreSettings.)
My specific need for these extensions is that I have an RME Fireface UFX+ with a large number of input & output channels, and I didn't want to have to open many channels when only a couple are required. The get_channel_name function lets me display obtain a set of user-friendly channel names from the underlying device driver (via PortAudio), which I may display to the operator / user.
I'm open for input to help get these changes incorporated into python-sounddevice.
Thank you,
David