Player restart proposal #699
Conversation
There was a problem hiding this comment.
Thanks for this proposal! It would probably be good if @maniac103 looked at this, too, as your change reverts some of what he did in #249.
|
I'll give this a try and report back ASAP. |
|
Looks like my use cases are working fine with this patch 👍 |
|
@maniac103's question made me re-think of a better solution. The initial one was partly made before I understood the issue with the reconnect bit/decoder restart in SB2 and it was a bit hacky anyway. Separating the Player::power() function in 2 blocks allows an unchanged feature of power() but the resume part can be called independently by Squeezebox::reconnect() when the player has been forgotten. At the end, this is all we need because Player::power() has been done in the Player::init() and StreamingController::playerReconnect() call is not needed. When the client has not been forgotten yet ($reconnect is defined) then the call to StreamingController::playerReconnect() will resume. Sorry for asking yet another round of test, but it seems better structured like that. @michaelherger, once/if this is approved, I can squash it for convenience. |
|
Thanks for looking into this, both of you! I'll let @maniac103 give his opinion before I try to understand what you're doing here 😀. |
|
My use cases still work fine with the latest patch. I'll leave the remainder of the review to you @michaelherger since my knowledge of the code base is pretty limited - aside from drive-by notices like the one above :-) |
|
I've tested the patch with SB2, which now resumes, great! Also tested with SB touch, which still resumes.
|
Did you power off your players?
This probably depends on your settings in Player/Audio.
And it wouldn't jump to Now Playing after 15s or the like?
Random Mix - I don't know why, but yes, this indeed was coded not to survive a server restart. For whatever reason. |
I've cut the power while playing, if that's what you mean. This is my use case, because the players are slave-powered by the stereo.
All players are set to "Pause at power off / Resume at power on", which matches my intention.
Just re-tested, in case I was too impatient: The SB Touch indeed transitions to Now Playing after playing a long ~45 seconds. The SB2 stays in Home.
Coded as in not implemented, or coded as in deliberately excluded? Where? |
|
Re SB2, this is strange b/c my classic transitions to "Now Playing" after a while as well. Now, wrt not resuming to the exact position, it is complicated. I can try to do something but I'm not sure of the result and it is really worth the effort? I feel it's a very unique use case. Can you gather a bit more feedback from others? |
I wouldn't expect the exact position to be resumed either. But maybe it depends on the use case - mine is music in my bathroom, and I turn that one on rarely enough to forget what was played when it was on for the last time ;-) |
|
I think I just find an option to make that happen fairly easily. Let me try that, I might update the PR later for your review. |
It's a deliberate decision: "If Random Play mode is active, we don't save the playlist". |
... done in 2007: 7282d23 ... is Triode still around? |
Unless he's hiding behind some pseudonym I have to say no, unfortunately he completely retired from the community. |
@philippe44 - are you still planning to apply more changes? |
|
Yes I am - I'm trying to find a way to restart from last position even in case of long disconnect - almost there. I just need to find the way to play the current track on the playlist from a given position (there is a seekdata in "playlist index" command, just need to remember how to find index of the current song... |
It might not work with streaming service that don't support timeOffset in seekdata
|
I've updated to the latest modification. It seems to have an issue with resuming on Tidal. For reason don't understand, the timeOffset cannot be turned into a byte offset although Tidal has been updated to support seeking. |
|
With #702, that should do... |
|
Thanks guys! Let's give this some exposure. |
|
I was about to write that I'm using it, too. :-) |
|
Thanks - We'll have to keep an eye on this one... |
You should have expressed your doubts a little earlier 😂 |
I mean I'm a bit nervous about some changes, so I stay on alert 😄 |
Change #699 changed the way how we initialize re-connecting devices. Previously the SlimProto state would have been initialized as a side-effect of calling `$client->stop`. With #699 we no longer do so all the time. Which resulted in SlimProto state to be uninitialized under certain circumstances. This change makes sure we initialize the SlimProto state whenever we receive a new SlimProto message and the state is undefined.
This is a fix to the changes introduced by Player restart proposal LMS-Community#699, LMS-Community#699. A client that disappears/disconnects for a few seconds/minutes before reconnecting may start playing immediately it reconnects, for no good reason. This change ensures that a client's playing state is recorded as soon as the client disappears/disconnects. The playing state is used to determine if the client should restart playback when it connects to LMS, so it is essential that it is recorded as soon as the client is disconnected. At present, the playing state (recorded in preference 'playingAtPowerOff') is not recorded until LMS starts to 'forget' the disconnected client. This will take place 5 minutes after 'slimproto_close', via a timer that calls 'forget_disconnected_client'. But if the client reconnects in the interim, '_hello_handler' will kill the relevant timer, and the true playing state is not recorded. It may, in fact, reflect a condition that existed some days/weeks in the past.
|
I'm having an issue with this when listening with the Radio Paradise plugin. Specifically, their "interactive" stream (where you can skip songs), haven't tried the others. When resuming, the internet radio station starts a current song from the beginning (fine with me), but LMS has memorized some previous position for the player when shut off. The display will continue from (I think) that position instead of from the beginning. It will even reach into the next song title while in fact still playing the first. This "time offset" will continue with further titles. In fact most of the time the title display is wrong. It gets fixed when actively skipping a song. I'm still using LMS v8.2.0 release with only this patch ontop, didn't dare nightlys. It is possible that I'm missing fixes done elsewhere meanwhile. Radio Paradise plugin is v3.1.1. |
|
@IDC-Dragon please confirm with 8.3. |
|
There is an 8.3 release? |
|
It's a "nightly" - new build whenever there's a change. But it's very stable at this point: https://downloads.slimdevices.com/nightly/?ver=8.3 |
|
Quickly tested, the issue remains. The "time offset" gives wrong playback position and makes it show the next title already. Side note: |
|
Quickly tested, the issue remains. The "time offset" gives wrong playback position and makes it show the next title already.
Oh, BTW (and this might be an important BTW): IIRC you can’t resume Radio Paradise interactive streams. Their streaming model is a bit specific. We don’t get single files per track, or one infinite stream (which you couldn’t pause either). But it’s a 1 to x tracks per file. If you only see this issue with Radio Paradise, then this likely is a limitation of that service implementation, rather than LMS itself.
|
|
I don't get the 1 to x... |
|
Please try to confirm whether this is a RP only issue or not. If it is, then I invite you to be the first to submit a bug report on https://github.com/michaelherger/RadioParadise/issues 😀. |
|
AFAICT it's Radio Paradise only, however my listening habits are limited. |
|
I'm not sure that either the 1 file per X tracks (typically 4) and the complicated guess that is made to assess position,I'm not sure there is a solution there... This resume is really a nightmare |
|
…t seeking At first I thought this was a [Radio Paradise problem](michaelherger/RadioParadise#6), but it affects any source which doesn't support seeking. Don't store the current position. Just start over.
This is a fix to the changes introduced by Player restart proposal LMS-Community#699, LMS-Community#699. A client that disappears/disconnects for a few seconds/minutes before reconnecting may start playing immediately it reconnects, for no good reason. This change ensures that a client's playing state is recorded as soon as the client disappears/disconnects. The playing state is used to determine if the client should restart playback when it connects to LMS, so it is essential that it is recorded as soon as the client is disconnected. At present, the playing state (recorded in preference 'playingAtPowerOff') is not recorded until LMS starts to 'forget' the disconnected client. This will take place 5 minutes after 'slimproto_close', via a timer that calls 'forget_disconnected_client'. But if the client reconnects in the interim, '_hello_handler' will kill the relevant timer, and the true playing state is not recorded. It may, in fact, reflect a condition that existed some days/weeks in the past.
|
Hi @mw9, does your commit additional fix the following issue, which I observed after the installation of logitechmediaserver_8.3.0~1658898470_arm.deb because the #699 is included?
Result: The SB Radio will be reconnected to LMS, but no playback is possible until the second SB Radio is powered on and connected too. |
|
thank you for your clear response. |
Well, I'm not so sure that it does. I did have a quick look after you first raised this. I can still replicate your original observation. White & Black Radio are synced: If I turn Black Radio off, then White Radio off, then Black Radio on, the Black Radio will come on in "Paused" state. It doesn't play (because it's paused). Until I turn White Radio on, then it starts. If I turn Black Radio off, then White Radio off, then White Radio on, the White Radio will come on and play. It is not in "Paused" state. If I then turn Black Radio on, it will play. And vice versa. I don't think this behaviour depends on which Radio is "sync master". I haven't investigated further. There's something slightly odd going on ! It may be worth remarking that "Synchronize Power" is set to "Power off/on separately". Although I guess that should be obvious... |
This is a proposal to handle differently #698 as well as allowing playback to restart for Boom, Classic and Duet (maybe Transporter as well) when they reconnect before LMS has forgotten them.
This last piece took me a while as the issue is that StreamingController is setting the slimproto 'reconnect' flag in the '_Continue' event. That flag tells player to not restart their internal decoder and IMHO, this can only be done if the player set the 'reconnect' bit on wlan_channellist in the 'HELO' message (this means that only the control channel was lost, so we should continue with what we have in buffers, no need to reset decoder). But in that case, Slimproto::_hello_hander calls Squeezebox::reconnect with the reconnect flag set which mean we don't use the client->playerReconnect which is the only case where '_Continue' event is used (and _Continue means send STRMs). In other terms, there is no case where STRMs is used but the player has set the 'reconnect' slimproto bit in HELO. Anyway, long story short, and I might be wrong, but I think this 'don't reset decoder' flag was never used properly, but as it was honored by SB2 devices, it caused them to fail when restarting before being forgotten.
Otherwise, I think this takes care of restarting the track in the playlist for all cases if a player reconnects either before being forgotten (5 mins) or after. I'm moved back to earlier version of some files, that includes using original API pattern for Player::power() and Player::init, so feel free to critic 😄