feelfreelinux
commented
1 year ago Hi, thanks a lot for your changes! The proposed solutions make a lot of sense, happy to merge it :)
Closed philippe44 closed 1 year ago
feelfreelinux
commented
1 year ago Hi, thanks a lot for your changes! The proposed solutions make a lot of sense, happy to merge it :)
As discussed, this is the cspot part of the proposed changes (see also Bell's https://github.com/feelfreelinux/bell/pull/33)
It compiles and run under Window and Linux, including with or without codecs. Most of the changes are to handle the case where the PCM buffers are large enough so that there is an important time gap between the event when a new track is detected (in the chunk feed callback) and when it is actually aired (heard)t. It creates the following issues
1- Track fully received long time before it ends This directly impacts cspot (and I guess euphonium). The issue is that when TrackPlayer reaches EoF for track N, it requests track N+1 which replaces its track N Vorbis' context. Although N is still being aired, if the user wants to seek, it will fail as it is trying to seek on Vorbis context of N+1. I think the best solution is to create an "airing" status that combines isNextTrackPreloaded and isRequestFromLoad. This way, when we receive a seek request and we are not "airing", it means that the vorbis context in TrackPlayer is already N+1, so we need to re-start a normal download process, starting at seek point. It works nicely, even if we in reality still download N (see case 2- of long start delay).
There is also the case where N+1 is fully downloaded while N is still airing. We should not try to request N+2! The busy loop in TrackPlayer handles that and make that task wait until the track is aired (or user stop/skips). It's also important to not empty currentTrackStream in TrackPlayer because the notifyAudioReachedPlayback method needs it and it might not have been called yet (see 2-).
2- Long delay before first track starts (lots of audio in buffer) This is a sort of sub-case and does not affect cspotcli as much, but in general, the (virtual) DAC can have its own large buffer and there might be a substantial delay between when the 1st chunk of a track is processed and when that chunk is aired. If the first track reaches EoF before the first call to notifyAudioReachedPlayback is made, it should be blocked otherwise we'll have a change track detection even before having started airing
3- Change track detection fails Another consequence is that if track N has reached EoF early and N+1 has started being downloaded, the chunk's id are N+1. If the user presses "next", Spotify will stop the download process and start a new one, but the chunk's id are unchanged, so the feedPCM will not detect anything and we'll miss a change. We could use the "NEXT" event to handle this, but it seems better to use a sequencing number in TrackPlayer so that we never has the same identifier and expect the "controller" to do that gymnastic, especially as I'm not user how synchronized can be these events (receiving next, clearing buffer, and resetting the chunkId change detection...)
You can see in the edits of that PR that I've done a lot of back & forth, including in these comments. Net net, I've removed the variable playerStatus in TrackPlayer's class but in fact with the latest solution, I could bring it back. But It seems that it was not really used anyway and it's not the player's status, I've also changed a bit the way to initialize callbacks in TrackPlayer as the have to be there at creation time, why not simply pass them as contructor's parameters.