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

Updated dependencies, new audio resampling library, performance improvements and volume control #10

Merged
merged 17 commits into from Jan 2, 2022

Conversation

sibartel
Copy link
Contributor

@sibartel sibartel commented Dec 30, 2021

Heyho @codetheweb,

as promised some days ago in issue #9, here are my changes open to be discussed:

  • Updated dependencies:

  • Switched the library for audio resampling from samplerate to rubato:

    Samplerate is based on a c library and the function call used is not recommended for streamed audio because it does not work over chunk boundaries. As written in the docs:

    Perform a simple samplerate conversion of a large chunk of audio. This calls src_simple of libsamplerate which is not suitable for streamed audio.

    Therefore, this switch should make aoede more portable and additionally improve the audio quality.

  • Performance improvements:

    • Implemented non blocking EmittedSink::read.
    • Increased EmittedSink internal sync_channel buffer size to fit at least one resampling frame. And therefore, most times no synchronisation is necessary.
    • Changed datatype in internal sync_channel to fit the floats of both channels. This reduces the amount of synchronisations needed between EmittedSink::read and EmittedSink::write.
  • Implemented volume control using librespot::SoftMixer. Should close Volume Control not working #9.

I'm looking forward to hear from you.
Best regards!

Copy link
Owner

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this approach makes much more sense than what I was doing. Downsides of learning a language by writing something in it. 😛

Looks like CPU usage while streaming dropped from 10-12% to 5-6% which is excellent as well. 👍

src/lib/player.rs Outdated Show resolved Hide resolved
src/lib/player.rs Outdated Show resolved Hide resolved
@sibartel
Copy link
Contributor Author

sibartel commented Jan 2, 2022

Heyho @codetheweb,
happy new year! Thx for the review, I made the following changes:

  • Added error to inform about minimum buffer size for EmittedSink::read call
  • Fixed spelling
  • Added comment to explain the upper bound of sync_channel
  • Replaced vague calculation of sync_channel upper bound with the correct value for our settings

Should i squash the commits in some ways?

Regarding the CPU usage: Using the release build I have permanently less than 0.3% on my machine. Maybe we can investigate and optimize the cpu usage further in the future.

@codetheweb
Copy link
Owner

Looks good!

Happy New Year to you too.

I'll squash when merging.

Regarding the CPU usage: Using the release build I have permanently less than 0.3% on my machine. Maybe we can investigate and optimize the cpu usage further in the future.

I should have clarified that my usage stats were assuming full utilization of one core is 100%. If you're measuring based on overall utilization of the CPU, that would explain the difference. I'm on a M1 Max. I'm happy with performance as-is, but if you think it can further be optimized go for it. :)

@codetheweb codetheweb merged commit 90ca6b7 into codetheweb:main Jan 2, 2022
@codetheweb
Copy link
Owner

Released in v0.3.0.

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

Successfully merging this pull request may close these issues.

Volume Control not working
2 participants