Skip to content

Make Player::get_pos and try_seek independent of playback speed - #934

Open
UnknownSuperficialNight wants to merge 1 commit into
RustAudio:masterfrom
UnknownSuperficialNight:fix-get-pos
Open

UnknownSuperficialNight wants to merge 1 commit into
RustAudio:masterfrom
UnknownSuperficialNight:fix-get-pos

Conversation

@UnknownSuperficialNight

Copy link
Copy Markdown
Contributor

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.

Speed does not change the samples, it only reports a scaled sample rate.
TrackPosition sat after Speed in the chain and divided its sample count
by 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_seek was also affected: Speed::try_seek multiplies the
target by the speed factor, so try_seek(30s) at 0.5x landed at 15s of
the media.

Changes

  • Player::append: track_position() now comes before speed(), so
    samples 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 to
    cancel the multiplication in Speed::try_seek. The division uses f64 and
    falls back to the unscaled position if the result is out of range.
  • Updated the .inner() chains in the periodic_access closure for the
    new adapter order and documented the order.
  • Docs: rewrote TrackPosition::get_pos, updated the speed() and
    set_speed docs and the get_pos/try_seek docs, replaced an outdated
    example in SpatialPlayer::get_pos, and linked the related items.
  • Added a test for the track_position_before_speed ordering.

Behaviour

  • get_pos() is the position in the media, independent of speed.
  • try_seek(pos) seeks to pos in the media at any speed.
  • At speed 1.0 nothing changes.

Testing

  • Played a track at 0.5x and 1.5x: get_pos() advances at half and 1.5x
    the 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 test and cargo doc

Crude 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.

diagram

Additional notes

  1. This branch is forked from my old AGC branch as the main branch at the moment still has Clicking/glitching sound when set_speed is higher or lower than 1.0 using opus #917 and or something else after that branch added the issue, so I just based it off my own for working AGC branch.
  2. I also made this fix simply because I needed it fixed for my program as I'm releasing soon just figured I would submit a PR in case.

closes #837

- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_pos does not seem to account for the current speed

1 participant