Tracking issue for redesigning the Sink API

[dvdsk]

There are currently two seperate PR's that expand on the Sink's functionality to be more easily used as a 'playlist':

• Allow retrieving the queue signal in a sink when using crossbeam-channel feature #600
• Add a SourcesIdQueueInput that allows manipulating items in the queue #506
• As long as the sink is the way to control sources it should be clone

This issue is to discuss a new Sink which supports both those applications, maybe by merging those PR's or by making breaking changes to the current Sink.

This should take #613 into account.

[teliosdev]

I think having append return a type that allows the user to register interest in when the sound is finished playing, whether that's sync or async. You may be able to avoid exposing channels at all with this, as the returned (opaque) type can have fn wait(&self) (which blocks the calling thread until the sound is done playing), and async fn stopped(&self), which only completes when the sound is done.

This also allows extension in the future to perform acts upon the specific sound, if that's desired, as well.

You can have this type take &self instead of &mut self by replacing the use of std::sync::mpsc with a Mutex/Condvar for the blocking API. For the async API, you may be able to get away with an AtomicBool/AtomicWaker.

[dvdsk]

We might not strictly need an append with interest since there is EmptyCallback. You could append this to the sink after the sound of interest then you would get a callback fired when the source is done.

This is hard to find, so some documentation effort may be needed. Even then it might be worth wile handling this in the sink via an append_with_callback that does append(source) then append(emptycallback).

[PetrGlad]

A problem I see with the Sink implementation is that forces to have all the functions embedded every time even if users do not need them. So maybe that makes Sink::append(...) implementation so cumbersome. It combines all the layers at once and then unwraps them in reverse in the callback. For example, to update the speed from a control it takes src.inner_mut().inner_mut().inner_mut().inner_mut().

I think, atomics can be used to control parameters without using a periodic callback. For f32 one can use AtomicCell<T> from crossbeam-utils. It is lock-free for f32. While sink is constructed the control handles can be collected and used directly to adjust parameters. Currently parameters are plain numeric values, one can even support both dynamic controls with atomics and plain values by allowing any Deref as the parameter value. So for dynamic control it could be for example Arc<AtomicCell<f32>> and for static config f32.

[dvdsk]

A problem I see with the Sink implementation is that forces to have all the functions embedded every time even if users do not need them.

Ahh yes, that could possibly slow down code. I thought of that a while back one of the solutions I came up with is to make Sink generic over a ton of const generic parameters like Pausable, Amplifyable these are then true or false. The member pausable will only be available on Sink<true , .. > (given Sink<Pausable, Amplifyable, other things..>).

It combines all the layers at once and then unwraps them in reverse in the callback.

Users do not see this though, and I do not really mind it (but it does look funny when you see it for the first time).

I think, atomics can be used to control parameters without using a periodic callback.

They could, and that are probably fast enough. On x86 using Relaxed ordering it should even be the same. On arm there is a difference. Since its about half a million atomic reads per second! It should definitely be bench-marked. Would require a lot of work especially since you need to do it for multiple cpu-architectures.

What will probably wreck performance is using Arc. That is going to ruin cache locality and the optimizer also gets limited in what it can do.

one can even support both dynamic controls with atomics and plain values by allowing any Deref as the parameter value.

Nice trick, every Source getting a generic parameter is a bit scary though. We could always solve that by having dynamic_source and source (for example rodio::dynamic_source::pausable(&an_atomic_f32). Then it would not break the current API either.

[PetrGlad]

I am not sure that cache locality will be a problem since the value behind the Arc will be very small (4 bytes) although it will likely use up a cache line (usually 64 bytes).
It looks like Arc just relies on it's handles to count properly, so normal access to it's value is the same as de-referencing a pointer. Arc also uses atomics for its counters but those are only needed at initialization and at drop.
Indeed, targeting CPU without atomics support is a problem then.

[dvdsk]

Indeed, targeting CPU without atomics support is a problem then.

Oh no thats not what I meant, just that on some platforms there is are atomics but they are slower.

I am not sure that cache locality will be a problem since the value behind the Arc will be very small (4 bytes) although it will likely use up a cache line (usually 64 bytes).

Or 6 cache line's that is not in the hand of the allocates and where it found space for 6 small values that might not necessarily have been created at the same time. I don't know.

normal access to it's value is the same as de-referencing a pointer

Right now access does involve de-referencing a pointer. Since the variables are currently stack allocated the compiler can more easily apply a ton of optimizations.

[dvdsk]

I have enjoyed our conversation today :) Its very nice to discuss this in depth. It is getting rather late here so I am going to call it for now.

I wish you a very good night, and I look forward to reading more 👍

[dvdsk]

I think, atomics can be used to control parameters without using a periodic callback.

I have thought about this some more and its really something we should try. It will allow users to modify sources that are being mixed separate, something that is impossible now. Could you open an issue for this (since its your idea, credit goes to you)? Then we can discuss it further there. Ill see if I can add a benchmark suite to rodio to make it easy to measure the performance impact.

[UnknownSuperficialNight]

For f32 one can use AtomicCell from crossbeam-utils. It is lock-free for f32.

We should also look into benching atomic_float and AtomicCell<T> to see if there is a difference in performance if we do go down that path.