tehkillerbee / mopidy-tidal

Tidal Backend plugin for Mopidy
Apache License 2.0
93 stars 28 forks source link

Caching and lazy connection (WIP) #70

Closed 2e0byo closed 2 years ago

2e0byo commented 2 years ago

Being on holiday with intermittent mobile internet I got round to implementing some features I've been thinking about adding for a while:

I've also blackened and isorted everything. Mopidy 'encourages' this for extensions, but if you don't like it and still want to take this PR I can extract the relevant commits and conform to the style the repo was using.

Still to do (and likely not going to happen for a few months):

*Currently we use lock files (.dirty) to avoid downoading the same track twice. There's a race condition where the user requests the same track twice and thread 2 launches before thread 1 has made the lockfile, but I strongly suspect mopidy can't actually spawn a new thread that quickly (but 'strongly suspect' isn't proof).

** The hash is from an earlier draft which used the whole url as a key. But obviously the token is in the url, so that doesn't work. I'll remove the hashing code when I no longer need the large cache directory I have full of files named by hashes...

The basic idea is that we intercept transform_url and fetch the url to a local file. After a sufficient buffer has downloaded we return a file:// url which is passed to gstreamer. This has the disadvantage that the user can't skip forward beyond the downloaded portion, but this only applies when initially downloading. I don't know if mopidy tries to support gapless playing, but this would also likely break that unless we can inform mopidy of how early to fetch the next track.

The cache is disabled by default, as is the lazy connection.

Is this something you'd like to see in mopidy-tidal?

2e0byo commented 2 years ago

Incidentally I've just seen the v7 PR: if that gets merged I'll rebase.

tehkillerbee commented 2 years ago

@2e0byo Very nice to see another contributor and very interesting additions - especially the local cache and offline mode. Looking forward to test it. Code cleanup is also nice; at the moment there are no strict formatting guidelines.

Yes, as you have noticed, work is in progress to update the codebase to support v7 so we should rebase your PR as soon as #68 is merged.

2e0byo commented 2 years ago

Excellent; I'll work on the rebase when #68 is merged then. I've a few more ideas, but I'll probably leave this until I'm back (in a few months!) as I'm all over the place before then and it mostly works well enough.

  1. background fetching should at least be a concurrent.Future so it can be cancelled (otherwise there's a vicious cycle if the internet slows to the point that the buffer isn't large enough to maintain playback, as each failed track launches a new download thread, further eroding bandwidth). Alternatively it could be a proper Actor, but I've never done actor-based programming.

  2. all the (few) current problems come from using a file:// url to gstreamer, which naturally assumes the file is fully available. An alternative would be a proper streaming proxy, either over a local socket (gstreamer can read from sockets) or a full http proxy. Assuming gstreamer sends standard range requests it would be possible to intercept them and translate them into equivalent requests upstream (or just block until the data was available) which would match current streaming behaviour (currently gsreamer just waits for the streaming source to provide the requisite block). Note that seeking problems basicaly don't appear over a fast connection with reasonable-length tracks (i.e. not symphonies), and never appear once the file is cached.

  3. A simpler version of 2 would be to pass the http url to gstreamer if the file isn't in the cache, and then download it as well. This wold result in two parallel connections (not great over mobile internet) but would prevent dropouts if the buffer empties.

  4. A more intelligent version of the current code would be to estimate the buffer size from the current download rate. This would be fairly trivial and I might add it soon if dropouts keep happening.

2 is a lot more code and loses some of the simplicity of having gstreamer handle the entire audio side. But it would have the advantage of allowing us to move away from unencrypted urls if/when tidal finally retire them entirely. When the secrets last changed I did some work on using the encrypted endpoints (which tidal uses for everything except legacy clients). Getting the files is easy; someone has reverse engineered the encryption: I couldn't get it going properly but the scheme is simple enough. OFC this work would belong on python-tidal, but it would mean python-tidal would only provide a stream.

I won't make any attempt to implement 2. for now, but it might be worth bearing in mind---I strongly suspect tidal are going to pull the unencrypted endpoints at some point.

Would appreciate your thoughts (when you get a moment!) on 2/3 and the general direction of the project---is caching something exceptional for low-bandwidth users and can cope with the odd dropout from buffer emptying, or should it be transparent (in which case I guess 3 at least for now?)?

Many thanks for mopidy-tidal btw! I've used it daily for a fair while now.

2e0byo commented 2 years ago

Added cancellable (using a Process as I always forget that Future.cancel() doesn't actually do anything), and a makefile for linting (make lint).

blacklight commented 2 years ago

@2e0byo as mentioned by @tehkillerbee, most of the efforts are now focused on migrating the codebase to tidalapi 0.7.x - it should be released this month if everything goes well. The new version has actually broken a lot of the previous interfaces, and I'm finalizing the last things in https://github.com/tehkillerbee/mopidy-tidal/pull/68. Maybe better to hold on the black/isort addition until the PR is merged, in order to avoid massive conflicts?

2e0byo commented 2 years ago

@BlackLight wonderful, I didn't realise #68 was so close to being merged. I'll sought this PR out when it is. For now it's working as I wanted against the old branch, so we've got music on a dodgy connection and I'm happy.

tehkillerbee commented 2 years ago

@2e0byo FYI #67 has now been merged. Feel free to rebase, then I will look forward to testing it.