Implements Seek
Adresses #176, see https://github.com/RustAudio/rodio/issues/176#issuecomment-1741860432 for the motivations behind the design.
Adds a fail-able method fn try_seek(&mut self, pos: Duration) -> Result<(), SeekNotSupported> to all sources and decoders. The error struct SeekNotSupported contains the type name of the source that did not support seek. This is particularly useful when you have a chain of sources.
I commented out some working functionality that could not be merged yet:
- Mixing sources are not seek-able, they need to know the current position in a source to be able to roll back when one of the sources fails to seek.
- I added seeking to Minimp3 however the minimp3_fixed maintainer has engaged with my PR yet. I want to give them some more time to do so before looking at other options.
Note: this is currently still a draft, early reviews are welcome though :)
So the motivation behind can_seek was to use it in mixing sources. That way we can prevent scenarios where we seek in one source (A) and cant in the other (B). The problem is that the seek can still fail if can_seek returns true.
A better approach is to use #510 to seek back in A if B returns a (non critical) error while seeking. Such an error could be our SeekError::NotSupported or something like symphonia's SeekErrorKind::Unseekable.
Thank you for your work!
I lack the expertise to give a thorough review of the actual implementation, so these are mostly typos I've noticed.
I applied all the spelling/grammar suggestions directly, thanks a lot for them! The others are great too they just require a little more attention. I will apply them tomorrow!
rebased to make the commit history a bit cleaner. @est31 I am fine with squashing it all to a single commit if you prefer that.
Could use this in a project, is there anything I can do to help here @dvdsk @naglis @est31 ?
Could use this in a project, is there anything I can do to help here @dvdsk @naglis @est31 ?
In the meanwhile that they will merge it in main, you can directly do this in your Cargo.toml of your project
rodio = { git = "https://github.com/dvdsk/rodio", branch= "seek_runtime_err", features = ["symphonia-all"] }
I did it for my project and there is no problem.
IS something blocking from pulling into main?
New commit on main made this unmergable, should be fixed now.
@est31 I would love to help out maintaining Rodio. Let me know if that's something you would like to discuss.
@dvdsk it seems that some changes from my comments were lost at some point, e.g. the example is still broken (uses incorrect path). Could you please check?
@dvdsk it seems that some changes from my comments were lost at some point, e.g. the example is still broken (uses incorrect path). Could you please check?
good catch, no idea what went wrong there... Fixed now :)
It's been a few months, is there anything blocking this from being merged? And if so, is there anything I can do to help?
Would love to see this get merged in
@dvdsk
@est31 I would love to help out maintaining Rodio. Let me know if that's something you would like to discuss.
I've sent an invite. Sorry, I don't have much time recently and the PR seemed so big so I only read this message now. Feel free to merge PRs but ideally they should get a review at least by one other person. Not sure I'll find the time to review seek support myself though. In the worst case, you can use someone who is not a maintainer, but they do need to be someone you trust.
I've sent an invite. Sorry, I don't have much time recently and the PR seemed so big.
I completely understand, it is a big PR. If you have little time don't spend it on this, there are more important things in life. I have a few engineers I trust, I will see if I can get one of them to go through it. It helps that some community members already went over things. We will get this merged responsibly.
@Siirko (and everyone else already running this) did you run into any problems regarding this PR (including unclear documentations etc) in your project?
@Siirko (and everyone else already running this) did you run into any problems regarding this PR (including unclear documentations etc) in your project?
0 problems in my case
Just wanted to drop props for @dvdsk and everyone else who has reviewed this, this is great work and I'll be excited to see this land!
Thanks to @Rintse for his extensive review! We caught a few cases where seek could be extended. The review also inspired me to extend the test suite with as result a ton of bugs discovered :smile:.
Those have now all been fixed! Time to merge this PR :tada: :partying_face: :balloon:.
Whats next?
In a few days I plan to cut a new release and push that to crates.io and maybe an announcement on users.rust-lang.org and the reddit.
In the long term I am planning a few other features to make rodio more suitable as a backend for music players. They will land over the next year probably (no promises). Meanwhile I'll be spending a little time every week to work through the PR backlog. So if you see anything you would like in rodio, do not hesitate to open an issue. If it fits for rodio we can work on it together.