From 47f1362453d7b4b99933578c044709251da5918b Mon Sep 17 00:00:00 2001 From: Jason Gray Date: Mon, 14 Feb 2022 05:15:19 -0600 Subject: [PATCH] Port remove unsafe code and catch up with dev (#956) --- CHANGELOG.md | 4 + connect/src/spirc.rs | 6 +- core/src/cache.rs | 17 +- core/src/file_id.rs | 7 +- core/src/spclient.rs | 14 +- core/src/spotify_id.rs | 62 ++++--- examples/playlist_tracks.rs | 12 +- metadata/src/episode.rs | 2 +- metadata/src/playlist/annotation.rs | 2 +- metadata/src/playlist/list.rs | 4 +- metadata/src/track.rs | 2 +- playback/src/config.rs | 2 +- playback/src/convert.rs | 40 ++--- playback/src/dither.rs | 6 +- playback/src/mixer/alsamixer.rs | 15 +- playback/src/player.rs | 266 +++++++++++++++++----------- src/main.rs | 23 +-- src/player_event_handler.rs | 123 +++++++++---- 18 files changed, 366 insertions(+), 241 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0a91a29..6db058bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [main] Prevent hang when discovery is disabled and there are no credentials or when bad credentials are given. - [main] Don't panic when parsing options. Instead list valid values and exit. - [main] `--alsa-mixer-device` and `--alsa-mixer-index` now fallback to the card and index specified in `--device`. +- [core] Removed unsafe code (breaking) +- [playback] Adhere to ReplayGain spec when calculating gain normalisation factor. +- [playback] `alsa`: Use `--volume-range` overrides for softvol controls +- [connect] Don't panic when activating shuffle without previous interaction. ### Removed - [playback] `alsamixer`: previously deprecated option `mixer-card` has been removed. diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 91256326..db5ff0c9 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -859,15 +859,15 @@ impl SpircTask { self.state.set_shuffle(update.get_state().get_shuffle()); if self.state.get_shuffle() { let current_index = self.state.get_playing_track_index(); - { - let tracks = self.state.mut_track(); + let tracks = self.state.mut_track(); + if !tracks.is_empty() { tracks.swap(0, current_index as usize); if let Some((_, rest)) = tracks.split_first_mut() { let mut rng = rand::thread_rng(); rest.shuffle(&mut rng); } + self.state.set_playing_track_index(0); } - self.state.set_playing_track_index(0); } else { let context = self.state.get_context_uri(); debug!("{:?}", context); diff --git a/core/src/cache.rs b/core/src/cache.rs index 9b81e943..f4fadc67 100644 --- a/core/src/cache.rs +++ b/core/src/cache.rs @@ -368,12 +368,17 @@ impl Cache { } pub fn file_path(&self, file: FileId) -> Option { - self.audio_location.as_ref().map(|location| { - let name = file.to_base16(); - let mut path = location.join(&name[0..2]); - path.push(&name[2..]); - path - }) + match file.to_base16() { + Ok(name) => self.audio_location.as_ref().map(|location| { + let mut path = location.join(&name[0..2]); + path.push(&name[2..]); + path + }), + Err(e) => { + warn!("{}", e); + None + } + } } pub fn file(&self, file: FileId) -> Option { diff --git a/core/src/file_id.rs b/core/src/file_id.rs index 79969848..5422c428 100644 --- a/core/src/file_id.rs +++ b/core/src/file_id.rs @@ -2,7 +2,7 @@ use std::fmt; use librespot_protocol as protocol; -use crate::spotify_id::to_base16; +use crate::{spotify_id::to_base16, Error}; #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct FileId(pub [u8; 20]); @@ -14,7 +14,8 @@ impl FileId { FileId(dst) } - pub fn to_base16(&self) -> String { + #[allow(clippy::wrong_self_convention)] + pub fn to_base16(&self) -> Result { to_base16(&self.0, &mut [0u8; 40]) } } @@ -27,7 +28,7 @@ impl fmt::Debug for FileId { impl fmt::Display for FileId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.write_str(&self.to_base16()) + f.write_str(&self.to_base16().unwrap_or_default()) } } diff --git a/core/src/spclient.rs b/core/src/spclient.rs index 78cccbf0..37a125ad 100644 --- a/core/src/spclient.rs +++ b/core/src/spclient.rs @@ -354,7 +354,7 @@ impl SpClient { } pub async fn get_metadata(&self, scope: &str, id: SpotifyId) -> SpClientResult { - let endpoint = format!("/metadata/4/{}/{}", scope, id.to_base16()); + let endpoint = format!("/metadata/4/{}/{}", scope, id.to_base16()?); self.request(&Method::GET, &endpoint, None, None).await } @@ -379,7 +379,7 @@ impl SpClient { } pub async fn get_lyrics(&self, track_id: SpotifyId) -> SpClientResult { - let endpoint = format!("/color-lyrics/v1/track/{}", track_id.to_base62()); + let endpoint = format!("/color-lyrics/v1/track/{}", track_id.to_base62()?); self.request_as_json(&Method::GET, &endpoint, None, None) .await @@ -392,7 +392,7 @@ impl SpClient { ) -> SpClientResult { let endpoint = format!( "/color-lyrics/v2/track/{}/image/spotify:image:{}", - track_id.to_base62(), + track_id.to_base62()?, image_id ); @@ -416,7 +416,7 @@ impl SpClient { pub async fn get_audio_storage(&self, file_id: FileId) -> SpClientResult { let endpoint = format!( "/storage-resolve/files/audio/interactive/{}", - file_id.to_base16() + file_id.to_base16()? ); self.request(&Method::GET, &endpoint, None, None).await } @@ -459,7 +459,7 @@ impl SpClient { .get_user_attribute(attribute) .ok_or_else(|| SpClientError::Attribute(attribute.to_string()))?; - let mut url = template.replace("{id}", &preview_id.to_base16()); + let mut url = template.replace("{id}", &preview_id.to_base16()?); let separator = match url.find('?') { Some(_) => "&", None => "?", @@ -477,7 +477,7 @@ impl SpClient { .get_user_attribute(attribute) .ok_or_else(|| SpClientError::Attribute(attribute.to_string()))?; - let url = template.replace("{file_id}", &file_id.to_base16()); + let url = template.replace("{file_id}", &file_id.to_base16()?); self.request_url(url).await } @@ -488,7 +488,7 @@ impl SpClient { .session() .get_user_attribute(attribute) .ok_or_else(|| SpClientError::Attribute(attribute.to_string()))?; - let url = template.replace("{file_id}", &image_id.to_base16()); + let url = template.replace("{file_id}", &image_id.to_base16()?); self.request_url(url).await } diff --git a/core/src/spotify_id.rs b/core/src/spotify_id.rs index b8a1448e..7591d427 100644 --- a/core/src/spotify_id.rs +++ b/core/src/spotify_id.rs @@ -191,7 +191,8 @@ impl SpotifyId { /// Returns the `SpotifyId` as a base16 (hex) encoded, `SpotifyId::SIZE_BASE16` (32) /// character long `String`. - pub fn to_base16(&self) -> String { + #[allow(clippy::wrong_self_convention)] + pub fn to_base16(&self) -> Result { to_base16(&self.to_raw(), &mut [0u8; Self::SIZE_BASE16]) } @@ -199,7 +200,9 @@ impl SpotifyId { /// character long `String`. /// /// [canonically]: https://developer.spotify.com/documentation/web-api/#spotify-uris-and-ids - pub fn to_base62(&self) -> String { + + #[allow(clippy::wrong_self_convention)] + pub fn to_base62(&self) -> Result { let mut dst = [0u8; 22]; let mut i = 0; let n = self.id; @@ -237,14 +240,12 @@ impl SpotifyId { dst.reverse(); - unsafe { - // Safety: We are only dealing with ASCII characters. - String::from_utf8_unchecked(dst.to_vec()) - } + String::from_utf8(dst.to_vec()).map_err(|_| SpotifyIdError::InvalidId.into()) } /// Returns a copy of the `SpotifyId` as an array of `SpotifyId::SIZE` (16) bytes in /// big-endian order. + #[allow(clippy::wrong_self_convention)] pub fn to_raw(&self) -> [u8; Self::SIZE] { self.id.to_be_bytes() } @@ -257,7 +258,9 @@ impl SpotifyId { /// be encoded as `unknown`. /// /// [Spotify URI]: https://developer.spotify.com/documentation/web-api/#spotify-uris-and-ids - pub fn to_uri(&self) -> String { + + #[allow(clippy::wrong_self_convention)] + pub fn to_uri(&self) -> Result { // 8 chars for the "spotify:" prefix + 1 colon + 22 chars base62 encoded ID = 31 // + unknown size item_type. let item_type: &str = self.item_type.into(); @@ -265,21 +268,24 @@ impl SpotifyId { dst.push_str("spotify:"); dst.push_str(item_type); dst.push(':'); - dst.push_str(&self.to_base62()); + let base_62 = self.to_base62()?; + dst.push_str(&base_62); - dst + Ok(dst) } } impl fmt::Debug for SpotifyId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.debug_tuple("SpotifyId").field(&self.to_uri()).finish() + f.debug_tuple("SpotifyId") + .field(&self.to_uri().unwrap_or_else(|_| "invalid uri".into())) + .finish() } } impl fmt::Display for SpotifyId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.write_str(&self.to_uri()) + f.write_str(&self.to_uri().unwrap_or_else(|_| "invalid uri".into())) } } @@ -312,16 +318,17 @@ impl NamedSpotifyId { }) } - pub fn to_uri(&self) -> String { + pub fn to_uri(&self) -> Result { let item_type: &str = self.inner_id.item_type.into(); let mut dst = String::with_capacity(37 + self.username.len() + item_type.len()); dst.push_str("spotify:user:"); dst.push_str(&self.username); dst.push_str(item_type); dst.push(':'); - dst.push_str(&self.to_base62()); + let base_62 = self.to_base62()?; + dst.push_str(&base_62); - dst + Ok(dst) } pub fn from_spotify_id(id: SpotifyId, username: String) -> Self { @@ -342,14 +349,24 @@ impl Deref for NamedSpotifyId { impl fmt::Debug for NamedSpotifyId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.debug_tuple("NamedSpotifyId") - .field(&self.inner_id.to_uri()) + .field( + &self + .inner_id + .to_uri() + .unwrap_or_else(|_| "invalid id".into()), + ) .finish() } } impl fmt::Display for NamedSpotifyId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.write_str(&self.inner_id.to_uri()) + f.write_str( + &self + .inner_id + .to_uri() + .unwrap_or_else(|_| "invalid id".into()), + ) } } @@ -495,7 +512,7 @@ impl TryFrom<&protocol::playlist_annotate3::TranscodedPicture> for SpotifyId { } } -pub fn to_base16(src: &[u8], buf: &mut [u8]) -> String { +pub fn to_base16(src: &[u8], buf: &mut [u8]) -> Result { let mut i = 0; for v in src { buf[i] = BASE16_DIGITS[(v >> 4) as usize]; @@ -503,10 +520,7 @@ pub fn to_base16(src: &[u8], buf: &mut [u8]) -> String { i += 2; } - unsafe { - // Safety: We are only dealing with ASCII characters. - String::from_utf8_unchecked(buf.to_vec()) - } + String::from_utf8(buf.to_vec()).map_err(|_| SpotifyIdError::InvalidId.into()) } #[cfg(test)] @@ -623,7 +637,7 @@ mod tests { item_type: c.kind, }; - assert_eq!(id.to_base62(), c.base62); + assert_eq!(id.to_base62().unwrap(), c.base62); } } @@ -646,7 +660,7 @@ mod tests { item_type: c.kind, }; - assert_eq!(id.to_base16(), c.base16); + assert_eq!(id.to_base16().unwrap(), c.base16); } } @@ -672,7 +686,7 @@ mod tests { item_type: c.kind, }; - assert_eq!(id.to_uri(), c.uri); + assert_eq!(id.to_uri().unwrap(), c.uri); } } diff --git a/examples/playlist_tracks.rs b/examples/playlist_tracks.rs index 2f53a8a3..fdadc61d 100644 --- a/examples/playlist_tracks.rs +++ b/examples/playlist_tracks.rs @@ -19,11 +19,13 @@ async fn main() { } let credentials = Credentials::with_password(&args[1], &args[2]); - let uri_split = args[3].split(':'); - let uri_parts: Vec<&str> = uri_split.collect(); - println!("{}, {}, {}", uri_parts[0], uri_parts[1], uri_parts[2]); - - let plist_uri = SpotifyId::from_base62(uri_parts[2]).unwrap(); + let plist_uri = SpotifyId::from_uri(&args[3]).unwrap_or_else(|_| { + eprintln!( + "PLAYLIST should be a playlist URI such as: \ + \"spotify:playlist:37i9dQZF1DXec50AjHrNTq\"" + ); + exit(1); + }); let session = Session::new(session_config, None); if let Err(e) = session.connect(credentials).await { diff --git a/metadata/src/episode.rs b/metadata/src/episode.rs index d04282ec..c5b65f80 100644 --- a/metadata/src/episode.rs +++ b/metadata/src/episode.rs @@ -74,7 +74,7 @@ impl InnerAudioItem for Episode { Ok(AudioItem { id, - spotify_uri: id.to_uri(), + spotify_uri: id.to_uri()?, files: episode.audio, name: episode.name, duration: episode.duration, diff --git a/metadata/src/playlist/annotation.rs b/metadata/src/playlist/annotation.rs index 587f9b39..fd8863cf 100644 --- a/metadata/src/playlist/annotation.rs +++ b/metadata/src/playlist/annotation.rs @@ -52,7 +52,7 @@ impl PlaylistAnnotation { let uri = format!( "hm://playlist-annotate/v1/annotation/user/{}/playlist/{}", username, - playlist_id.to_base62() + playlist_id.to_base62()? ); ::request(session, &uri).await } diff --git a/metadata/src/playlist/list.rs b/metadata/src/playlist/list.rs index 612ef857..0a359694 100644 --- a/metadata/src/playlist/list.rs +++ b/metadata/src/playlist/list.rs @@ -104,7 +104,7 @@ impl Playlist { let uri = format!( "hm://playlist/user/{}/playlist/{}", username, - playlist_id.to_base62() + playlist_id.to_base62()? ); ::request(session, &uri).await } @@ -152,7 +152,7 @@ impl Metadata for Playlist { type Message = protocol::playlist4_external::SelectedListContent; async fn request(session: &Session, playlist_id: SpotifyId) -> RequestResult { - let uri = format!("hm://playlist/v2/playlist/{}", playlist_id.to_base62()); + let uri = format!("hm://playlist/v2/playlist/{}", playlist_id.to_base62()?); ::request(session, &uri).await } diff --git a/metadata/src/track.rs b/metadata/src/track.rs index 4808b3f1..001590f5 100644 --- a/metadata/src/track.rs +++ b/metadata/src/track.rs @@ -88,7 +88,7 @@ impl InnerAudioItem for Track { Ok(AudioItem { id, - spotify_uri: id.to_uri(), + spotify_uri: id.to_uri()?, files: track.files, name: track.name, duration: track.duration, diff --git a/playback/src/config.rs b/playback/src/config.rs index 4070a26a..f1276adb 100644 --- a/playback/src/config.rs +++ b/playback/src/config.rs @@ -130,7 +130,7 @@ pub struct PlayerConfig { pub normalisation: bool, pub normalisation_type: NormalisationType, pub normalisation_method: NormalisationMethod, - pub normalisation_pregain_db: f32, + pub normalisation_pregain_db: f64, pub normalisation_threshold_dbfs: f64, pub normalisation_attack_cf: f64, pub normalisation_release_cf: f64, diff --git a/playback/src/convert.rs b/playback/src/convert.rs index 962ade66..1bc8a88e 100644 --- a/playback/src/convert.rs +++ b/playback/src/convert.rs @@ -23,14 +23,15 @@ pub struct Converter { impl Converter { pub fn new(dither_config: Option) -> Self { - if let Some(ref ditherer_builder) = dither_config { - let ditherer = (ditherer_builder)(); - info!("Converting with ditherer: {}", ditherer.name()); - Self { - ditherer: Some(ditherer), + match dither_config { + Some(ditherer_builder) => { + let ditherer = (ditherer_builder)(); + info!("Converting with ditherer: {}", ditherer.name()); + Self { + ditherer: Some(ditherer), + } } - } else { - Self { ditherer: None } + None => Self { ditherer: None }, } } @@ -52,18 +53,15 @@ impl Converter { const SCALE_S16: f64 = 32768.; pub fn scale(&mut self, sample: f64, factor: f64) -> f64 { - let dither = match self.ditherer { - Some(ref mut d) => d.noise(), - None => 0.0, - }; - // From the many float to int conversion methods available, match what // the reference Vorbis implementation uses: sample * 32768 (for 16 bit) - let int_value = sample * factor + dither; // Casting float to integer rounds towards zero by default, i.e. it // truncates, and that generates larger error than rounding to nearest. - int_value.round() + match self.ditherer.as_mut() { + Some(d) => (sample * factor + d.noise()).round(), + None => (sample * factor).round(), + } } // Special case for samples packed in a word of greater bit depth (e.g. @@ -79,11 +77,12 @@ impl Converter { let max = factor - 1.0; if int_value < min { - return min; + min } else if int_value > max { - return max; + max + } else { + int_value } - int_value } pub fn f64_to_f32(&mut self, samples: &[f64]) -> Vec { @@ -109,12 +108,7 @@ impl Converter { pub fn f64_to_s24_3(&mut self, samples: &[f64]) -> Vec { samples .iter() - .map(|sample| { - // Not as DRY as calling f32_to_s24 first, but this saves iterating - // over all samples twice. - let int_value = self.clamping_scale(*sample, Self::SCALE_S24) as i32; - i24::from_s24(int_value) - }) + .map(|sample| i24::from_s24(self.clamping_scale(*sample, Self::SCALE_S24) as i32)) .collect() } diff --git a/playback/src/dither.rs b/playback/src/dither.rs index 0f667917..4b8a427c 100644 --- a/playback/src/dither.rs +++ b/playback/src/dither.rs @@ -3,7 +3,7 @@ use rand::SeedableRng; use rand_distr::{Distribution, Normal, Triangular, Uniform}; use std::fmt; -const NUM_CHANNELS: usize = 2; +use crate::NUM_CHANNELS; // Dithering lowers digital-to-analog conversion ("requantization") error, // linearizing output, lowering distortion and replacing it with a constant, @@ -102,7 +102,7 @@ impl GaussianDitherer { pub struct HighPassDitherer { active_channel: usize, - previous_noises: [f64; NUM_CHANNELS], + previous_noises: [f64; NUM_CHANNELS as usize], cached_rng: SmallRng, distribution: Uniform, } @@ -111,7 +111,7 @@ impl Ditherer for HighPassDitherer { fn new() -> Self { Self { active_channel: 0, - previous_noises: [0.0; NUM_CHANNELS], + previous_noises: [0.0; NUM_CHANNELS as usize], cached_rng: create_rng(), distribution: Uniform::new_inclusive(-0.5, 0.5), // 1 LSB +/- 1 LSB (previous) = 2 LSB } diff --git a/playback/src/mixer/alsamixer.rs b/playback/src/mixer/alsamixer.rs index 55398cb7..c04e6ee8 100644 --- a/playback/src/mixer/alsamixer.rs +++ b/playback/src/mixer/alsamixer.rs @@ -84,7 +84,7 @@ impl Mixer for AlsaMixer { warn!("Alsa rounding error detected, setting maximum dB to {:.2} instead of {:.2}", ZERO_DB.to_db(), max_millibel.to_db()); max_millibel = ZERO_DB; } else { - warn!("Please manually set with `--volume-ctrl` if this is incorrect"); + warn!("Please manually set `--volume-range` if this is incorrect"); } } (min_millibel, max_millibel) @@ -104,12 +104,23 @@ impl Mixer for AlsaMixer { let min_db = min_millibel.to_db() as f64; let max_db = max_millibel.to_db() as f64; - let db_range = f64::abs(max_db - min_db); + let mut db_range = f64::abs(max_db - min_db); // Synchronize the volume control dB range with the mixer control, // unless it was already set with a command line option. if !config.volume_ctrl.range_ok() { + if db_range > 100.0 { + debug!("Alsa mixer reported dB range > 100, which is suspect"); + warn!("Please manually set `--volume-range` if this is incorrect"); + } config.volume_ctrl.set_db_range(db_range); + } else { + let db_range_override = config.volume_ctrl.db_range(); + debug!( + "Alsa dB volume range was detected as {} but overridden as {}", + db_range, db_range_override + ); + db_range = db_range_override; } // For hardware controls with a small range (24 dB or less), diff --git a/playback/src/player.rs b/playback/src/player.rs index cdbeac16..e4002878 100644 --- a/playback/src/player.rs +++ b/playback/src/player.rs @@ -47,6 +47,7 @@ use crate::SAMPLES_PER_SECOND; const PRELOAD_NEXT_TRACK_BEFORE_END_DURATION_MS: u32 = 30000; pub const DB_VOLTAGE_RATIO: f64 = 20.0; +pub const PCM_AT_0DBFS: f64 = 1.0; // Spotify inserts a custom Ogg packet at the start with custom metadata values, that you would // otherwise expect in Vorbis comments. This packet isn't well-formed and players may balk at it. @@ -264,7 +265,6 @@ impl Default for NormalisationData { impl NormalisationData { fn parse_from_ogg(mut file: T) -> io::Result { const SPOTIFY_NORMALIZATION_HEADER_START_OFFSET: u64 = 144; - let newpos = file.seek(SeekFrom::Start(SPOTIFY_NORMALIZATION_HEADER_START_OFFSET))?; if newpos != SPOTIFY_NORMALIZATION_HEADER_START_OFFSET { error!( @@ -296,31 +296,62 @@ impl NormalisationData { } let (gain_db, gain_peak) = if config.normalisation_type == NormalisationType::Album { - (data.album_gain_db as f64, data.album_peak as f64) + (data.album_gain_db, data.album_peak) } else { - (data.track_gain_db as f64, data.track_peak as f64) + (data.track_gain_db, data.track_peak) }; - let normalisation_power = gain_db + config.normalisation_pregain_db as f64; - let mut normalisation_factor = db_to_ratio(normalisation_power); + // As per the ReplayGain 1.0 & 2.0 (proposed) spec: + // https://wiki.hydrogenaud.io/index.php?title=ReplayGain_1.0_specification#Clipping_prevention + // https://wiki.hydrogenaud.io/index.php?title=ReplayGain_2.0_specification#Clipping_prevention + let normalisation_factor = if config.normalisation_method == NormalisationMethod::Basic { + // For Basic Normalisation, factor = min(ratio of (ReplayGain + PreGain), 1.0 / peak level). + // https://wiki.hydrogenaud.io/index.php?title=ReplayGain_1.0_specification#Peak_amplitude + // https://wiki.hydrogenaud.io/index.php?title=ReplayGain_2.0_specification#Peak_amplitude + // We then limit that to 1.0 as not to exceed dBFS (0.0 dB). + let factor = f64::min( + db_to_ratio(gain_db + config.normalisation_pregain_db), + PCM_AT_0DBFS / gain_peak, + ); - if normalisation_power + ratio_to_db(gain_peak) > config.normalisation_threshold_dbfs { - let limited_normalisation_factor = - db_to_ratio(config.normalisation_threshold_dbfs as f64) / gain_peak; - let limited_normalisation_power = ratio_to_db(limited_normalisation_factor); + if factor > PCM_AT_0DBFS { + info!( + "Lowering gain by {:.2} dB for the duration of this track to avoid potentially exceeding dBFS.", + ratio_to_db(factor) + ); - if config.normalisation_method == NormalisationMethod::Basic { - warn!("Limiting gain to {:.2} dB for the duration of this track to stay under normalisation threshold.", limited_normalisation_power); - normalisation_factor = limited_normalisation_factor; + PCM_AT_0DBFS } else { + factor + } + } else { + // For Dynamic Normalisation it's up to the player to decide, + // factor = ratio of (ReplayGain + PreGain). + // We then let the dynamic limiter handle gain reduction. + let factor = db_to_ratio(gain_db + config.normalisation_pregain_db); + let threshold_ratio = db_to_ratio(config.normalisation_threshold_dbfs); + + if factor > PCM_AT_0DBFS { + let factor_db = gain_db + config.normalisation_pregain_db; + let limiting_db = factor_db + config.normalisation_threshold_dbfs.abs(); + warn!( - "This track will at its peak be subject to {:.2} dB of dynamic limiting.", - normalisation_power - limited_normalisation_power + "This track may exceed dBFS by {:.2} dB and be subject to {:.2} dB of dynamic limiting at it's peak.", + factor_db, limiting_db + ); + } else if factor > threshold_ratio { + let limiting_db = gain_db + + config.normalisation_pregain_db + + config.normalisation_threshold_dbfs.abs(); + + info!( + "This track may be subject to {:.2} dB of dynamic limiting at it's peak.", + limiting_db ); } - warn!("Please lower pregain to avoid."); - } + factor + }; debug!("Normalisation Data: {:?}", data); debug!( @@ -792,7 +823,16 @@ impl PlayerTrackLoader { position_ms: u32, ) -> Option { let audio = match AudioItem::get_file(&self.session, spotify_id).await { - Ok(audio) => audio, + Ok(audio) => match self.find_available_alternative(audio).await { + Some(audio) => audio, + None => { + warn!( + "<{}> is not available", + spotify_id.to_uri().unwrap_or_default() + ); + return None; + } + }, Err(e) => { error!("Unable to load audio item: {:?}", e); return None; @@ -805,6 +845,7 @@ impl PlayerTrackLoader { ); let is_explicit = audio.is_explicit; + if is_explicit { if let Some(value) = self.session.get_user_attribute("filter-explicit-content") { if &value == "1" { @@ -814,22 +855,15 @@ impl PlayerTrackLoader { } } - let audio = match self.find_available_alternative(audio).await { - Some(audio) => audio, - None => { - error!("<{}> is not available", spotify_id.to_uri()); - return None; - } - }; - if audio.duration < 0 { error!( "Track duration for <{}> cannot be {}", - spotify_id.to_uri(), + spotify_id.to_uri().unwrap_or_default(), audio.duration ); return None; } + let duration_ms = audio.duration as u32; // (Most) podcasts seem to support only 96 kbps Ogg Vorbis, so fall back to it @@ -863,25 +897,23 @@ impl PlayerTrackLoader { ], }; - let entry = formats.iter().find_map(|format| { - if let Some(&file_id) = audio.files.get(format) { - Some((*format, file_id)) - } else { - None - } - }); - - let (format, file_id) = match entry { - Some(t) => t, - None => { - error!("<{}> is not available in any supported format", audio.name); - return None; - } - }; + let (format, file_id) = + match formats + .iter() + .find_map(|format| match audio.files.get(format) { + Some(&file_id) => Some((*format, file_id)), + _ => None, + }) { + Some(t) => t, + None => { + warn!("<{}> is not available in any supported format", audio.name); + return None; + } + }; let bytes_per_second = self.stream_data_rate(format); - // This is only a loop to be able to reload the file if an error occured + // This is only a loop to be able to reload the file if an error occurred // while opening a cached file. loop { let encrypted_file = AudioFile::open(&self.session, file_id, bytes_per_second); @@ -1416,73 +1448,98 @@ impl PlayerInternal { // For the basic normalisation method, a normalisation factor of 1.0 indicates that // there is nothing to normalise (all samples should pass unaltered). For the // dynamic method, there may still be peaks that we want to shave off. - if self.config.normalisation - && !(f64::abs(normalisation_factor - 1.0) <= f64::EPSILON - && self.config.normalisation_method == NormalisationMethod::Basic) - { - // zero-cost shorthands - let threshold_db = self.config.normalisation_threshold_dbfs; - let knee_db = self.config.normalisation_knee_db; - let attack_cf = self.config.normalisation_attack_cf; - let release_cf = self.config.normalisation_release_cf; + if self.config.normalisation { + if self.config.normalisation_method == NormalisationMethod::Basic + && normalisation_factor < 1.0 + { + for sample in data.iter_mut() { + *sample *= normalisation_factor; + } + } else if self.config.normalisation_method + == NormalisationMethod::Dynamic + { + // zero-cost shorthands + let threshold_db = self.config.normalisation_threshold_dbfs; + let knee_db = self.config.normalisation_knee_db; + let attack_cf = self.config.normalisation_attack_cf; + let release_cf = self.config.normalisation_release_cf; - for sample in data.iter_mut() { - *sample *= normalisation_factor; // for both the basic and dynamic limiter + for sample in data.iter_mut() { + *sample *= normalisation_factor; - // Feedforward limiter in the log domain - // After: Giannoulis, D., Massberg, M., & Reiss, J.D. (2012). Digital Dynamic - // Range Compressor Design—A Tutorial and Analysis. Journal of The Audio - // Engineering Society, 60, 399-408. - if self.config.normalisation_method == NormalisationMethod::Dynamic - { - // steps 1 + 2: half-wave rectification and conversion into dB - let abs_sample_db = ratio_to_db(sample.abs()); + // Feedforward limiter in the log domain + // After: Giannoulis, D., Massberg, M., & Reiss, J.D. (2012). Digital Dynamic + // Range Compressor Design—A Tutorial and Analysis. Journal of The Audio + // Engineering Society, 60, 399-408. - // Some tracks have samples that are precisely 0.0, but ratio_to_db(0.0) - // returns -inf and gets the peak detector stuck. - if !abs_sample_db.is_normal() { - continue; - } + // Some tracks have samples that are precisely 0.0. That's silence + // and we know we don't need to limit that, in which we can spare + // the CPU cycles. + // + // Also, calling `ratio_to_db(0.0)` returns `inf` and would get the + // peak detector stuck. Also catch the unlikely case where a sample + // is decoded as `NaN` or some other non-normal value. + let limiter_db = if sample.is_normal() { + // step 1-4: half-wave rectification and conversion into dB + // and gain computer with soft knee and subtractor + let bias_db = ratio_to_db(sample.abs()) - threshold_db; + let knee_boundary_db = bias_db * 2.0; - // step 3: gain computer with soft knee - let biased_sample = abs_sample_db - threshold_db; - let limited_sample = if 2.0 * biased_sample < -knee_db { - abs_sample_db - } else if 2.0 * biased_sample.abs() <= knee_db { - abs_sample_db - - (biased_sample + knee_db / 2.0).powi(2) - / (2.0 * knee_db) + if knee_boundary_db < -knee_db { + 0.0 + } else if knee_boundary_db.abs() <= knee_db { + // The textbook equation: + // ratio_to_db(sample.abs()) - (ratio_to_db(sample.abs()) - (bias_db + knee_db / 2.0).powi(2) / (2.0 * knee_db)) + // Simplifies to: + // ((2.0 * bias_db) + knee_db).powi(2) / (8.0 * knee_db) + // Which in our case further simplifies to: + // (knee_boundary_db + knee_db).powi(2) / (8.0 * knee_db) + // because knee_boundary_db is 2.0 * bias_db. + (knee_boundary_db + knee_db).powi(2) / (8.0 * knee_db) + } else { + // Textbook: + // ratio_to_db(sample.abs()) - threshold_db, which is already our bias_db. + bias_db + } } else { - threshold_db as f64 + 0.0 }; - // step 4: subtractor - let limiter_input = abs_sample_db - limited_sample; - - // Spare the CPU unless the limiter is active or we are riding a peak. - if !(limiter_input > 0.0 + // Spare the CPU unless (1) the limiter is engaged, (2) we + // were in attack or (3) we were in release, and that attack/ + // release wasn't finished yet. + if limiter_db > 0.0 || self.normalisation_integrator > 0.0 - || self.normalisation_peak > 0.0) + || self.normalisation_peak > 0.0 { - continue; + // step 5: smooth, decoupled peak detector + // Textbook: + // release_cf * self.normalisation_integrator + (1.0 - release_cf) * limiter_db + // Simplifies to: + // release_cf * self.normalisation_integrator - release_cf * limiter_db + limiter_db + self.normalisation_integrator = f64::max( + limiter_db, + release_cf * self.normalisation_integrator + - release_cf * limiter_db + + limiter_db, + ); + // Textbook: + // attack_cf * self.normalisation_peak + (1.0 - attack_cf) * self.normalisation_integrator + // Simplifies to: + // attack_cf * self.normalisation_peak - attack_cf * self.normalisation_integrator + self.normalisation_integrator + self.normalisation_peak = attack_cf + * self.normalisation_peak + - attack_cf * self.normalisation_integrator + + self.normalisation_integrator; + + // step 6: make-up gain applied later (volume attenuation) + // Applying the standard normalisation factor here won't work, + // because there are tracks with peaks as high as 6 dB above + // the default threshold, so that would clip. + + // steps 7-8: conversion into level and multiplication into gain stage + *sample *= db_to_ratio(-self.normalisation_peak); } - - // step 5: smooth, decoupled peak detector - self.normalisation_integrator = f64::max( - limiter_input, - release_cf * self.normalisation_integrator - + (1.0 - release_cf) * limiter_input, - ); - self.normalisation_peak = attack_cf * self.normalisation_peak - + (1.0 - attack_cf) * self.normalisation_integrator; - - // step 6: make-up gain applied later (volume attenuation) - // Applying the standard normalisation factor here won't work, - // because there are tracks with peaks as high as 6 dB above - // the default threshold, so that would clip. - - // steps 7-8: conversion into level and multiplication into gain stage - *sample *= db_to_ratio(-self.normalisation_peak); } } } @@ -1981,15 +2038,8 @@ impl PlayerInternal { } fn send_event(&mut self, event: PlayerEvent) { - let mut index = 0; - while index < self.event_senders.len() { - match self.event_senders[index].send(event.clone()) { - Ok(_) => index += 1, - Err(_) => { - self.event_senders.remove(index); - } - } - } + self.event_senders + .retain(|sender| sender.send(event.clone()).is_ok()); } fn load_track( diff --git a/src/main.rs b/src/main.rs index 6f837c02..f81bd1c0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -192,7 +192,7 @@ fn get_setup() -> Setup { const VALID_INITIAL_VOLUME_RANGE: RangeInclusive = 0..=100; const VALID_VOLUME_RANGE: RangeInclusive = 0.0..=100.0; const VALID_NORMALISATION_KNEE_RANGE: RangeInclusive = 0.0..=10.0; - const VALID_NORMALISATION_PREGAIN_RANGE: RangeInclusive = -10.0..=10.0; + const VALID_NORMALISATION_PREGAIN_RANGE: RangeInclusive = -10.0..=10.0; const VALID_NORMALISATION_THRESHOLD_RANGE: RangeInclusive = -10.0..=0.0; const VALID_NORMALISATION_ATTACK_RANGE: RangeInclusive = 1..=500; const VALID_NORMALISATION_RELEASE_RANGE: RangeInclusive = 1..=1000; @@ -671,6 +671,7 @@ fn get_setup() -> Setup { let opt = key.trim_start_matches('-'); if index > 0 + && key.starts_with('-') && &args[index - 1] != key && matches.opt_defined(opt) && matches.opt_present(opt) @@ -1306,12 +1307,7 @@ fn get_setup() -> Setup { normalisation_method = opt_str(NORMALISATION_METHOD) .as_deref() .map(|method| { - warn!( - "`--{}` / `-{}` will be deprecated in a future release.", - NORMALISATION_METHOD, NORMALISATION_METHOD_SHORT - ); - - let method = NormalisationMethod::from_str(method).unwrap_or_else(|_| { + NormalisationMethod::from_str(method).unwrap_or_else(|_| { invalid_error_msg( NORMALISATION_METHOD, NORMALISATION_METHOD_SHORT, @@ -1321,16 +1317,7 @@ fn get_setup() -> Setup { ); exit(1); - }); - - if matches!(method, NormalisationMethod::Basic) { - warn!( - "`--{}` / `-{}` {:?} will be deprecated in a future release.", - NORMALISATION_METHOD, NORMALISATION_METHOD_SHORT, method - ); - } - - method + }) }) .unwrap_or(player_default_config.normalisation_method); @@ -1352,7 +1339,7 @@ fn get_setup() -> Setup { .unwrap_or(player_default_config.normalisation_type); normalisation_pregain_db = opt_str(NORMALISATION_PREGAIN) - .map(|pregain| match pregain.parse::() { + .map(|pregain| match pregain.parse::() { Ok(value) if (VALID_NORMALISATION_PREGAIN_RANGE).contains(&value) => value, _ => { let valid_values = &format!( diff --git a/src/player_event_handler.rs b/src/player_event_handler.rs index d5e4517b..ef6d195c 100644 --- a/src/player_event_handler.rs +++ b/src/player_event_handler.rs @@ -1,59 +1,116 @@ +use log::info; + use std::{ collections::HashMap, - io, + io::{Error, ErrorKind, Result}, process::{Command, ExitStatus}, }; -use log::info; use tokio::process::{Child as AsyncChild, Command as AsyncCommand}; use librespot::playback::player::{PlayerEvent, SinkStatus}; -pub fn run_program_on_events(event: PlayerEvent, onevent: &str) -> Option> { +pub fn run_program_on_events(event: PlayerEvent, onevent: &str) -> Option> { let mut env_vars = HashMap::new(); match event { PlayerEvent::Changed { old_track_id, new_track_id, - } => { - env_vars.insert("PLAYER_EVENT", "changed".to_string()); - env_vars.insert("OLD_TRACK_ID", old_track_id.to_base62()); - env_vars.insert("TRACK_ID", new_track_id.to_base62()); - } - PlayerEvent::Started { track_id, .. } => { - env_vars.insert("PLAYER_EVENT", "started".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - } - PlayerEvent::Stopped { track_id, .. } => { - env_vars.insert("PLAYER_EVENT", "stopped".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - } + } => match old_track_id.to_base62() { + Err(_) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + "PlayerEvent::Changed: Invalid old track id", + ))) + } + Ok(old_id) => match new_track_id.to_base62() { + Err(_) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + "PlayerEvent::Changed: Invalid new track id", + ))) + } + Ok(new_id) => { + env_vars.insert("PLAYER_EVENT", "changed".to_string()); + env_vars.insert("OLD_TRACK_ID", old_id); + env_vars.insert("TRACK_ID", new_id); + } + }, + }, + PlayerEvent::Started { track_id, .. } => match track_id.to_base62() { + Err(_) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + "PlayerEvent::Started: Invalid track id", + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "started".to_string()); + env_vars.insert("TRACK_ID", id); + } + }, + PlayerEvent::Stopped { track_id, .. } => match track_id.to_base62() { + Err(_) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + "PlayerEvent::Stopped: Invalid track id", + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "stopped".to_string()); + env_vars.insert("TRACK_ID", id); + } + }, PlayerEvent::Playing { track_id, duration_ms, position_ms, .. - } => { - env_vars.insert("PLAYER_EVENT", "playing".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - env_vars.insert("DURATION_MS", duration_ms.to_string()); - env_vars.insert("POSITION_MS", position_ms.to_string()); - } + } => match track_id.to_base62() { + Err(_) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + "PlayerEvent::Playing: Invalid track id", + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "playing".to_string()); + env_vars.insert("TRACK_ID", id); + env_vars.insert("DURATION_MS", duration_ms.to_string()); + env_vars.insert("POSITION_MS", position_ms.to_string()); + } + }, PlayerEvent::Paused { track_id, duration_ms, position_ms, .. - } => { - env_vars.insert("PLAYER_EVENT", "paused".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - env_vars.insert("DURATION_MS", duration_ms.to_string()); - env_vars.insert("POSITION_MS", position_ms.to_string()); - } - PlayerEvent::Preloading { track_id, .. } => { - env_vars.insert("PLAYER_EVENT", "preloading".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - } + } => match track_id.to_base62() { + Err(_) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + "PlayerEvent::Paused: Invalid track id", + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "paused".to_string()); + env_vars.insert("TRACK_ID", id); + env_vars.insert("DURATION_MS", duration_ms.to_string()); + env_vars.insert("POSITION_MS", position_ms.to_string()); + } + }, + PlayerEvent::Preloading { track_id, .. } => match track_id.to_base62() { + Err(_) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + "PlayerEvent::Preloading: Invalid track id", + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "preloading".to_string()); + env_vars.insert("TRACK_ID", id); + } + }, PlayerEvent::VolumeSet { volume } => { env_vars.insert("PLAYER_EVENT", "volume_set".to_string()); env_vars.insert("VOLUME", volume.to_string()); @@ -71,7 +128,7 @@ pub fn run_program_on_events(event: PlayerEvent, onevent: &str) -> Option io::Result { +pub fn emit_sink_event(sink_status: SinkStatus, onevent: &str) -> Result { let mut env_vars = HashMap::new(); env_vars.insert("PLAYER_EVENT", "sink".to_string()); let sink_status = match sink_status {