Skip to content
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

Fix Symphonia seek for a duration with a fractional element of 0 #687

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

will3942
Copy link
Contributor

@will3942 will3942 commented Jan 23, 2025

Fixes: #686

@will3942 will3942 marked this pull request as draft January 23, 2025 13:58
@will3942 will3942 marked this pull request as ready for review January 23, 2025 13:58
@will3942 will3942 marked this pull request as draft January 23, 2025 14:04
@will3942 will3942 marked this pull request as ready for review January 23, 2025 14:16
@will3942 will3942 marked this pull request as draft January 23, 2025 14:17
@will3942 will3942 marked this pull request as ready for review January 23, 2025 14:29
@will3942
Copy link
Contributor Author

@dvdsk Ready for review - it was harder to get a 0ms test file than it should have been...

src/decoder/symphonia.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

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

Ready to merge as is, could make the unit test an integration test. Let me know what you choose and ill merge.

edit: oh and good find! thanks for fixing my bug (this one is my fault) 🎉

@will3942
Copy link
Contributor Author

Ready to merge as is, could make the unit test an integration test. Let me know what you choose and ill merge.

edit: oh and good find! thanks for fixing my bug (this one is my fault) 🎉

No worries - have moved this to be an integration test :)

@@ -301,10 +301,13 @@ fn skip_back_a_tiny_bit(
}: Time,
) -> Time {
frac -= 0.0001;
Copy link
Collaborator

@PetrGlad PetrGlad Jan 23, 2025

Choose a reason for hiding this comment

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

Maybeif frac < 0.0001 { } else { } instead?

Next to the beginning of the Source saturating_sub will leave seconds unchanged, and the whole effect would be to actually seek forward almost a second.

Copy link
Collaborator

@dvdsk dvdsk Jan 24, 2025

Choose a reason for hiding this comment

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

its never called at the beginning of the source. This handles seeking beyond a source end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

so this is good to merge right?

Copy link
Collaborator

@PetrGlad PetrGlad Jan 24, 2025

Choose a reason for hiding this comment

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

If it fixes the problem - yes, we can merge. Since it is already an improvement.
We can handle other cases later if necessary.
The source itself can be less than a second long. The check compares with < 1ms overlap, but tick-back is 0.1ms. what about overlaps in range 0.1...1.0 ms? I wonder if there are any symphonia functions to work with its timestamps or ways to convert those from/to Duration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The source itself can be less than a second long

ahh damn, I did not think of that. Thanks for clarifying. No I agree now, lets fix this properly only then merge. This will need a comment in the code explaining this case or a future refactor might throw the code out.

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.

Thread panic: Invalid range for Time fractional part.
3 participants