Add TrackPosition to Source #585

Gusted · 2024-06-09T21:18:40Z

This adds a new TrackPosition wrapper 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 Sink 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 #457
Closes #510

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

src/source/position.rs

dvdsk · 2024-06-10T12:03:21Z

src/source/position.rs

+{
+    #[inline]
+    fn current_frame_len(&self) -> Option<usize> {
+        self.input.current_frame_len()


after current_frame_len the sample rate may change.
One way to deal with that is to:

collect samples until the frame rate changes

convert the collected samples using the old frame-rate and add them to a duration

change get_pos() to return the newly collected samples * new frame rate + duration

Feel free to experiment with other approaches to find what fits best. Performance matters but as always premature optimization is the root of all evil. An if statement that fires every frame_len in next() might very well pulled up by the optimizer and merged with the iterator in some more optimal fashion. Better to measure the impact before spending time on optimizing.

Do you happen to know which decoder currently handle this correctly? I've tried with FLAC using https://github.com/ietf-wg-cellar/flac-test-files/blob/main/uncommon/01%20-%20changing%20samplerate.flac with the symphonia decoder and after switching to the next sample rate it stops working and with claxon it does work, but doesn't seem to record the new sample rate (just reports the 32 kHz all the way trough) and it ends up 'pitching' the audio. (FWIW, VLC also borks on this only mpv seems to somewhat handle it but has incorrect position after switching the sample rate and has the incorrect total duration). Shouldn't be hard to handle these cases correctly, but at least the FLAC decoder is stopping me from testing this and I don't know of any other format that supports variable sample rate 😅

I've gone ahead and opened PRs against claxon and symphonia to correctly handle such cases. ruuda/claxon#35 & pdeljanov/Symphonia#287. For the claxon support a single line would need to be added to rodio once the PR is merged:

diff --git a/src/decoder/flac.rs b/src/decoder/flac.rs index 9dbc0c4..b3b69dd 100644 --- a/src/decoder/flac.rs +++ b/src/decoder/flac.rs @@ -118,6 +118,7 @@ where let buffer = mem::take(&mut self.current_block); match self.reader.blocks().read_next_or_eof(buffer) { Ok(Some(block)) => { + self.sample_rate = block.sample_rate(); self.current_block_channel_len = (block.len() / block.channels()) as usize; self.current_block = block.into_buffer(); }

Do you happen to know which decoder currently handle this correctly?

On the rodio side at least the symphonia and vorbis code takes care to handle this. I had no idea it was broken in symphonia & claxon (rodio's test suit is kinda sparse atm). Like us they are desperate for maintainers/contributers :).

I've gone ahead and opened PRs against claxon and symphonia

nice! thanks a lot for that 👍

Once the decoders are fixed 0e6d5a1 should make the position tracking work correctly with. Doing a few local tests doesn't hint at performance problems (but only a profiler would be able to confirm that) and tracking the position of https://github.com/ietf-wg-cellar/flac-test-files/blob/main/uncommon/01%20-%20changing%20samplerate.flac works flawlessly.

dvdsk · 2024-06-10T12:20:11Z

src/sink.rs

                }
                {
                    let mut to_clear = controls.to_clear.lock().unwrap();
                    if *to_clear > 0 {
                        src.inner_mut().skip();
                        *to_clear -= 1;
+                        *controls.position.lock().unwrap() = 0.0;
+                    } else {
+                        *controls.position.lock().unwrap() = src.inner().inner().inner().inner().get_pos();


I am planning to overhaul this whole module, the every 5ms check for things to do feels very iffy. One might want more precision. Though that is for later right now. If you have ideas on how to redo this module I would love to hear them.

src/source/position.rs

dvdsk

I have also been thinking about the API. Most use cases will take the form of some kind of music player with a UI showing the current position. In that case the position will need to be updated once every second.

Right now you need access to the source to get the position which in turn requires the every_x_ms closure thing in Sink.

So here is a wild (and possibly bad) idea, lets store the current position in an Arc. We can use relaxed ordering so the performance impact of using atomics should be negligible. The Arc is Clone, Send and Sync, it could be returned by the factory function* that creates the stream. The Sink could store it in its handle, but also readily hand it out. Users could even put it in their UI loop.

Is this worth exploring?

*thats how to refer to members that create Self right?

Gusted · 2024-06-16T16:12:06Z

I've implemented the Arc idea in ca43ad0 I'm not sure if we should hand out references to the position Arc or if we should just provide the get_pos function. As the latter works fine in my case (music player) and avoids having users of this library having to handle that extra Arc for just position.

dvdsk · 2024-06-16T22:45:09Z

src/sink.rs

@@ -12,6 +12,28 @@ use crate::stream::{OutputStreamHandle, PlayError};
 use crate::{queue, source::Done, Sample, Source};
 use cpal::FromSample;

+#[derive(Debug)]
+pub struct AtomicF64 {


nice solution to not having an atomicf64!

dvdsk · 2024-06-16T23:29:19Z

I'm not sure if we should hand out references to the position Arc or if we should just provide the get_pos function.

I do not think we should expose the fact there is an Arc involved, wrapping it with a struct that provides a get_pos would be a good solution to that.

Fundamentally the question is whether you need to own the source to get the position (no arc) or if there is a position handle (the arc) that you can freely move around your program.

Right now rodio uses the first approach for everything. Concretely if you want interact with any source you need to own that source*. To kinda work around that rodio introduces a smart wrapper the periodic_access wrapper. That swaps the need to own disadvantage for a slight delay before the interaction or outdated info disadvantage. This is luckily hidden by the Sink for most users. The Sink itself uses a ton synchronization primitives to communicate between a handle and the closure in periodic access.

We (probably I 😅 ) could improve the Sink by swapping out periodic_access for signaled_access, that would run a closure on a signal. That would get rid of the delay and outdated info.

As the latter works fine in my case (music player) and avoids having users of this library having to handle that extra Arc for just position.

If you think its more of a hassle then we should not go for it. The only advantage the Arc is that is would be clone (the sink is not clone atm). That could make using the arc easier for the end user. If you think it is actually easier to query the Sink then it is better do away with the arc.

I have some ideas of how to make the Sink clone, so even that disadvantage can be negated in the future.

Thanks for trying it out though!

*A source being a stack of wrappers (a trackposition wrapping pausable wrapping decoder)

Gusted · 2024-06-17T01:49:45Z

If you think its more of a hassle then we should not go for it. The only advantage the Arc is that is would be clone (the sink is not clone atm). That could make using the arc easier for the end user. If you think it is actually easier to query the Sink then it is better do away with the arc.

My use case just relies on the fact that Sink exists and it (ab)uses that. So having that and have it exposes functions like get_pos and volume etc. makes it a nice high level struct. I think you would end up with some boilerplate code for the end users if it was a low level struct where you will be given Arcs.

Fundamentally the question is whether you need to own the source to get the position (no arc) or if there is a position handle (the arc) that you can freely move around your program.

I assume users either have a Source or a Sink in their state that's available in an event loop (assuming there's one). From what I understand, it could become no Arc because in the Source case, you do own the Source and in the Sink case Sink owns the Source and could do the necessary code to return the position. I don't think owning an Arc would be an good idea per the previous paragraph.

dvdsk · 2024-06-17T10:49:06Z

Agreed.

So lets revert the Arc solution and then we can merge I think. You can largely just revert the last commit though there are some changes in there unrelated to the Arc stuff.

dvdsk · 2024-06-17T11:17:50Z

src/source/mod.rs

@@ -333,6 +335,13 @@ where
        skippable::skippable(self)
    }

+    fn trackable(self) -> TrackPosition<Self>


I like the name TrackPosition, it communicates that it has to do with the position of the source and tracking it. Would you be okay with changing the name trackable to track_position? Then the factory function clearly communicates that what is tracked is the position.

dvdsk · 2024-06-17T16:29:46Z

Merged! Thanks a lot for the work!

dvdsk · 2024-06-17T23:38:46Z

I changed the return type from f64 to Duration, let me know if you disagree on that then we can discuss it

Gusted mentioned this pull request Jun 9, 2024

Elapsed Duration functionality for the Decoder/LoopedDecoder #510

Closed

dvdsk reviewed Jun 10, 2024

View reviewed changes

Handle variable channels and sample rate

0e6d5a1

dvdsk reviewed Jun 14, 2024

View reviewed changes

src/source/position.rs Outdated Show resolved Hide resolved

dvdsk reviewed Jun 14, 2024

View reviewed changes

src/source/position.rs Outdated Show resolved Hide resolved

dvdsk reviewed Jun 14, 2024

View reviewed changes

src/source/position.rs Outdated Show resolved Hide resolved

dvdsk reviewed Jun 14, 2024

View reviewed changes

Refactoring

ca43ad0

dvdsk reviewed Jun 16, 2024

View reviewed changes

Revert Arc stuff

80277e0

dvdsk reviewed Jun 17, 2024

View reviewed changes

Better naming

16a1440

dvdsk merged commit fd9747d into RustAudio:master Jun 17, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TrackPosition to Source #585

Add TrackPosition to Source #585

Gusted commented Jun 9, 2024

dvdsk Jun 10, 2024

Gusted Jun 10, 2024 •

edited

Loading

Gusted Jun 10, 2024

dvdsk Jun 10, 2024 •

edited

Loading

Gusted Jun 10, 2024

dvdsk Jun 10, 2024

dvdsk left a comment

Gusted commented Jun 16, 2024

dvdsk Jun 16, 2024

dvdsk commented Jun 16, 2024

Gusted commented Jun 17, 2024

dvdsk commented Jun 17, 2024

dvdsk Jun 17, 2024

dvdsk commented Jun 17, 2024

dvdsk commented Jun 17, 2024

Add TrackPosition to Source #585

Add TrackPosition to Source #585

Conversation

Gusted commented Jun 9, 2024

dvdsk Jun 10, 2024

Choose a reason for hiding this comment

Gusted Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Gusted Jun 10, 2024

Choose a reason for hiding this comment

dvdsk Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Gusted Jun 10, 2024

Choose a reason for hiding this comment

dvdsk Jun 10, 2024

Choose a reason for hiding this comment

dvdsk left a comment

Choose a reason for hiding this comment

Gusted commented Jun 16, 2024

dvdsk Jun 16, 2024

Choose a reason for hiding this comment

dvdsk commented Jun 16, 2024

Gusted commented Jun 17, 2024

dvdsk commented Jun 17, 2024

dvdsk Jun 17, 2024

Choose a reason for hiding this comment

dvdsk commented Jun 17, 2024

dvdsk commented Jun 17, 2024

Gusted Jun 10, 2024 •

edited

Loading

dvdsk Jun 10, 2024 •

edited

Loading