span_length is the wrong abstraction

When decoding audio you get chunks with constant sample rate and channel count. It seemed natural to extend that into rodio's audio pipeline. The consumer of audio gets the guarantee that the sample rate and channel count will not change for the next n-samples. The consumer can then deal with that like this:

let mut resampler = Resampler::new();
loop {
  resampler.set_sample_rate(source.sample_rate());
  for _ in 0..source.current_span_len() {
    resample.resample(source.next()?);
  }
}

This seems very efficient right? It is however no different then:

let mut resampler = Resampler::new();
loop {
  resampler.set_sample_rate(source.sample_rate());
  while !source.parameters_changed() {
    resample.resample(source.next()?);
  }
}

Especially if we let the compiler know that the parameters usually do not change using:

impl Source {
  #[cold]
  fn parameters_changed() -> bool;

  ..
}

Currently at least skipping, seeking and queue are non-trivial in rodio since we may not break our promised span_len. That span len is however never used except to create the boolean parameters_changed via a comparison to an increasing index. Replacing current_span_len by parameters_changed makes rodio code far simpeler. For example seek:

impl Source {
  fn seek(&mut self, some_params: some_type) {
    self.parameters_changed = true
    // and some code to guarantee frame alignment
   //  but nothing for span alignment since spans do not exist anymore!
    ..
  }

  fn parameters_changed() -> bool {
    self.parameters_changed
  }
}

All the complex code in queue can just be removed, it can simply forward parameters_changed until a source ends: then it just switches it to true. Right now it is has to play silence to fill the current span if a source ends early. So this change will not only make the code far far simpler it will improve audio latency!

Regarding breaking the Source API, an argument along the line of #690 (comment) can be made. This would also replace #690.

I think this is a wonderful fix/improvement. I might be wrong. Looking forward to your input.

@PetrGlad

Interesting idea.

Couple thoughts:

When would you set parameters_changed back to false? On the Sample after the Sample that changed parameters?
ChannelVolume / ChannelCountConverter / UniformSourceIterator may be hard to implement if there is no guarantee the parameters will not change between each 0..num_channels so perhaps the parameters should not change until all Samples have been emitted for the current num_channels (e.g. for a 6 channel source the parameters can only change every 6 channels). Or maybe we should move to typed streams like @PetrGlad suggested to avoid this (Fix(484) - ChannelVolume misbehaving #493 (comment))

I have to read it again with the fresh mind but even with this version the source should guarantee that whole frame (samples for all channels) is received, otherwise it invites some complicated logic again.
Instead of boolean I'd consider a revision counter that is increased every time parameters change, this is usually my favorite in such situations. Or if it is a boolean it should be be reset every time target sink sees it's value.
Even if spans are not visible, I guess the our decoder wrappers will still need to handle a lot of cases?

// This may need some optimizations, but I thought of having a struct that includes all the stream parameters that can be queried from a source. Besides our existing ones lie channel count, and sample rate that struct can include channel mapping (say channel 4 is "rear left").

ChannelVolume / ChannelCountConverter / UniformSourceIterator may be hard to implement if there is no guarantee the parameters will not change between each 0..num_channels

They currently already may, but they practically do not change in between frames.

perhaps the parameters should not change until all Samples have been emitted for the current num_channels (e.g. for a 6 channel source the parameters can only change every 6 channels).

It makes no sense to change sample-rate or channel count mid frame. I think that is a fine demand to add to the source API.

if we do that the API becomes stricter, adding new sources will be harder. It also opens up a new source of bugs. emitting a half frame at the end will be noticeable through subtle bugs in other parts of rodio (channels switching around).
Leaving the API as is puts all the work on us, though I think only queue needs to handle this right? And it is relatively doable.

Instead of boolean I'd consider a revision counter that is increased every time parameters change, this is usually my favorite in such situations. Or if it is a boolean it should be be reset every time target sink sees it's value.

my idea was to use the boolean as a flag. The consumer switches it back as soon as it reads it. There is no risk of race conditions since its all serial anyway. The consumer further more does not need to know if it missed a change, it just needs to know one or more have happened.

@dvdsk

@dvdsk What you mean by "stricter" there? Usually, better defined APIs are easier to integrate with. Did you mean those in-frame changes?

I think I like the idea, as long as that flag reading is cheap. And since it is all in the same thread, no additional sync is needed.

I thought of a scenario when the source is handed over somewhere else, but it looks like a boolean should still be sufficient there.

I would have read and reset of that boolean in the same function to avoid situation when a sink forgets to clear it. But I cannot think of a good name for it, since normally names like get_format_changed() imply that it is read only. It could be take_format_has_changed() but that looks verbose.

Do we demand sources only emit whole frames?

Its a balance,

if we do that the API becomes stricter, adding new sources will be harder. It also opens up a new source of bugs. emitting a half frame at the end will be noticeable through subtle bugs in other parts of rodio (channels switching around).

Leaving the API as is puts all the work on us, though I think only queue needs to handle this right? And it is relatively doable.

I don't understand the first point around "it opens up a new source of bugs"? Can you clarify please? In my mind adding a strict enforced requirement that sources can only emit full frames removed the class of bugs that are being solved at the moment where channels are switching around?

I think only queue needs to handle this right?

I think we have just scratched the surface of the issues of channels switching/frames becoming mixed up. (e.g. it also needs solving in the resampler #252)

IMO enforcing only emitting full frames would remove a class of bugs along with making the code easier to maintain by always being aware of which channel/frame you are at. Perhaps that is me being naive but that is my understanding.

In my mind adding a strict enforced requirement that sources can only emit full frames removed the class of bugs that are being solved at the moment where channels are switching around?

I do not count on end users to remember every detail of the Source documentation, a mistake is easily made. So any requirement not encoded by the type system is easily broken without being noticed. Especially since the result of breaking this requirement will emerge far away from where it was broken (for example in ChannelVolume).

I think we have just scratched the surface of the issues of channels switching/frames becoming mixed up. (e.g. it also needs solving in the resampler #252)

I can tell you the pile of issues related to span length is huge, take a look at the new queue code in that PR... Its scares me how many special cases need to be checked. That code is not refactored and probably wont be merged since switching to span_changed will remove all those edge cases.

IMO enforcing only emitting full frames would remove a class of bugs along with making the code easier to maintain by always being aware of which channel/frame you are at. Perhaps that is me being naive but that is my understanding.

No your right there, its just me worrying about users messing up that requirement, I would like to see how hard it is to take care of it for them.

@dvdsk

/// Sorry, I thought the previous comment was lost so reposted it.

@dvdsk What do you mean by "stricter"? Avoiding those in-frame changes? The problem is that I would agree to relax rules for sources if the complexity would be only on the source side. Say, it would be a lot harder to emit whole frames than to check for changes on the consuming side. But to me it seems that this would simplify a little on one side and make it a lot more complicated on the other. Queue is not the only consumer we or users may have. All the filters/sinks would have to handle it. Do we have input sources/decoders that may be difficult to implement with whole-frame requirement?

I think I like the idea of having a flag for format changes. Since this check is in the same thread, it should be cheap.
I thought of scenarios when a source is handed over somewhere else, but it looks like just a boolean is sufficient also in that case.

I would consider the getter to reset the flag immediately, with a method like take_format_has_changed(), or poll_format_has_changed() ("get" normally implies that it is read-only), so the name looks like it is reading from a message queue.

@dvdsk What you mean by "stricter" there? Usually, better defined APIs are easier to integrate with.

The current API is stricter then the proposed because the current API guarantees you no changes in parameters for the next N frames. The proposed API makes no such guarantee and only lets you know if the parameters just changed. Missing that time guarantee its less strict in my eyes. You could build the new API on top of the old. You can not however guarantee the parameters will not change for N-samples knowing only if they just changed.

(well you could, you could buffer N samples we will probably do that in the hifi resampler. Thats not a problem though, it needs a buffer anyway.)

I would have read and reset of that boolean in the same function to avoid situation when a sink forgets to clear it.

take_format_has_changed()

I like that idea!

/// Sorry, I thought the previous comment was lost..

it was not :)

So any requirement not encoded by the type system is easily broken without being noticed. Especially since the result of breaking this requirement will emerge far away from where it was broken (for example in ChannelVolume).

Well, yes usually it is a good idea to encode important constraints in a type system, but with this one can go only so far. The stricter it is the more complicated it becomes. In this case, I imagine, a good compromise could be is sending slices, one for frame, but then each channel count will require a different type... Maybe we can have separate types for aligned or unaligned sources. I think it is possible to have a wrapper that aligns frames. It can detect incomplete frame on format change and fill the remainder with some kind of default (like frame average or most recent known values). It could also emit a tracing event for diagnostics. An alternative implementation could collect and pass whole frames and discard incomplete ones. With this filter the alignment logic can be shared.

Another thing to think of is that when we check for parameter changes on every frame this may prevent some compiler optimizations.

I do not really think so. Maybe there is some loop unrolling but then that would need to be guarded by a branch because the current_span _length can have any value (as far as the compiler knows). On the other hand compilers are magic so maybe?

In any case if we want better throughput it is possible to use buffers

That would work at the exchange of latency, since we can not abort early.

Rubato says that it supports SIMD

The wip rubato backend I wrote takes a 'fixed' (dynamic) buffer size. It incurs some latency, I kept the buffer size to the minimum required (which was still a lot (2k samples I think?))

That would work at the exchange of latency, since we can not abort early.

Yes, that is what I said.
Also buffer can have some predictable size (say 32-64 samples).

2000 samples is about 45 ms at 44100? Or is this some kind of sliding window? What would be the delay introduced by the resampler then?

2000 samples is about 45 ms at 44100? Or is this some kind of sliding window? What would be the delay introduced by the resampler then?

I just checked it, it is about 1k samples so that's about 27ms. Not ideal for a rhythm game. But a rhythm game could pre-resample their files to common sample rates and then skip resampling when its not needed.

I was about to get started on implementing this however its worth it to think about the API a little.

The options I've investigated:

we replace current_span_len with a function returning whether a span has started, something like new_span_started or parameters_changed.
replace current_span_len, channels & sample_rate with current_params returning an option of a parameter struct. Nice from an API standpoint since its the impossible to forget re-checking channels and sample_rate. Probably does not perform well unless enough compiler magic applies.
remove current_span_len and let the consumer always check channels & sample_rate. Nice from an API standpoint, again the question is does it compile to something nice?

I'll thought about checking out the performance (seeing if the compiler can do its magic). As I was writing the perf tests I realized the latter two options wont scale. If we merge some version of #657 any consumer of Source now needs to track 3 continually changing outputs (channels, sample_rate and channel_mask).

This leaves us with the first option I like the name parameters_changed best atm. More ideas are very welcome. The bool returned will be true for one sample and then false again. The consumer only needs to check the return value every sample. When it is true one or more parameters has changed. A resampler might want to collect samples into a buffer as long as the buffer is not full and the parameters have not changed.

@roderickvd

I want to get started on this tomorrow, before I do we need to know one last thing. Also moving the discussion with @roderickvd from #697.

Do we require from sources that spans end at frame boundaries?

I am going with yes

We have discussed arguments for and against, I have the feeling that everyone is floating between yes or no. I think we just have to go with something and see what that means once we have implemented it. We can always, with some pain, change it.

I am planning to add a Check source that will check frame/span alignment. We could consider adding it automatically to mix during debug builds. That should take care of most instances of user mode sources messing up frame/span alignment.

@dvdsk

@dvdsk So the idea was to have the parameters getter return Option to indicate changes (return None if nothing has changed)? I thought of having a struct with parameters. I like the idea, but if is used we have to think how to pass these around. I would be wary if this would require heap allocation - this should happen in teal-time thread. If all these structs will have the same size then the parameters could just be copied. On positive side, if a filter does not change the stream parameters it can just pass the struct further.
I am not sure if just getting a boolean flag is necessarily more efficient than checking that an Option is None.

If we have channel descriptions, how those will be passed around? If it is a variable-sized collection that is returned from source, the code will be equally inefficient in both cases. We may have some optimization by having some registry holding all the mappings and let sources only pass a handle around (this may be an integer). Or maybe use Arcs for that.
I can imagine a channel map as, say 16 channels -> 16 speaker locations one-to-one map. That could be 4 bits per stream channel (e.g. 1 byte for stereo) up-to 8 bytes struct. So if we do not have too many speaker types and limit number of supported channels, this map can also be Copy.

// After 16 months without job I have recently found one, finally. So will likely spend some time on this project only on weekends.

It is my default choice to have some conventions that the code may rely on inside the program (inside rodio) and use stricter checks and assertions on data that comes in from outside.
The check source can be a normal one amplify(0.2).check_frames().

I will try submitting the samples conversion today.

// After 16 months without job I have recently found one, finally. So will likely spend some time on this project only on weekends.

first of all congratulations on the new job! I am grateful for all the work you put into rodio. I completely get that you have way less time now. Use your weekends to recharge and have fun, that could still be working on rodio but it might very well be something different. Good luck!

@dvdsk So the idea was to have the parameters getter return Option to indicate changes (return None if nothing has changed)?

yes that is one of the options, however it makes getting a partially consumed source difficult. An alternative is to let the consumer watch for changes. Disadvantages:

you are doing 2 or 3 compares instead of checking a single boolean
you have to store the parameters so you can compare them

If we have channel descriptions, how those will be passed around?

no idea, that issue has not made progress (and that is not a problem!) but I want to take a future where its merged into account

If it is a variable-sized collection that is returned from source, the code will be equally inefficient in both cases.

Yes but a source could cache them more efficiently if it can rely on the boolean to signal change.

The check source can be a normal one amplify(0.2).check_frames()

I like that :) that's similar to iterators inspect. I still would like to include the check functionality in mix by default on debug builds. Better to catch things early. Since a user can add their own rodio source's (like filters & effects on other sources) simply guarding the incoming edge does not seem like enough.

I will try submitting the samples conversion today.

Please take all the time you need. And if you rather drop it I can take it over? No need to finish that while also starting at a new job.

Another thing to think of is that when we check for parameter changes on every frame this may prevent some compiler optimizations.

I do not really think so. Maybe there is some loop unrolling but then that would need to be guarded by a branch because the current_span _length can have any value (as far as the compiler knows). On the other hand compilers are magic so maybe?

In any case if we want better throughput it is possible to use buffers

That would work at the exchange of latency, since we can not abort early.

Rubato says that it supports SIMD

The wip rubato backend I wrote takes a 'fixed' (dynamic) buffer size. It incurs some latency, I kept the buffer size to the minimum required (which was still a lot (2k samples I think?))

w.r.t. this discussion of SIMD auto-vectorization, branching, and what not, I thought this article was very interesting: https://matklad.github.io/2023/04/09/can-you-trust-a-compiler-to-optimize-your-code.html.

Key takeaways are that to leverage compiler SIMD auto-magic, we need to:

Process in chunks of fixed or at least predictable size
Within that processing, do not have any branches
Take care to not have short-circuiting evaluations by using bit-wise instead of logical operators: & instead of && etc.

Key takeaways are that to leverage compiler SIMD auto-magic, we need to:

1. Process in _chunks_ of fixed or at least predictable size

2. Within that processing, do not have any branches

3. Take care to not have short-circuiting evaluations by using bit-wise instead of logical operators: `&` instead of `&&` etc.

I wonder if its enough to have the cpal take the samples or if we should add optional buffering (to force chunking in the rest of the chain).
this one would only really be possible when abolishing spans. I found it impossible to not have an if in a next call here and there when dealing with spans.

Only real way to find this out is to test it I think. We need span & spanless rodio and for each put a little effort in optimizations. Only then we can really make a choice.

Another blog post on SIMD that I found intriguing: https://orlp.net/blog/taming-float-sums/

Apparently the compiler is shy about auto-vectorizing floating point for all the precision problems we know about, and then there's NaN and inf and all that. Quite understandable actually, but never thought about it like that.

The linked article goes into summation and shows how particular intrinsics can be 20-100x faster and more accurate*. Looking at https://doc.rust-lang.org/nightly/std/intrinsics/index.html there are a lot more f____ intrinsics that apply beyond summation.

I imagine none of these will be stabilized anytime soon, but they could make for an interesting feature flag.

*) and for DSP we may not even care so much about inaccuracy due to rounding errors or "The Accountant's Problem" beyond the nth-digit.

would be interesting to add where we can and see what happens to performance 👍

If we want SIMD, then we should work on buffers, at least smaller ones (say 4 or more frames/samples).
Yes, some of the sources may benefit from this.

If we want SIMD, then we should work on buffers, at least smaller ones (say 4 or more frames/samples).

if the iterator chain is properly in-lined we can simply buffer a bit at every mixer. Then we should get SIMD in the chain up till the mixer.

Navigation Menu

Repository navigation

span_length is the wrong abstraction #694

related

6 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

span_length is the wrong abstraction #694

Description

Activity

will3942 commented on Jan 26, 2025

PetrGlad commented on Jan 26, 2025

PetrGlad commented on Jan 26, 2025

yara-blue commented on Jan 27, 2025

related

yara-blue commented on Jan 27, 2025

PetrGlad commented on Jan 28, 2025

will3942 commented on Jan 28, 2025

yara-blue commented on Jan 28, 2025

PetrGlad commented on Jan 28, 2025

yara-blue commented on Jan 28, 2025

yara-blue commented on Jan 28, 2025

yara-blue commented on Jan 28, 2025

PetrGlad commented on Feb 1, 2025

6 remaining items

yara-blue commented on Feb 17, 2025

PetrGlad commented on Feb 17, 2025

yara-blue commented on Feb 17, 2025

yara-blue commented on Feb 19, 2025

yara-blue commented on Feb 22, 2025

PetrGlad commented on Feb 22, 2025

PetrGlad commented on Feb 22, 2025

yara-blue commented on Feb 23, 2025

yara-blue commented on Feb 23, 2025

roderickvd commented on Mar 6, 2025

yara-blue commented on Mar 6, 2025

roderickvd commented on Mar 31, 2025

yara-blue commented on Mar 31, 2025

PetrGlad commented on Apr 6, 2025

yara-blue commented on Apr 7, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions