make current_span_len take &mut

[dvdsk]

Context:
Many of the fixes regarding span len require keeping track of something within this function. The only way to do that right now is with interior mutability (Cell/RefCell/Mutex etc). As far as I know the only reason current_span_len takes &self is that it simply did not need &mut self.

Advantage:
The code around these fixes will become simpler and more maintainable. Enables optimizations by moving mutations from a conditional space in Iterator::next to current_span_len.

Disadvantage:
Can not call current_span_len while holding a reference to Source somewhere else. Unknown if that is used anywhere.

Help:
Need input whether this change is possible and worth it.

[PetrGlad]

This looks like a breaking change, since any uses that assumed read-only access will now need a unique reference to the source. So this is also leaking the implementation to users. Cells usually are right way to update cached data.

Do you have particular example where such change may help?

[dvdsk]

Do you have particular example where such change may help?

Pretty complex but: if a user calls current_span_len in queue and we do not have anything playing yet we still need to return something. We return the length of a silence span. If a new source is added before the call to QueueSource::next it will not play silence but that source. That is a logical race condition.

To 'fix' this we set a flag after we return the length of a silence span in current_span_len. If that flag is set we always add a silence source in QueueSource::next even if there is a new source provided by the user.

There are more of these kind of flags and counters that would not be needed if we could just start the silence in QueueSource::current_span_len. Queue is complex and this would simplify it. Now if users depend on the &self we should not do it but rodio is generally the consumer of Source's. You only make a source so you can put it in a mixer or queue and rodio plays it.

Nowhere do we store read only references to a source with the motivation to access current_span_len. It makes no sense to do so since these values change after a next() call or other mutable interaction with the Source. In that case you have a &mut already.

I might be horribly wrong here, and that would break... well everything, so feel free to find counter examples or break my logic.

[PetrGlad]

// Adding everything to Source trait certainly helps with API discovery and auto-completion, but also makes Rodio not to be particularly friendly to extensions by users.
Yes, sources are normally used from the audio thread, and all the control/automation from the application threads should already be properly synchronized. So in that regard I would agree that this should not break much even in Sources implemented by users.

Regarding that case, can we handle Silence just like any other source in the Queue? Let it be short enough to be played completely and not cause noticeable delays but long enough to not require too much queuing work.

[dvdsk]

Regarding that case, can we handle Silence just like any other source in the Queue? Let it be short enough to be played completely and not cause noticeable delays but long enough to not require too much queuing work.

That is what I do atm

Let me give you a test that highlights the issue (will be in the PR in tests/queu.rs):

#[test]
fn parameters_queried_before_next() {
    let test_source = TestSource::new(&[0.1; 5])
        .with_channels(1)
        .with_sample_rate(1);

    let (controls, mut source) = queue::queue(true);

    assert_eq!(source.current_span_len(), Some(400));
    controls.append(test_source);
    assert_eq!(source.next(), Some(0.0));
    for _ in 0..399 {
        assert_eq!(source.next(), Some(0.0));
    }
    assert_eq!(source.next(), Some(1.1));
}

Because the user asks for the span len before the source is appended we have to answer something non-zero (or the source would end). Then they append something and call next, however we promised them a certain span, with a certain resample rate and channel count. So we have to play silence for the duration of the span we returned.

This would be easier to implement if we could start the silence inside the current_span_len function.