Optimistic update to controls.position in try_seek

[lautarodragan]

Hi! Big fan of Rodio here. I've been using it in a project of mine for about a month and absolutely love it. Thanks for maintaining it ❤️

I'm using this change and it's working, but I haven't thoroughly tested it, nor deeply thought about possible side effects, and I'm pretty new to Rust... so I hope this PR isn't too bad 🙏

The goal of this PR is that the following no longer happens:

let pos_a = sink.get_pos();
let target = pos_a + Duration::from_seconds(30);

sink.try_seek(target).unwrap();

let pos_b = sink.get_pos();
// At this point, pos_b is usually pos_a + ~5ms, not target

thread::sleep(Duration::from_millis(20)); // 20ms = arbitrary number, bigger than the hard-coded 5ms + some (exaggerated) room for imprecision.

let pos_b = sink.get_pos();
// At this point, pos_b is, roughly, target

With this PR, we won't need the thread::sleep(Duration::from_millis(20));. Since the try_seek blocks, get_pos should return the correct value immediately after calling try_seek (if it succeeds).

What comes to mind when I think of what this change could break is: is anything relying on controls.position not changing at all here? As far as I can see, we only read this value in pub fn get_pos(&self), and don't do any math with it, so I guess it should be fine.

Maybe there could be a race condition here, if periodic_access happens to run after TrackPosition.samples_counted has been updated with the new position but before we get to run *self.controls.position.lock().unwrap() = pos;, or, more generally, if Source.try_seek() isn't in sync with TrackPosition.next(), in which case we'd overwrite the correct value with a less-correct one. But, even then, wouldn't that error be better than the current one?

Also now try_seek locks on controls.position, but I think all read/writes from/to it are pseudo-atomic (lock, read/write, release). I can't think of a way this could dead-lock, but I n ever did any professional work with threads.

[dvdsk]

The call to recv() blocks until try_seek has completed. Which means the underlying sources have all executed try_seek. Most do not do anything like stoppable or amplify but some need to do some work. For example the speed Source (you can view the sources as a stack of layers one atop another) modifies the position so any source below speed works with a different position. Trackposition is also part of this stack of Source objects and its implementation of try_seek modifies the position it holds internally.

Once every 5 millis the position the sync reports gets synced with the one in Trackposition. So an update directly after Sink::try_seek is a good idea!

This whole design with the update every 5ms should be overhauled. But to be honest that has been on my list for about a year now, and I have still not found the time.

Your PR is a minimal change that fixes one of the issues with the 5ms update design, very nice!

There are just two more things that you need to do and we can get this merged:

1. run cargo fmt (thats the test that fails)
2. add a line to CHANGELOG.md with a bugfix (top of the changelog explains the format)

Thanks!

[lautarodragan]

Thanks for reviewing the PR and the thorough explanation ❤️

Just pushed the CHANGELOG and cargo fmt changes. Does that look ok?

[dvdsk]

perfect! have a great day 👍