Send playback position as player event

[fragsalat]

Fixing #1493

Hey there, I did an example implementation how the playback position could be sent to the user of the Player struct.
The change should be backward compatible and I did throttle the sending of events by only informing every 250ms.
Please tell me in case you prefer a different solution or would like to change something else.

[photovoltex]

I still don't think this is a very good idea, especially every 250ms and with no option to change or disable it.

There are event's (Playing, Paused, Seeked and PositionCorrection) which provide a info about the current position. They also tell you if the position will change, so you can update the given position incrementally until a new event happens that updates the position.

Anyhow, could you please explain your use case in detail so that I can better understand your situation and requirement of this addition?

[fragsalat]

I'm fine to make this configurable. I wasn't sure if I should add a prop to PlayerConfig or create some kind of callback setter function to only notify the callback.

My use case is that I have built a Music player for my son where he can play preselected Tracks. Part of every player are controls like play, pause, next track, prev track and a progress bar with the option to seek to a specific point in the track. Similar to the spotify interface.
I did log the current emitted events and there is none telling me the position constantly. The correction event is only under certain circumstances emitted. Playing, Paused, Seeking are only once sent after the trigger play, pause, seek have been emitted.

[photovoltex]

Yeah, we don't send this position constantly, because it seems like an information overload. There is also no need to do so when you know if the playback state in my opinion?

For example you could write a simple loop like the following:

required packages: librespot (git version), tokio, log

async fn progress(mut channel: librespot::playback::player::PlayerEventChannel) {
    use librespot::playback::player::PlayerEvent;
    use std::time::Duration;

    const ONE_SEC_INTERVAL: Duration = Duration::from_secs(1);

    let mut is_playing = false;
    let mut progress = Duration::default();

    loop {
        tokio::select! {
            channel_event = channel.recv() => {
                match channel_event {
                    None => break,
                    Some(event) => {
                        progress = update_position(&event).unwrap_or(progress);
                        is_playing = update_playing(&event).unwrap_or(is_playing);
                    }
                }
            },
            _ = tokio::time::sleep(ONE_SEC_INTERVAL), if is_playing => {
                progress += ONE_SEC_INTERVAL;

                let min = progress.as_secs() / 60;
                let sec = progress.as_secs() % 60;
                log::info!("progress (playing {is_playing}): {min}:{sec:0<2}", )
            }
        }
    }

    log::info!("progress tracker stopped");

    fn update_position(event: &PlayerEvent) -> Option<Duration> {
        match event {
            PlayerEvent::Playing { position_ms, .. }
            | PlayerEvent::Paused { position_ms, .. }
            | PlayerEvent::Seeked { position_ms, .. } => {
                Some(Duration::from_millis((*position_ms).into()))
            }
            _ => None,
        }
    }

    fn update_playing(event: &PlayerEvent) -> Option<bool> {
        match event {
            PlayerEvent::Playing { .. } | PlayerEvent::Stopped { .. } => Some(false),
            PlayerEvent::Paused { .. } => Some(true),
            _ => None,
        }
    }
}

Adding the position update event wouldn't do any good from my point of view. @roderickvd, @kingosticks do you have any opinion on the topic?

[kingosticks]

I think a way to query the current position is really useful and a noticeable omission from our API. What you have there will drift from the actual position over time, it'll be more noticeable for long songs, probably unusable on a busy system when that tokio timer doesn't get to fire monotonically.

For what it's worth, I have previously tried to keep position state in this way for a player. It didn't work very well and drifted surprisingly quickly, the solution was to periodically query for the actual position.

[fragsalat]

I experience the same. The simple fact that the processing of the data consumes time and is not realtime already leads to differences between a tokio timer. I could not achieve an accurate result as well. With my previous solution using spotifyd I did poll the spotify API every 10 seconds as the API call is also quite expensive and slow. With librespot this works way smoother and a progress update should be doable imo :)

I did change the solution to a opt-in by setting a progress callback. Does this solution fit better in your opinion? I have to admit, that it feels like a broken design mixing event consumption to get the duration and callbacks to get the position^^

Btw, this is my code using the player right now

let progress = Arc::new(Mutex::new(Progress::default()));
        {
            let progress = progress.clone();

            player.set_progress_callback(
                Box::new(move |position_ms: u32| {
                    warn!("Position: {}", position_ms);
                    let mut progress = progress.lock().unwrap();
                    progress.position = Duration::from_millis(position_ms as u64);
                }),
                Duration::from_millis(250),
            );
        }
        {
            let progress = progress.clone();
            let player = player.clone();
            spawn(async move {
                let mut channel = player.get_player_event_channel();
                loop {
                    let event = channel.recv().await;
                    if let Some(event) = event {
                        match event {
                            PlayerEvent::Stopped { .. } => {
                                let mut progress = progress.lock().unwrap();
                                progress.duration = Duration::from_millis(0);
                                progress.position = Duration::from_millis(0);
                            }
                            PlayerEvent::Seeked { position_ms, .. } => {
                                let mut progress = progress.lock().unwrap();
                                progress.position = Duration::from_millis(position_ms as u64);
                            }
                            PlayerEvent::TrackChanged { audio_item } => {
                                let mut progress = progress.lock().unwrap();
                                progress.duration =
                                    Duration::from_millis(audio_item.duration_ms as u64);
                                progress.position = Duration::from_millis(0);
                            }
                            PlayerEvent::EndOfTrack { .. } => {
                                let mut progress = progress.lock().unwrap();
                                progress.position = progress.duration;
                            }
                            _ => {}
                        }
                    }
                }
            });
        }

I basically store the duration and position in a object which I can poll every second from my progress bar update interval.
While this callback approach work, it feels a bit poluted. But I won't complain as long as the goal is achieved 😄

[photovoltex]

Hmm, yeah. Maybe I was a bit to focused on "everything is already there what you need to get the correct position". Sometimes an update might just be a simpler and handier solution then one might think.

@fragsalat You didn't had to redesign the whole thing (especially if it doesn't fit well in the existing flow/logic). As is (with the new changes), it seems fine, but if you did like the Event solution more (because, well it did seem to fit better overall), then just change it back and add an option to opt-in the position updates (probably bool or Option<Duration>) to the PlayerConfig.

I also just noticed you mentioned that whole topic with the timer already in the issue 🫠. I'm so sorry for all that back and forth then.

Just add me as reviewer when you are done with the changes and I will give it a look again. And also thanks for the submission^^;

[fragsalat]

I mean you are the maintainers and I would seek your opinion here :)
Do you prefer the event approach, the callback or even something else?
I'll revert then the last commit and add the configuration option

[roderickvd]

I'm not too much a fan of sending messages every so often, which seems wasteful.

Instead, I'd vote for either the callback route, or storing the timestamp gotten from each next_packet call. That'd be a bit lower entry than a callback, but, then I think it'd be wise to refactor the way we get and send position_ms overall between the player and its controller.

[fragsalat]

That was another option, to store the position in the PlayerInternal and make it available via a getter in the Player struct. I could add this and leave the refactoring later to you if you don't mind :)

PS: Are you doing squash merges or merge commits? If not squashing I would cleanup the commit history in this PR.

[roderickvd]

Policy is to squash merging, and keeping PR history intact for ease of reviewing.

[fragsalat]

Did for now revert to the event approach with opt-in config. I figured out the PlayerInternal is not easily accessible from the Player struct. There is just a channel into the player thread but none in the other direction which could allow me to access the position. I guess thats part of the refactoring you were speaking of.

[photovoltex]

Looks good so far (as far as the current construct allows it to be at least hehe).

[roderickvd]

Nice and low key implementation! Don't forget to update the changelog and - down the line - the wiki.

[fragsalat]

Nice and low key implementation! Don't forget to update the changelog and - down the line - the wiki.

The changelog already has an entry in this PR and in regards to the Wiki I'm not sure as the Wiki does not have information related to the usage as library but only to the usage of the standalone binary. As your team decided to not offer this feature for the binary I wouldn't know what to update there. So if you have a hint where to update, I'm happy to do so :)

[roderickvd]

Yes, you are right. Thanks.

[fragsalat]

Awesome. I wanted to thank you all for this great collaboration and your active maintenance and effort in this nice library <3