Skip to content

Improve seeking behavior and error handling #745

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

Conversation

roderickvd
Copy link
Collaborator

I think I've found the compromise I like best:

  • Automatically enable is_seekable when byte_len is set
  • Add helpful error message for random access seeking without byte_len
  • Clarify documentation for builder methods with default states
  • Test forward-only vs. random access seeking scenarios
  • Ensure MP4s open without depending on is_seekable

I also tried setting is_seekable to true by default, but that only increases random access seeking weirdness without byte_len set. Which is why I think this is the sweet spot of compromises.

Mostly fixes #740
Supersedes #744

I say "mostly" fixes because symphonia-isomp4 can still report it's successfully seeked backwards, when in fact it hasn't without byte_len set. In Symphonia v0.5 the MP4 support is working, but with a number of bugs that are targeted for some v0.6 release. In the meantime, I propose we regard MP4 as a "second tier" target and we don't go too far out of a way to fix upstream issues here.

@roderickvd
Copy link
Collaborator Author

@dvdsk if you have time, I feel we should slip this one into v0.21 as well.

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.

I really like it! It clearly documents why things have to be (as complicated) as they are while simultaneously setting the defaults so that the average user will get a good result without looking at the docs.

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.

one suggestion, feel free to ignore it though.

@dvdsk
Copy link
Collaborator

dvdsk commented Jun 4, 2025

@dvdsk if you have time, I feel we should slip this one into v0.21 as well.

We will, documentation made this an easy review :)

If you made the branch on the rodio repo I would have solved that merge conflict for you 😝

- Automatically enable is_seekable when byte_len is set
- Add specific error for random access seeking without byte_len
- Clarify documentation for builder methods with default states
- Test forward-only vs random access seeking scenarios

Fixes RustAudio#740
Supersedes RustAudio#744
@roderickvd roderickvd force-pushed the feature/helpful-seeking-error branch from 82cfccc to a32fe7f Compare June 4, 2025 19:50
@roderickvd
Copy link
Collaborator Author

If you made the branch on the rodio repo I would have solved that merge conflict for you 😝

What's the policy, you want me to do that (branch on the repo) instead going forward?

@roderickvd roderickvd merged commit d234cc6 into RustAudio:master Jun 4, 2025
9 checks passed
@roderickvd roderickvd deleted the feature/helpful-seeking-error branch June 4, 2025 20:01
@dvdsk
Copy link
Collaborator

dvdsk commented Jun 4, 2025

What's the policy, you want me to do that (branch on the repo) instead going forward?

I branch on the repo for three reasons:

  • other maintainers can jump in and help out
  • could help to see what is going on
  • if I have to take a step back its a little easier to take over

There are probably just as many reasons to branch on a fork. We could formulate a policy, I don't think
there is one now? On the other hand its not really that important. I just wanted to make sure you
knew that there is no objection from me to creating branches on this repo.

Semi-related: governance of Rodio.

I think of us as a flat organization, none one has more say then anyone else. I would not know who
would ultimately be in charge otherwise... maybe Pierre since he made rodio...

Of course on my CV I am Project lead of Rodio 😄

@roderickvd
Copy link
Collaborator Author

Thanks, I'll just branch here then. No need for any formalities at this point.

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.

Potential Regression: try_seek Panicking When Seeking Backwards
2 participants