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

rkilgore features #94

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rick-kilgore
Copy link

add a couple of functions I needed for my project and I replaced a deprecated constant

@rick-kilgore rick-kilgore changed the title Rkilgore features rkilgore features Oct 26, 2022
@rick-kilgore
Copy link
Author

sorry for the confusion, I'm not very good with git bc I haven't used it much in the last 5 years. There are 3 changes here. I accidentally named the 3rd one the same as the second when it should have actually had the message "add query function get_audio_device_supports_scope(devid)". I tried to run "git commit --amend" so that I could edit the message, and it wanted me to git pull first, and when I did that it got all messed up. So the last 4th and 5th changes are noops and the 3rd one should have the description of the 4th.

Copy link
Member

@simlay simlay left a comment

Choose a reason for hiding this comment

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

I'm not sure how to do it correctly but I think this can be done without using libc.

@@ -44,7 +46,7 @@ pub fn get_default_device_id(input: bool) -> Option<AudioDeviceID> {
let property_address = AudioObjectPropertyAddress {
mSelector: selector,
mScope: kAudioObjectPropertyScopeGlobal,
mElement: kAudioObjectPropertyElementMaster,
mElement: kAudioObjectPropertyElementMain,
Copy link
Member

Choose a reason for hiding this comment

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

Huh. Interesting. I figured the name change was due to a bindgen change. Looks like kAudioObjectPropertyElementMaster is deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am on macOS 10, and kAudioObjectPropertyElementMain only exists on macOS 12+ (I haven't upgraded because of reports that moving off Catalina for old macbooks can brick them.)

Does this mean that the current code on master won't work on macOS 13? If we wanted to support all macOS versions would we need OS version selectors or something?

// audio_buffers.reserve_exact(count as usize);
// unsafe { audio_buffers.set_len(count as usize) };

let status = unsafe {
Copy link
Member

Choose a reason for hiding this comment

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

We don't have clippy checks on in this repo but I think it'd complain about an unsafe block in an unsafe block.

mElement: kAudioObjectPropertyElementWildcard,
};

macro_rules! try_status_or_return {
Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. This is what #93 was about.

@simlay simlay requested a review from MichaelHills November 10, 2022 16:03
@@ -44,7 +46,7 @@ pub fn get_default_device_id(input: bool) -> Option<AudioDeviceID> {
let property_address = AudioObjectPropertyAddress {
mSelector: selector,
mScope: kAudioObjectPropertyScopeGlobal,
mElement: kAudioObjectPropertyElementMaster,
mElement: kAudioObjectPropertyElementMain,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am on macOS 10, and kAudioObjectPropertyElementMain only exists on macOS 12+ (I haven't upgraded because of reports that moving off Catalina for old macbooks can brick them.)

Does this mean that the current code on master won't work on macOS 13? If we wanted to support all macOS versions would we need OS version selectors or something?

&data_size as *const _ as *mut _,
)
};
try_status_or_return!(status);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this macro invoked only once in this function? inline it?

// let count = data_size / mem::size_of::<sys::AudioBuffer>() as u32;
// let mut audio_buffers: Vec<sys::AudioBuffer> = vec![];
// audio_buffers.reserve_exact(count as usize);
// unsafe { audio_buffers.set_len(count as usize) };
Copy link
Contributor

Choose a reason for hiding this comment

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

remove if not needed?

try_status_or_return!(status);

unsafe {
let buffers: *mut sys::AudioBufferList = libc::malloc(data_size as usize) as *mut sys::AudioBufferList;
Copy link
Contributor

Choose a reason for hiding this comment

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

Other parts of this repo use vec u8 as a way to allocate some bytes (see render_callback.rs let mut data = vec![0u8; data_byte_size as usize]; but there is also a TODO there saying that it is leaking len and capacity fields of the vec.

A few answers here, https://stackoverflow.com/questions/45306575/how-can-you-allocate-a-raw-mutable-pointer-in-stable-rust the first answer suggests std::alloc.

On closer inspection, is it possible to remove libc and malloc/free by using a vec u8 .as_mut_ptr() and then you don't need to free because the vec will drop itself?

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