RustAudio / coreaudio-rs

A friendly rust interface to Apple's Core Audio API.
Apache License 2.0
210 stars 44 forks source link

Add proper support for handling different sample types and stream formats at runtime. #12

Closed mitchmindtree closed 8 years ago

mitchmindtree commented 9 years ago

My favourite solution to this is using something like CPAL's UnknownTypeBuffer has an intermediary.

yupferris commented 8 years ago

After looking into it, I agree that this is a pretty cool solution. I'm going to try to implement this on my fork, on a branch based on my "no-builder" branch, as it should be easiest there. If it goes well, all of that stuff will be ready to merge, as then we can provide an API that's actually safe when breaking the old one :)

yupferris commented 8 years ago

Actually, in the interest of getting those changes off of my fork specifically, do you mind if I merge them to a no-builder branch on this repo? I'd still like to keep them off of master until this issue's fixed, but I'd like to get them here and visible instead of floating away on my fork. Apologies for the unrelated discussion on this ticket; I can remove the unrelated messages afterwards :)

mitchmindtree commented 8 years ago

@yupferris yeah go for it! Though I'd probably merge your no-builder stuff with master first, and then start working on the "different sample support" as they are mostly separate issues (though I don't really mind either way :smile_cat: )

yupferris commented 8 years ago

Yeah, I'll just go ahead and merge that now. Then I'll start a branch and play with this ticket :)

mitchmindtree commented 8 years ago

Just thought I'd mention that, while doing this, it might be best to review the whole RenderCallback function in general. The buffer and number of samples (the only to arguments to the RenderCallback fn atm) are clearly insufficient :)

We should aim to provide all information produced by the extern "C" fn input_proc in a nice, rust-esque manner.

I think it'd probably be best to wrap this information in a RenderCallbackArgs struct, so that users on the api end don't have to write multiple arguments, import less types into their namespace and take advantage of pattern de-structuring if they wish.

I imagine it might look something like this:

pub struct RenderCallbackArgs {
    pub buffer: UnknownBuffer,
    pub time_stamp: TimeStamp,
    etc,
}
yupferris commented 8 years ago

Love it!

mitchmindtree commented 8 years ago

Ok, I think the crate should be in a decent enough state to implement this now :smile_cat:

While I think of it, there's another couple types that may need to be added:

Then we could have something like:

pub struct RenderCallbackArgs {
    pub buffer: UnknownBuffer,
    pub time_stamp: TimeStamp,
    pub flags: RenderActionFlags,
    pub bus_number: u32,
    pub num_frames: u32,
}

Then we can finally clean up the CPAL osx backend mess hehe.

mitchmindtree commented 8 years ago

Hey! So I've just been reading through the Audio Unit Docs and it looks like the API guarantees that the "input procedure" callback will always be called with the requested audio format as long as an error isn't returned.

AUHALs have a built-in AudioConverter to do this transformation for you. The AUHAL determines what kind of AudioConverter is required by comparing the flattened device format with the client's desired format.

You can also request the device's (default) stream format using the get_property(kAudioUnitProperty_StreamFormat...) function (which we wrapped in a AudioUnit::stream_format method).

This seems to me that API-wise, it should still be possible to create a stream with a sample format given as a generic type, handling any errors involved before giving the stream callback, rather than using an UnknownBuffer in the actual callback itself which forces a user to handle every possible format at run-time.

This is a very similar situation to the portaudio library, which also handles conversions etc behind the scenes, ensuring that the sample format you request will indeed be the one given in the callback provided it is supported and no errors are returned. I recently re-wrote the API to typify this specified behaviour, associating the stream format used to construct a stream with the callback arguments given to the callback procedure which turned out really nice.

I think we could possibly do something similar for the audio unit API here, making it possible to convert a dynamic StreamFormat (given by the AudioUnit::stream_format method) into a similar stream format type but with the audio format given as a type parameter, rather than a struct field as in the original api. This would encourage users to think about handling supported stream formats before running the stream, rather than during.

Any errors would be handled at the point of set_stream_format or set_render_callback, rather than during the stream's runtime. This behaviour should more closely reflect the behaviour specified in the technical docs. Obviously C doesn't have the type system that we do so they couldn't originally specify it using types, but we should be able to.

yupferris commented 8 years ago

Hmm, this sounds really cool! I admit it's a bit unclear to me though; are you talking about something like this?

let au = AudioUnit::new(IOType::HalOutput).unwrap();

// StreamFormat takes a type param, and now needs to return
// a Result to make sure it's a valid type
let some_format = StreamFormat::new<f32>(...).unwrap();

// Error if format not supported, otherwise we're ok (as usual)
au.set_stream_format(some_format).unwrap();

// This also takes a type param, so RenderCallback etc will also
// be parameterized and type-checked. At this point we also check
// against the stream format type and fail if it doesn't match
au.set_render_callback<f32>(...).unwrap();
yupferris commented 8 years ago

Hmm, just realized we would also need to handle the case of changing the stream format while an incompatible render callback is already set.

mitchmindtree commented 8 years ago

Yeah this crossed my mind also. I was imagining we could still provide a dynamic format option if that is what is required, but it'd be nice to find another way if possible. Keep in mind that the AudioUnit itself might not have to be type parameterised, as the callback gets Boxed upon being set, meaning that if set_render_callback::<f32> fails, it should be able to be called again afterwards in a new location. i.e.

// Bad name, but you get the idea:
fn spawn_stream_trying_format_by_preference(au: &mut Au) -> Result<(), Error> {

    fn spawn_stream<S: Sample>(au: &mut Au) -> Result<(), au::Error> {
        let some_format = StreamFormat::<S>::new(..);
        try!(au.set_stream_format(some_format));
        let callback = |args| {
            // process samples of type S, which would probably be inferred from below (or vice versa).
        };
        try!(au.set_render_callback::<S>(callback));
        Ok(())
    }

    if spawn_stream::<f32>().is_ok()
    || spawn_stream::<i32>().is_ok()
    || spawn_stream::<i16>().is_ok()
    || spawn_stream::<i8>().is_ok()
        Ok(())
    else  {
        Err(Error::DeviceDoesNotSupportAnySampleFormatsSupportedByApp)
    }
}

My preferred/ideal solution would be to distinguish the thing that spawns/runs the streams, from the streams themselves - requiring the user to spawn a new stream if they want to change formats. Changing formats is normally expensive and requires re-allocation in the backend anyway, so I highly doubt it would cause any more performance overhead to drop and spawn a new stream rather than using the C setter (this would require investigation though).

This is how the new portaudio API does it (see here) with the distinction being PortAudio -> Stream. Perhaps we could do CoreAudio -> AudioUnit? or AudioUnit -> Stream? I am a little concerned that this might be diverging from the original API though - it would require some research/experimenting to see how close we could keep it.

yupferris commented 8 years ago

I feel like we could offer the low-level native API wrapper (with safer type stuff you're talking about) as well as a higher level stream-like abstraction as you mention here, and I think that's a good way to do it. As a consumer of this library I'd expect to be able to use the API more or less by following Apple's guides but from Rust instead, and as long as that's preserved, I'm thrilled. Any additional niceties we can add on top is just icing on the cake as far as I'm concerned, and this particular icing is quite appealing :)

yupferris commented 8 years ago

In any case,

let some_format = StreamFormat::<S>::new(..);
try!(au.set_stream_format(some_format));

This rules :)

mitchmindtree commented 8 years ago

As a consumer of this library I'd expect to be able to use the API more or less by following Apple's guides but from Rust instead, and as long as that's preserved, I'm thrilled.

Yeah, I totally agree with you here!

Going to have a stab at this today and see how it goes :+1: will keep you in the loop!

yupferris commented 8 years ago

Cool, looking forward to it!

mitchmindtree commented 8 years ago

OMG I just read this

Now define the ASBD field values. Specify kAudioFormatLinearPCM for the mFormatID field. Audio units use uncompressed audio data, so this is the correct format identifier to use whenever you work with audio units.

_from Audio Unit Hosting Guide for iOS._

I thought I had to allow for supporting all of these! This would have made life sooo much easier had I known this earlier heh. It truly blows my mind how we ever got anything done with C APIs haha.

I'm going to re-write the audio_unit::StreamFormat type so that it just assumes LinearPCM audio format and infers the bytes_per_packet and what not from the sample type.

When writing rust bindings for C APIs I feel like I spend 90% of the time porting the documentation rather than the code heh :confused:

yupferris commented 8 years ago

Dude wow, yeah, that list scared me off too; why do you think I kept putting this off? XD