Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Runtime controlling sources in rodio, can we do better? #658

Open
dvdsk opened this issue Dec 11, 2024 · 11 comments
Open

Runtime controlling sources in rodio, can we do better? #658

dvdsk opened this issue Dec 11, 2024 · 11 comments

Comments

@dvdsk
Copy link
Collaborator

dvdsk commented Dec 11, 2024

Chains of rodio Source's are currently only controllable using periodic callback. It has a few great advantages namely:

  • Users get to pick between latency and performance through longer and shorter periods between callbacks. Those periods are defined in real time not number of samples
  • Can have very little overhead as one periodic callback at the end can control the entire chain. That means only one conditional is added for most samples.

It does have its downsides it require users to either: have lots of periodic callbacks which negates the advantages, for example:

let controlled source = sine(..).periodic_callback(function_controlling_sine)
  .amplify(1.0).periodic_callback(function_controlling_amplify)
  .pausable().periodic_callback(function_controlling_pausable)

Or to use lots of inner_mut to navigate through the pipeline/chain. This feels akward and is less readable.

let controlled source = sine(..).amplify(1.0).pausable().periodic_callback(|pausable| {
  function_controlling_pausable(pausable);
  function_controlling_amplify(pausable.inner_mut())
  function_controlling_sine(pausable.inner_mut().inner_mut())
})

Users also have to set up their own method of moving intentions/commands into the callbacks. Two solutions are: checking a queue receiver or always updating inner sources from loaded atomic's. This can be hard for newer rust users or those not skilled at concurrency.

Over the last few months a few alternatives have been proposed such as:

  • heap allocated atomics.
  • a command queue.

There are also other audio projects we can take inspiration from, such as:
hodaun and
kira.

Not all of these solutions are optimal for every source. Where atomics might
work well for simple few paramater sources like amplify and pausable they could
really hamper performance for things like channel_router.

Lets discuss our options highlighting advantages and potential drawbacks then
evaluate those.

@iluvcapra
Copy link
Contributor

I was thinking couldn't there just be a trait like LiveSource or something where you could define a State type that contained all the parameters that a controller would change, and then some implementations to do the work, and that way abstract away the implementation of this so just by taking a type, adopting the LiveSource trait, creating a State type and filling out some boilerplate you would get an object that was live controllable wihtout having to worry about the implementation details.

@iluvcapra
Copy link
Contributor

iluvcapra commented Dec 11, 2024

Also I think Periodic could use some work, especially the fact that the modifier F is called by the audio thread, it'd be better if it spun this off to a different thread, queue, runloop, whatever so audio didn't stop processing.

If a client needed a source to update itself at precise sample times, what you could do is a do a Scheduled source that allows a client to provide a schedule of updates to happen at exact times in the future of the render. This schedule could then be updated on a slow thread and events added ahead of the audio render.

ETA- In fact you could do something like how complications work on the Apple Watch, where you provide a clousure that you can use to construct future events for a scheduled source, the API contract promises this might be called once a second or so in advance of the relevant samples, you return a set of changes and when they should happen, and the machinery does them when the samples come up. But client code never runs on the audio thread.

@dvdsk
Copy link
Collaborator Author

dvdsk commented Dec 12, 2024

I was thinking couldn't there just be a trait like LiveSource or something where you could define a State type that contained all the parameters that a controller would change

So each source gets an associated type State which is a controller of that source (and maybe Clone + Send + Sync for ease of use)? I do have ideas for an API that allows controller/atomic based and lean minimal periodic callback sources to co-exist without overhead:

When you use some_source.pausable() you would still get the current pausable layer/wrapper that is only controllable using periodic callback. If you append controller to it you get back two things, the audio pipeline and a controller object.

let (source, amplify_controller) = some_source.amplify().controller();
let (source, pausable_controller) = source.pausable().controller();

play(source);

amplify_controller.set_gain(0.5);
pausable_controller.set_paused(true);

Also I think Periodic could use some work, especially the fact that the modifier F is called by the audio thread, it'd be better if it spun this off to a different thread, queue, runloop, whatever so audio didn't stop processing.

The idea is that the user does the minimal amount of work in the closure, for example check if a queue has received a message. That requires the user to be efficient and usually it will boil down to something like this:

  some_source.periodic_callback(move |src| match rx.recv() {
      Cmd::Pause => src.inner_mut().set_paused(true),
      Cmd::Resume => src.inner_mut().set_paused(false),
      Cmd::Amplify(va) => src.inner_mut().inner_mut().set_amplify(val)
    }
)

But by allowing the user to run on the audio thread we eliminate a ton of synchronization overhead. None of the source parameters need to be atomic, neither do we need any mutexes at all. All parameters live on the stack and because the final source is one big type the optimizer can go crazy. That should lead to really good performance, I say should since it has never been bench-marked.

If a client needed a source to update itself at precise sample times, what you could do is a do a Scheduled source that allows a client to provide a schedule of updates to happen at exact times in the future of the render. This schedule could then be updated on a slow thread and events added ahead of the audio render.

We could do something like scheduled_callback? A regular update every n-seconds would be the existing periodic_callback.

@dvdsk
Copy link
Collaborator Author

dvdsk commented Dec 12, 2024

periodic callback has a big issue when one of the stages in the audio pipelines is mix() since it does type erasure because it works using a vec of Box. You can not ask for those sources, in other words there is no inner_mut.

I think we can address the most common mix cases by having a mix_n() where n is 2,3,4,5,6 etc, up to some reasonable limit. The type would be Mixn (again an a number) with n generics, one generic for each stream that's being mixed. Then you could have nth_mut() with a n again 2,3,4,5,6 etc. Might also give a speedup since all sources are then on the stack and dynamic dispatch is no longer used.

example:

let source_a: Sine = ..
let source_a = source_a.pausable();
let source_b: Square = ..

let (rx, tx) = mpsc::channel();
let source: Mix2<Sine, Square> = source_a.mix2(source_b);
let source = source.with_periodic_callback(move |mix| match rx.recv() {
  Cmd::SetSineFreq(new) => mix.get_1th_mut().freq = new,
  Cmd::SetSquareFreq(new) => mix.get_2th_mut().freq = new,
  Cmd::PauseSine => mix.get_1th_mut().into_inner().set_paused(true),
})

play(source)

// from UI thread or somewhere else in the program (tx can be cloned)
tx.send(SetSineFreq(9001);
// do something else then
tx.send(Cmd::PauseSine);

A similar problem seems to exist with queue. Though for short constant length queue's we can again do queue_n (n a number) maybe?

@PetrGlad
Copy link
Collaborator

PetrGlad commented Dec 12, 2024

Brittle chain navigation in Source is unnecessary. It is just a consequence of having all the controls in the same place. I even wanted to rewrite that.
What I would prefer is having a constructor functions that makes a filter together with it's control. Something like

let external source = open_mp3();
let (pausable, pause_ctl) = pausable(external_source);
let (amplified, volume_ctl) = amplify(pausable);

This way there is always only one periodic callback right next to its source. And the source's constructor knows what to change in there. So only pausable implementation needs to know how to pause itself.

API-wise we'll need constructors for static (the existing ones) and controllable versions of a source. Or alternatively in addition to normal constructors add also wrapping functions in form fn(pausable) -> (pausable, pause_ctl).

If we still want to keep default source with all the gadgets included, then it could do the above and keep all the *_ctls values in self. Or alternatively turn current Source implementation into a pure builder.

This also lets users to construct only filters they need.

@iluvcapra
Copy link
Contributor

I like the returned-controller approach as well, I could take a swing at writing generics that have this pattern.

@dvdsk
Copy link
Collaborator Author

dvdsk commented Dec 14, 2024

Brittle chain navigation in Source is unnecessary. It is just a consequence of having all the controls in the same place.

Its slightly more nuanced in my opinion. I would say its a consequence of wanting all parameters on the stack in the audio thread with minimal overhead. I do not see another way of doing that.

API-wise we'll need constructors for static (the existing ones)

I would keep the current API for that, that way we do not lose the unique performance advantage (I am pretty sure) rodio has.

I like this builder like pattern I proposed earlier:
let (source, amplify_controller) = some_source.amplify().controller();

The amplify returns the current Amplify struct which just uses a float internally. controller (or another name) is a member on Amplify with this signature: fn controller(self) -> (ControlledAmplify, AmplifyController).

The controller should be really easy to use, it should implement : Send, Sync and Clone in addition to Debug ofc.

@PetrGlad
Copy link
Collaborator

Regarding current source implementation, yes it looks indeed that the current implementation of Sink is the tightest one (except it bundles controls that users may not always need). The Controls struct is still allocated on heap (since it is behind Arc), it is not on stack. But since all the knobs are packed in the same struct it may be cache friendly. Separate allocation may be still be a reasonable choice if only necessary steps are added. It can even be just as good if a custom arena for allocation is used.

What I had in mind for the callback version of Sink, is almost as lean as the current implementation but would use one callback per step (so instead of one it will have several):

let source = source
  .speed(1.0)
  .periodic_access(Duration::from_millis(5), move |src| {
    // Update speed via src here
  }).
  .track_position()
  .periodic_access(Duration::from_millis(5), move |src| {
    // Update position via src here
  }).
  .pausable(false)
  .periodic_access(Duration::from_millis(5), move |src| {
    // Update pausable via src here
  }).
  .amplify(1.0)
  .periodic_access(Duration::from_millis(5), move |src| {
    // Update amplify via src here
  }).
  .skippable()
  .periodic_access(Duration::from_millis(5), move |src| {
    // Update skippable via src here
  }).
  .stoppable()
  .periodic_access(Duration::from_millis(5), move |src| {
    // Control stoppable via src here
  })

instead of

let source = source
  .speed(1.0)
  .track_position()
  .pausable(false)
  .amplify(1.0)
  .skippable()
  .stoppable()
  .periodic_access(Duration::from_millis(5), move |src| {
      // src.inner().inner().inner() ...
  })

@dvdsk
Copy link
Collaborator Author

dvdsk commented Dec 18, 2024

What I had in mind for the callback version of Sink, is almost as lean as the current implementation but would use one callback per step (so instead of one it will have several):

could you explain the advantage of this approach? Is it for code readability/maintainability? Or is the 'callback version' something else entirely?

@PetrGlad
Copy link
Collaborator

The advantage is that this approach is composable, and easier to read. Although I admit that if all existing Sink functionality is required, the existing implementation is probably the most efficient (at least if updates are very frequent, at 5ms poll it may not be the case). That is because no indirection or dyn dispatch is used when updating the controls and there is only one callback.

@PetrGlad
Copy link
Collaborator

PetrGlad commented Dec 24, 2024

Just in case rustc does have config option that tells which atomics are available
https://doc.rust-lang.org/reference/conditional-compilation.html#target_has_atomic
maybe we can use that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants