-
-
Notifications
You must be signed in to change notification settings - Fork 14
Provide helpers to get I/O buffers in OnAudioBuffer handlers #56
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! This works. I would suggest some minor changes. Sorry if they seem a bit like nitpicking. I'm just trying to keep the medium-level API as consistent as possible.
Let me know if you want to do the changes yourself. Alternatively, I can also merge them and adjust the code afterwards.
| /// Get access to the underlying samples of an output channel | ||
| pub fn output_channel_samples(&self, ch: usize, args: &OnAudioBufferArgs) -> Option<&mut [ReaSample]> { | ||
| unsafe { | ||
| if let Some(get_buffer) = self.0.as_ref().GetBuffer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, GetBuffer is supposed to always return Some in the context in which we access it (as parameter of OnAudioBuffer). Therefore it would be consequent to panic here with expect() instead of returning None. This makes sure that if REAPER really returns a null pointer one day, we become aware of that change in the contract and can handle that accordingly (e.g. by adjusting the signature). That's at least the approach I have pursued so far in reaper-rs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll change it.
| } | ||
|
|
||
| /// Get access to the underlying samples of an output channel | ||
| pub fn output_channel_samples(&self, ch: usize, args: &OnAudioBufferArgs) -> Option<&mut [ReaSample]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One goal of the medium-level API is to not change the naming too much. It would be more in line with the other code to have only one method get_buffer() and introduce a BufferKind enum to distinguish between input and output buffer (built analogously to e.g. enum RecordArmMode in misc_enums.rs).
Another goal is to use consistent data types. Therefore I would prefer u32 as channel type instead of usize (input_nch() and output_nch() both return u32 values).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, will change accordingly.
| } | ||
|
|
||
| /// Get access to the underlying samples of an output channel | ||
| pub fn output_channel_samples(&self, ch: usize, args: &OnAudioBufferArgs) -> Option<&mut [ReaSample]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing the OnAudioBufferArgs parameter, I think it would make sense to make get_buffer() a method of the OnAudioBufferArgs struct itself - because it contains a reference to the AudioHookRegister and therefore has all information necessary to implement the method.
The final signature in OnAudioBufferArgs would look like this:
pub fn get_buffer(&self, kind: BufferKind, ch: u32) -> Option<&mut [ReaSample]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try it this way and update the PR.
Update remote tracking branch
Pulling from helgoboss
Added
output_channel_samplesandinput_channel_samplesto get to I/O buffers inside OnAudioBuffer handlers.