spmn / vlc-win10smtc

Integrate VLC Media Player with Windows 10 System Media Transport Controls (SMTC)
GNU General Public License v3.0
46 stars 5 forks source link

Advertise changes to the timeline position #7

Closed ungive closed 2 weeks ago

ungive commented 2 weeks ago

Fixes #5. Haven't worked with VLC before so if you see anything wrong with it, lmk!

spmn commented 2 weeks ago

Thanks for the fix! It is in line with the one I had in mind. I have just some questions:

Why are last_advertised_position_timestamp, position_timestamp and last_advertised_position needed? It would be much simplier to post an update every time an INPUT_EVENT_POSITION event comes. The update frequency is low enough (250ms) that it does not require additional filtering logic.

Also, the current implementation does not work for me since abs(sys->position - previous) is always zero and sys->advertise is false, so no the update thread never gets to post time updates to SMTC.

spmn commented 2 weeks ago

Welp, I wanted to push a commit to your branch with the changes mentioned in the previous comment, but somehow GitHub automatically merged the PR too.

So, please let me know if the changes I've made in https://github.com/spmn/vlc-win10smtc/commit/6cbf4fcba7aee9a671f57cd1b26a77ef47dfa999 are ok with you.

ungive commented 2 weeks ago

Why are last_advertised_position_timestamp, position_timestamp and last_advertised_position needed? It would be much simplier to post an update every time an INPUT_EVENT_POSITION event comes. The update frequency is low enough (250ms) that it does not require additional filtering logic.

Without the filtering you will continuously update the position and especially LastUpdatedTime. To consumers of the SMTC API it will look like the user is scrubbing the timeline multiple times a second, so I added these filters to only advertise actual changes to the timeline, not changes imposed by only the time passing.

Also, the current implementation does not work for me since abs(sys->position - previous) is always zero and sys->advertise is false, so no the update thread never gets to post time updates to SMTC.

It worked for me when I tested it. I scrubbed the song and my Discord presence was updated with the correct playback position. I didn't specifically check the values returned by WinRT/SMTC, but it must have been working. When sys->position changes, abs(sys->position - previous) can't really be zero though, can it?

spmn commented 2 weeks ago

To be honest I am not that knowledgeable when it comes to consumers of that API. I always thought Windows (through the media flyouts that appear when you press change volume/song key combos) is the only consumer of that API.

What change rate is considered scrubbing by these consumers? Unless I've misinterpreted your code, you want to limit the updates to one per 1000us. With my change, the update rate is 250ms, which is 250 times slower. Isn't it better now?

I did not test the actual values returned by the API. I installed ModernFlyouts and looked at how VLC interacts with it. Without the changes, the song progress bar would always stay at 0, unless you manually update the position by clicking the progress bar either directly in VLC or from the media flyout.

When sys->position changes, abs(sys->position - previous) can't really be zero though, can it?

Yes, it can be. Note that previous is not really the previous time position -- you add a delta to it, which is roughly equal to the time passed since the last update.

ungive commented 2 weeks ago

I always thought Windows (through the media flyouts that appear when you press change volume/song key combos) is the only consumer of that API.

Other apps can consume that info to see what media is playing on the device, which is very useful!

What change rate is considered scrubbing by these consumers? Unless I've misinterpreted your code, you want to limit the updates to one per 1000us. With my change, the update rate is 250ms, which is 250 times slower. Isn't it better now?

I think you missed that you don't need to send updates in intervals, it's enough to send an update once every time the user clicks on the timeline. SMTC records not only the playback position at the time of advertising it, but also the point in time at which that position was reported. So e.g. 50 seconds for the position and 23:49:13 for the time at which that was reported. If it's 23:49:23 now you can calculate that the playback position is now 60 seconds, since 10 seconds passed.

previous contains this 60 seconds value, i.e. it calculates what the consumer would calculate to determine the current playback position. abs(sys->position - previous) < allowed_error_usec then checks if the actual position is significantly different from that calculated position, which would mean the user clicked on the timeline.

If you report multiple times a second you won't necessarily report any wrong data, but that point in time I mentioned above would always be a couple milliseconds ago. Consumers could think the user keeps clicking on the VLC timeline. I've "consumed" this data a lot while working on Music Presence and most players only advertise changes when the user actually chooses to click somewhere on the timeline.

Another issue that can occur when you report too often, is that the calculation above for the current playback position might yield slightly different values after each update, which could lead to consumers thinking the song was interrupted or scrubbed for a very short duration, while in fact it was playing continuously. Personally I'd try to update SMTC as little as needed.

I did not test the actual values returned by the API. I installed ModernFlyouts and looked at how VLC interacts with it. Without the changes, the song progress bar would always stay at 0

ModernFlyouts shows 0 for TIDAL too, when I play music, seems like a serious bug in their software to me. They don't seem to do the calculation I've mentioned above which is the obvious way how to calculate the current playback position if you look at the API docs. Not sure how they missed that. 😅

Yes, it can be. Note that previous is not really the previous time position -- you add a delta to it, which is roughly equal to the time passed since the last update.

It's 0 or close to 0 so long as sys->position doesn't change. But if it changes, then that subtraction yields a value wildly different from 0, which indicates the user clicked somewhere on the timeline. That's why I said "When sys->position changes".


You can observe the exact data with my little program: https://github.com/ungive/media-session-dump

AppUserModelId: vlc.exe
Title: Was du nicht sagst
Subtitle: <empty>
Artist: Lotte
AlbumTitle: Glück
AlbumArtist: <empty>
PlaybackStatus: Playing
Position (live): 60928ms
Position (fixed): 40511ms
LastUpdatedTime: 1728255416063ms
EndTime - StartTime (duration): 169013ms
MinSeekTime: 0ms
MaxSeekTime: 169013ms
MaxSeekTime - MinSeekTime (duration): 169013ms
PlaybackType: Music
TrackNumber: 0
AlbumTrackCount: 0
Genres:

I've made a release for it, so you can use it without having to build it: https://github.com/ungive/media-session-dump/releases

I can't observe any bugs with my changes, release is here: https://github.com/ungive/vlc-win10smtc/releases/tag/advertise-timeline-changes

ungive commented 2 weeks ago

It's 0 or close to 0 so long as sys->position doesn't change.

With "doesnt change" I mean that the position doesnt change only because of time passing.

spmn commented 2 weeks ago

I am aware that other apps can consume it, but I wasn't aware of any that actually do it. Thanks for pointing that out. Also, great projects you're working on. Congrats!

I think you missed that you don't need to send updates in intervals, it's enough to send an update once every time the user clicks on the timeline. SMTC records not only the playback position at the time of advertising it, but also the point in time at which that position was reported. So e.g. 50 seconds for the position and 23:49:13 for the time at which that was reported. If it's 23:49:23 now you can calculate that the playback position is now 60 seconds, since 10 seconds passed.

Ok, that timestamp code that I've deleted makes perfect sense now. I wasn't aware that most music servers do not regularly send timeline updates and instead they rely on the clients to infer the live position.

Personally I'd try to update SMTC as little as needed.

It makes perfect sense to keep the updates at a minimum and I agree with the general idea, but unfortunately writing programs often means working around bugs that are out of your control.

In this case, there are clearly consumers (Modern Flyouts) that do not calculate the live position and rely on the music server to provide the correct time. Spotify and Apple Music seem to know that too since they also update the playback position regularly.

As a middle ground I would settle on updating the position once per second to work around buggy clients. How does that sound for you?

Now, I am just curious -- why exactly do you need to detect scrubbing for? What action do you take based on that information? :D

ungive commented 2 weeks ago

Also, great projects you're working on. Congrats!

Thanks, I really appreciate that!

I wasn't aware that most music servers do not regularly send timeline updates and instead they rely on the clients to infer the live position.

That's not what I meant, the local music player of course knows the current timeline position at all times (it's what's playing the music).

From how I understand it, the contract of the Windows API is that consumers have to infer the live position using the position value and the position timestamp. There is no other way to know the actual live position. The Position field in SMTC is not meant to contain a continuously updated live value.

In this case, there are clearly consumers (Modern Flyouts) that do not calculate the live position

As a middle ground I would settle on updating the position once per second to work around buggy clients. How does that sound for you?

Imho that is not how the API is meant to be used and designing around broken consumers isn't the way to go, but in effect it likely won't break anything! If it does, it can always be fixed later. Feel free to implement it like that!

An alternative might be to implement it like I did, make a branch with your changes, you can use that then with the flyouts app and you could open a bug report (if there isn't one already) so they show the correct live position. I understand you use that app a lot and want it to work.

Now, I am just curious -- why exactly do you need to detect scrubbing for? What action do you take based on that information? :D

Whenever the live position changes I update the Discord presence to reflect it. It basically is in absolute sync with the media player's timeline :)

spmn commented 2 weeks ago

That's not what I meant, the local music player of course knows the current timeline position at all times (it's what's playing the music).

That's what I meant too. My initial assumption was that music servers (or players, I used server because ... well, they act as servers in this case) are supposed to send position updates to SMTC regularly. I understand now that is not the case for most media players ... unless I again misunderstand things.

I understand you use that app a lot and want it to work.

Yeah, that application is my only "consumer" for the API and self building and sideloading Metro/Store/UWP/whatever-new-name apps is not something I would like to do.

Whenever the live position changes I update the Discord presence to reflect it. It basically is in absolute sync with the media player's timeline :)

Then I don't get whats the big fuss about limiting the position updates :-?. It would work just as good with no unnecessary updates or with many updates. I am not advocating for the latter, but my first impression from your previous comments was that frequent updates would break somethiing.

ungive commented 2 weeks ago

Then I don't get whats the big fuss about limiting the position updates :-?. It would work just as good with no unnecessary updates or with many updates. I am not advocating for the latter, but my first impression from your previous comments was that frequent updates would break somethiing.

I'm just a pesky little perfectionist 😭

When something is designed one way (Windows API: "please only update when you need to"), used another way (VLC plugin: "updating every second, I just want this Flyout to work!") because a different party uses it wrong (Flyout: "How does the Windows API work exactly?") it bugs me in the back of my mind :')

Plus: I think we also both have a conflict of interest, imo this VLC plugin should be "generally correct", i.e. it should adhere to the API contract (update as little as possible) and buggy consumers should go fix their bugs, while you want to make sure it also works in your specific case.

Maybe you can add a checkbox in the settings to update it frequently for "buggy consumers"? Probably unnecessary work though, it really doesn't matter at this point 😆