issue of streaming track fully streamed and adding one more

[philippe44]

This is WiP, please don't merge yet

Let's continue here. This is a proper attempt to solve the issue that happens with very large buffers. You know my opinion on that, but still, where you have a point is that it's not so much about large buffer, but about tracks that are small compared to the buffer size.

LMS currently handles "short tracks" in a sort of incomplete way which means that when N plays, if N+1 has already been fully buffered, then any action on N+2 (move/remove/insert) will be messed-up. The reason is that as soon as N+1 is streamed out, N+2 is requested and held into the streamer backburner. It is not streamed, but it's requested.

So if you do anything that impacts it, then it will fail because you can't really touch that backburner. Well, you can, but there are so many permutation and combination that I don't want to think about it. The other solution is to prevent it from getting into the backburner, in other words when N+1 is streamed, we don't request N+2, we delay the request to when N+1 will start playing.

It will probably work nicely except that really short tracks might now be messed up. The issue is that N+1 will be streamed of course, N+2 will not be requested and as soon as N+1 starts, N+2 is requested. But, if N+1 stops even before N+2 is available, then playback will stop.

I'm not sure which evil is worse...

[986ster]

I have this installed and have have run a couple of cursory tests that looked good. I'll exercise it more thoroughly later.

[klslz]

TestCase with patches applied:

Play Track1 >> Append to Queue Track 2 >> Append to Queue Track 3 >> Remove Track 2 >> switches immediately to Play Track 3 - TC failed

[philippe44]

This is because you probably don't use the patched version of squeezelite as well. See @986ster who has compiled it for his needs. There has been forever a bug in squeezelite that did not differentiate a flush from a quit. See the PR I've done for that.

[klslz]

Yep. You were right. I didn't have squeezelite updated on my main system.

I did some happy testing. Looks promising so far. Great.

[986ster]

I listened to a ton of music yesterday, and kept adding tracks, adding/quickly removing, and then adding other tracks. I've also started moving tracks around, and I haven't had one instance of it not working as excepted so far. So it's looking very promising.

[philippe44]

Now you'd have to look at the other side of the spectrum: really really short tracks, below 5s, probably around 1s. I've not made test yet with these.

[klslz]

What exactly and how do you want to get it tested? I volunteer.

[philippe44]

Per previous, post, a quick description of Slimproto so that you can see what type of tests are required:

Players can only handle 2 tracks in parallel : the one they stream (receive) and decode and put in the PCM output buffer and the one that they play from the PCM output buffer. Can be the same track, but maximum is one of each, not more. When player starts to play a new track, it sends an event TRACK_STARTED to LMS. When a track has been fully put into PCM output buffer, it sends a READY_FOR_NEXT event and when the PCM buffer is empty and it stops, it sends a STOPPED event.

Now, players don't control the fact that only one track can be streaming at the same time, if LMS sends more than one, they'll take it and it will mess them up.

In LMS, a key state machine is the Streaming Controller ("SC") that has the player's state (PLAYING, STOPPED) and receives the above mentioned events from player. The SC also holds its streaming state which can be IDLE (obviously), STREAMING (track is being sent to player), STREAMOUT (fully sent, but that one does not always matter, so let's forget it) and TRACK_WAIT (track has been requested to the source/provider but has not been received yet).

The SC is in charge of enforcing the 2-tracks problem so in normal mode, at the beginning, all is STOPPED/IDLE and SC requests the first track N to the source and the following happens (let's use track number N, N+1, N+2, ...)

1- streaming state is in TRACK_WAIT mode
2- track N arrives and SC moves streaming state to STREAMING and sends a "PLAY" request (strms message) to the player
3- when the player starts to play, it sends a TRACK_STARTED and SC moves player state to PLAYING
4- when player has sent the whole track N to output buffer, it sends a READY_FOR_NEXT to the SC
5- the SC then request the next track N+1 to the provider/source and moves streaming state to TRACK_WAIT
6- tracks N+1 arrives and SC moves streaming state to STREAMING and sends a strms message to player. At this point, the SC has 2 tracks in queue, one streaming, one playing
7-Then we move back to point 3- and so on

As I hope you see, there are 2 types of difficulties

1- At level 7, if player has a huge playback buffer, N+1 will be quickly acquired and it will send a READY_FOR_NEXT even before N+1 has started. The way the SC is handling that was imperfect because it used to request N+2 immediately to the source but it would remove it from the streaming queue, so it would not be sent to player until player sends a TRACK_STARTED event. Still, it's the SC has N+2 track in some buffer zone. Now shit happens when the user wants to remove/move N+2 or add something after N+1. at this point the SC does not know really how to "cancel" N+2 and it has already been requested to the source provider anyway (it should not touch N+1 of course). And there are tons of cases in the code where I would have to deal with that case and differentiate it from the previous PR I made where it's just N+1 being replaced while being streamed (remember here N+1 is fully streamed). What too many permutation and combination, too many risks of regression.

2- So the better option, and this is what I did, is to prevent the SC from even requesting N+2 to the source and wait for player to send the TRACK_STARTED event of N+1 and then only then request N+2. That works fine and in fact that even make some "hacky" code of LMS (referred to bug 5103) useless. Looks great, no? But this is where more shit happens and I need you to run some test. Think now about very short tracks. The problem is that if you don't request N+2 soon enough and wait for N+1 to start, in fact N+1 might either stop before you receive TRACK_STARTED event of more likely before the SC starts receiving N+2 because we only did that when we receive information that N+1 was started . So the player will send a STOPPED event while the SC's streaming state is TRACK_WAIT. As a result, although I think what I have now is that the player will restart, there will be a gap.

This is that case I need you to test extensively, short tracks (maybe about 1 sec) in all permutation and combination, playing 1,2 or 3 of these, surrounded by "normal" tracks. Also, uses cases like N is long, N+1 is very short and N+2 one or the other, but user changes the playlist while N is still playing. As I hope you can see now, there is room for you to imagine test cases 😄

[986ster]

Okay, so I need to create a bunch of super short test tracks that I can also definitively identify to know when it is and isn't behaving then. This is also going to be a challenge to be able to queue them up quickly enough, but I'm game for the challenge.

[philippe44]

You don't have to queue them manually, you can just use a playlist. I did with 5s tracks and it worked but I could not find the time to create a set of 1s tracks. Also, another test is 1 normal + 1 short. Then when "normal" is playing, replace short or add another one after normal and then after short. Same thing with 1 normal + 1 short + 1 ShortOrNormal and do the same changes after track 2 or after track 3...

[986ster]

Okay, so I downloaded a ton of really short spoken word numbers from this site: https://evolution.voxeo.com/library/audio/prompts/numbers/index.jsp and converted them to 16/44.1 flac files to have a fair test. I created a playlist of 10 of the short tracks and then a regular music track afterwards. What I found was the first 5 played perfectly, but then it seemed to stutter and there was a delay before starting tracks 6, 7, 8, 9, and 10, and then it flowed straight into the regular length track after 10.

So we definitely have a problem with really short tracks.

[philippe44]

How does it behave with current version of LMS?

[986ster]

Exactly the same actually, so it turns out we haven't actually broken anything, at least with regards to this particular test. That was going back to an off-the-shelf nightly 8.4 from prior to your OggFLAC mods, with Squeezelite set to use default buffers.

Since the original behaviour isn't perfect, do you want me to look for new problems that these changes may have introduced that weren't there originally, i.e. compare the versions?

[philippe44]

Exactly the same actually, so it turns out we haven't actually broken anything, at least with regards to this particular test. That was going back to an off-the-shelf nightly 8.4 from prior to your OggFLAC mods, with Squeezelite set to use default buffers.

Since the original behaviour isn't perfect, do you want me to look for new problems that these changes may have introduced that weren't there originally, i.e. compare the versions?

That's "kind of" good news as at least I did not introduced regressions. And yes, if you don't mind that would be great if you could look for such issues.

I'll think about it, but I don't think there is a solution to the stuttering problem you see. I was afraid my modification would make it worse, but I guess the change I'm adding is sort of "hidden". With a lot of small tracks, there is always a moment where LMS, the track provider, can not provide. The only option would be to have the players being able to gobble all tracks but that is not going to happen and even that, at some point the source (where LMS gets its data) will not be ready on time.

[philippe44]

I've added a special log that we should watch to be sure I did not forget a use case, but I'd say that this is good to go in 8.4 is @mherger wants to. In case something is wrong, and with all the tests you've done, we should hear about it and be able to trace it.

Otherwise, it solves a real issue when track N+1 is fully in the output buffer and N still playing. Of course, that issue is very visible for the huge buffer fans (and I won't make yet another comment 😄), but a track of around 30s would be impacted with a buffer of 10MB, which cannot be called abnormal values.

[philippe44]

BTW, @klslz, your feedback would also be welcome on that topic. I've put a decent amount of time and there is a real risk of regression for everybody else using LMS, so the more tests we do, the better it is.

[michaelherger]

@philippe44 Investigating #1214 I stumbled across this change: this would call _getNextTrack() twice. Is this expected? Or should the old (second) call be removed? If I disable line 1002 I wouldn't get the infinite "bad state" message locking up LMS.