Skip to content

Saturate seek position to track duration #1483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 6, 2025

Conversation

roderickvd
Copy link
Member

The fix prevents seeking past the end of a track by capping the requested seek position to the actual duration. It also makes timestamp calculation more robust.

This does not entirely fix #1472, but does make the decoder more robust against invalid calls.

Please test this for me, because I no longer have a Spotify account.

The fix prevents seeking past the end of a track by capping the requested
seek position to the actual duration.
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

playback/src/player.rs:1783

  • Changing the match from 'PlayerState::Invalid { .. }' to 'PlayerState::Invalid' may fail to match variants that carry additional data. Confirm that the state does not need to account for inner fields for proper error handling.
if matches!(self.state, PlayerState::Invalid) {

playback/src/decoder/symphonia_decoder.rs:137

  • [nitpick] The conversion to Duration and subsequent use of as_millis may change the rounding behavior compared to the original calculation. Verify that truncating sub-millisecond precision is acceptable for timestamp accuracy.
let time = Duration::from(time_base.calc_time(ts));

@roderickvd
Copy link
Member Author

P.S. testing also means just doing some random seeks, to verify there is no regression in the normal case.

Copy link
Member

@photovoltex photovoltex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improved handling :)

Just some minor thoughts to the conversion handling^^

@roderickvd roderickvd merged commit cb958a0 into librespot-org:dev Apr 6, 2025
13 checks passed
@roderickvd roderickvd deleted the fix/saturate-seeks branch April 6, 2025 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected exit due to "seek timestamp is out-of-range"
2 participants