Cannot restart stopped sink

[mbillingr]

Hi there! Thank you for this awesome library. While exploring the API I stumbled over the following behavior, which I believe is a bug:

A Sink no longer plays newly appended sounds after .stop() has been called on the Sink.

Looking at the sources, it seems that .stop() sets Controls.stopped to true, but I could not find anything that would set it back to false other than constructing a new Sink.

[tomaka]

Hi! By design we cannot restart a stopped source. The next() method of the Iterator produces a None, which indicates that the source is over.

[mbillingr]

Thank you for the response! I am aware of the Iterator based design behind sources. However, I have meant to refer to sinks instead. Chances are that I made a mistake, though. Here is what I did in code:

    let device = rodio::default_output_device().unwrap();
    let sink = rodio::Sink::new(&device);

    let source_a = rodio::source::SineWave::new(440);
    let source_b = rodio::source::SineWave::new(880);

    sink.append(source_a);  // played correctly
    thread::sleep_ms(1000);
    sink.stop();
    thread::sleep_ms(500);
    sink.append(source_b);  // only short "click" sound
    thread::sleep_ms(1000);

If I do not use sink.stop() but limit the length of source_a with .take_duration() source_b is played correctly.

[tomaka]

Once you have stopped a sound, it's not possible to restart it even with append. I guess this should either be properly documented, or append should return a Result. Also you're not supposed to hear a click.

[mbillingr]

I think I'm misunderstanding something obvious. To clarify, if I stop one sound and then append a different one I'm not supposed to hear it? In other words I cannot reuse a Sink? (I thought the purpose of Sink::stop() was emptying the queue so that other sounds could be played.)

Sorry if my questions are annoying. I'm somewhat confused at the moment :)

[tomaka]

To clarify, if I stop one sound and then append a different one I'm not supposed to hear it? In other words I cannot reuse a Sink?

Yes!

[TheEdda]

So can we talk about why the API is designed in such way? It seems counter-intuitive that you get an unusable sink after you call .stop() on it. Am I supposed to create a new sink every time I want to restart playback? If so, why even expose .stop() method, why not just Drop, since the Sink is useless after calling it anyway?

[mbillingr]

@TheEdward162 These are excellent questions.

Working around the stop == drop issue in ggez had me do some ugly things with context and default devices, that will probably bite someone in the future. This could have been easily avoided with different stop semantics.

I'm wondering now, since .stop() apparently needs to behave like a drop(), couldn't we get another function (e.g. sink.clear()) that just clears the queue while leaving the sink operational?

[tomaka]

The code of Sink is very very simple, and this struct exists mainly to make it easier to get started with rodio. In other words, using a Sink is not the primary way you'd play a sound in a production app. In order to get a behaviour different from the default one, the way to go is to copy-paste the code of Sink and modify it.

[TheEdda]

Well okay then. While I can't say I agree with this, it seems like the most reasonable way to do this. The reason why I think it's unreasonable is because the Sink struct hides some of the lower level implementation details which are then exposed to the user who wants to reimplement their own version of Sink, just because they need this .clear() functionality.

Additionaly, I would like to ask you to reflect this intention in the docs. I for one tend to take libraries mostly as an opaque object that I shouldn't mess with if it isn't really important/broken. This breaks the illusion and I think it's best to inform the user.

[svenstaro]

I implore you to revisit that design decision. I think this is needlessly convoluted. Besides, when do you ever want to just call something on an object which makes it unusable but doesn't drop it?

stop() should work like you would expect and according to the current docs: Clear the queue and reset the sink.

[willstott101]

In my PR at #221 I've added some tests to Sink. Which shows that calling .stop() on a Sink does not actually cause it to return None but rather 0.0. The Sink only ever "stops" in terms of iterators when it's dropped. After that PR is merged I'll look into at least giving stop() some unit tests, making sure that the behaviour is as expected.

[Zizico2]

The code of Sink is very very simple, and this struct exists mainly to make it easier to get started with rodio. In other words, using a Sink is not the primary way you'd play a sound in a production app. In order to get a behaviour different from the default one, the way to go is to copy-paste the code of Sink and modify it.

Shouldn't .stop() actually behave in an intuitive manner if Sink is supposed to "make it easier to get started with rodio"? Clearing the queue of sounds should be basic functionality. And to be honest many apps would not need to look any further than Sink if a .clear() like function was implemented. At least it should be made absolutely clear that .stop() makes a sink useless, even though it is redundant considering .drop() exists...

I'm sorry if I sound a bit annoyed (because I am), I just spent a solid hour looking at the docs before coming here. I don't agree with that design decision, but if we shall stick with it, it should be abundantly clear in the docs how .stop() works.

[ChiefMilesEdgeworth]

I agree whole-heartedly with the comments of people so far. The documentation does not make it clear that a Sink is not usable after using the .stop() method, as it just says

Stops the sink by emptying the queue.

which does not indicate in any way that the Sink is unusable. A .clear() method would be fantastic. What issues are there with the implementation that keeps this from being a quick fix? Wouldn't it be as simple as emptying the queue? Perhaps it could even .pause() after emptying, and the client would be required to .play() in order to restart it after .append()ing more Sources.

Regardless, I would be happy to try to help this get going. It seems like a really important part of the library unless we want people to have to make new Sinks every time. As Zizico2 pointed out, the Sink is supposed to "make it easier to get started with rodio", and this seems like a major blocking point for a lot of people.

[lopeetall]

I'd like to help with this also! I am trying to level up my Rust skills by writing a minimalist music player using rodio and gtk-rs. A .clear() would be very very helpful.

[ChiefMilesEdgeworth]

So, I'm trying to work through a solution right now. My idea is to have another impl Iterator struct like Stoppable for sources, and to make it Skippable. The iterator's next() will run the inner iterator's next() method if skip is false, and consume the entire iterator if it is true. This would make the sink move on to the next source on its list, or stop if it runs out.

The sink would have a skip() method which sets the current iterator's Skippable struct field to true, and the clear() method would simply skip() as many times as there are sources, and then pause() itself so it outputs zeros.

How does this sound so far? I struggle with handling some of the concurrency bits, but this seems like a reasonable approach to take, and would create two new useful methods instead of just the one.