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

buffer_size should likely be in ProcessHandler instead of NotificationHandler #137

Closed
ollpu opened this issue Mar 14, 2021 · 3 comments
Closed

Comments

@ollpu
Copy link
Contributor

ollpu commented Mar 14, 2021

From the API docs on JackBufferSizeCallback:

Prototype for the bufsize_callback that is invoked whenever the JACK engine buffer size changes. Although this function is called in the JACK process thread, the normal process cycle is suspended during its operation, causing a gap in the audio flow. So, the bufsize_callback can allocate storage, touch memory not previously referenced, and perform other operations that are not realtime safe.

This indicates that the buffer_size callback is called on the execution thread, but it is allowed to perform blocking operations in this case.

Indeed, with a quick test on JACK2,

thread_init called @ ThreadId(2)
sample_rate called @ ThreadId(1)
buffer_size called @ ThreadId(3)
thread_init called @ ThreadId(3)
thread_init called @ ThreadId(4)
process called @ ThreadId(3)
...

it seems that the function is called on the execution thread.

buffer_size not receiving a reference to the ProcessHandler means that carrying out the kinds of allocations mentioned in the documentation for example becomes needlessly cumbersome. This would be useful in the CPAL implementation which has to reallocate temporary buffers for interleaving.

Am I missing an important detail or would this make sense?

@wmedrano
Copy link
Member

This makes sense. Feel free to create a PR moving the callback to process or else I can handle it this weekend.

@ollpu
Copy link
Contributor Author

ollpu commented Mar 24, 2021

Will do!

ollpu added a commit to ollpu/rust-jack that referenced this issue Mar 26, 2021
ollpu added a commit to ollpu/rust-jack that referenced this issue Mar 26, 2021
@wmedrano
Copy link
Member

This is live in version 0.7.0

nyanpasu64 added a commit to nyanpasu64/cpal that referenced this issue Jan 3, 2022
It almost compiles.

Original (outdated) commit message:

The way the temporary interlacing buffers are handled currently is not
great. The root of the issue seems to be that JACK actually provides a
callback that allows performing allocations on the realtime thread when
buffer size changes, but this is not exposed as such in rust-jack.

See: RustAudio/rust-jack#137

The duplex implementation now mirrors input, and this panicks if buffer
size becomes greater than the original.
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

No branches or pull requests

2 participants