jmshrv / finamp

A Jellyfin music client for mobile
Mozilla Public License 2.0
1.9k stars 124 forks source link

New Download Package #213

Closed jmshrv closed 7 months ago

jmshrv commented 2 years ago

Currently, Finamp uses flutter_downloader to handle downloading songs and images. This package isn't the greatest, and has been the root cause of issues such as janky download indicators and the app freezing when downloading a lot of songs at once. The main focus of 0.7 will be creating a new plugin which would make managing downloads much easier. This could include stuff like adding streams to listen to download progress, and the ability to handle URL download paths for better custom path support. This package will also have a generic Dart fallback for Windows/Linux, as currently flutter_downloader only supports iOS and Android (the iOS code should run on macOS, but I'd rather not inherit any code from flutter_downloader).

drazil100 commented 2 years ago

Honestly this can't come soon enough. The current solution is extremely buggy. The inclusion of thumbnail downloading really confused my install and I ended up deleting it and redownloading it to start completely fresh. I am currently in the process of redownloading all of my music and it has hung multiple times with where there queue was 4 or there were 2 running but half of multiple albums were not downloaded and the enqueued/running numbers did not change through multiple attempts to close and reopen the app.

axelsimon commented 2 years ago

Glad to see this is already reported in some way. Could the issue be retitled with something more user-oriented, like "Current download system provides no information and can create a mess" or something like that? :)

I, like others i expect, have had a mess created by trying to download a single album, lose connectivity, and end up with a "failed" download with no way to check, clean-up or retry.

jmshrv commented 2 years ago

I don't want to be harsh on flutter_downloader, it works for it's intended purpose - Finamp has just outgrown that and needs proper dependency management built-in.

In Finamp, you can delete failed albums and they should clean themselves up. At some point I'm going to add easier to access delete buttons and a retry button to the download screen.

jmshrv commented 2 years ago

I've started work on this at https://github.com/UnicornsOnLSD/flutter_download_manager. It's still very early on - I've pretty much only just got a blueprint for a download and download group class, so don't expect it like next week lol.

axelsimon commented 2 years ago

Still, a good start! Good to hear :slightly_smiling_face:

jmshrv commented 2 years ago

From @rodcramp in #62 - we will want to model genres in this plugin, which can be implemented by making a genre a group. To follow how Finamp works, genres will have albums as their children.

MrPotatoBobx commented 2 years ago

Will transcoded downloads be included in the release?

Chaphasilor commented 2 years ago

Will transcoded downloads be included in the release?

It's probably unrelated, but they most likely won't be included before this is done \^\^

jmshrv commented 2 years ago

As far as I know, Jellyfin doesn't really have a way of downloading transcoded songs. We could probably just save the transcoded stream, although I don't know if that would cause issues regarding the stream not having a confirmed content length or anything.

Fale commented 2 years ago

would a local transcoding be an option?

jmshrv commented 2 years ago

In theory yes, but that would be a pretty overengineered way of doing things. I'll look into it more deeply once the package work is done.

Fale commented 2 years ago

yeah, leveraging the jellyfin server would make more sense, but if there are issues, it could be a fallback. I think this is an important feature to at least ensure that not too much storage gets used

MrPotatoBobx commented 2 years ago

would a local transcoding be an option?

that would put alot of strain on the device, especially when downloading albums en masse.

Emporea commented 1 year ago

Will this also enable to download a whole music library at once?

jmshrv commented 1 year ago

It's technically possible with the current system, but I wouldn't trust it to reliably succeed

axelsimon commented 1 year ago

It makes sense to want to store transcoded tracks on a mobile device, as it's likely to have lower quality headphones or earphones, and storing FLAC can end up consuming a lot of storage space, but it sounds like it should be a different issue.

jmshrv commented 1 year ago

@axelsimon Transcoded downloads has its own issue (#215), I'll write about my progress there

jmshrv commented 1 year ago

https://pub.dev/packages/background_downloader may have beat me to it (putting this off for a year has its benefits)

jmshrv commented 1 year ago

There's an interesting issue at flutter_downloader which ticks basically all of the boxes that I wanted - https://github.com/fluttercommunity/flutter_downloader/issues/823

781flyingdutchman commented 1 year ago

Hi, author of the background_downloader here. I think my motivation to write an alternative is very similar to yours :) I'd love your feedback on the background_downloader package, as it really does improve reliability and gives me ideas for new features, so please check it out and log issues in the repo.

jmshrv commented 1 year ago

I'll let you know if I run into anything! I read through the documentation and it looked like the perfect package. Stuff like getting the path relatively will help me remove horrible patches I've had to make because I made the mistake of storing absolute paths:

https://github.com/jmshrv/finamp/blob/0932469b1906f7a425d3fc73cbf561f3c58e1760/lib/services/downloads_helper.dart#L598-L636

jmshrv commented 1 year ago

I've started work on a document that details why I'm doing this rewrite, and the impacts that it will have. I'll send another message here once I've filled out the actual plan on what will be changing.

https://github.com/jmshrv/finamp/blob/main/DOWNLOADS_PLAN.md

781flyingdutchman commented 1 year ago

Since you're planning to use a central database, my recommendation would be to not use the trackTasks and database functionality of background_downloader. It works, but it's very simple, and in my experience if you need to track more than the basics you're better off using a separate database, listening to task status updates and updating that database yourself. Managing two database only adds to the complexity. To make sure you don't miss any status updates, the plugin saves updates that it could not post to Flutter (e.g. when the app is suspended) in native storage. Those updates can be retrieved by calling resumeFromBackground right after you have registered your callbacks (on app startup): this will trigger regular callback status updates for those updates stored natively.

jmshrv commented 1 year ago

Ah cool, I'll manage the database myself then :)

781flyingdutchman commented 1 year ago

Hi @jmshrv I wanted to let you know that the latest version of the background_downloader now allows you to specify a different 'persistent storage database' for storing, updating and retrieving download information. You need to implement the PersistentStorage interface in a class you write (for example using sqflite as the underlying database, and that class may also manage your other database needs, unrelated to the downloader) and then pass an instance of that class to the very first call to FileDownloader. If you do this, you can use the trackTasks method and use the FileDownloader's database object as intended, and everything will now use the backing database that you created (so there won't be two databases to maintain).

I think this may be an elegant solution for what you are looking for, and saves you effort tracking the task status yourself. The example app includes an example implementation of the PersistentStorage interface using sqflite (you need to uncomment a line to activate it).

jmshrv commented 1 year ago

That's a really cool solution, I'll look into it :)

Currently I'm mostly working on #220, but flutter_downloader is no longer maintained so I should really get off it soon

Chaphasilor commented 9 months ago

Hey @781flyingdutchman, got a quick question about your package! In the docs for background_downloader it says that if a task has requiresWifi = true, it will only start if the device is connected to WiFi. But in our case, it would be good if the task would automatically pause if the devices switches from WiFi to cellular, and then resume upon reconnecting to WiFi. Is that handled by the package, would we need to build our own logic around it?
In the docs it says

For example, the OS may choose to pause the task if someone walks out of WiFi coverage.

but I'm not sure what the preconditions for this are, and if it works on all platforms.

Also, it seems like task.pause() will immediately pause all current downloads, and use a HTTP range request for resuming. Given that we're only dealing with small files, for us it would probably be better if any outstanding downloads would be completed but new ones wouldn't be resumed (in a batch task). Is there an option for this?

Oh and Merry Christmas 🎄, in case you're celebrating :D

781flyingdutchman commented 9 months ago

Hi, thanks for taking another look. When you lose Wifi during a download for a task that has requiresWifi = true the behavior is slightly different between iOS and Android:

So, in both cases you get roughly the behavior you want, except in Android it does require retries > 0 and if the Wifi goes in and out a few times you may run out of retries and the task may fail.

On task.pause: not sure exactly what you mean, as there is no pause method on Task. There is FileDownloader.pause(task) which simply pauses that specific task. There is no way to pause all current downloads - you'll have to get all running tasks and pause each of them independently. That said, have a look at TaskQueues as they allow for more fancy queue management before as task gets actually enqueued on the native side, and it would be trivial to add a pause method there that stops tasks getting enqueued until you resume the TaskQueue (but you'll have to extend the MemoryTaskQueue that is provided as part of the package and build that functionality in there).

Hope this helps.

Chaphasilor commented 9 months ago

Okay yeah that definitely helps, thanks! I guess we'll just go with the requiresWiFi-approach then.
Regarding the pausing behavior, how are batch tasks handled? I notices a Batch is not a subtype of Task, so what exactly is the purpose? Is it just somewhere between starting a single task and using a task queue?

781flyingdutchman commented 9 months ago

The downloadBatch is a 'convenience function' that makes it easy to monitor download of a group of tasks. Importantly, it is implemented on the Flutter side, not native, and it simply enqueues all tasks (with some intermittent delay to avoid choking the UI thread) and intercepts the task status callbacks such that it can provide a batch progress updated (in files completed vs files in the batch). It is meant for simple, smaller batches that can reasonably be assumed to complete while the app is in the foreground. TaskQueue is a variant of that, with more flexibility (e.g. you can keep adding tasks to a queue, not to a batch) but with less monitoring (no batch progress callbacks etc).

If you have a larger number of files to download, or if they are big, then I recommend using enqueue for each task and monitor using the database (see here) and if necessary use SqlitePersistentStorage or create your own implementation of PersistentStorage to track your downloads in a more persistent manner.

Chaphasilor commented 9 months ago

Got it. We actually already have a PR doing just that, using Isar as the database. It seems to work well, just the requiresWiFi option doesn't fully match our requirements, but I don't think that's in your hands anyway...
Basically, we're missing a way to update the requiresWiFi property while the items are enqueued / waiting to be enqueued. So that if a user starts downloading a large playlists, then needs to leave the house, they can just go to settings and retroactively enable "Download over WiFi only" for all remaining download tasks in the queue. But it's not super important, we'll probably just default the setting to "on" and show a warning when a user tries to start a download over mobile data :)

Chaphasilor commented 7 months ago

Implemented as part of the beta release (https://github.com/jmshrv/finamp/releases/tag/0.9.2-beta)