Add volume normalisation support #162

sashahilton00 · 2018-02-23T04:10:41Z

The heavy lifting was done by @herrernst. Since his PR has been quiet for a while, and there has been a significant reordering of librespot's structure, this is a reorder/rewrite, taking onboard feedback from #114. Review/suggestion welcomed. Closes #114

sashahilton00 · 2018-02-23T08:16:37Z

Having just tested this addition, the difference is noticeable, and normalising volume does improve the playback experience. My vote would be to include this as a standard feature, and not as a runtime flag, whilst keeping the pregain flag for those who wish to modify it. Thoughts on doing this appreciated.

othiman · 2018-02-23T09:03:58Z

Hi,
for a nice and readable commit history I would propose to combine the two commits into one. E.g., by this: https://stackoverflow.com/questions/2563632/how-can-i-merge-two-commits-into-one/24690646#24690646.
Concerning making the normalization standard I would propose to keep it like the original Spotify client as an option which is disabled by default (isn't it?). Some users really don't like it if the music is altered in any way, so there should be at least an option to disable it.
Best regards,
Thomas

sashahilton00 · 2018-02-23T09:11:35Z

Have combined the two commits and left as a runtime flag.

herrernst · 2018-02-23T09:53:15Z

Sorry, didn't have the time lately. Thanks for taking care of this.
But please don't enable it by default, for the reasons outlined in #15 (comment) (playing an album would cause volume jumps between tracks).

sashahilton00 · 2018-02-23T14:33:27Z

@herrernst no problem. I was just having a look at the protobuf files, and it does not look as if there is an indicator as to whether the track is currently being played as part of an album. What I suspect is happening, based on this assumption, is that the track being played always has the track and album normalisation applied, as one would not notice it if songs were played individually, but one would if it were being played as part of an album. This still does not answer the question of how playlists/compilations handle volume normalisation though. If you have any thoughts/revelations on this later on though, adding album normalisation shouldn't be too hard once this is implemented.

I'll merge this later tonight if there are no objections.

plietar · 2018-02-23T13:53:41Z

playback/src/player.rs

@@ -43,6 +44,14 @@ enum PlayerCommand {
    Seek(u32),
 }

+#[derive(Debug)]


Derive Copy and Clone as well

plietar · 2018-02-23T18:03:49Z

playback/src/player.rs

+            normalisation_factor = f32::powf(10.0, (normalisation_config.track_gain_db + self.config.normalisation_pregain) / 20.0);
+
+            if normalisation_factor * normalisation_config.track_peak > 1.0 {
+                debug!("Reducing normalisation factor to prevent clipping. Please add negative pregain to avoid.");


This should be a warn!

plietar · 2018-02-23T18:07:25Z

playback/src/player.rs

@@ -43,6 +44,14 @@ enum PlayerCommand {
    Seek(u32),
 }

+#[derive(Debug)]
+struct NormalisationConfig {


This shouldn't be called Config. Something like Data or Values or Parameters would be more appropriate.

plietar · 2018-02-23T18:08:20Z

playback/src/player.rs

+        track_gain_db = file.read_f32::<LittleEndian>().unwrap();
+        debug!("Track gain: {}db", track_gain_db);
+
+        file.seek(SeekFrom::Start(148)).unwrap();


There's no need to seek everytime, reading an f32 will already move the cursor

plietar · 2018-02-23T18:09:48Z

playback/src/player.rs

+
+        file.seek(SeekFrom::Start(144)).unwrap();
+        track_gain_db = file.read_f32::<LittleEndian>().unwrap();
+        debug!("Track gain: {}db", track_gain_db);


Use a single debug! line for all 4 values (eg debug!("Normalization: {:?}", data);), potentially in load_track.

plietar · 2018-02-23T18:11:54Z

playback/src/player.rs

+
+        if self.config.normalisation {
+            let normalisation_config = self.parse_normalisation(&mut decrypted_file);
+            normalisation_factor = f32::powf(10.0, (normalisation_config.track_gain_db + self.config.normalisation_pregain) / 20.0);


I would move this code (and the clipping prevention) into a method of NormalisationCofig

sashahilton00 · 2018-02-23T18:16:07Z

I'm actually away this weekend, so don't think I'll be at a computer until next week. Feel free to implement the changes if you want to expedite things, otherwise this'll probably happen sometime next week.

sashahilton00 · 2018-02-23T19:11:47Z

Turns out editing on an iPad isn't so bad. @plietar let me know if there's any other changes you want to suggest, otherwise will merge in a couple of hours.

herrernst · 2018-02-23T19:19:49Z

@sashahilton00 When a track is played in album context, album gain should be applied; otherwise, track gain should be applied. Never should both gains be applied. Replay gain spec.
@plietar Do you know if it is possible to determine if an album is playing vs a mixed playlist?

sashahilton00 · 2018-02-23T19:22:56Z

Didn't realise Spotify implemented ReplayGain, though I didn't really look tbh. Makes sense though. If you have any thoughts as to how to add album support I'm all ears.

kingosticks · 2018-02-23T19:27:06Z

Can we add a named constant for the magic 144 number and/or say where it came from (@herrernst said https://sourceforge.net/p/despotify/code/HEAD/tree/java/trunk/src/main/java/se/despotify/client/player/SpotifyOggHeader.java#l106)

herrernst · 2018-02-23T19:30:24Z

@sashahilton00 Yes, these four values are the same you would get if running vorbisgain against the ogg files. Still don't know why Spotify hides them in this weird header.

kingosticks · 2018-02-23T19:32:26Z

Also, the logging uses inconsistent American vs British spelling of "normalisation".

sashahilton00 · 2018-02-23T19:42:46Z

Good spot, I copied the debug messages, slipped through the net. What do you mean when you say you want a named constant for the 144? Or do you just want a comment with a link to despotify for reference? I've avoided commenting the code just to keep it neat.

herrernst · 2018-02-23T19:45:22Z

I think the reference to despotify is not necessary. I've discovered the values by searching for meaningful float values in a hex editor, only finding the despotify reference later.

kingosticks · 2018-02-23T19:47:22Z

I think a descriptive name would be helpful here. I'm not a fan of magic numbers. SPOTIFY_HEADER_START_OFFSET = 144 or something.

plietar · 2018-02-24T00:34:00Z

playback/src/player.rs

+        }
+    }
+
+    fn get_factor(config: &PlayerConfig, data: NormalisationData) -> f32 {


Since you already have a if self.config.normalisation && normalisation_factor != 1.0 condition before applying it, you can ignore the config here.

missed that. removed.

plietar · 2018-02-24T00:34:32Z

playback/src/player.rs

+        static SPOTIFY_HEADER_START_OFFSET: u64 = 144;
+        file.seek(SeekFrom::Start(SPOTIFY_HEADER_START_OFFSET)).unwrap();
+
+        let track_gain_db: f32 = file.read_f32::<LittleEndian>().unwrap();


No need for the : f32 type annotations.

plietar · 2018-02-24T00:35:13Z

playback/src/player.rs

+
+impl NormalisationData {
+    fn new<T: Read + Seek>(file: &mut AudioDecrypt<T>) -> NormalisationData {
+        static SPOTIFY_HEADER_START_OFFSET: u64 = 144;


Make that a const, and call it SPOTIFY_NORMALIZATION_HEADER_START_OFFSET

plietar · 2018-02-24T00:37:10Z

playback/src/player.rs

+}
+
+impl NormalisationData {
+    fn new<T: Read + Seek>(file: &mut AudioDecrypt<T>) -> NormalisationData {


Function should be called parse_from_file or something, and it's argument can just be a mut file: T
AudioDecrypt<T> (and by extension &mut AudioDecrypt<T>) already implement Read + Seek

Also it can return an io::Result<NormalisationData> and use ? instead of unwrap, moving the unwrap to the caller.

have changed it to return a Result, but I'm using match instead of ? as I personally don't like the ? operator as it's confusing (feels like it should mean optional) and feel that match is more readable, also as normalisation is not something that should fail hard if the data can't be read, I've set the behaviour to warn if the normalisation data cannot be read, and set it to the default value of 1.0

plietar · 2018-02-24T00:41:33Z

playback/src/player.rs

+
+        let normalisation_data = NormalisationData::new(&mut decrypted_file);
+
+        let normalisation_factor: f32 = NormalisationData::get_factor(&self.config, normalisation_data);


No need for : f32

sashahilton00 · 2018-02-24T15:49:48Z

Will merge in a couple of hours further feedback notwithstanding.

slartibartfast3 · 2020-06-29T12:49:37Z

Regarding the difficulty in knowing if a track is part of an album or a playlist and whether album or track gain should be applied, would it be possible or practical to add an option for the user to select either album gain or track gain? On my squeezeboxes there is an option to select album gain, track gain or smart gain which uses album gain for an album and track gain for a playlist of individual tracks. I find that using album gain all the time is preferable to smart gain and definitely preferable to permanent track gain.
An option to select permanent album gain would be a worthwhile addition for many users.

DeLub · 2021-01-21T14:41:55Z

Yes, this would be a very nice option. I was just listening to The Wall, but with the volume jumps it was unbearable.

slartibartfast3 · 2021-01-21T15:12:33Z

Yes, this would be a very nice option. I was just listening to The Wall, but with the volume jumps it was unbearable.

That album makes the case perfectly. It was the album I used to test Spotify normalisation. The final track sounds particularly ridiculous. An option to choose between Track and Album gain is essential for many users.

sashahilton00 · 2021-01-21T16:54:36Z

I'd suggest opening a new issue and/or making a PR detailing the desired change and a possible implementation so that it can be discussed, since this PR is closed.

herrernst · 2021-01-21T19:02:34Z

I've read a study (or discussion) somewhere with the result that applying album gain should be the default option (at least if you can't figure out the context of a track). Different tracks of an album often don't deviate much from the average album gain, and even if they do (like the last track on The Wall), it's probably an artistic choice that the track should be quieter compared to others.

So IMHO album gain should be default option, with the option of selecting track gain (for people that absolutely need exact track gain when playing playlist with mixed tracks) and disabling it. If I read the code correctly, librespot uses track gain at the moment, but it better should use album gain as default.

sashahilton00 force-pushed the normalisation branch from 524021e to ff1f7e1 Compare February 23, 2018 09:07

sashahilton00 mentioned this pull request Feb 23, 2018

Rustfmt syntax correction #163

Closed

plietar requested changes Feb 23, 2018

View reviewed changes

Add volume normalisation support

f8db550

sashahilton00 force-pushed the normalisation branch from ff1f7e1 to f8db550 Compare February 23, 2018 19:08

[ci skip] Correct spelling

fc6c414

Add constant for readability

127f8b7

plietar reviewed Feb 24, 2018

View reviewed changes

Minor style changes to normalisation code

542ec9d

sashahilton00 merged commit eed2bb6 into master Feb 24, 2018

sashahilton00 deleted the normalisation branch February 25, 2018 00:04

sashahilton00 mentioned this pull request Mar 22, 2018

Volume normalization #15

Closed

mavit mentioned this pull request Jun 29, 2020

[Feature Request] Volume Level Option michaelherger/Spotty-Plugin#15

Closed

sashahilton00 mentioned this pull request Jan 21, 2021

Add option to choose between track or album normalisation gain #573

Merged


		let normalisation_data = NormalisationData::new(&mut decrypted_file);

		let normalisation_factor: f32 = NormalisationData::get_factor(&self.config, normalisation_data);

Add volume normalisation support #162

Add volume normalisation support #162

Conversation

sashahilton00 commented Feb 23, 2018

sashahilton00 commented Feb 23, 2018

othiman commented Feb 23, 2018

sashahilton00 commented Feb 23, 2018

herrernst commented Feb 23, 2018

sashahilton00 commented Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashahilton00 commented Feb 23, 2018

sashahilton00 commented Feb 23, 2018

herrernst commented Feb 23, 2018

sashahilton00 commented Feb 23, 2018

kingosticks commented Feb 23, 2018

herrernst commented Feb 23, 2018

kingosticks commented Feb 23, 2018

sashahilton00 commented Feb 23, 2018

herrernst commented Feb 23, 2018

kingosticks commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plietar Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashahilton00 commented Feb 24, 2018 • edited Loading

slartibartfast3 commented Jun 29, 2020

DeLub commented Jan 21, 2021

slartibartfast3 commented Jan 21, 2021

sashahilton00 commented Jan 21, 2021

herrernst commented Jan 21, 2021

sashahilton00 commented Feb 23, 2018 •

edited

Loading

plietar Feb 24, 2018 •

edited

Loading

sashahilton00 commented Feb 24, 2018 •

edited

Loading