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

A lot of improvements to OS X output plugin #14

Merged
merged 8 commits into from
Sep 19, 2016
Merged

A lot of improvements to OS X output plugin #14

merged 8 commits into from
Sep 19, 2016

Conversation

Wang-Yue
Copy link
Contributor

@Wang-Yue Wang-Yue commented Sep 18, 2016

Add sample rate synchronization, which ensures mpd play bit perfectly on OS X. User can enable this by setting sync_sample_rate to "true".

Add device hogging capability. This forbids other process interfere with selected user devices. User may enable this by setting "hog_device" to "true".

Kill the mutex, locks, extra buffer in original code. Now you'll see greater than 3X performance improvement.

Set the right buffer length for the device.

Fix an audio unit initialization problem.

Code borrowed from the CMUS project. I am the author of cmus coreaudio output plugin. So I have full rights to redistribute the code. The coding style may be slightly different, but we can always polish/refactor it in another PR.

@jacobvosmaer Please take a look.

@MaxKellermann please merge.

@Wang-Yue Wang-Yue changed the title Add sample rate synchronization and device hogging to core audio plugin A lot of improvements to OS X output plugin Sep 19, 2016
@@ -101,6 +103,8 @@ osx_output_configure(OSXOutput *oo, const ConfigBlock &block)
}

oo->channel_map = block.GetBlockValue("channel_map");
oo->hog_device = block.GetBlockValue("hog_device", false);
oo->sync_sample_rate = block.GetBlockValue("sync_sample_rate", false);
Copy link
Member

Choose a reason for hiding this comment

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

These must be documented in doc/user.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -665,9 +616,9 @@ osx_output_disable(AudioOutput *ao)

AudioComponentInstanceDispose(oo->au);

if (oo->hog_device) {
if (oo->hog_device) {
Copy link
Member

Choose a reason for hiding this comment

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

What's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a tab. I previously used spaces for indent.

osx_output_hog_device(oo->dev_id, false);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

And this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

@MaxKellermann MaxKellermann merged commit 1e17d5b into MusicPlayerDaemon:master Sep 19, 2016
@jacobvosmaer
Copy link
Contributor

Thanks @Wang-Yue ! I am too late to review this but I am going to ask a few questions so I can learn.


return noErr;
int count = in_number_frames * od->asbd.mBytesPerFrame;
buffer_list->mBuffers[0].mDataByteSize =
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you know there is only one output buffer? Is this documented by Apple anywhere?

To be clear, I am not asking because I think this is wrong, but because I wonder how one is supposed to find out about these things.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know about the comments in /System/Library/Frameworks/CoreAudio.framework/Headers/*.h but it is hard to learn from those comments if you don't know what to look for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is undocumented (The entire CoreAudio is poorly document) but I was told by an Apple engineer.

int count = in_number_frames * od->asbd.mBytesPerFrame;
buffer_list->mBuffers[0].mDataByteSize =
od->ring_buffer->pop((uint8_t *)buffer_list->mBuffers[0].mData,
count);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I missed it but when reading the spsc_queue docs I nowhere saw any promises about how many elements will be popped. How do we know we get count bytes, or at least a multiple of mBytesPerFrame?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't get this promise. But if it's not multiple mBytesPerFrame it won't cause any trouble. During the next call CoreAudio will get the remaining bytes.


// We set the frame size to a power of two integer that
// is larger than buffer_frame_size.
while (*frame_size < buffer_frame_size + 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to call this variable buffer_size instead of frame_size? When I read frame_size I think of the size of a single frame. The buffer (spsc_queue) holds multiple frames.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to rename it.

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