From 0aaf78f84534237149a5d602852afe60a84853d8 Mon Sep 17 00:00:00 2001 From: UnknownSuperficialNight <88142731+UnknownSuperficialNight@users.noreply.github.com> Date: Mon, 26 Oct 2026 10:06:35 +1300 Subject: [PATCH] fix: Make `Player::get_pos` and `try_seek` independent of playback speed - Move `track_position()` before `speed()` in `Player::append` so samples are counted at the source's real sample rate. Previously it counted against the scaled rate that Speed reports, so `get_pos()` returned elapsed playback time instead of the position in the media (at 0.5x it advanced 1s per second instead of 0.5s) - Divide the seek target by the current speed in `SeekOrder::attempt` to cancel the scaling in `Speed::try_seek`, so `try_seek(pos)` lands at pos in the media at any speed - Do the division in `f64` and fall back to the unscaled position instead of panicking on an out of range duration - Update the .inner() chains in the periodic_access closure for the new adapter order and document the order - Rewrite the `TrackPosition::get_pos` docs, which recommended placing track_position after speed (the opposite of what Player now does), and describe what try_seek reports in each ordering - Add a test for the `track_position_before_speed` ordering - Update the docs of Player::get_pos, Player::try_seek, Player::set_speed and Source::speed: playback duration instead of "total duration", seek targets are scaled by the speed, and positions do not depend on it - Fix the SpatialPlayer::speed getter doc, which said "Changes", and replace the outdated example in SpatialPlayer::get_pos - Link the docs of Speed, TrackPosition and Player to each other --- src/player.rs | 97 ++++++++++++++++++++++++++++++++---------- src/source/mod.rs | 16 +++++-- src/source/position.rs | 50 +++++++++++++++++++--- src/spatial_player.rs | 28 +++--------- 4 files changed, 136 insertions(+), 55 deletions(-) diff --git a/src/player.rs b/src/player.rs index c3547be00..378fee7f0 100644 --- a/src/player.rs +++ b/src/player.rs @@ -48,11 +48,25 @@ impl SeekOrder { (Self { pos, feedback: tx }, rx) } - fn attempt(self, maybe_seekable: &mut S) + /// Performs the seek. `speed` is the current playback speed. + /// + /// [`Speed`](crate::source::Speed) multiplies the target position of its + /// [`try_seek`](Source::try_seek) by the speed factor, so the target is divided by + /// the speed first. That makes `self.pos` a position in the source's own timeline, + /// independent of the playback speed. + fn attempt(self, maybe_seekable: &mut S, speed: f32) where S: Source, { - let res = maybe_seekable.try_seek(self.pos); + let pos = if speed.is_finite() && speed > 0.0 { + // f64 keeps full precision for long sources; an out of range result + // falls back to the unscaled position instead of panicking. + Duration::try_from_secs_f64(self.pos.as_secs_f64() / f64::from(speed)) + .unwrap_or(self.pos) + } else { + self.pos + }; + let res = maybe_seekable.try_seek(pos); let _ignore_receiver_dropped = self.feedback.send(res); } } @@ -119,11 +133,20 @@ impl Player { let start_played = AtomicBool::new(false); let sound_count_clone = self.sound_count.clone(); + // Adapter order, outermost first (`periodic_access` wraps all of it): + // Done -> Stoppable -> Skippable -> Amplify -> Pausable -> Speed -> TrackPosition -> source + // + // `track_position()` must come before `speed()`. `Speed` does not change the samples, + // it only reports a scaled sample rate, and `TrackPosition` divides its sample count by + // the rate it sees. After `Speed` it would measure elapsed playback time instead of the + // position in the media. It stays inside `pausable()` so that the silence played while + // paused is not counted. + // + // The `.inner()` / `.inner_mut()` chains in the closure below follow this order. let source = Done::new( source - .speed(1.0) - // Must be placed before pausable but after speed & delay .track_position() + .speed(1.0) .pausable(false) .amplify(1.0) .skippable() @@ -147,20 +170,33 @@ impl Player { *to_clear -= 1; *controls.position.lock().unwrap() = Duration::ZERO; } else { - *controls.position.lock().unwrap() = - src.inner().inner().inner().inner().inner().get_pos(); + // The position comes from `TrackPosition`, the innermost adapter. + *controls.position.lock().unwrap() = src + .inner() // Stoppable + .inner() // Skippable + .inner() // Amplify + .inner() // Pausable + .inner() // Speed + .inner() // TrackPosition + .get_pos(); } } - let amp = src.inner_mut().inner_mut().inner_mut(); + let amp = src + .inner_mut() // Stoppable + .inner_mut() // Skippable + .inner_mut(); // Amplify amp.set_factor(*controls.volume.lock().unwrap()); - amp.inner_mut() + amp.inner_mut() // Pausable .set_paused(controls.pause.load(Ordering::SeqCst)); - amp.inner_mut() - .inner_mut() - .inner_mut() - .set_factor(*controls.speed.lock().unwrap()); + + let speed = *controls.speed.lock().unwrap(); + amp.inner_mut() // Pausable + .inner_mut() // Speed + .set_factor(speed); + if let Some(seek) = controls.seek.lock().unwrap().take() { - seek.attempt(amp) + // The seek needs the current speed to undo the scaling in `Speed::try_seek`. + seek.attempt(amp, speed) } start_played.store(true, Ordering::SeqCst); }); @@ -203,11 +239,12 @@ impl Player { /// lowering its pitch. /// - If you set the speed to 2 the frequency will double raising the /// pitch of the sound. - /// 2. **Change in the speed affect the total duration inversely** - /// - If you set the speed to 0.5, the total duration will be twice as long. - /// - If you set the speed to 2 the total duration will be halve of what it - /// was. - /// + /// 2. **A change in the speed affects the playback duration inversely** + /// - If you set the speed to 0.5, playing the sound takes twice as long. + /// - If you set the speed to 2, playing the sound takes half as long. + /// 3. **Positions do not depend on the speed** + /// - [`Player::get_pos`] and [`Player::try_seek`] use positions in the sound + /// itself, so they are not scaled by the speed. #[inline] pub fn set_speed(&self, value: f32) { *self.controls.speed.lock().unwrap() = value; @@ -227,6 +264,14 @@ impl Player { /// Attempts to seek to a given position in the current source. /// + /// `pos` is a position in the source's own timeline and does not depend on the + /// current playback speed: seeking to 30 seconds lands 30 seconds into the media + /// whether the speed is 0.5, 1.0 or 2.0. This is the same timeline as + /// [`Player::get_pos`]. + /// + /// The target is divided by the current speed before it reaches + /// [`Speed`](crate::source::Speed), which would otherwise scale it. + /// /// This blocks between 0 and ~5 milliseconds. /// /// As long as the duration of the source is known, seek is guaranteed to saturate @@ -338,13 +383,19 @@ impl Player { self.sound_count.load(Ordering::Relaxed) } - /// Returns the position of the sound that's being played. + /// Returns the playback position within the current source. + /// + /// The position is measured in the source's own timeline, so it does not depend + /// on the playback speed. For example, at a speed of `0.5` it advances by half a + /// second for every second of real time, and changing the speed during playback + /// does not make it jump. /// - /// This takes into account any speedup or delay applied. + /// After [`Player::try_seek`] this is the position that was sought to. /// - /// Example: if you apply a speedup of *2* to an mp3 decoder source and - /// [`get_pos()`](Player::get_pos) returns *5s* then the position in the mp3 - /// recording is *10s* from its start. + /// The position is read from a [`TrackPosition`](crate::source::TrackPosition) that + /// sits before [`Speed`](crate::source::Speed) in the adapter chain. See + /// [`TrackPosition::get_pos`](crate::source::TrackPosition::get_pos) for why the + /// order matters. #[inline] pub fn get_pos(&self) -> Duration { *self.controls.position.lock().unwrap() diff --git a/src/source/mod.rs b/src/source/mod.rs index e6b9f604b..3a530a448 100644 --- a/src/source/mod.rs +++ b/src/source/mod.rs @@ -505,10 +505,18 @@ pub trait Source: Iterator { /// lowering its pitch. /// - If you set the speed to 2 the frequency will double raising the /// pitch of the sound. - /// 2. **Change in the speed affect the total duration inversely** - /// - If you set the speed to 0.5, the total duration will be twice as long. - /// - If you set the speed to 2 the total duration will be halve of what it - /// was. + /// 2. **A change in the speed affects the playback duration inversely** + /// - If you set the speed to 0.5, playing the source takes twice as long. + /// - If you set the speed to 2, playing the source takes half as long. + /// - [`total_duration`](Source::total_duration) of the returned source reports + /// this playback duration. + /// 3. **Seek targets are scaled by the speed** + /// - [`try_seek`](Source::try_seek) multiplies its target by the speed: at a + /// speed of 2, seeking to 10 seconds seeks to 20 seconds in the original source. + /// - Whether [`track_position`](Source::track_position) reports the position in + /// the original source or the elapsed playback time depends on whether it is + /// applied before or after `speed`, see + /// [`TrackPosition::get_pos`](super::TrackPosition::get_pos). /// /// See [`Speed`] for details #[inline] diff --git a/src/source/position.rs b/src/source/position.rs index f41848eb6..513e0a55b 100644 --- a/src/source/position.rs +++ b/src/source/position.rs @@ -54,14 +54,37 @@ where { /// Returns the position of the underlying source relative to its start. /// - /// If a speedup and or delay is applied after applying a - /// [`Source::track_position`] it will not be reflected in the position - /// returned by [`get_pos`](TrackPosition::get_pos). + /// The position is the number of samples read from the inner source, divided + /// by the sample rate and channel count that the inner source reports. /// - /// This can get confusing when using [`get_pos()`](TrackPosition::get_pos) - /// together with [`Source::try_seek()`] as the the latter does take all - /// speedup's and delay's into account. Its recommended therefore to apply - /// track_position after speedup's and delay's. + /// [`Source::speed`] (see [`Speed`](super::Speed)) works by changing the reported + /// sample rate, so the result depends on where `track_position()` is placed + /// relative to it: + /// + /// - **Before `speed()`**, as in `source.track_position().speed(x)`: the sample + /// rate is the real one, so this is the position in the original source. It + /// does not depend on the speed, also not when the speed changes during + /// playback. + /// - **After `speed()`**, as in `source.speed(x).track_position()`: the sample + /// rate is the scaled one, so this is the elapsed playback time. At a speed + /// of `2.0` it advances by `0.5` seconds per sample read from the source. + /// This is only the position in the original source if the speed never + /// changes, and then only after multiplying it by the speed. + /// + /// # Seeking + /// + /// [`Source::try_seek`] forwards the target to the inner source and then reports + /// the target as the new position. [`Speed`](super::Speed) multiplies the targets + /// its [`try_seek`](Source::try_seek) receives by its speed factor, so with + /// `track_position()` before `speed()` a seek through the outer source reports + /// the multiplied target: + /// `source.track_position().speed(2.0).try_seek(10s)` seeks to `20s` in the + /// original source and `get_pos()` then returns `20s`. To seek to a position + /// `p` in the original source through a speed adapter, seek to `p / speed`. + /// + /// [`Player`](crate::Player) uses the first ordering and does that division, see + /// [`Player::get_pos`](crate::Player::get_pos) and + /// [`Player::try_seek`](crate::Player::try_seek). #[inline] pub fn get_pos(&self) -> Duration { let seconds = self.span.samples_counted as Float @@ -180,4 +203,17 @@ mod tests { assert!(source.try_seek(Duration::new(1, 0)).is_ok()); assert_eq!(source.get_pos().as_secs_f32(), 1.0); } + + #[test] + fn test_position_before_speedup_is_independent_of_speed() { + let inner = SamplesBuffer::new(nz!(1), nz!(1), vec![10.0, -10.0, 10.0, -10.0, 20.0, -20.0]); + let mut source = inner.track_position().speed(2.0); + + assert_eq!(source.inner().get_pos().as_secs_f32(), 0.0); + source.next(); + assert_eq!(source.inner().get_pos().as_secs_f32(), 1.0); + + source.next(); + assert_eq!(source.inner().get_pos().as_secs_f32(), 2.0); + } } diff --git a/src/spatial_player.rs b/src/spatial_player.rs index d1fe3cb20..5c7511566 100644 --- a/src/spatial_player.rs +++ b/src/spatial_player.rs @@ -95,20 +95,10 @@ impl SpatialPlayer { self.player.set_volume(value); } - /// Changes the play speed of the sound. Does not adjust the samples, only the playback speed. + /// Gets the speed of the sound. /// - /// # Note: - /// 1. **Increasing the speed will increase the pitch by the same factor** - /// - If you set the speed to 0.5 this will halve the frequency of the sound - /// lowering its pitch. - /// - If you set the speed to 2 the frequency will double raising the - /// pitch of the sound. - /// 2. **Change in the speed affect the total duration inversely** - /// - If you set the speed to 0.5, the total duration will be twice as long. - /// - If you set the speed to 2 the total duration will be halve of what it - /// was. - /// - /// See [`Speed`](crate::source::Speed) for details + /// See [`SpatialPlayer::set_speed`] for what the speed means, and + /// [`Speed`](crate::source::Speed) for details. #[inline] pub fn speed(&self) -> f32 { self.player.speed() @@ -197,10 +187,10 @@ impl SpatialPlayer { /// /// # Errors /// This function will return [`SeekError::NotSupported`] if one of the underlying - /// sources does not support seeking. + /// sources does not support seeking. /// /// It will return an error if an implementation ran - /// into one during the seek. + /// into one during the seek. /// /// When seeking beyond the end of a source this /// function might return an error if the duration of the source is not known. @@ -208,13 +198,9 @@ impl SpatialPlayer { self.player.try_seek(pos) } - /// Returns the position of the sound that's being played. - /// - /// This takes into account any speedup or delay applied. + /// Returns the playback position within the current source. /// - /// Example: if you apply a speedup of *2* to an mp3 decoder source and - /// [`get_pos()`](Player::get_pos) returns *5s* then the position in the mp3 - /// recording is *10s* from its start. + /// See [`Player::get_pos`] for details. #[inline] pub fn get_pos(&self) -> Duration { self.player.get_pos()