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

WIP use audio crate's traits for generic input & output buffers #52

Draft
wants to merge 1 commit into
base: next-0.13
Choose a base branch
from

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Dec 11, 2022

This doesn't compile yet; there's more work to do. I am opening this draft PR early to get feedback on whether this is a direction worth continuing in.

Fixes #51

@HEnquist
Copy link
Owner

I did not yet find the time to get familiar with the audio crate, so it's difficult to say much at this point.
But I can mention a few things I think are important:

  • The current performance level must be maintained (or improved), in terms of processing speed and quality.
  • It must be reasonably easy to add Rubato to an existing project that doesn't use the audio buffers (yet).
  • It must be reasonably easy to upgrade an existing project using non-audio Rubato. This and the previous point mean that it should be possible to somehow still use vecs of vecs, perhaps by wrapping them up as audio buffers. I don't want to force people to copy data back and forth between buffers, or to migrate the entire project to use audio buffers.
  • Both the asynchronous Sinc resampler and the synchronous one rely on the exact layout of the data in their internal buffers. This is required for the simd load/store instructions of the former, and for feeding the data to the FFT in the latter. I see the internal buffer of SincFixedIn/Out was changed to an audio buffer. Does this guarantee that the values for a channel are stored sequentially? It can't be generic, trying to use an interleaved layout here will produce garbage. Is there any reason to use an audio buffer in a place where it's anyway not possible to be generic over the layout?

Probably a few of these are non-issues, but as I mentioned I haven't had the time yet to get familiar with audio. Unfortunately, it will be a while before I'll manage to do anything about that.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 11, 2022

The current performance level must be maintained (or improved), in terms of processing speed and quality.

The only potential issue that has stood out to me so far is all the .get(i).unwrap() on Channels in the interp_* functions of asyncro_fast.rs. Previously those used safe slice indexing with [] that checks bounds, so I'm doubting there will be a substantial difference (maybe not even measurable). But we'll see how benchmarks are before and after.

It must be reasonably easy to add Rubato to an existing project that doesn't use the audio buffers (yet)
It must be reasonably easy to upgrade an existing project using non-audio Rubato.

Re-exporting audio's wrap module should take care of this. Also, in the unfortunate event someone would get stuck with two incompatible versions of audio in their dependency graph and try to pass buffers from a different version of audio to Rubato, using the re-exported wrap module on the buffer's underlying slice of memory should help with that too.

Both the asynchronous Sinc resampler and the synchronous one rely on the exact layout of the data in their internal buffers.

For the internal buffers, we get to pick the exact layout while keeping the user's input and output buffers generic. If there's any performance difference, I'd expect the audio::Sequential buffer to be slightly more performant than the old Vec<Vec<T>> because there's only one heap allocation versus a heap allocation per channel (one less pointer to dereference, maybe better cache locality). Also, using one of audio's buffers internally makes it easy to use the audio::channel::copy and audio::buf::copy functions together with the user's input and output buffers.

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.

None yet

2 participants