Handle case where user moves/adds/removes tracks in a playlist while the next track is already fully streamed

[philippe44]

This replace my previous PR and extend to the case where users add/move/remove track N+1 when track N is currently playing and has been fully streamed (in other words, N+1 is already streaming). It's very visible for players with squeeezelite with large output buffers or with bridges (to work well with squeezelite, it requires a patched version)

So far, LMS (the StreamingController) knows that case and requires the player to do a "flush" of N+1, but then it does not handle the next steps properly.

1- It enters IDLE, and immediately requests the new track (call it A), which moves to TRACKWAIT and expects the callback when the track arrives to send a nextTrackReady event that should start the streaming of track A. The problem is that the players are not in "readyToStream" state and so the controller stays in TRACKWAIT until track N ends. Then the controller would re-check that there is something to stream and that would create a gap. Player objects are in ReadyToStream state when they have been reset of after the physical device have sent a "decoding finished" event to LMS or after they have stopped.

I had two option, the first one I did was to get player's response to the flush request strm 'f' (STMf) to make the player ReadyToStream again and re-use the StreamingController's event "ReadyToStream" so that it resumes. That works but it means that each player must send a STMf event for sure and so far, there are 2 bugs in SqueezeLite there: on "flush", it only sends STMf if it has an actual streaming connection. But that's not sufficient because the streaming of N+1 could also be fully done (and we have no connection to terminate), so we had to also verify there was no track N+1 in the outpufbuffer. In addition, the other bug with SqueezeLite is that is treats strm 'f' as a full stop request which is not at all the expectation (see PR). In addition, because an STMf is also sent every time on strm 'q' command, we had to differentiate that and not call ReadyToStream then (would make a state error)

The other option, which came to my mind while writing that doc, hence there is nothing better than explaining things 😄, is that when LMS request the player's object to "flush", it should simply declare it ready to stream. I can't think of a case where it's not accurate.

That second option is less changes in the StreamingController, in Player's object and in players themselves (it seems that Boom and SqueezePlay do the right thing on strm 'f' but I'm not even sure). So less is always better...

2- Unfortunately, there is more to that problem because if the user moves/adds/removes tracks multiple times after N while N is still playing (say, it adds A, B and C...) then the StreamingController was trying to "flush" while in "TRACKWAIT" (due to at least the first change) and that was not a valid transition, so that had to be changed.

3- In addition and in that special case, when moving/adding/removing track, LMS was not correctly re-calculating the index of songs. Precisely, playlist handling/shuffling should be done before the flush request, but the indexes to decide if we are in that special case added to be captured before. Then the flush can be done (it will get the "next" song based on actualized playlist. Currently, LMS verifies it's not in the case, then flushes, then changes the index. The problem is that flush calls a getNextTrack which works on non-updated index.

A last related but different issue was that if user adds a track after the last track has been fully streamed (StreamingController is IDLE but PLAYING), LMS would not realize that and would stop.

[philippe44]

See #963

[michaelherger]

How confident are you about this change? We don't want to ruin people's Christmas parties 😂.

[philippe44]

Not highly. I think it has low chances to break things, more chances to not fully work. The reason I'm saying that is because these changes are only activated when the streaming track is gone and these tests existed before. Just what happens then is new, and this does not happen often: you have to have a large buffer and change just the track(s) following the one that plays.

Having said that, I think we can be cautious and wait. Ideally some users would make the change on their own LMS and see. Frankly, I don't have a great track record of not messing up every time I can, so... 😄

[philippe44]

And it also needs ralph-irving/squeezelite#194 (or some version of my bridges that I've not released) so there is no rush. I just need to find a way to have more people ready to help for testing.

[986ster]

I can test this, with both standard and larger buffers, since I'm pretty familiar with how the issue can be triggered. I haven't compiled SqueezeLite since the days when it was on Adrian's Google Code site, so if you can give me a quick primer on the steps to clone it with git and compile it with specific options then I'll put it through its paces.

[philippe44]

Best is probably that you tell me which platform you want and I can build it for you. The LMS patches, you'll have to edit the Perl source code directly

[986ster]

Thanks. If you could do an ARVv8 version for a Pi 4 B with these build options (to match the version from my current DietPi distro) then that would be awesome:
Build options: LINUX ALSA EVENTFD RESAMPLE FFMPEG VISEXPORT IR DSD SSL LINKALL

I have the LMS patches from the other day ready to reapply, but I think you may have added some more, so I can re-apply all of those when I try the new SqueezeLite version alongside them.

[philippe44]

I've made a different LMS patch version that is more simple

[986ster]

Fantastic! So I don't need the LMS patches from the other day, that revealed the bigger problem, and just need the one here that is labelled as the simplified version?

[philippe44]

Sorry, was the wrong build, please use this one.
squeezelite.zip
To your question, the changes you must add for LMS are the ones in this PR

[986ster]

I just made all the LMS changes ready to test, but when I try to execute this version of squeezelite I'm getting the following:
-bash: ./squeezelite: cannot execute: required file not found

[philippe44]

What pi model do you have? BTW, I just disabled the "add after last track fix" because it was not covering enough cases. I'll work. On that today. Only the case where you move/add/remove tracks in the playlist are supposed to work. Not adding or moving around the last track

[986ster]

Pi 4 Model B, running DietPi.

[philippe44]

Right - I think this version has the proper structure in place. The previous one was too simplistic wrt how to handle the "last track moved or track added after last" issue. @986ster: I'm sorry but you'll have to re-patch. Now, on 8.4 or 8.3, you can simply get the 3 updated files from my branch and replace directly the ones in /usr/share/perl5/Slim/Player (of course, save the originals)

[philippe44]

Pi 4 Model B, running DietPi.

A crap - you're sure you're not using a 64 bits OS version (aarch64)

[986ster]

I am running aarch64.

I'll get the latest versions of the patches and reapply later. Once question I had though: in the Simplified version you added the line $client->readyToStream(1); to Squeezebox2.pm, but not to SqueezeSlave.pm. Did you mean to add it to that one as well?

[philippe44]

I am running aarch64.

I'll get the latest versions of the patches and reapply later. Once question I had though: in the Simplified version you added the line $client->readyToStream(1); to Squeezebox2.pm, but not to SqueezeSlave.pm. Did you mean to add it to that one as well?

Well, I don't think anybody still uses SqueezeSlave, do they?
squeezelite-aarch64.zip

[986ster]

Well, I don't think anybody still uses SqueezeSlave, do they?

I very much doubt it, but it was the fact that the SqueezeSlave code had one of the two mods in the Squeezebox2 code applied to it made me wonder if you were intending to update them both in parallel or not. Not an issue for my environment anyway.

I have some good news, and some sub-optimal news so far:

Added track to end of playlist during buffer, removed track, added different track -> played correct track without stopping
Added track to middle of playlist before buffer filled -> track was added after the one playing and played correctly
Added track to middle of playlist during buffer -> track was added after the track after the one playing (so it's being inserted in the wrong place)

I'll keep testing, and I'll also add streaming services into the mix rather than just local files once I get my test cases fully defined.

[philippe44]

Can you tell me how you add tracks in the middle of playlist? Are you using material? Do you have shuffle activated?

[986ster]

Can you tell me how you add tracks in the middle of playlist? Are you using material?

Standard LMS web browser with Additional Playlist Buttons enabled from the Interface tab in order to gain access to the Play Next button. Other times I use iPeng, but I'm keeping this testing as basic as I can at the moment to avoid introducing unnecessary variables yet.

[philippe44]

Weird, I've not been able to reproduce. Now, there is likely some remaining race condition if you change track in the last few seconds, that I will not fix it as it is probably a lot of pain to sort out. These PR are not so many lines of code, as you can see, but figuring out in a a15 years old code with no documentation was the dev wanted to do with all the corner cases that exist is pretty tricky. That's also the reason why I always try very hard to minimize the amount of changes I'm introducing.

[986ster]

I tried using a large buffer that could buffer an entire track, and I can repeat the behavior with the track/album being added in the wrong place from right after the track has started playing, so it seems to definitely be tied to whether the track is playing from the buffer or not, rather than it being tied to the very end of the track and a timing issue around that. Not sure if that helps since, as you say, this is old and sparsely-documented code.

[philippe44]

I'm not sure I've changed much of the part of the code that decides where a track is being inserted. What could be wrong is which track is being played, but I did not change the indexes of insertion. Maybe you can try with a small buffer and see if it still happens or not. Another thing with super huge buffer vs just large is that 1/ you can't even have a chance to do test with track N not fully streamed, 2/ there the extra use case I was referring to is that N+1 is fully streamed as well. So for these tests, I'd prefer a case where typically a track is fully streamed 30~60s before it ends (which is more realistic I think). So I move ~45s toward the end of track N, then I can insert/move/delete right after N.

I can try to reproduce what you see, but I'd need a bit more details about what you do: how many tracks in total, track N and +1 and added codecs, all durations, buffer sizes and very precisely how you insert the track, if this is an insertion vs a move. I'm also asking that because I can only insert at a given place using material. The classic UI adds at the bottom. Material adds at the bottom and then moves the track, so it's 2 steps, AFAIK.

NB: I used the term "streamed" because this is LMS' terminology: it means the track has been fully sent by LMS and the player has it entirely (it's called STREAMOUT). From the player PoV, there is another state where the track has been fully decoded and so it signals LMS when it is ready to receive a new track for DECODING and LMS can go back to load a track (TRACKWAIT) then streaming (STREAMING) it.

[mherger]

Thanks a lot, both of you!

[986ster]

Thanks guys. Feel better, @mherger.

[klslz]

I just tested the recent related changes for LMS and squeezelite. Wow. Great job #philippe44
Works great. Things like "add next" while working with large replay buffers work now.
I tested inserting a track and adding a track to playlist. Sequence is properly kept. And playback won't stop.
I do also think that full buffered tracks are no longer cut off / flushed before these are finished.

Again. Thx a lot. I was hoping to see such a fix for several years.

[986ster]

Now, if you have a HUGE outputbuffer, then we might have another problem that I did not take care of which is that the track 2 might be fully streamed as well when you do a next on track 3 to put it at 2nd position. Can you try ~50 MB buffer, 44kHz tracks and position around 30s before end of track (wait 5 sec) and then do the changes?

@philippe44, Remind me, was the change to allow flush from IDLE (streaming) because streaming track might be complete as well in commit 5974b61 supposed to fix this?

I've been running the latest LMS version which includes this merge for a couple of days, plus Squeezelite 1.9.9-1463, and found that I could be playing one track, add a second, add a third, and then only the second will play after the first one finishes, and it will stop rather than play the third, when the buffer is large enough to already have the second track in the buffer. I thought this was covered by the last change you made, but it was from a fuzzy point in my brain at the time!

I'm trying to figure out whether we were testing somehow got missed, or whether this was a scenario that didn't get covered.

[philippe44]

Yes, that was this commit and at least it solved one case. There might be more, unfortunately.

[klslz]

@philippe44

I am running your "flush" changes on LMS and SL 2.x for two weeks now.
It's working nicely.

One issue perhaps remained while working with large buffers. It can happen that a track gets flushed (cut-off) a couple of seconds before it ends.

It's not happening on all tracks. Usually it happens on classical albums (larger tracks!?!?). I can't really nail it. I just wanted to put it on record. I'll keep an eye on it.

[philippe44]

It's really unlikely that large tracks being cut-off has anything to with these changes. Are these tracks local or remote? If they are remote, it's likely and issue with "idle sockets" and I've added a few options in LMS network to handle that (see networking tab)

[klslz]

Yep. Remote streams - Qobuz. And it feels "random". I'll check out the "idle sockets". Thx a lot.

[klslz]

@philippe44.

FYi: When running following procedure:

1. Play track
2. Play Next
3. Append to Playlist

Done during playback of 1., 3. will not be played back. Playback stops after track 2.

[philippe44]

What size of output buffer do you have

[986ster]

This is the same new scenario I also discovered and mentioned several posts back, that I thought I must have tested before. In my case the buffer was large enough to absorb both tracks 1 and 2.

[klslz]

500MB - As you know, it's not dynamically allocated, so I try to fit all kind of filesizes in there.

I am thinking for quite some time how to adapt squeezelite so that it dynamically allocates exact the memory
requireds for a full track for its output buffer. If squeezelite knows the tracksize, that could easily be accomplished I guess.
But that's a different story.

@986ster - sorry for the redundant post - just confirming your finding. ;)

[philippe44]

Ok guys - this is a very unusual case. Nothing justifies having a 500 MB output buffer, it does not serve any purpose. I'm sorry but I will not further investigate that one, I've already fixed a lot of issues for what I'd consider a corner case, but that could still reasonably happen. Just reduce the output buffer to something like 10MB or below and all will be fine.

[klslz]

That's not quite right. There's nothing unusual about it. And that's not the issue of the topic here.
Squeezelite allows the variable buffer size and people are using it.
LMS and squeezelite are not coping with it properly. That's the issue.

I know of numerous people using it this way (I am blogging about this stuff since years).
Running large streaming or in this case output buffers, basically enforcing a bulk full track processing,
leads to a much lower continuous load on the system and on the network. That's what I consider a major benefit.
LMS meanwhile offers it on its side as well - as you know. Perhaps you ask Michael for his reasons.
Another example: Qobuz. The app bulk processes full tracks to the device cache. Same reason.

When it comes to the size of the output buffer.
A bit of math will do. Example: 192000 * 32 * 2 * 300 / 8 / 1024 /1024 ~ 440MB
And that's just 5 minutes.

Most people who run serious audio equipment run these rates and also have more
than enough RAM space on the streamers.
RPis with nowadays mostly 4G/8G RAM running just squeezelite come with plenty of RAM for the task.

All I am saying. This is not just a freaky setup.

[philippe44]

When triode did created squeezelite, the default size was 4MB. The only size you really need is to do a crossfade at higher sampling rate, so say 192kHz*8bytes*10s. And you are already way beyond any reasonable need for network hiccup handling.

LMS does not "allow" it, LMS just sends data and players handle it as they wish, as long as they comply with the slimproto. And yes, in slimproto, there is variable for outputbuf fullness which is a 32 bits int, buts not because there is a need for 32 bits, it's because of size of data in computing are 8,16 which are not enough so here you go 32. We, engineers are simple minds 😃.

Qobuz and some streaming service are fine or even prefer sending all quicky because it avoids them having many open sockets in parallel, which means more computers instances which mean more cost. That's all. I've made various upgrades to LMS to handle these cases, including the persistent and cache modes, so I'm pretty familiar with the topic.

Having a large buffer, beyond the crossfade does not change anything to the decoded data and the way the processing is done is totally irrelevant to the rendering of audio. Any system is capable of streaming and decoding in parallel. I have ported squeezelite on esp32 where the output buffer is 1.5 to 2MB...

Finally, many people can do what they want, it does not make it relevant, but if they chose to, of course they can. I'm not the buffer police 😄

[986ster]

I'm not going to get into any purported pros or cons of a huge output buffer, because that's to one's own on why you may want or choose to do it.

I was still running the 20/30MB input/output buffer combination I'd been testing with when I encountered this, purely by accident while listening to some music.

I think the reality is if you're close enough to the end of track 1 when you add track 3 then you only need the buffer big enough to accommodate track 2 and the last remaining part of track 1 before this issue will show up, so it could be a more common possibility of occurring that some of the definite corner cases that have already been fixed. If track 2 isn't particularly long then the odds increase.

That being said, there are plenty of other features that could be added and bugs that could be fixed in LMS that would also be valuable, so this isn't going to cause me to lose sleep, but if need be I'm happy to test some more.

[philippe44]

Indeed as you pointed out, there is an issue with 2 tracks when 1 is still playing and 2 is fully buffered but you add another one. As said, it's a corner case but these things have a high risk of introducing regression and making more harm than good and this can be very time consuming, hence I'm not keen to investigate it further. My plate is very full these days as well...

[philippe44]

You might want to replace, in Slim::Player::Playlist.pm

sub addTracks {
	my ($client, $tracksRef, $insert) = @_;

	my $playlist = playList($client);

	my $maxPlaylistLength = $prefs->get('maxPlaylistLength');
	
	# do we need to plan for restart of streaming?
	my $restart = (Slim::Player::Source::playingSongIndex($client) == Slim::Player::Source::streamingSongIndex($client)) &&
				  (Slim::Player::Source::playingSongIndex($client) == count($client) - 1);

with

sub addTracks {
	my ($client, $tracksRef, $insert) = @_;

	my $playlist = playList($client);

	my $maxPlaylistLength = $prefs->get('maxPlaylistLength');
	
	# do we need to plan for restart of streaming?
	my $restart = ((Slim::Player::Source::playingSongIndex($client) == Slim::Player::Source::streamingSongIndex($client)) ||
                                (Slim::Player::Source::streamingSongIndex($client) == 0)) &&
			      (Slim::Player::Source::playingSongIndex($client) == count($client) - 1);

Might work...

[klslz]

My LMS installation won't even start with above patch in place.

[philippe44]

I probably made a syntax error, like an extra parenthesis.

[986ster]

I tried this and it started up just fine (the parentheses do all match up), but it didn't actually make any difference to the particular scenario in my case.

I played track 1, added track 2, and then added track 3 while track 1 was playing, and it still stopped at the end of track 2.

I also tried playing track 1, added track 2, and then added an entire album while track 1 was still playing. Again it stopped at the end of track 2. This was the same behaviour as with the default regular Playlist.pm.

[philippe44]

ok, you can do that instead

				  	my $restart = (Slim::Player::Source::streamingSongIndex($client) == count($client) - 1);

That will work but I'm not sure it will not break something else. There will be log entry in the server because you are breaking the nature of slimproto which is one playing and one streaming track and only one.

[986ster]

Rather than breaking the slimproto rules, would it be possible to catch the scenario where it's going to either stop playing or return to track 1 (because repeat is enabled), and insert a check for whether any tracks had been added prior to stopping or going back to track 1? This would mean that the additional track(s) wouldn't get buffered immediately that they were added, but I don't see that being an issue.

[philippe44]

At the end, it would more risk of regression. After looking more carefully, I think all this means that those tracks will be considered as short track but it seems that LMS already has provision to deal with these.

[986ster]

Does this mean you think your latest code snippet is worth trying then, or is it a bit of a kludge?

[philippe44]

Does this mean you think your latest code snippet is worth trying then, or is it a bit of a kludge?

No I think it's worth trying, I think it might be good enough in fact

[986ster]

It's promising, and sort of works. But if you try to trick it by adding a third track, quickly removing it, and then adding a different third track it'll play the one you originally added instead of the replacement. I'm having a déjà vu moment with this, because that was one of the issues we had previously, and resolved for the particular scenario in which it occurred!

[klslz]

ok, you can do that instead

				  	my $restart = (Slim::Player::Source::streamingSongIndex($client) == count($client) - 1);

That will work but I'm not sure it will not break something else. There will be log entry in the server because you are breaking the nature of slimproto which is one playing and one streaming track and only one.

Just tested it.

play track1 >> play next track2 >> append to queue track 3 # OK - all tracks played back

There's another small issue though:

If the last track of a playlist is playing and you change the playlist with e.g. "play next" or you eg. "add two tracks and
you remove the first of them" ( funny testing) it happens that the ongoing replay gets cut off and the playlist switches to the next track.

[philippe44]

I looked further and yeah, this one is an absolute minefield. The reason is that, as I explained before, LMS is made to handle 2 tracks: a playing one and a streaming one - no more. The state machine does push and pull according to that logic. There is one exception which is short tracks where the track N+1 (short) might be streamed while N is still playing so the machine can put N+2 in the backburner.

Now, this exception is handled... as an exception because as a short track, there is very little chances the user can change N+2 before N+1is finished. So the handling logic is a bit of a hack. Now, by having huge buffers, you make everything short tracks and then you will hit every issue that was left by the hack.

I'm sure you're thinking that I should prevent the backburner mechanism then... well I can think about that, but that mean I will potentially break short tracks handling for everybody else.

Dealing with what happens when we remove/move/replace the streaming track was one thing, but dealing with that when the streaming track is fully streamed is a very different problem.

Last a small issue : N+2 has already been requested to the system, so if you use a protocol handler that only skips forward, that track is gone forever.

[philippe44]

Please move to that PR #985