`source::Done` Not decrementing signal

[Drewol]

In this example the 2nd assert fails even though the take_duration has ended.

use std::{
    sync::{
        atomic::{AtomicBool, AtomicUsize},
        Arc,
    },
    thread,
    time::Duration,
};

use rodio::{dynamic_mixer::mixer, source, OutputStream, Sink, Source};

fn main() {
    // Set up output and long running mixer
    let (_output_stream, stream_handle) = OutputStream::try_default().unwrap();
    let sink = Sink::try_new(&stream_handle).unwrap();
    let (mixer_controls, mixer) = mixer::<f32>(2, 44100);
    sink.append(mixer);
    mixer_controls.add(source::Zero::new(2, 44100));
    sink.play();

    let stop_signal = Arc::new(AtomicBool::new(false));
    let done_signal = Arc::new(AtomicUsize::new(2));

    mixer_controls.add(source::Done::new(
        source::SineWave::new(440.0).take_duration(Duration::from_secs(2)),
        done_signal.clone(),
    ));

    let source_stop = stop_signal.clone();
    mixer_controls.add(source::Done::new(
        source::SineWave::new(493.88).stoppable().periodic_access(
            Duration::from_millis(20),
            move |s| {
                if source_stop.load(std::sync::atomic::Ordering::Relaxed) {
                    s.stop()
                }
            },
        ),
        done_signal.clone(),
    ));

    thread::sleep(Duration::from_millis(1000));
    stop_signal.store(true, std::sync::atomic::Ordering::Relaxed);
    thread::sleep(Duration::from_millis(500));
    assert!(done_signal.load(std::sync::atomic::Ordering::Relaxed) == 1); //Stopped should have fired by now
    thread::sleep(Duration::from_millis(1500));
    assert!(done_signal.load(std::sync::atomic::Ordering::Relaxed) == 0); //TakeDuration should be finished now
}

Appending both sources to a Sink instead of a mixer appears to work as expected but I would like to avoid that in the project where I encountered this.

[will3942]

Yes - we've just ran into this issue as well with a modified version of Done called DoneCallback. It's a unique issue caused by combining Mixer and Done made worse now due to the fact OutputStream always uses Mixer.

The issue is in UniformSourceIterator / Done, for specific Sources where eventually for the last span , current_span_len returns 0 and so .next() is never called on the underlying Done source - this is due to Take in UniformSourceIterator where if n === 0 (which it will be for a Span of length 0) the underlying Source's (in this case Done's) next() is never called.

I believe this issue rears its head depending on the length of source. The attached .mp3 file will always reproduce this error.

We are fixing this issue by calling the Done callback on the penultimate Sample which works fine for us but isn't a great solution.

This is the code we use:

#[inline]
    fn next(&mut self) -> Option<I::Item> {
        let next = self.next_sample.take();
        let next_sample = self.input.next();

        if !self.signal_sent && next_sample.is_none() {
            (self.callback)();
            self.signal_sent = true;
        }

        self.next_sample = next_sample;

        next
    }

The only "proper" solution I can think of is to somehow call the .next() inside of the UniformSourceIterator rather than returning None - this is the offending line I believe https://github.com/RustAudio/rodio/blob/master/src/source/uniform.rs#L182

Any ideas @dvdsk or @PetrGlad?

done-fail.mp3.zip

[yara-blue]

Cant look at this now nor in the near future, I'm focusing on making needed changes to the rodio core. I would suggest looking at https://docs.rs/rodio/latest/rodio/source/struct.EmptyCallback.html. You can decrement an atomic in the callback simulating Done.

[PetrGlad]

Done made worse now due to the fact OutputStream always uses Mixer.

I worked on that output stream initialization revamp. Output stream has always been using mixer, the new API just makes this explicit.

I had an idea of "unopinionated" output stream that would accept a plain iterator but could not find a good solution for that then. Maybe someone may have a better idea how to do that.

[will3942]

Done made worse now due to the fact OutputStream always uses Mixer.

I worked on that output stream initialization revamp. Output stream has always been using mixer, the new API just makes this explicit.

I had an idea of "unopinionated" output stream that would accept a plain iterator but could not find a good solution for that then. Maybe someone may have a better idea how to do that.

Ah yes you're right. This issue flagged up for us due to the issues within Queue that prevent us from using Sink and so we can no longer use EmptyCallback as David suggested and so Done is our next best solution.

[yara-blue]

Ah yes you're right. This issue flagged up for us due to the issues within Queue that prevent us from using Sink and so we can no longer use EmptyCallback as David suggested and so Done is our next best solution.

Its going to take a while before queue gets fixed, you might be able to get away with the current queue PR for now... (no guarantees, especially around performance). As soon as we are done migrating from current_span_len to something better fixing queue should be quite easy.

[will3942]

Ah yes you're right. This issue flagged up for us due to the issues within Queue that prevent us from using Sink and so we can no longer use EmptyCallback as David suggested and so Done is our next best solution.

Its going to take a while before queue gets fixed, you might be able to get away with the current queue PR for now... (no guarantees, especially around performance). As soon as we are done migrating from current_span_len to something better fixing queue should be quite easy.

No worries - thanks for the update. Our modified Done is working fine - albeit we're having to disable any resampling for the time being as that is broken for 6 channel audio (and maybe all >2 channel) which is more of an issue.