rodio icon indicating copy to clipboard operation
rodio copied to clipboard

Implements Seek

Open yara-blue opened this issue 2 years ago • 12 comments

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.

yara-blue avatar Oct 04 '23 10:10 yara-blue

Note: this is currently still a draft, early reviews are welcome though :)

yara-blue avatar Oct 04 '23 10:10 yara-blue

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.

yara-blue avatar Oct 07 '23 12:10 yara-blue

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!

yara-blue avatar Oct 12 '23 17:10 yara-blue

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.

yara-blue avatar Oct 13 '23 13:10 yara-blue

Could use this in a project, is there anything I can do to help here @dvdsk @naglis @est31 ?

nick42d avatar Dec 04 '23 13:12 nick42d

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.

aaalloc avatar Dec 04 '23 13:12 aaalloc

IS something blocking from pulling into main?

shimekukuri avatar Jan 26 '24 18:01 shimekukuri

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.

yara-blue avatar Jan 26 '24 21:01 yara-blue

@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?

naglis avatar Jan 31 '24 12:01 naglis

@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 :)

yara-blue avatar Jan 31 '24 13:01 yara-blue

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?

AnthonyMichaelTDM avatar Mar 12 '24 04:03 AnthonyMichaelTDM

Would love to see this get merged in

shimekukuri avatar Mar 14 '24 15:03 shimekukuri

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

est31 avatar Mar 24 '24 18:03 est31

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.

yara-blue avatar Mar 24 '24 21:03 yara-blue

@Siirko (and everyone else already running this) did you run into any problems regarding this PR (including unclear documentations etc) in your project?

yara-blue avatar Mar 24 '24 21:03 yara-blue

@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

aaalloc avatar Mar 24 '24 21:03 aaalloc

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!

Houndie avatar Apr 05 '24 11:04 Houndie

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.

yara-blue avatar Apr 06 '24 10:04 yara-blue