Skip to content
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

Merged
merged 1 commit into from
Apr 25, 2017

Conversation

dholl
Copy link
Contributor

@dholl dholl commented Apr 15, 2017

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

@dholl dholl changed the title Support audio interfaces under Core Audio Core Audio API for audio interfaces with large #'s of channels Apr 15, 2017
@dholl dholl changed the title Core Audio API for audio interfaces with large #'s of channels Core Audio API for interfaces with large #'s of channels Apr 15, 2017
@dholl dholl changed the title Core Audio API for interfaces with large #'s of channels Core Audio for interfaces with large #'s of channels Apr 15, 2017
@tgarc
Copy link
Contributor

tgarc commented Apr 17, 2017

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.

@dholl
Copy link
Contributor Author

dholl commented Apr 17, 2017

For the two API changes in my current commits, here are two alternative implementations that cross my mind:

  1. get_channel_name
    Would this functionality be better merged into with query_devices? For example, in the dict returned for each device, perhaps each dict could contain additional fields called "input_channel_names" and "output_channel_names" which each contain a list (or tuple) of strings, 1 string per channel. This way, hostapi's such as ASIO and Core Audio could be queried for whatever underlying names. (For ASIO, is the equivalent underlying API "PaAsio_GetOutputChannelName" and "PaAsio_GetInputChannelName"?)

  2. MacCoreSettings
    This struct has at least field in common with AsioSettings, specifically channel_selectors which is the only attribute presently exposed in AsioSettings. Regarding the Core Audio API, I considered calling this field channel_map in MacCoreSettings, but upon reading how similar in functionality it is to the field in AsioSettings, I added some minor conversion logic to translate channel_selectors into the specific format that Core Audio required -- as mentioned in my code's doc string.
    Could all 3 of the API-specific settings classes (AsioSettings, MacCoreSettings, WasapiSettings) be merged into 1 simple dict? Presently, a client application needs to select the correct ...Settings class based on the relevant device's hostapi, correct? Instead of this, how about we get rid of the classes in favor of having the application just pass in a simple dict instead?

    extra_settings_i = {
    'channel_selectors' = [8, 9], # RME UFX+ 2 mic inputs
    'change_dev_params' = True, # Let Core Audio (or any underlying API) change hardware params as needed to minimize host overhead
    'allow_rate_conv' = False, # I would rather fail and alert the user than let underlying layer (Core Audio) have to resort to sample rate conversion.
    }
    extra_settings_o = {
    'channel_selectors' = [0, 1], # RME UFX+ first 2 outputs
    'change_dev_params' = True, # same as extra_settings_i
    'allow_rate_conv' = False,  # same as extra_settings_i
    }
    with sd.RawStream(device=..., extra_settings=(extra_settings_i, extra_settings_o), ...)
      ...
    

    These dicts could then be passed to a correct hostapi-specific class or function from within _get_stream_parameters. For example:

    def _get_stream_parameters(kind, device, channels, dtype, latency,
                               extra_settings, samplerate):
      ...
      if samplerate is None:
          samplerate = info['default_samplerate']
      if extra_settings:
          # TODO: build dict extra_settings_xlate to map hostapi to a callable to translate extra_settings into hostapi-specific _streaminfo.
          #extra_settings_xlate = {
          #    ...
          #}
          # Invoke the callable (function or object) that is specific for this hostapi:
          this_streaminfo = extra_settings_xlate[info['hostapi']](kind=kind, device=device, channels=channels, sampleformat=sampleformat, latency=latency, extra_settings=extra_settings)
      else:
          this_streaminfo = _ffi.NULL
      parameters = _ffi.new('PaStreamParameters*', (
          device, channels, sampleformat, latency,
          this_streaminfo))
    
    • Aside: Doing so would also make the implementation of MacCoreSettings a little clearer, since I require access to the device's max_output_channels to translate channel_selectors into Core Audio's channel_map. At the moment, I defer creating a PaMacCoreStreamInfo struct until _get_stream_parameters, be having that function discover that MacCoreSettings._streaminfo is callable and then invoking it with device-specific params passed in via **kwargs.
      • I'll admit this is implementation is more complicated that I care for, but at least if an application tries using my current revision of MacCoreSettings, then it doesn't have to explicitly care about max_output_channels, and it may use use the same format of channel_selectors for both MacCoreSettings and AsioSettings.
    • So to keep things simple in my initial pull request, I just created MacCoreSettings in a similar fashion to AsioSettings and WasapiSettings, to keep my changes as similar to the rest of python-sounddevice as possible.

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.

@dholl
Copy link
Contributor Author

dholl commented Apr 17, 2017

To help with brainstorming, what are the ASIO functions you'd like to integrate?

@tgarc
Copy link
Contributor

tgarc commented Apr 17, 2017

Most of all I'd like to get GetAvailableBufferSizes added. But GetInputChannelName/GetOutputChannelName could also be useful.

@dholl
Copy link
Contributor Author

dholl commented Apr 17, 2017

Hmm, and Core Audio offers similar data via kAudioDevicePropertyBufferFrameSizeRange, which is exposed via PaMacCore_GetBufferSizeRange, though this call does not return preferredBufferSizeFrames or granularity.

Perhaps could this data also be returned by query_devices along with channel names? Maybe via a dict key such as:

{
'buffer_sizes': range(minBufferSizeFrames, maxBufferSizeFrames, granularity) if granularity!=-1 else power_of_two_range(minBufferSizeFrames, maxBufferSizeFrames),
}
#TODO: define power_of_two_range

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 list() or tuple() or whatever as desired.) To prevent accidental expansion of the range of values, xrange would be used on Python 2, whereas Python 3 can stick with just range.

On Core Audio, I think granularity may be hard coded to +1 (is there a way to tell this?), but I'm not sure what's the equivalent to preferredBufferSizeFrames. Is preferredBufferSizeFrames desirable, or could knowledge of preferredBufferSizeFrames be incorporated into the library in another fashion such as setting the default latency or equivalent?

@tgarc
Copy link
Contributor

tgarc commented Apr 18, 2017

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.

@mgeier
Copy link
Member

mgeier commented Apr 18, 2017

Thanks @dholl and @tgarc!

I'm open to changes in the host API specific settings, including abandoning the current AsioSettings and WasapiSettings if we find something better.

I'm reluctant to add one or more top-level functions that are only available on a subset of host APIs.
I think it would be nice to clearly see which features are platform-specific and which aren't.
However, I have no idea how this should be done ...

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 PaAlsa_EnableRealtimeScheduling, which has to be called on the stream (but before starting it) and PaAlsa_SetNumPeriods which (I guess) has to be called before the stream is created.

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 channelSelectors from ASIO as they are (without converting the numbers to 1-based), wouldn't it be better to expose the CoreAudio channel maps also without converting them to something else?
In the end this would make it easier for people who already know the native CoreAudio settings and it would simplify the implementation.

sounddevice.py Outdated

>>> maccore_in = sd.MacCoreSettings(channel_selectors=[8, 9])
>>> sd.default.extra_settings = maccore_in, maccore_out
>>> sd.playrec(..., channels=1, ...)
Copy link
Contributor

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?

Copy link
Contributor Author

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:
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Member

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 ints, 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounddevice.py Outdated
# settings such as AsioSettings or WasapiSettings and not allow
# any accidental reliance on positional parameters.

if not isinstance(channels, int):
Copy link
Contributor

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.

Copy link
Contributor Author

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:
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@dholl
Copy link
Contributor Author

dholl commented Apr 20, 2017

I'll update the pull request to incorporate the feedback thus far... :)

@dholl
Copy link
Contributor Author

dholl commented Apr 20, 2017

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 d is my device id...

hostapi_id = sd.query_devices(device=d)['hostapi']

# My device happens to be from Core Audio:
coreaudio = sd.query_hostapis(hostapi_id)['api']
min_size, max_size = coreaudio.get_buffer_size_range(device=d, channel=0, input=True)
out_chan0_name = coreaudio.get_channel_name(device=d, channel=0, input=False)
in_chan0_name = coreaudio.get_channel_name(device=d, channel=0, input=True)

On someone else's system, hostapi_id 0 might be ASIO, so perhaps that namespace might look like:

# This device happens to be from ASIO:
asio = sd.query_hostapis(hostapi_id)['api']
min_size, max_size, preferred_size, gran = asio.get_available_buffer_sizes(device=d)
out_chan0_name = asio.get_output_channel_name(device=d, channel=0)
in_chan0_name = asio.get_input_channel_name(device=d, channel=0)

An application may of course need to check sd.query_hostapis(hostapi_id)['name'] before assuming a given specific API.

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?

@mgeier
Copy link
Member

mgeier commented Apr 20, 2017

I like this idea!
I would even remove one level of indirection. Instead of returning dicts from query_hostapis(), we could return custom objects which have __getitem__() for key/value access but also provide attribute access directly. The dict-like access could stay backwards compatible this way.

The detour over query_devices() seems a bit tedious (especially if you want to write code that works with several host APIs), we could probably extend query_hostapis() to take a string like query_hostapis('alsa')?
If the host API is not available, this could return None, allowing something like this:

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).
I doubt that all features can be provided in a meaningful way in the suggested scheme, but probably many of them can.

@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)?

@dholl
Copy link
Contributor Author

dholl commented Apr 20, 2017

@mgeier Regarding query_hostapis(), would using a named parameter be acceptable? This would make it explicit when a caller wants to look up an API by string name?

alsa = query_hostapis(name='alsa')
if alsa:
    alsa.set_something('blah')

So the function would be defined as:

def query_hostapis(index=None, name=None):
    ...

@dholl
Copy link
Contributor Author

dholl commented Apr 20, 2017

OK, I just simplified MacCoreSettings a great deal, thus making it very similar in mentality to Asio and Wasapi. Questions or comments?

If I get rid of get_channel_name() (in favor of moving that functionality to query_hostapis() in a separate PR), is MacCoreSettings getting closer to being acceptable for inclusion? :)

[For now with this PR, I'm holding back on further ideas for refactoring the *Settings classes...]

@mgeier
Copy link
Member

mgeier commented Apr 21, 2017

Thanks @dholl!
Yes, the name parameter sounds very good!

is MacCoreSettings getting closer to being acceptable for inclusion? :)

Yes, definitely, that's a much simpler and less intrusive change.
I will do a thorough review soon, but for now let me start with the name: The PortAudio implementation indeed uses MacCore..., but the user-visible host API name is Core Audio. Wouldn't CoreAudioSettings therefore make more sense?

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;*/
Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Member

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

Copy link
Member

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"?

Copy link
Contributor Author

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):
Copy link
Member

@mgeier mgeier Apr 21, 2017

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

Copy link
Contributor Author

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
Copy link
Member

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]?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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
Copy link
Member

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!

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

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.)

Copy link
Member

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'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line too long.

Copy link
Contributor Author

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 ,
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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"?

Copy link
Contributor Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str

Copy link
Member

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?

Copy link
Contributor Author

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. ;) )

Copy link
Member

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.

@mgeier
Copy link
Member

mgeier commented Apr 22, 2017

@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:
Copy link
Member

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?

Copy link
Contributor Author

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):
Copy link
Member

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.

Copy link
Contributor Author

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:
Copy link
Member

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:
    ...

Copy link
Member

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.

Copy link
Contributor Author

@dholl dholl Apr 22, 2017

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.

Copy link
Member

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(
Copy link
Member

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?

Copy link
Contributor Author

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. :)

Copy link
Member

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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must not

Copy link
Contributor Author

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.
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

querying

Copy link
Contributor Author

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:
Copy link
Member

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?

Copy link
Contributor Author

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)
Copy link
Member

@mgeier mgeier Apr 23, 2017

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.

Copy link
Contributor Author

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))
"""
Copy link
Member

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 ...

Copy link
Contributor Author

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
Copy link
Member

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:
Copy link
Member

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.

Copy link
Contributor Author

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:
Copy link
Member

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnoticed

Copy link
Contributor Author

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(
Copy link
Member

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'):
Copy link
Member

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.

Copy link
Contributor Author

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())))

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it!

@dholl dholl force-pushed the master branch 3 times, most recently from ca8b1eb to 71d767d Compare April 23, 2017 16:08
@dholl
Copy link
Contributor Author

dholl commented Apr 24, 2017

@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?

Copy link
Member

@mgeier mgeier left a 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
Copy link
Member

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?

Copy link
Contributor Author

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. :)

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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:
Copy link
Member

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 ==.

Copy link
Contributor Author

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!

@tgarc
Copy link
Contributor

tgarc commented Apr 24, 2017

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?

@mgeier
Copy link
Member

mgeier commented Apr 24, 2017

@tgarc AFAIU, @dholl will make further suggestions in new PRs, see #83 (comment).

I think it's good to get this CoreAudioSettings thing out of the way and then concentrate on a new and improved API for platform-specifics.

@dholl
Copy link
Contributor Author

dholl commented Apr 24, 2017

@tgarc Yep! I dropped get_channel_name from this PR, because I started working on a larger PR that I won't bother submitting until this one is done.

Prelude:
I created a branch in my repo fork called "hostapis", that I'm eager to start discussing. It incorporates the name= parameter to query_hostapis(), as well as an alternative idea could replace query_hostapis() completely, if folks like it. Of course this branch's code has a bunch of style issues..., but at first, I'm looking forward to at least start exploring this alternative access strategy (simply called hostapis), or brainstorming upon some hybrid between query_hostapis and hostapis. The underlying implementation isn't pretty (namedtuple's galore!), but I hope that an open code-sharing will ameliorate any implementation ugliness.

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 hostapis branch:

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 CoreAudioSettings... :)

@tgarc
Copy link
Contributor

tgarc commented Apr 24, 2017

@dholl Okay, I'm looking forward to discussing this!

@dholl dholl force-pushed the master branch 2 times, most recently from 0e72c5b to a350aeb Compare April 24, 2017 17:34
@dholl
Copy link
Contributor Author

dholl commented Apr 24, 2017

Squashed into 1 commit! I also shortened the example text a bit more:

        This example assumes a device having 6 input and 6 output
        channels.  Input is from the second and fourth channels, and
        output is to the device's third and fifth channels:

@dholl
Copy link
Contributor Author

dholl commented Apr 24, 2017

@mgeier

But there is still one question open: does this stuff actually work?

I hope so. ;)

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).

I tested with 2 mic's plugged into different ports on a UFX+, and it does let me switch.

How can I check if change_device_parameters does anything?

When I use it, it changes default_samplerate as returned from query_devices(). Here's a device list with my laptop's built-in hardware and my own test application:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -L                                    
Device list:
ID |         name          |   hostapi    | default_samplerate | max_input_channels | default_high_input_latency | default_low_input_latency | max_output_channels | default_high_output_latency | default_low_output_latency
---+-----------------------+--------------+--------------------+--------------------+----------------------------+---------------------------+---------------------+-----------------------------+---------------------------
 0 | 'Built-in Microphone' | 'Core Audio' |      48000.0       |         2          |    0.012104166666666666    |   0.0027708333333333335   |          0          |             0.1             |            0.01           
 1 |   'Built-in Output'   | 'Core Audio' |      48000.0       |         0          |            0.1             |           0.01            |          2          |    0.020020833333333335     |         0.0106875

Channel names for device 0, Built-in Microphone:
        Ch# | Input | Output
        ----+-------+-------
          0 | ''    |
          1 | ''    |

Channel names for device 1, Built-in Output:
        Ch# | Input | Output
        ----+-------+-------
          0 |       | ''
          1 |       | ''

Now, run at 96 kHz without change_device_parameters. Here I'm inputting 1 channel from the microphone, and outputting to the right speaker:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 -s 96000
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.10139583333333334, 0.03536458333333333)
time = 342188.05329327105
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 96000.0
samplesize = (4, 4)

And list devices again:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -L                                                    
Device list:
ID |         name          |   hostapi    | default_samplerate | max_input_channels | default_high_input_latency | default_low_input_latency | max_output_channels | default_high_output_latency | default_low_output_latency
---+-----------------------+--------------+--------------------+--------------------+----------------------------+---------------------------+---------------------+-----------------------------+---------------------------
 0 | 'Built-in Microphone' | 'Core Audio' |      48000.0       |         2          |    0.012104166666666666    |   0.0027708333333333335   |          0          |             0.1             |            0.01           
 1 |   'Built-in Output'   | 'Core Audio' |      48000.0       |         0          |            0.1             |           0.01            |          2          |    0.020020833333333335     |         0.0106875         

Channel names for device 0, Built-in Microphone:
        Ch# | Input | Output
        ----+-------+-------
          0 | ''    |       
          1 | ''    |       

Channel names for device 1, Built-in Output:
        Ch# | Input | Output
        ----+-------+-------
          0 |       | ''    
          1 |       | ''    

Now we'll do the same run again, but this time, my --tweakdev 1 enables change_device_parameters:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 --tweakdev 1 -s 96000
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.04921875, 0.019135416666666665)
time = 342375.305821312
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 96000.0
samplesize = (4, 4)

And when we re-run query_devices(), we see that default_samplerate is now 96 kHz:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -L                                                                 
Device list:
ID |         name          |   hostapi    | default_samplerate | max_input_channels | default_high_input_latency | default_low_input_latency | max_output_channels | default_high_output_latency | default_low_output_latency
---+-----------------------+--------------+--------------------+--------------------+----------------------------+---------------------------+---------------------+-----------------------------+---------------------------
 0 | 'Built-in Microphone' | 'Core Audio' |      96000.0       |         2          |    0.006552083333333333    |   0.0018854166666666668   |          0          |             0.1             |            0.01           
 1 |   'Built-in Output'   | 'Core Audio' |      96000.0       |         0          |            0.1             |           0.01            |          2          |    0.013802083333333333     |    0.009135416666666667   

Channel names for device 0, Built-in Microphone:
        Ch# | Input | Output
        ----+-------+-------
          0 | ''    |       
          1 | ''    |       

Channel names for device 1, Built-in Output:
        Ch# | Input | Output
        ----+-------+-------
          0 |       | ''    
          1 |       | ''    

I also notice subtle pops from the speakers when I'm changing the sampling rate with change_device_parameters, but if I specify the same sampling rate, using change_device_parameters does not introduce any pops on my laptop. (late 2016 MacBook Pro)

How can I provoke a failure with fail_if_conversion_required?

Trying 8 kHz without change_device_parameters works fine, with a subtle change in sound when just rubbing my finger over the mic location on my laptop:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 -s 8000              
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.05025, 0.108875)
time = 342676.486970847
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 8000.0
samplesize = (4, 4)

Note the default sample rate is still 96000:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -L                                                   
Device list:
ID |         name          |   hostapi    | default_samplerate | max_input_channels | default_high_input_latency | default_low_input_latency | max_output_channels | default_high_output_latency | default_low_output_latency
---+-----------------------+--------------+--------------------+--------------------+----------------------------+---------------------------+---------------------+-----------------------------+---------------------------
 0 | 'Built-in Microphone' | 'Core Audio' |      96000.0       |         2          |    0.006552083333333333    |   0.0018854166666666668   |          0          |             0.1             |            0.01           
 1 |   'Built-in Output'   | 'Core Audio' |      96000.0       |         0          |            0.1             |           0.01            |          2          |    0.013802083333333333     |    0.009135416666666667   

Channel names for device 0, Built-in Microphone:
        Ch# | Input | Output
        ----+-------+-------
          0 | ''    |       
          1 | ''    |       

Channel names for device 1, Built-in Output:
        Ch# | Input | Output
        ----+-------+-------
          0 |       | ''    
          1 |       | ''    

(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 change_device_parameters, the same 8 kHz request produces a couple error messages, but playback continues as expected:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 --tweakdev 1 -s 8000
||PaMacCore (AUHAL)|| Error on line 406: err=''!dat'', msg=Audio Device: Unsupported Format
||PaMacCore (AUHAL)|| Error on line 406: err=''!dat'', msg=Audio Device: Unsupported Format
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.077, 0.062125)
time = 342830.96096854704
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 8000.0
samplesize = (4, 4)

But now the default_sample_rate is 44.1 kHz:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -L                                                                
Device list:
ID |         name          |   hostapi    | default_samplerate | max_input_channels | default_high_input_latency | default_low_input_latency | max_output_channels | default_high_output_latency | default_low_output_latency
---+-----------------------+--------------+--------------------+--------------------+----------------------------+---------------------------+---------------------+-----------------------------+---------------------------
 0 | 'Built-in Microphone' | 'Core Audio' |      44100.0       |         2          |    0.01310657596371882     |   0.0029478458049886623   |          0          |             0.1             |            0.01           
 1 |   'Built-in Output'   | 'Core Audio' |      44100.0       |         0          |            0.1             |           0.01            |          2          |    0.021156462585034015     |    0.010997732426303855   

Channel names for device 0, Built-in Microphone:
        Ch# | Input | Output
        ----+-------+-------
          0 | ''    |       
          1 | ''    |       

Channel names for device 1, Built-in Output:
        Ch# | Input | Output
        ----+-------+-------
          0 |       | ''    
          1 |       | ''    

So running one last time to test fail_if_conversion_required, my equivalent command line option is --norateconv 1:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 --tweakdev 1 --norateconv 1 -s 8000
||PaMacCore (AUHAL)|| Error on line 406: err=''!dat'', msg=Audio Device: Unsupported Format
Traceback (most recent call last):
  File "./dholl_wire_sounddevice.py", line 302, in <module>
    _main()
  File "./dholl_wire_sounddevice.py", line 280, in _main
    channels=args.channels, callback=callback, clip_off=not args.clip, dither_off=not args.dither) as s:
  File "/Users/dholl/Library/Python/2.7/lib/python/site-packages/sounddevice.py", line 1551, in __init__
    **_remove_self(locals()))
  File "/Users/dholl/Library/Python/2.7/lib/python/site-packages/sounddevice.py", line 1077, in __init__
    'Error opening {0}'.format(self.__class__.__name__))
  File "/Users/dholl/Library/Python/2.7/lib/python/site-packages/sounddevice.py", line 3149, in _check
    raise PortAudioError(msg)
sounddevice.PortAudioError: Error opening RawStream: Invalid sample rate
zsh: exit 1     ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1  1 

So that's good that we see an error.

However, running again without --tweakdev 1 but still with --norateconv 1, we find that no error messages are printed nor is any exception raised:

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 --norateconv 1 -s 8000 
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.077, 0.062125)
time = 343026.211521728
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 8000.0
samplesize = (4, 4)

This unintuitive enforcement of fail_if_conversion_required only when using change_device_parameters is consistent with the doc string in CoreAudioSettings, which I shamelessly lifted from http://portaudio.com/docs/v19-doxydocs/pa__mac__core_8h.html#a481969bad982aec7ba2b8503557533c7 . Specifically, the part stating In combination with the above flag, ...:

        change_device_parameters : bool, optional
            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
            the default.
        fail_if_conversion_required : bool, optional
            In combination with the above flag, ``True`` causes the
            stream opening to fail, unless the exact sample rates are
            supported by the device.

So from a user application standpoint, if it wants fail_if_conversion_required=True, then it must also set change_device_parameters=True.

I think I'm too lazy to investigate if conversion_quality works, did you check this?

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.

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 -s 8000 --convquality min
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.077, 0.062125)
time = 343735.471557744
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 8000.0
samplesize = (4, 4)

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 -s 8000 --convquality low
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.077, 0.062125)
time = 343741.264996927
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 8000.0
samplesize = (4, 4)

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 -s 8000 --convquality medium
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.077, 0.062125)
time = 343972.305505788
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 8000.0
samplesize = (4, 4)

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 -s 8000 --convquality high
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.077, 0.062125)
time = 343747.151818073
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 8000.0
samplesize = (4, 4)

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -c 1 --input-chan-map _0 --output-chan-map _1 -s 8000 --convquality max   
################################################################################
press Return to quit
################################################################################
cpu_load = 0.0
latency = (0.077, 0.062125)
time = 343976.846742895
blocksize = 0
channels = (1, 1)
device = (0, 1)
dtype = ('float32', 'float32')
samplerate = 8000.0
samplesize = (4, 4)

zero(ttys002):...Python/sound> ./dholl_wire_sounddevice.py -L                                                                     
Device list:
ID |         name          |   hostapi    | default_samplerate | max_input_channels | default_high_input_latency | default_low_input_latency | max_output_channels | default_high_output_latency | default_low_output_latency
---+-----------------------+--------------+--------------------+--------------------+----------------------------+---------------------------+---------------------+-----------------------------+---------------------------
 0 | 'Built-in Microphone' | 'Core Audio' |      44100.0       |         2          |    0.01310657596371882     |   0.0029478458049886623   |          0          |             0.1             |            0.01           
 1 |   'Built-in Output'   | 'Core Audio' |      44100.0       |         0          |            0.1             |           0.01            |          2          |    0.021156462585034015     |    0.010997732426303855   

Channel names for device 0, Built-in Microphone:
        Ch# | Input | Output
        ----+-------+-------
          0 | ''    |       
          1 | ''    |       

Channel names for device 1, Built-in Output:
        Ch# | Input | Output
        ----+-------+-------
          0 |       | ''    
          1 |       | ''    

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%

@mgeier
Copy link
Member

mgeier commented Apr 25, 2017

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 'max' setting anyway.

@mgeier mgeier merged commit 85729e8 into spatialaudio:master Apr 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants