Repository navigation
Make Player::get_pos and try_seek independent of playback speed - #934
Open
UnknownSuperficialNight wants to merge 1 commit into
Open
UnknownSuperficialNight wants to merge 1 commit into
UnknownSuperficialNight wants to merge 1 commit into
Conversation
- 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm not sure if this is something we want to even commit while the rewrite is happening in #901 but ill add the PR in case feel free to close if it's irrelevant in the future or if we need updating ping me here
Problem
Player::get_pos()is wrong whenever the speed is not 1.0.Speeddoes not change the samples, it only reports a scaled sample rate.TrackPositionsat afterSpeedin the chain and divided its sample countby that scaled rate, so it measured elapsed playback time instead of the
position in the media. At 0.5x the position advanced 1s per second while
the audio advanced 0.5s per second, and changing the speed during
playback made it wrong in other ways too.
Player::try_seekwas also affected:Speed::try_seekmultiplies thetarget by the speed factor, so
try_seek(30s)at 0.5x landed at 15s ofthe media.
Changes
Player::append:track_position()now comes beforespeed(), sosamples are counted at the source's real sample rate. It still sits
inside
pausable(), so silence played while paused is not counted.SeekOrder::attempt: the target is divided by the current speed tocancel the multiplication in
Speed::try_seek. The division uses f64 andfalls back to the unscaled position if the result is out of range.
.inner()chains in theperiodic_accessclosure for thenew adapter order and documented the order.
TrackPosition::get_pos, updated thespeed()andset_speeddocs and theget_pos/try_seekdocs, replaced an outdatedexample in
SpatialPlayer::get_pos, and linked the related items.track_position_before_speedordering.Behaviour
get_pos()is the position in the media, independent of speed.try_seek(pos)seeks toposin the media at any speed.Testing
get_pos()advances at half and 1.5xthe rate of real time, and stays continuous when the speed changes
during playback.
try_seek(30s)lands at 30s at 0.5x, 1.0x and 2.0x.cargo testandcargo docCrude Diagram
This is a crude diagram showing what changed I was going to use the mermaid diagram renderer on GitHub, but its sucks so here is a PNG instead.
This does not account for pause and all that in the diagram It's simply just the core idea basically.
Additional notes
1.0using opus #917 and or something else after that branch added the issue, so I just based it off my own for working AGC branch.closes #837