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

Add support for Audio Toolbox output (MacOS) #153

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

Conversation

stronk-dev
Copy link

Adapted from #59

Notable changes:

  • Instead of overwriting newOutput, make it a separate toolboxOutput which can be activated by setting the backend to audio-toolbox.
  • Stubs added for outputs with go:build constraint printing a generic 'unsupported' message
    NOTE: we could enhance the stubs so that the output knows which outputs are supported and then automatically choose a platform-specific output like audio-toolbox.
  • Use a C struct to give the audio callback (which asks for samples when required) access to the reader directly.
    NOTE: depending on how the float32 reader is implemented, maybe having a FIFO buffer in the output could be useful? Ideally these kinds of callbacks have instant access to data without blockage (and fill remaining samples with silence if there's no data in the buffer).
    Removing the local buffer in the output and having the CB read directly does not seem to cause any issues so far...
  • Major simplification of buffer initialization and buffering logic for easier maintainability
  • Generic code cleanup: added some comments, checks, error cases, etc...

The PR is pronbably ready for review as-is, but there might be some minor issues like making sure that out.err gets set where necessary, more comments and testing to see how it handles error cases.

@stronk-dev
Copy link
Author

Has been mostly stable. I did ran into some issues at one point where during a track switch the out.reader.Read would give an EOF. At the moment this is a hard error, although we could just add silent samples when there is a buffer underflow.

Copy link
Owner

@devgianlu devgianlu left a comment

Choose a reason for hiding this comment

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

In addition to the comments, I would advise to not capitalize log messages to match go guidelines.

I think this is good stuff to add even if it's not perfect. We can improve on it overtime, thank you for putting the work to revisit it!


// We need the C.AudioContext to give the callback safe access to the output context
log.Tracef("Allocating audio context")
ctx := (*C.AudioContext)(C.malloc(C.size_t(unsafe.Sizeof(C.AudioContext{}))))
Copy link
Owner

Choose a reason for hiding this comment

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

This can be extract to a C function like the free one.

ctx := (*C.AudioContext)(C.malloc(C.size_t(unsafe.Sizeof(C.AudioContext{}))))
if ctx == nil {
out.err <- errors.New("failed to allocate AudioContext")
return nil, errors.New("failed to allocate AudioContext")
Copy link
Owner

Choose a reason for hiding this comment

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

Do not create the error twice, use a local variable.

paused bool
volume float32
err chan error
stopChan chan struct{}
Copy link
Owner

Choose a reason for hiding this comment

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

This can be removed.

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