Channel names: CoreAudio and ASIO - #1254
Conversation
|
Sorry for the late reply on this one. I think this is a worthwhile addition as a trait method. You could add a default implementation to the trait method to prevent the duplication at each backend that doesn't support it. Before we go further, could you rebase this onto the Happy to take a deeper look once it's rebased! |
c27ac0a to
f2ba085
Compare
doc # Conflicts: # src/platform/mod.rs # src/traits.rs introduce default implementation a update example device
simplify unsafe structure
f2ba085 to
8374eff
Compare
|
No problem regarding the delay. I rebased and implemented the comments while trying to keep a compact, readable git history. Not sure why that specific Linux check is failing, though... |
roderickvd
left a comment
roderickvd
left a comment
There was a problem hiding this comment.
Thanks again, please find my first review attached.
I've promoted develop to master, so please rebase on master where we'll continue the 0.19.0 work. This should be easier to the last rebase, as it's just continuing the develop branch under a new name, basically.
|
|
||
| fn get_channel_name_for_device( | ||
| device_id: AudioDeviceID, | ||
| channel_index: u16, |
There was a problem hiding this comment.
Querying an out-of-range channel_index will return a BackendError I think, when InvalidInput would be preferred.
| .collect(); | ||
|
|
||
| let input_channel_names: Box<[String]> = (0..channels.ins) | ||
| .map(|ch| driver.channel_name(ch, true).unwrap_or_default()) |
There was a problem hiding this comment.
Question: rather than defaulting to an empty string, what if we returned something recognizable like "Channel {ch}"?
| pub fn channel_name(&self, channel: i32, is_input: bool) -> Result<String, AsioError> { | ||
| let _guard = self.inner.lock_state(); | ||
| let info = asio_channel_info(channel, is_input)?; | ||
| Ok(driver_name_to_utf8(&info.name).into_owned()) |
There was a problem hiding this comment.
I think this was already the case elsewhere with other names, but it's occurring to me that this will UB if there's ever a driver that doesn't NUL-terminate.
| .filter(|&r| driver.can_sample_rate(r.into()).unwrap_or(false)) | ||
| .collect(); | ||
|
|
||
| let input_channel_names: Box<[String]> = (0..channels.ins) |
There was a problem hiding this comment.
We may want to check that the number of channels is greater than (or equal to) 0. I remember that ASIO often returns an i32 and we wouldn't want a negative value overflowing this.
| channel_index: u16, | ||
| input: bool, | ||
| ) -> Result<String, Error> { | ||
| let mut channel_name: *mut CFString = std::ptr::null_mut(); |
There was a problem hiding this comment.
As this CFString stuff is getting used more often, we could consider having a helper for it.
| } | ||
|
|
||
| fn get_channel_name(&self, channel_index: u16, input: bool) -> Result<String, Error> { | ||
| if input && !self.supports_input() { |
There was a problem hiding this comment.
supports_input/output aren't free, so if it's just to detail the error message then maybe we should just use the input argument but not re-query the device (or cache it, but that may be scope creep).
| input_sample_format: Option<SampleFormat>, | ||
| output_sample_format: Option<SampleFormat>, | ||
| supported_sample_rates: Box<[SampleRate]>, | ||
| input_channel_names: Box<[String]>, |
There was a problem hiding this comment.
CoreAudio doesn't seem to cache this. What's the preferred approach? Lazily like CoreAudio or caching it during enumeration here?
| @@ -943,6 +943,18 @@ impl Driver { | |||
| let mut dcb = DRIVER_EVENT_CALLBACKS.lock().unwrap(); | |||
| dcb.retain(|&(id, _)| id != rem_id); | |||
There was a problem hiding this comment.
cpal doesn't use get_ for accessors.
| /// (`false`) direction. | ||
| /// | ||
| /// The driver must already be loaded (i.e. this `Driver` instance must be alive). | ||
| pub fn channel_name(&self, channel: i32, is_input: bool) -> Result<String, AsioError> { |
There was a problem hiding this comment.
Not sure about the input: bool or is_input: bool arguments in public functions. In the rest of cpal, that's split between supports_input/output, default_input/output_config, etc. That's more readable than channel_name(1, true) - what argument isn't self-explanatory.
2 participants
This PR refers to this issue.
I implemented a version of this to showcase how I imagined it. When I wrote the issue, I hadn't realized that the error convention had changed from 0.16 to 0.17. This PR adheres to the current error convention.