Various code improvements (#777)

* Remove deprecated use of std::u16::MAX
* Use `FromStr` for fallible `&str` conversions
* DRY up strings into constants
* Change `as_ref().map()` into `as_deref()`
* Use `Duration` for time constants and functions
* Optimize `Vec` with response times
* Move comments for `rustdoc` to parse

Author: roderickvd

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 - [audio, playback] Moved `VorbisDecoder`, `VorbisError`, `AudioPacket`, `PassthroughDecoder`, `PassthroughError`, `AudioError`, `AudioDecoder` and the `convert` module from `librespot-audio` to `librespot-playback`. The underlying crates `vorbis`, `librespot-tremor`, `lewton` and `ogg` should be used directly. (breaking)
+- [audio, playback] Use `Duration` for time constants and functions (breaking)
 - [connect, playback] Moved volume controls from `librespot-connect` to `librespot-playback` crate
 - [connect] Synchronize player volume with mixer volume on playback
 - [playback] Store and pass samples in 64-bit floating point

audio/src/fetch/mod.rs

@@ -18,70 +18,70 @@ use tokio::sync::{mpsc, oneshot};
 use self::receive::{audio_file_fetch, request_range};
 use crate::range_set::{Range, RangeSet};
 
+/// The minimum size of a block that is requested from the Spotify servers in one request.
+/// This is the block size that is typically requested while doing a `seek()` on a file.
+/// Note: smaller requests can happen if part of the block is downloaded already.
 const MINIMUM_DOWNLOAD_SIZE: usize = 1024 * 16;
-// The minimum size of a block that is requested from the Spotify servers in one request.
-// This is the block size that is typically requested while doing a seek() on a file.
-// Note: smaller requests can happen if part of the block is downloaded already.
 
+/// The amount of data that is requested when initially opening a file.
+/// Note: if the file is opened to play from the beginning, the amount of data to
+/// read ahead is requested in addition to this amount. If the file is opened to seek to
+/// another position, then only this amount is requested on the first request.
 const INITIAL_DOWNLOAD_SIZE: usize = 1024 * 16;
-// The amount of data that is requested when initially opening a file.
-// Note: if the file is opened to play from the beginning, the amount of data to
-// read ahead is requested in addition to this amount. If the file is opened to seek to
-// another position, then only this amount is requested on the first request.
-
-const INITIAL_PING_TIME_ESTIMATE_SECONDS: f64 = 0.5;
-// The pig time that is used for calculations before a ping time was actually measured.
-
-const MAXIMUM_ASSUMED_PING_TIME_SECONDS: f64 = 1.5;
-// If the measured ping time to the Spotify server is larger than this value, it is capped
-// to avoid run-away block sizes and pre-fetching.
-
-pub const READ_AHEAD_BEFORE_PLAYBACK_SECONDS: f64 = 1.0;
-// Before playback starts, this many seconds of data must be present.
-// Note: the calculations are done using the nominal bitrate of the file. The actual amount
-// of audio data may be larger or smaller.
-
-pub const READ_AHEAD_BEFORE_PLAYBACK_ROUNDTRIPS: f64 = 2.0;
-// Same as READ_AHEAD_BEFORE_PLAYBACK_SECONDS, but the time is taken as a factor of the ping
-// time to the Spotify server.
-// Both, READ_AHEAD_BEFORE_PLAYBACK_SECONDS and READ_AHEAD_BEFORE_PLAYBACK_ROUNDTRIPS are
-// obeyed.
-// Note: the calculations are done using the nominal bitrate of the file. The actual amount
-// of audio data may be larger or smaller.
-
-pub const READ_AHEAD_DURING_PLAYBACK_SECONDS: f64 = 5.0;
-// While playing back, this many seconds of data ahead of the current read position are
-// requested.
-// Note: the calculations are done using the nominal bitrate of the file. The actual amount
-// of audio data may be larger or smaller.
-
-pub const READ_AHEAD_DURING_PLAYBACK_ROUNDTRIPS: f64 = 10.0;
-// Same as READ_AHEAD_DURING_PLAYBACK_SECONDS, but the time is taken as a factor of the ping
-// time to the Spotify server.
-// Note: the calculations are done using the nominal bitrate of the file. The actual amount
-// of audio data may be larger or smaller.
-
-const PREFETCH_THRESHOLD_FACTOR: f64 = 4.0;
-// If the amount of data that is pending (requested but not received) is less than a certain amount,
-// data is pre-fetched in addition to the read ahead settings above. The threshold for requesting more
-// data is calculated as
-// <pending bytes> < PREFETCH_THRESHOLD_FACTOR * <ping time> * <nominal data rate>
-
-const FAST_PREFETCH_THRESHOLD_FACTOR: f64 = 1.5;
-// Similar to PREFETCH_THRESHOLD_FACTOR, but it also takes the current download rate into account.
-// The formula used is
-// <pending bytes> < FAST_PREFETCH_THRESHOLD_FACTOR * <ping time> * <measured download rate>
-// This mechanism allows for fast downloading of the remainder of the file. The number should be larger
-// than 1 so the download rate ramps up until the bandwidth is saturated. The larger the value, the faster
-// the download rate ramps up. However, this comes at the cost that it might hurt ping-time if a seek is
-// performed while downloading. Values smaller than 1 cause the download rate to collapse and effectively
-// only PREFETCH_THRESHOLD_FACTOR is in effect. Thus, set to zero if bandwidth saturation is not wanted.
 
+/// The ping time that is used for calculations before a ping time was actually measured.
+const INITIAL_PING_TIME_ESTIMATE: Duration = Duration::from_millis(500);
+
+/// If the measured ping time to the Spotify server is larger than this value, it is capped
+/// to avoid run-away block sizes and pre-fetching.
+const MAXIMUM_ASSUMED_PING_TIME: Duration = Duration::from_millis(1500);
+
+/// Before playback starts, this many seconds of data must be present.
+/// Note: the calculations are done using the nominal bitrate of the file. The actual amount
+/// of audio data may be larger or smaller.
+pub const READ_AHEAD_BEFORE_PLAYBACK: Duration = Duration::from_secs(1);
+
+/// Same as `READ_AHEAD_BEFORE_PLAYBACK`, but the time is taken as a factor of the ping
+/// time to the Spotify server. Both `READ_AHEAD_BEFORE_PLAYBACK` and
+/// `READ_AHEAD_BEFORE_PLAYBACK_ROUNDTRIPS` are obeyed.
+/// Note: the calculations are done using the nominal bitrate of the file. The actual amount
+/// of audio data may be larger or smaller.
+pub const READ_AHEAD_BEFORE_PLAYBACK_ROUNDTRIPS: f32 = 2.0;
+
+/// While playing back, this many seconds of data ahead of the current read position are
+/// requested.
+/// Note: the calculations are done using the nominal bitrate of the file. The actual amount
+/// of audio data may be larger or smaller.
+pub const READ_AHEAD_DURING_PLAYBACK: Duration = Duration::from_secs(5);
+
+/// Same as `READ_AHEAD_DURING_PLAYBACK`, but the time is taken as a factor of the ping
+/// time to the Spotify server.
+/// Note: the calculations are done using the nominal bitrate of the file. The actual amount
+/// of audio data may be larger or smaller.
+pub const READ_AHEAD_DURING_PLAYBACK_ROUNDTRIPS: f32 = 10.0;
+
+/// If the amount of data that is pending (requested but not received) is less than a certain amount,
+/// data is pre-fetched in addition to the read ahead settings above. The threshold for requesting more
+/// data is calculated as `<pending bytes> < PREFETCH_THRESHOLD_FACTOR * <ping time> * <nominal data rate>`
+const PREFETCH_THRESHOLD_FACTOR: f32 = 4.0;
+
+/// Similar to `PREFETCH_THRESHOLD_FACTOR`, but it also takes the current download rate into account.
+/// The formula used is `<pending bytes> < FAST_PREFETCH_THRESHOLD_FACTOR * <ping time> * <measured download rate>`
+/// This mechanism allows for fast downloading of the remainder of the file. The number should be larger
+/// than `1.0` so the download rate ramps up until the bandwidth is saturated. The larger the value, the faster
+/// the download rate ramps up. However, this comes at the cost that it might hurt ping time if a seek is
+/// performed while downloading. Values smaller than `1.0` cause the download rate to collapse and effectively
+/// only `PREFETCH_THRESHOLD_FACTOR` is in effect. Thus, set to `0.0` if bandwidth saturation is not wanted.
+const FAST_PREFETCH_THRESHOLD_FACTOR: f32 = 1.5;
+
+/// Limit the number of requests that are pending simultaneously before pre-fetching data. Pending
+/// requests share bandwidth. Thus, havint too many requests can lead to the one that is needed next
+/// for playback to be delayed leading to a buffer underrun. This limit has the effect that a new
+/// pre-fetch request is only sent if less than `MAX_PREFETCH_REQUESTS` are pending.
 const MAX_PREFETCH_REQUESTS: usize = 4;
-// Limit the number of requests that are pending simultaneously before pre-fetching data. Pending
-// requests share bandwidth. Thus, havint too many requests can lead to the one that is needed next
-// for playback to be delayed leading to a buffer underrun. This limit has the effect that a new
-// pre-fetch request is only sent if less than MAX_PREFETCH_REQUESTS are pending.
+
+/// The time we will wait to obtain status updates on downloading.
+const DOWNLOAD_TIMEOUT: Duration = Duration::from_secs(1);
 
 pub enum AudioFile {
     Cached(fs::File),
@@ -131,10 +131,10 @@ impl StreamLoaderController {
         })
     }
 
-    pub fn ping_time_ms(&self) -> usize {
-        self.stream_shared.as_ref().map_or(0, |shared| {
-            shared.ping_time_ms.load(atomic::Ordering::Relaxed)
-        })
+    pub fn ping_time(&self) -> Duration {
+        Duration::from_millis(self.stream_shared.as_ref().map_or(0, |shared| {
+            shared.ping_time_ms.load(atomic::Ordering::Relaxed) as u64
+        }))
     }
 
     fn send_stream_loader_command(&self, command: StreamLoaderCommand) {
@@ -170,7 +170,7 @@ impl StreamLoaderController {
             {
                 download_status = shared
                     .cond
-                    .wait_timeout(download_status, Duration::from_millis(1000))
+                    .wait_timeout(download_status, DOWNLOAD_TIMEOUT)
                     .unwrap()
                     .0;
                 if range.length
@@ -271,10 +271,10 @@ impl AudioFile {
         let mut initial_data_length = if play_from_beginning {
             INITIAL_DOWNLOAD_SIZE
                 + max(
-                    (READ_AHEAD_DURING_PLAYBACK_SECONDS * bytes_per_second as f64) as usize,
-                    (INITIAL_PING_TIME_ESTIMATE_SECONDS
+                    (READ_AHEAD_DURING_PLAYBACK.as_secs_f32() * bytes_per_second as f32) as usize,
+                    (INITIAL_PING_TIME_ESTIMATE.as_secs_f32()
                         * READ_AHEAD_DURING_PLAYBACK_ROUNDTRIPS
-                        * bytes_per_second as f64) as usize,
+                        * bytes_per_second as f32) as usize,
                 )
         } else {
             INITIAL_DOWNLOAD_SIZE
@@ -368,7 +368,7 @@ impl AudioFileStreaming {
 
         let read_file = write_file.reopen().unwrap();
 
-        //let (seek_tx, seek_rx) = mpsc::unbounded();
+        // let (seek_tx, seek_rx) = mpsc::unbounded();
         let (stream_loader_command_tx, stream_loader_command_rx) =
             mpsc::unbounded_channel::<StreamLoaderCommand>();
 
@@ -405,17 +405,19 @@ impl Read for AudioFileStreaming {
         let length_to_request = match *(self.shared.download_strategy.lock().unwrap()) {
             DownloadStrategy::RandomAccess() => length,
             DownloadStrategy::Streaming() => {
-                // Due to the read-ahead stuff, we potentially request more than the actual reqeust demanded.
-                let ping_time_seconds =
-                    0.0001 * self.shared.ping_time_ms.load(atomic::Ordering::Relaxed) as f64;
+                // Due to the read-ahead stuff, we potentially request more than the actual request demanded.
+                let ping_time_seconds = Duration::from_millis(
+                    self.shared.ping_time_ms.load(atomic::Ordering::Relaxed) as u64,
+                )
+                .as_secs_f32();
 
                 let length_to_request = length
                     + max(
-                        (READ_AHEAD_DURING_PLAYBACK_SECONDS * self.shared.stream_data_rate as f64)
-                            as usize,
+                        (READ_AHEAD_DURING_PLAYBACK.as_secs_f32()
+                            * self.shared.stream_data_rate as f32) as usize,
                         (READ_AHEAD_DURING_PLAYBACK_ROUNDTRIPS
                             * ping_time_seconds
-                            * self.shared.stream_data_rate as f64) as usize,
+                            * self.shared.stream_data_rate as f32) as usize,
                     );
                 min(length_to_request, self.shared.file_size - offset)
             }
@@ -449,7 +451,7 @@ impl Read for AudioFileStreaming {
             download_status = self
                 .shared
                 .cond
-                .wait_timeout(download_status, Duration::from_millis(1000))
+                .wait_timeout(download_status, DOWNLOAD_TIMEOUT)
                 .unwrap()
                 .0;
         }

audio/src/fetch/receive.rs

@@ -1,8 +1,9 @@
 use std::cmp::{max, min};
 use std::io::{Seek, SeekFrom, Write};
 use std::sync::{atomic, Arc};
-use std::time::Instant;
+use std::time::{Duration, Instant};
 
+use atomic::Ordering;
 use byteorder::{BigEndian, WriteBytesExt};
 use bytes::Bytes;
 use futures_util::StreamExt;
@@ -16,7 +17,7 @@ use crate::range_set::{Range, RangeSet};
 
 use super::{AudioFileShared, DownloadStrategy, StreamLoaderCommand};
 use super::{
-    FAST_PREFETCH_THRESHOLD_FACTOR, MAXIMUM_ASSUMED_PING_TIME_SECONDS, MAX_PREFETCH_REQUESTS,
+    FAST_PREFETCH_THRESHOLD_FACTOR, MAXIMUM_ASSUMED_PING_TIME, MAX_PREFETCH_REQUESTS,
     MINIMUM_DOWNLOAD_SIZE, PREFETCH_THRESHOLD_FACTOR,
 };
 
@@ -57,7 +58,7 @@ struct PartialFileData {
 }
 
 enum ReceivedData {
-    ResponseTimeMs(usize),
+    ResponseTime(Duration),
     Data(PartialFileData),
 }
 
@@ -74,7 +75,7 @@ async fn receive_data(
 
     let old_number_of_request = shared
         .number_of_open_requests
-        .fetch_add(1, atomic::Ordering::SeqCst);
+        .fetch_add(1, Ordering::SeqCst);
 
     let mut measure_ping_time = old_number_of_request == 0;
 
@@ -86,14 +87,11 @@ async fn receive_data(
         };
 
         if measure_ping_time {
-            let duration = Instant::now() - request_sent_time;
-            let duration_ms: u64;
-            if 0.001 * (duration.as_millis() as f64) > MAXIMUM_ASSUMED_PING_TIME_SECONDS {
-                duration_ms = (MAXIMUM_ASSUMED_PING_TIME_SECONDS * 1000.0) as u64;
-            } else {
-                duration_ms = duration.as_millis() as u64;
+            let mut duration = Instant::now() - request_sent_time;
+            if duration > MAXIMUM_ASSUMED_PING_TIME {
+                duration = MAXIMUM_ASSUMED_PING_TIME;
             }
-            let _ = file_data_tx.send(ReceivedData::ResponseTimeMs(duration_ms as usize));
+            let _ = file_data_tx.send(ReceivedData::ResponseTime(duration));
             measure_ping_time = false;
         }
         let data_size = data.len();
@@ -127,7 +125,7 @@ async fn receive_data(
 
     shared
         .number_of_open_requests
-        .fetch_sub(1, atomic::Ordering::SeqCst);
+        .fetch_sub(1, Ordering::SeqCst);
 
     if result.is_err() {
         warn!(
@@ -149,7 +147,7 @@ struct AudioFileFetch {
 
     file_data_tx: mpsc::UnboundedSender<ReceivedData>,
     complete_tx: Option<oneshot::Sender<NamedTempFile>>,
-    network_response_times_ms: Vec<usize>,
+    network_response_times: Vec<Duration>,
 }
 
 // Might be replaced by enum from std once stable
@@ -237,7 +235,7 @@ impl AudioFileFetch {
 
             // download data from after the current read position first
             let mut tail_end = RangeSet::new();
-            let read_position = self.shared.read_position.load(atomic::Ordering::Relaxed);
+            let read_position = self.shared.read_position.load(Ordering::Relaxed);
             tail_end.add_range(&Range::new(
                 read_position,
                 self.shared.file_size - read_position,
@@ -267,26 +265,23 @@ impl AudioFileFetch {
 
     fn handle_file_data(&mut self, data: ReceivedData) -> ControlFlow {
         match data {
-            ReceivedData::ResponseTimeMs(response_time_ms) => {
-                trace!("Ping time estimated as: {} ms.", response_time_ms);
-
-                // record the response time
-                self.network_response_times_ms.push(response_time_ms);
+            ReceivedData::ResponseTime(response_time) => {
+                trace!("Ping time estimated as: {}ms", response_time.as_millis());
 
-                // prune old response times. Keep at most three.
-                while self.network_response_times_ms.len() > 3 {
-                    self.network_response_times_ms.remove(0);
+                // prune old response times. Keep at most two so we can push a third.
+                while self.network_response_times.len() >= 3 {
+                    self.network_response_times.remove(0);
                 }
 
+                // record the response time
+                self.network_response_times.push(response_time);
+
                 // stats::median is experimental. So we calculate the median of up to three ourselves.
-                let ping_time_ms: usize = match self.network_response_times_ms.len() {
-                    1 => self.network_response_times_ms[0] as usize,
-                    2 => {
-                        ((self.network_response_times_ms[0] + self.network_response_times_ms[1])
-                            / 2) as usize
-                    }
+                let ping_time = match self.network_response_times.len() {
+                    1 => self.network_response_times[0],
+                    2 => (self.network_response_times[0] + self.network_response_times[1]) / 2,
                     3 => {
-                        let mut times = self.network_response_times_ms.clone();
+                        let mut times = self.network_response_times.clone();
                         times.sort_unstable();
                         times[1]
                     }
@@ -296,7 +291,7 @@ impl AudioFileFetch {
                 // store our new estimate for everyone to see
                 self.shared
                     .ping_time_ms
-                    .store(ping_time_ms, atomic::Ordering::Relaxed);
+                    .store(ping_time.as_millis() as usize, Ordering::Relaxed);
             }
             ReceivedData::Data(data) => {
                 self.output
@@ -390,7 +385,7 @@ pub(super) async fn audio_file_fetch(
 
         file_data_tx,
         complete_tx: Some(complete_tx),
-        network_response_times_ms: Vec::new(),
+        network_response_times: Vec::with_capacity(3),
     };
 
     loop {
@@ -408,10 +403,8 @@ pub(super) async fn audio_file_fetch(
         }
 
         if fetch.get_download_strategy() == DownloadStrategy::Streaming() {
-            let number_of_open_requests = fetch
-                .shared
-                .number_of_open_requests
-                .load(atomic::Ordering::SeqCst);
+            let number_of_open_requests =
+                fetch.shared.number_of_open_requests.load(Ordering::SeqCst);
             if number_of_open_requests < MAX_PREFETCH_REQUESTS {
                 let max_requests_to_send = MAX_PREFETCH_REQUESTS - number_of_open_requests;
 
@@ -424,14 +417,15 @@ pub(super) async fn audio_file_fetch(
                 };
 
                 let ping_time_seconds =
-                    0.001 * fetch.shared.ping_time_ms.load(atomic::Ordering::Relaxed) as f64;
+                    Duration::from_millis(fetch.shared.ping_time_ms.load(Ordering::Relaxed) as u64)
+                        .as_secs_f32();
                 let download_rate = fetch.session.channel().get_download_rate_estimate();
 
                 let desired_pending_bytes = max(
                     (PREFETCH_THRESHOLD_FACTOR
                         * ping_time_seconds
-                        * fetch.shared.stream_data_rate as f64) as usize,
-                    (FAST_PREFETCH_THRESHOLD_FACTOR * ping_time_seconds * download_rate as f64)
+                        * fetch.shared.stream_data_rate as f32) as usize,
+                    (FAST_PREFETCH_THRESHOLD_FACTOR * ping_time_seconds * download_rate as f32)
                         as usize,
                 );