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

consider renaming Sequential and Dynamic #14

Open
Be-ing opened this issue Dec 2, 2022 · 5 comments
Open

consider renaming Sequential and Dynamic #14

Be-ing opened this issue Dec 2, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@Be-ing
Copy link
Contributor

Be-ing commented Dec 2, 2022

I haven't seen these terms used to refer to audio (or image/video) buffers in other contexts. What "Sequential" does is more commonly called "planar", which I think would be a good name for the struct.

"Dynamic" kinda implies that the others are "static", but they all allow resizing both the number of frames and channels. "Disjoint" would describe how "Dynamic" organizes memory, but that feels like an awkward name...

@udoprog udoprog added the enhancement New feature or request label Dec 3, 2022
@udoprog
Copy link
Owner

udoprog commented Dec 4, 2022

Planar seems like a good change. I'd mourn the loss of inherent comprehension. It's specialised terminology that in my mind is harder to grok without looking at reference documentation, but it is what pcm buffers with a linear channel topology tends to be called elsewhere.

Dynamic I'm not entirely sure about. I have half a mind yeeting the type myself, since I only introduced it because it mimics the default topology in rubato (Vec<Vec<T>>) but I don't think the behavior is super useful. I.e. spurious resizes are really not a thing once you look into it and other types can largely cope.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 4, 2022

Dynamic I'm not entirely sure about. I have half a mind yeeting the type myself, since I only introduced it because it mimics the default topology in rubato (Vec<Vec>) but I don't think the behavior is super useful. I.e. spurious resizes are really not a thing once you look into it and other types can largely cope.

I've also been trying to come up with a real world use case for Dynamic and... I can't think of one. Even if a strange use case involved resizing a buffer more frequently than reading or writing it, resizing Dynamic requires a reallocation for every channel. By contrast, Sequential and Interleaved only require a single reallocation to resize, which I would think would be substantially faster, even though data has to be copied around within the memory.

@udoprog
Copy link
Owner

udoprog commented Dec 4, 2022

Yeah, it really is all about having a type which can ensure that it's compatible with rubato's output. But since I haven't found the use for it anywhere I'm very sceptical as well.

Even if a strange use case involved resizing a buffer more frequently than reading or writing it, resizing Dynamic requires a reallocation for every channel. By contrast, Sequential and Interleaved only require a single reallocation to resize, which I would think would be substantially faster, even though data has to be copied around within the memory.

There's two aspects to resizing: growing / shrinking and topology changes.

  • A planar buffer requires a lot of work when changing the number of samples per channel where potentially a lot of things need to be copied around.
  • An interleaved channel requires a lot of copying on topology changes (change in number of channels).

Dynamic only requires additional work when growing the number of samples beyond its current capacity (which grows by a factor for low amortized overhead), everything else is "free" including shrinking since that doesn't touch the underlying allocations.

This can be useful to avoid work in something like a scratch buffer that is used with more than one topology.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 4, 2022

A planar buffer requires a lot of work when changing the number of samples per channel where potentially a lot of things need to be copied around.

Sequential could be implemented such that it doesn't move data when shrinking the number of frames and keeps track of the distance in memory between channels separately from the current frames per channel, like Vec. I doubt that's worth the implementation complexity and spreading out the data more in memory (potentially worse cache locality) to optimize resizing, which is generally done much less frequently than reading/writing.

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 4, 2022

Yeah, it really is all about having a type which can ensure that it's compatible with rubato's output.

FWIW, I got Sequential to work with Rubato's next-0.13 branch (HEnquist/rubato#45) which has the API introduced by HEnquist/rubato#50. However, this required some ugly hacks creating arrays of LinearChannels/mut slices. If I didn't assume the number of channels, creating an array for that wouldn't be possible and a Vec or something would be needed on the heap somewhere. I considered using rsor inside Sequential to return a slice of slices, but still, that would require storing an rsor::Slice inside Sequential and resizing the rsor::slice when Sequential's number of channels changes. That didn't seem worthwhile just to satisfy the current API of Rubato considering the Rubato maintainer has already expressed support for using audio::Buf in HEnquist/rubato#51.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants