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

Add and default to "auto" normalisation type #844

Merged
merged 3 commits into from
Sep 20, 2021

Conversation

roderickvd
Copy link
Member

As discussed in #780, this adds an "auto" normalisation type, and makes it the default. This automatically picks the album or track ReplayGain values, depending on whether the current context is an album context or not.

When autoplay is enabled and the playlist is extended when an album reaches its end, then "auto" starts picking the track values until an album is put up again.

@roderickvd
Copy link
Member Author

Weird that the nightly test fails, that seems totally unrelated to my changes. I'll try the CI again tomorrow.

@roderickvd
Copy link
Member Author

roderickvd commented Sep 3, 2021

There's one stray issue: when a song is playing from a playlist or radio, and while it's playing, that same song is played again but now from an album context, "auto" won't switch from track to album ReplayGain values until the next song. This is probably because the player does not call get_factor again when the song is already loaded. I'll look into that later, too.

@JasonLG1979
Copy link
Contributor

Weird that the nightly test fails, that seems totally unrelated to my changes. I'll try the CI again tomorrow.

It's just a Error: Process completed with exit code 101. You're good.

There's one stray issue: when a song is playing from a playlist or radio, and while it's playing, that same song is played again but now from an album context, "auto" won't switch from track to album ReplayGain values until the next song. This is probably because the player does not call get_factor again when the song is already loaded. I'll look into that later, too.

Would that also mean that in that situation pausing and then playing again would would trigger the switch? If that's the case it would kinda be weird if there was a perceivable volume difference.

@roderickvd
Copy link
Member Author

Would that also mean that in that situation pausing and then playing again would would trigger the switch? If that's the case it would kinda be weird if there was a perceivable volume difference.

No, it's not so much as forcing a switch, as changing the player to always call get_factor so it picks up on the right context.

@JasonLG1979
Copy link
Contributor

JasonLG1979 commented Sep 4, 2021

No, it's not so much as forcing a switch, as changing the player to always call get_factor so it picks up on the right context.

As far as the audio backends are concerned there is no such thing as paused there is just start and stop. There is very little to no difference between going from nothing at the initial start up of librespot to start and going from stop to start when going from paused to play. As far as the audio backends are concerned going from play to pause to play might as well be different tracks, ofc they have no concept of tracks but you get the idea. I just want to make sure that the same is not true for the behavior of auto I would hate in your example edge case for the gain to change if play/pause was toggled.

@roderickvd roderickvd force-pushed the auto-normalisation-type branch from 52ac9c8 to 0ca7261 Compare September 5, 2021 20:43
@roderickvd
Copy link
Member Author

This should do it.

@JasonLG1979 yes that would be terrible. For sure that cannot happen with this PR. The player is instructed to pick track or album ReplayGain if the context changes (i.e. album, playlist, artist, track). When you pause-unpause, you stay within the same context, and so the gain cannot change.

A bit of testing would be welcome before I merge this one. Be on the lookout for verbose messages like the following.

Playing a track from a radio or playlist:

[2021-09-05T20:49:14Z DEBUG librespot_playback::player] command=SetAutoNormaliseAsAlbum(false)
[2021-09-05T20:49:14Z INFO  librespot_playback::player] Loading <Don’t You Get the Feeling (You’ve Been Had)> with Spotify URI <spotify:track:5YCZA6n6ppuD6XT7cDCYLa>
[2021-09-05T20:49:14Z DEBUG librespot_playback::player] Normalisation Data: NormalisationData { track_gain_db: -8.870003, track_peak: 1.1344055, album_gain_db: -7.25, album_peak: 1.1367749 }
[2021-09-05T20:49:14Z DEBUG librespot_playback::player] Calculated Normalisation Factor for Track: 12.78%

Then playing the same song but now from its album:

[2021-09-05T20:50:34Z DEBUG librespot_playback::player] command=SetAutoNormaliseAsAlbum(true)
[2021-09-05T20:50:34Z DEBUG librespot_playback::player] Normalisation Data: NormalisationData { track_gain_db: -8.870003, track_peak: 1.1344055, album_gain_db: -7.25, album_peak: 1.1367749 }
[2021-09-05T20:50:34Z DEBUG librespot_playback::player] Calculated Normalisation Factor for Album: 15.40%

Or any other order of events.

playback/src/player.rs Outdated Show resolved Hide resolved
@roderickvd
Copy link
Member Author

Anyone been able to take this for a spin?

@JasonLG1979
Copy link
Contributor

JasonLG1979 commented Sep 12, 2021

Anyone been able to take this for a spin?

I've been running it since 5b63ef6. I haven't really been reading debug logs but I haven't noticed anything that struck me as off or ran into any obvious bugs that I can tell. I'd say my quick take is that so far it seems to work fine. I will more actively listen and monitor logs tomorrow for a bit.

@roderickvd
Copy link
Member Author

I'll leave this up here over the weekend, then merge it if there are no more issues.

@JasonLG1979
Copy link
Contributor

Pull the trigger with this and everything else you want in 0.3 and then I'll fix any conflicts in #820 and you can merge it and then 0.3 will be good to go I think right?

@roderickvd roderickvd merged commit 949ca4f into librespot-org:dev Sep 20, 2021
@roderickvd roderickvd deleted the auto-normalisation-type branch September 20, 2021 17:22
@roderickvd
Copy link
Member Author

Pull the trigger with this and everything else you want in 0.3 and then I'll fix any conflicts in #820 and you can merge it and then 0.3 will be good to go I think right?

Done and agreed. And what an exciting release it will be!

roderickvd added a commit to roderickvd/librespot that referenced this pull request Oct 5, 2021
 * When autoplay is disabled, then loop back to the first track
   instead of 10 tracks back. Continue or stop playing depending
   on the state of the repeat button.

 * When autoplay is enabled, then extend the playlist *after* the
   last track. librespot-org#844 broke this such that the last track of an album
   or playlist was never played.

Fixes: librespot-org#434
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.

2 participants