Rework player progress reporting

[Maxr1998]

Still need to test this thoroughly and fix one added TODO before this is ready for a full review. Wanted to get this out early nonetheless.

On to the basic ideas behind this change: currently, the player reports playback stop events after the track index changes, with no easy way to know the previous state.
For that, it subscribes to _player.currentIndexStream, which simply maps the (distinct) index from _player.playbackEventStream. So, my idea was to drop this index stream completely, and instead handle the changes in the _player.playbackEventStream manually by comparing the new and previous index. This comes with the advantage that of knowing the complete previous state as well in that handler.

I also heavily rewrote generatePlaybackProgressInfo and split it into three functions.
The original function still handles the reporting, but now receives all attributes as parameters. No default handling, no reading information from the player anymore.

The two new functions handle the following cases:

• the default case where we simply want to publish the current playback progress (e.g., a presently running track progressed a certain duration), where all data is resolved directly from the player.
• the playback start/stop case, where we can provide the specific media item and playback state that was valid for this item.

[Chaphasilor]

Hi @Maxr1998, you might also wanna check out my changes for the upcoming redesign at https://github.com/Chaphasilor/finamp/tree/better-queue!

I completely rewrote the queuing system to be more flexible, and have a separate service for playback history that also handles playback reporting. I hope you didn't spent too long on the rewrite 😅

Hopefully we can integrate some of your improvements there, if necessary?

[Chaphasilor]

FYI, the new QueueService I implemented there keeps track of the current state and queues, and the PlaybackHistoryService is passed the current QueueItem whenever it changes. Upon change, it reports the old item as stopped, before replacing it with the new item and reporting that as started.
Should also be pretty flexible wrt looping, etc.

[Maxr1998]

Hi @Chaphasilor!

Unfortunately I didn't see your changes before making my own. I looked through some of your recent commits, the history service seems really useful. Very excited for that already, I thought about a feature like this myself in the past.

Regarding the new queue system and playback reporting, I believe it still has the same problems as the current main branch, specifically those outlined in #87 and handled by my workaround in #200. The root cause is a race condition where using the current player state in the player event handler may return mismatched data if the event or generated PlaybackProgressInfo are too old, i.e. were recorded before the track was changed and the player started a new track.

I think we could work to integrate my changes to the new system as well.

[jmshrv]

This looks very nice! We could still get it into main, since the redesign changes won't be in a full release for a while.

[Maxr1998]

Sounds good to me! Any idea about the volume reporting? Should we just default isMuted to false if we don't have the info.

[jmshrv]

Yeah, we could just leave that out if just_audio doesn't let us get to it.

[Maxr1998]

Alright, I'll do the changes later then!

[Chaphasilor]

The root cause is a race condition where using the current player state in the player event handler may return mismatched data if the event or generated PlaybackProgressInfo are too old, i.e. were recorded before the track was changed and the player started a new track.

Looking at your PR description from #200:

stop events submitted to the server seem to use the playback progress of the following song that's already playing, which causes scrobbling plugins to drop the listen.

I believe this shouldn't be happening with the new system because the PlaybackHistoryService actually get's a copy of the MediaItem, and uses the old media item to generate the stop event, even after the new item is already playing. But maybe I'm missing something :)
Once the redesign beta is live I'd appreciate if you could test it out and let me know if there are any issues, I'd be happy to fix it!

Edit: nevermind, seems like I was lazy and used the old implementation for now. This will be changed later on to reflect my comment above 😅

[Maxr1998]

Had to make some more testing because the player doesn't update the playback state as often as I'd like (sometimes lagging behind over two minutes if the app is in the background...).

Thus, I reverted that extra workaround for now. The current behavior should work pretty well and is ready for review.

[Maxr1998]

Found a better workaround, now it's fine 👍

[Maxr1998]

I've been actively using this for a while now and can confirm it's nearly perfect. There are still some conditions where duplicate playback events are sent, but those are quite rare, thankfully, and otherwise the events are sent very reliably (as long as the network connection also is).

[Chaphasilor]

That's awesome to hear! I think this could be merged in then, it won't be worse than the old system ^^

Also, do you know if it's possible to send playback events after-the-fact, i.e. caching them in offline mode and then retro-actively pushing them to the server? That would be super neat!

[Maxr1998]

Unfortunately, the Jellyfin API doesn't really support this at the moment. You can technically do this through the markPlayed API but that's kinda hacky and not the default on playback reporting plugins (read more here).

The best solution would be a stop timestamp on the playback stopped event or an additional activity submission endpoint in Jellyfin that playback reporting plugins could use then, but that's a breaking API change which I doubt would arrive anytime soon.

[Chaphasilor]

Hmm, that's a bummer. I think proper playback statistics are very important and educational, and help with curating personalized suggestions. But I'm biased (created Jellyfin Rewind :D)

One thing that I would really like to see at some point is the ability to submit a parent ID of a playback item, for example an album or a playlist. I created an issue for it in the Playback Reporting repo, but never received a reply. Maybe you have some more influence there?
But that's a different discussion. Maybe we could discuss it in the forum? :)

[jmshrv]

Merging :)

As for the idea of caching for offline mode, I think just throwing events at Jellyfin will suffice. We could add an option to attempt to send in offline mode, so that people who only ever use offline mode can still regularly update their playback (and cached events won't be too inaccurate)

[Chaphasilor]

I'm not sure what exactly you mean by "just throwing events at Jellyfin". Once Jellyfin supports that? Because submitting the event after-the-fact would cause problems...

[jmshrv]

Would it? We could just "replay" the events, i.e., send a start event and an end event (or wherever the song was stopped). Ideally we'd be able to say when the event happened, but we can't right now.

[Chaphasilor]

Ideally we'd be able to say when the event happened, but we can't right now.

yeah that's the problem. we could track that it was played at all, yes, but the playback duration would be screwed up

Edit: or can we specify the position for the stop event?

[Chaphasilor]

@Maxr1998 I'm currently porting this over into #484 and just have two questions:

1. Am I understanding it correctly that this will still not send a new playback event if a single track is looped? If yes, any idea how we could approach this?
2. Right now it's not supported, but theoretically, if the current track were to change index within the queue (i.e. dragging the track to a different spot in the queue), should a new playback state be reported? I think that would only make sense if we report the queue as well...

[Maxr1998]

Unfortunately not. I wish the used audio player library would expose a few more events.. on ExoPlayer (which is wrapped by this), a looped track should be much easier to detect.

I agree that if we send a queue, position changes (of any track in the queue) should report the new queue to the server.

[Chaphasilor]

Thanks for your input. My first shot at this is at ce5eee2 if you would like to take a look.
There still are a few issues, and because the queue service still has a few kinks to iron out not everything can be perfected yet. But I'd love to hear if you think this fixes your last.fm issues!