span_length is the wrong abstraction

[dvdsk]

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.

[will3942]

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))

[PetrGlad]

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?

[PetrGlad]

// 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").

[dvdsk]

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.

related

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.

[dvdsk]

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.

[PetrGlad]

@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.

[will3942]

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.

[dvdsk]

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.

[PetrGlad]

/// 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]

@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.)

[dvdsk]

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!

[dvdsk]

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

it was not :)

[PetrGlad]

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.