Remove the need to keep the stream around

[yara-blue]

Its a common footgun in using rodio. It should be possible to store the stream in the sink.

Remaining questions, does the stream need to be accessible when a sink is used? If so how? Does sink get a method to get the stream?

Implementation wise it seems like storing the stream in an Arc will work.

[PetrGlad]

Can Sink's output be not a stream but another source? Should they give some shared reference to the output stream in that case?

I would keep sink as simple as possible unless there is a compelling reason to add anything to it. It is a lot harder to remove things from API than to add them.

[PetrGlad]

I am trying to prototype now to see how best to do this.

[yara-blue]

Can Sink's output be not a stream but another source?

Thats actually quite a nice idea. I wonder if there is any reason thats not the case right now... It seems quite elegant!

[yara-blue]

How would it help out for this issue though? The stream still needs to be kept somewhere.

And Sink is not really a footgun atm I rather not change it, Rodio has a ton of users depending on Sink in its current form.

[PetrGlad]

Just to note, i am working on this while creating a stream builder.
My latest version is here https://github.com/PetrGlad/rodio/tree/init-revamp
The init procedure may not be exactly what you envisioned, so I still need to explain/justify some of the choices there.

[yara-blue]

Ahh good to hear, take your time to play around with it. Feel free open a (draft) PR once you want feedback. I had a quick look at a diff and saw some unrelated (I think?) changes (stuff with k == 0 in src/conversions/sample_rate.rs).

so I still need to explain/justify some of the choices there.

Comments explaining why you did things a certain way are invaluable for maintainers, both later on when the code needs to be changed/updated/fixed and during review. So that would be very nice :)

If at all possible please split unrelated changes to different PR's, that makes them far easier to discuss and review. I can help out with that.

[PetrGlad]

Yes, some changes are unrelated, I will check that but they are mosly non-functional. I can trim the changeset down if that is a problem. The branch went through some experimentation so maybe some parts like method renames, may be omitted.
Regarding sample_rate.rs, the change fixes failing tests. Also there are failing checks on the doc comments. These checks are normally performed by cargo test. Are the tests even run by the rodio master CI?

[PetrGlad]

More on sample_rate tests, the quickcheck generates random values for inputs and usually probes also very small and very large values, so this causes overflows. My fix prevents testing very large coefficients. If that case is expected to be handled, then some other fix is required there.

[yara-blue]

I can trim the changeset down if that is a problem

That would be nice, it makes reviewing easier and there is less of a chance of a bug slipping through.

[yara-blue]

Regarding sample_rate.rs, the change fixes failing tests. Also there are failing checks on the doc comments. These checks are normally performed by cargo test. Are the tests even run by the rodio master CI?

strange, those tests should run in CI. Could you turn those changes into a separate PR? Then we can take a more detailed look at them and merge them.

[PetrGlad]

Test fixes alone #640