Fix: songs not playing fully/skipping to next before the end #3
Conversation
This Fix is to limit the watchdog's actions on remote stream read operations only. The watchdog will no longer stop the worker thread when waiting for LMS to read data.
On windows, when streaming remote sources (e.g. spotty), playback buffer (stream controller, player, etc) would fill quickly and not request more data for a duration that caused the socketwrapper's WDT mechanism to trigger. This is a simple fix that introduces a new boolean indicator inside of the stage structure:
` BOOL bIsWriting;
Stage threads update the flag during both the read phase (bIsWriting=false) and the writing phase (bIsWriting=true). This allows the main socketwrapper's thread to figure out if a given stage thread is reading from a remote socket, where the wdt applies, or if it is waiting for the stage thread to write back to LMS., where the wdt should not apply.
if (info[i].fIsWorkerThread) {
if (0 == info[i].WatchDog _**&& !info[i].bIsWriting**_) {
stderrMsg("Watchdog expired - Thread for step %i stalled.\n", i);
if (bWatchdogEnabled) fDie = true;
}
info[i].WatchDog = 0;
}
|
Thanks @sle118 - this does indeed look like an easy enough change. Could we risk to run into situations where the watchdog would indeed have had to kick in? I'm not too familiar with this code... what would happen if LMS disappeared while socketwrapper was active? |
|
It's been over 10 years but I think the watchdog was added when using mplayer and AlienBBC. IIRC It was added because under some conditions would be i/o blocked and so processes were not shutdown. This left zombie Mplayer & socketwrapper instances. I think the the conditions were either (i) changing live station and (ii) pausing live streams. I'm surprised that a watchdog problem has started now. Mplayer had been used to be play RealAudio BBC live and "Listen again" streams for many years even newer player with large buffers such as Touch. I'll have a look at the code later. In the meantime I suggest checking that the fix doesn't affect playback of live streams (e.g. transcoding a radio station) where
|
|
Minor point - SW_ID needs to be updated to "1.12" |
|
I'm still working my way back into the code. However, AFAICT the new flag is also set when I/O is between processes when there are more than 2 exe in transcoding. Is that deliberate ? |
Good point; this is something I missed during my analysis of the code, as I didn't have all the possible scenarios in mind. Thanks for spotting that. Alternatively, and to preserve the semantics meaning of the new status variable, I could leave seeing/removing the flag as is, and add another condition in the wdt itself.
Let me know your thoughts |
|
So I went ahead and changed the version string to 1.12beta. I also implemented the additional criteria which keeps the wdt active during writes if the output is a socket. I haven't tested yet, but I suspect that this will circumvent the fix altogether, at least for Spotty. Typical command line to wrap Spotty is as follow:
This line shows that fOutputIsSocket will be true for the output side of Spotty, and that adding the new condition will end up maintaining the wdt active. I would thus recommend against having the wdt active on the output side for any scenario (i.e. aligned with my initial PR logic). |
|
With the complex "if", maintenance of this code may not be easy - two flags tested by logical OR of them NOTed (especially if I have to look at it after 5 years). Please add more detail to your 1.12 comment especially noting the flag you introduced. I've done a quick test of your original mod and watchdog still seems to work (albeit a bit slower) with stalled streams with STDIN. I have yet to test with URL input but Spotty uses URL so I'm hoping you already tested that watchdog still works with Spotty. I don't know of any current application which needs the #PIPE# construct so I have not tested it. As socketwrapper is used in various transcoding situations on Windows from XP to 10 - we'll need to be vigilant to see if users see odd Windows problems - possibly excess memory / processes if socketwrapper does not tidy up. |
|
@sle118 I know it's been a while but do you still have your last changes for this PR? Your final comment on Nov. 4 indicates that you made additional changes after the PR commit on Oct 27. We've started working on updating the windows build of LMS to use a recent perl and I thought it would be a good opportunity to apply your changes to socketwrapper. |
|
It's been a long time. I don't read into my Nov 5 comment about another change. Windows is not my main system so I'll have to see if I have a backup of socketwrapper dev from 2019. |
|
Thanks bpa. I was referring to the comment Sebastien (sle118) posted on Nov 4th just above yours, which mentions that he went ahead and made additional changes. I know it's been a long time thanks for checking. |
There was a problem hiding this comment.
Thanks for removing the other changes.
Is the dropped brace at the end of the if on line 607 in the untangling changes commit afbf58e on purpose?
|
In a forum PM discussion with @sle118 he confirmed that the dropped brace was an error and unfortunately the PR doesn't compile as a result. |
|
I've been testing socketwrapper.exe 1.12beta here: https://forums.slimdevices.com/showthread.php?111923-Announce-Spotty-4-0-integrate-local-library-with-your-Spotify-collection-(LMS-8-)&p=1049763&viewfull=1#post1049763 Unfortunately I am getting problems. I don't know if I am doing something wrong but I wanted to point it out here in case somebody can chime in with things to test, logs to activate, etc. |
|
I've opened a testing discussion on this: https://forums.slimdevices.com/showthread.php?116078-Spotty-and-socketwrapper-1-12beta&p=1049809#post1049809 |
|
(sorry, posted in wrong thread, now I can't delete it...) |
This Fix is to limit the watchdog's actions on remote stream read operations only. The watchdog will no longer stop the worker thread when waiting for LMS to read data.
On windows, when streaming remote sources (e.g. spotty), playback buffer (stream controller, player, etc) would fill quickly and not request more data for a duration that caused the socketwrapper's WDT mechanism to trigger. This is a simple fix that introduces a new boolean indicator inside of the stage structure:
` BOOL bIsWriting;
Stage threads update the flag during both the read phase (bIsWriting=false) and the writing phase (bIsWriting=true). This allows the main socketwrapper's thread to figure out if a given stage thread is reading from a remote socket, where the wdt applies, or if it is waiting for the stage thread to write back to LMS., where the wdt should not apply.