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

Respect disabled normalisation with maximum volume #1159

Merged
merged 3 commits into from
May 6, 2023

Conversation

AER00
Copy link
Contributor

@AER00 AER00 commented May 2, 2023

I've noticed an issue with my CPU usage on a really low end device while playing music with maximum volume. When volume is equal to 1.0 and normalisation is disabled the first condition failed (as it should) and librespot didn't check whether the normalisation was disabled anymore - it just normalised the song anyway using self.config.normalisation_method.

@kingosticks
Copy link
Contributor

This is a great catch.

michaelherger added a commit to michaelherger/librespot that referenced this pull request May 2, 2023
Comment on lines 1548 to 1551
if !self.config.normalisation && volume >= 1.0 {
// do not alter sample
}
else if !self.config.normalisation && volume < 1.0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe nice to group the two non-normalised cases in one branch

                        if !self.config.normalisation {
                            if volume < 1.0 {
                                for sample in data.iter_mut() {
                                    *sample *= volume;
                                }
                            }
                        }     

@kingosticks
Copy link
Contributor

Can you fix the formatting?

@roderickvd
Copy link
Member

roderickvd commented May 2, 2023

For reasons of style and saving one or two CPU instructions I propose to fold the branch like this:

if !self.config.normalisation {
  if volume < 0 {
      //
  }
} else if

@AER00
Copy link
Contributor Author

AER00 commented May 2, 2023

For reasons of style and saving one or two CPU instructions I propose to fold the branch like this:

if !self.config.normalisation {
  if volume < 0 {
      //
  }
} else if

Have you seen the recent commit?

@roderickvd
Copy link
Member

Ah very good, thanks!

JasonLG1979 added a commit to JasonLG1979/librespot that referenced this pull request May 3, 2023
@kingosticks
Copy link
Contributor

LGTM.

I think we said we wouldn't backport anything more to master but... What do you say?

michaelherger added a commit to michaelherger/librespot that referenced this pull request May 4, 2023
michaelherger added a commit to michaelherger/librespot that referenced this pull request May 4, 2023
@roderickvd
Copy link
Member

One final question @AER00: would you update the changelog?

@kingosticks I don't really consider it anymore for me to say. But to offer my advice, if anyone is going to cut a release, I would spend the time in cutting the 0.5.x release.

@roderickvd roderickvd merged commit 31d18f7 into librespot-org:dev May 6, 2023
@kingosticks
Copy link
Contributor

I'm just wondering if there's utility in cherry picking into master. I guess those that care already have their forks.

Thanks all for this good fix.

kingosticks pushed a commit to kingosticks/librespot that referenced this pull request May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants