librespot-org / librespot

Open Source Spotify client library
MIT License
4.52k stars 544 forks source link

Respect disabled normalisation with maximum volume #1159

Closed AER00 closed 1 year ago

AER00 commented 1 year ago

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 commented 1 year ago

This is a great catch.

kingosticks commented 1 year ago

Can you fix the formatting?

roderickvd commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Ah very good, thanks!

kingosticks commented 1 year ago

LGTM.

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

roderickvd commented 1 year ago

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.

kingosticks commented 1 year ago

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.