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

Elapsed Duration functionality for the Decoder/LoopedDecoder #510

Closed

Conversation

BastianHentschel
Copy link

@BastianHentschel BastianHentschel commented Sep 7, 2023

The commits add a member to the Decoders, which contains the required information for the elapsed duration.

The functionality is implemented in the impl<R> Iterator for Decoder<R>/LoopedDecoder<R> and ticks an AtomicUsize up each read sample. With the channel count and sample rate the effective elapsed duration of a Decoder can be calculated.

@BastianHentschel
Copy link
Author

BastianHentschel commented Sep 7, 2023

This could maybe be a solution to #474 and the relevant part of #457

The total duration can be gotten beforehand from the decoder and be cached for later use.

@dvdsk
Copy link
Collaborator

dvdsk commented Oct 4, 2023

Could also be useful for #443, I was planning to optimize that in PR #513 by seeking if the duration to skip is long however for that I need the elapsed playtime/duration.
(seek = elapsed + skip)

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.

Love that you are adding this! I have wanted it for a while, my current workaround is guessing the time between pausing and playing however that has a ton of issues.

I am also adding seeking support to rodio. It would be very nice if I or someone else could use that and this to fix: #443. For that we would need the elapsed_duration inside:

fn do_skip_duration<I>(input: &mut I, mut duration: Duration)

Is it possible to change the API/approach to this such that the elapsed_duration could be used there?

It could also be nice if we could access the current elapsed duration for the source currently playing in a Sink.

Again thanks for contributing!

edit: now that I think about it it would also make testing seek way easier! :)


#[inline]
pub fn elapsed_duration(&self) -> Option<Duration> {
Duration::try_from_secs_f64(
Copy link
Collaborator

Choose a reason for hiding this comment

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

forgive me but why do we return an Option here? is there a combination of elapsed_samples, sample rate and channels that makes try_from_secs_f64 fail? I would love a comment explaining that.

Or is it there because some combination of sources could not support elapsed_duration?

@dvdsk dvdsk mentioned this pull request Oct 5, 2023
}

#[inline]
pub fn elapsed_duration(&self) -> Option<Duration> {
Copy link
Collaborator

@dvdsk dvdsk Oct 6, 2023

Choose a reason for hiding this comment

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

This might not be the best name, elapsed sounds like its the time since the song started playing, however it is the position of the song right? So maybe name it .... position, playback_pos or something else

Cause this could get confusing with seeking.

@dvdsk
Copy link
Collaborator

dvdsk commented Apr 16, 2024

This might fit better as a new method on Source with a default implementation returning None (just like try_seek). Will be a little more work since now all the adapters need to be adjusted. Though one can easily follow what try_seek does for most cases.

The bigger question is what to do about Sink. Since that is what most media player like uses for rodio will use.

@geeseofbeverlyroad
Copy link

Really nice! Am using rodio for a small music app, and would be very helpful to have this feature.

@dvdsk
Copy link
Collaborator

dvdsk commented May 28, 2024

Note for myself in the future:

Implement this through adding a new source wrapping whatever is beneath.

  • The new source keeps track of the position (number of samples * sample rate for each frame).
  • Will work universally on any source.
  • The new source tracking position will also get a try_seek implementation that updates the position on success.
  • Using a relaxed atomic might enable easy access. Something like get_pos() -> &AtomicU32 (u32 fits 49.71027 days of milliseconds, is that enough and precise enough?).

Anyone should feel free to try something like this, I do not have the time atm. I do need this feature for a hobby project so I will implement it eventually, tho that could be in a few months.

@geeseofbeverlyroad
Copy link

@dvdsk Ah, should have read more carefully, I assumed the current state of the PR is mergeable, since the most recent comment was just a few weeks old.

I could try to make a new PR based on the design you just outlined, but am afraid the architectural design decisions are a bit out of my comfort zone as I'm quite unfamiliar with the library.

With a slightly more detailed set of instrucitons can definitely attempt to do this.

@BastianHentschel
Copy link
Author

I haven't looked at this in a while, but I can also later this week take a look at the requirements and see how the features could be implemented.
I also only did this because I needed the feature myself and I put it here, because it seemed like somebody else could also be interested.

@dvdsk
Copy link
Collaborator

dvdsk commented May 29, 2024

am afraid the architectural design decisions are a bit out of my comfort zone as I'm quite unfamiliar with the library.

I recommend taking a look at the text here: https://docs.rs/rodio/latest/rodio/source/trait.Source.html, browse a few of the structs implementing Source and take a look at the sink here:

let source = source

That should give you a feel for the relevant part of the code base. Then you or @BastianHentschel can give it a try. Let me know if its still unclear after reading up, then ill write up some pseudo code.

Gusted added a commit to Gusted/rodio that referenced this pull request Jun 9, 2024
This adds a new PositionTrack trait which counts the amount of times
`next()` is called (where it returns `Some`) and provide a function to
get the position, which is a simple calculation.

This is added to the Sink struct by default and the accuracy of this depends on
the interval of `periodic_access`.

I wasn't able to add testing to the sink counter part, because I
wanted to also test `try_seek`, but it seems like I would need to have
some threading code to call `next` on sink in another thread, which
didn't look that good and resulted in a flaky test so only a 'unit test'
in ``position.rs`.

Resolves RustAudio#457
Closes RustAudio#510
Gusted added a commit to Gusted/rodio that referenced this pull request Jun 9, 2024
This adds a new TrackPosition trait which counts the amount of times
`next()` is called (where it returns `Some`) and provide a function to
get the position, which is a simple calculation.

This is added to the Sink struct by default and the accuracy of this depends on
the interval of `periodic_access`.

I wasn't able to add testing to the sink counter part, because I
wanted to also test `try_seek`, but it seems like I would need to have
some threading code to call `next` on sink in another thread, which
didn't look that good and resulted in a flaky test so only a 'unit test'
in ``position.rs`.

Resolves RustAudio#457
Closes RustAudio#510
Gusted added a commit to Gusted/rodio that referenced this pull request Jun 9, 2024
This adds a new TrackPosition trait which counts the amount of times
`next()` is called (where it returns `Some`) and provide a function to
get the position, which is a simple calculation.

This is added to the Sink struct by default and the accuracy of this depends on
the interval of `periodic_access`.

I wasn't able to add testing to the sink counter part, because I
wanted to also test `try_seek`, but it seems like I would need to have
some threading code to call `next` on sink in another thread, which
didn't look that good and resulted in a flaky test so only a 'unit test'
in ``position.rs`.

Resolves RustAudio#457
Closes RustAudio#510
@Gusted
Copy link
Contributor

Gusted commented Jun 9, 2024

Hi, I've gone ahead and implemented what was described in #510 (comment) over at #585

@dvdsk
Copy link
Collaborator

dvdsk commented Jun 9, 2024

Hi, I've gone ahead and implemented what was described in #510 (comment) over at #585

Ill have a look tomorrow 👍 thanks for the work! Since that effort supersedes this I am gonna close this PR for now and move the discussion there.

@dvdsk dvdsk closed this Jun 9, 2024
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.

4 participants