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

Sink controls redesign #624

Open
PetrGlad opened this issue Sep 30, 2024 · 3 comments
Open

Sink controls redesign #624

PetrGlad opened this issue Sep 30, 2024 · 3 comments

Comments

@PetrGlad
Copy link
Contributor

PetrGlad commented Sep 30, 2024

A follow up on #619 (comment)
Current implementation of Sink unconditionally includes several audio controls like pause, volume, speed (see src/sink.rs). There are several things that can be improved over current implementation.

Users may not need all for these at once and prefer to have only necessary conversions. It is possible to make the controls composable so users can add only necessary parts and adjust audio parameters for each input separately.

Current implementation of controls (like volume control, see here ) only accepts fixed values. Sink allows to control these parameters dynamically, which is implemented with a callback that is invoked periodically. The callback applies values requested by user to the actual control. Currently this happens at about 5ms intervals.

It is possible to apply the parameters directly by using atomic values. This should reduce reaction time and simplify the 'Sink' implementation (see callback setup in src/source.rs). In case atomics turn out to be expensive on some architectures, it is still possible to simplify Sink implementation by allowing each audio converter to control its own parameters. In that case each converter will have to have its own periodic_callback.

Should be included into #619.

@dvdsk
Copy link
Collaborator

dvdsk commented Sep 30, 2024

In case atomics turn out to be expensive on some architectures, it is still possible to simplify Sink implementation by allowing each audio converter to control its own parameters.

I would say this is the key. We got to get benchmarks on this. I would suggest using the soon to be merged AGC source (has a lot of controls) + whatever more you can find to create some kind of horror scenario with a ton of controls. Create a benchmark in a PR. Then we merge that PR and that gives us a baseline (what the current setup gives for performance).

Then you give all the used sources controls and we merge that into a separate branch (lets call that branch atomic_controls). Now we run the benchmark again and see if the performance is close to that of the original benchmark. If it is we make all the sources have atomic controls. I would do the same as @UnknownSuperficialNight did for AGC and give each source a getter that returns a control.

If its not close enough to the baseline then I have some ideas on how we can make it more performant without resorting periodic callbacks.

Do you agree with this testing approach?

@PetrGlad
Copy link
Contributor Author

PetrGlad commented Oct 2, 2024

Yes, it is a good idea to try this in a separate branch. I have already tried Criterion it has its limitations but my first impression was good.
I see a difficulty with testing on various architectures, maybe Github actions may help with that. I can dig up my RaspberryPis (v2 and v3) for running these benchmarks.

I am trying to complete one of my projects right now, but otherwise I will have time for rodio if it helps.

@dvdsk
Copy link
Collaborator

dvdsk commented Oct 2, 2024

we've integrated divan a few comments ago. I have a pi3 I can test on. That is way easier then setting up a github action I think.

I am trying to complete one of my projects right now, but otherwise I will have time for rodio if it helps.

Have fun and good luck! Any help you can offer is more then welcome, however we've got time there is no rush.

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

No branches or pull requests

2 participants