finamp icon indicating copy to clipboard operation
finamp copied to clipboard

New Download Package

Open jmshrv opened this issue 3 years ago • 31 comments
trafficstars

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).

jmshrv avatar Apr 10 '22 16:04 jmshrv

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.

drazil100 avatar Apr 15 '22 06:04 drazil100

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.

axelsimon avatar May 06 '22 13:05 axelsimon

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 avatar May 06 '22 14:05 jmshrv

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.

jmshrv avatar Jun 15 '22 13:06 jmshrv

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

axelsimon avatar Jun 15 '22 13:06 axelsimon

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.

jmshrv avatar Aug 15 '22 23:08 jmshrv

Will transcoded downloads be included in the release?

MrPotatoBobx avatar Sep 01 '22 18:09 MrPotatoBobx

Will transcoded downloads be included in the release?

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

Chaphasilor avatar Sep 07 '22 20:09 Chaphasilor

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.

jmshrv avatar Sep 08 '22 13:09 jmshrv

would a local transcoding be an option?

Fale avatar Sep 09 '22 08:09 Fale

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.

jmshrv avatar Sep 09 '22 15:09 jmshrv

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

Fale avatar Sep 09 '22 15:09 Fale

would a local transcoding be an option?

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

MrPotatoBobx avatar Sep 10 '22 16:09 MrPotatoBobx

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

Emporea avatar Feb 08 '23 11:02 Emporea

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

jmshrv avatar Feb 08 '23 23:02 jmshrv

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.

axelsimon avatar Feb 09 '23 09:02 axelsimon

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

jmshrv avatar Feb 09 '23 10:02 jmshrv

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

jmshrv avatar Apr 24 '23 02:04 jmshrv

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

jmshrv avatar Apr 24 '23 02:04 jmshrv

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.

781flyingdutchman avatar Apr 27 '23 06:04 781flyingdutchman

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 avatar Apr 27 '23 14:04 jmshrv

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

jmshrv avatar Apr 27 '23 20:04 jmshrv

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.

781flyingdutchman avatar Apr 30 '23 06:04 781flyingdutchman

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

jmshrv avatar Apr 30 '23 08:04 jmshrv

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).

781flyingdutchman avatar May 30 '23 20:05 781flyingdutchman

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

jmshrv avatar May 30 '23 20:05 jmshrv

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

Chaphasilor avatar Dec 25 '23 09:12 Chaphasilor

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:

  • iOS will pause the task and resume once WiFi is available. However, the pause is not visible, in that no paused status update will be sent - the task simply remains in running state for a long time. You can configure the timeoutIntervalForResource parameter to force a task to fail when it takes longer than a certain time to complete
  • Android will fail the task immediately once it loses WiFi. If you have set retries = x (and you have retries remaining) then the task will be re-enqueued after a short delay, but won't start until WiFi is available. Once available, it will resume from where it failed

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.

781flyingdutchman avatar Dec 26 '23 19:12 781flyingdutchman

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?

Chaphasilor avatar Dec 27 '23 14:12 Chaphasilor

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.

781flyingdutchman avatar Dec 27 '23 16:12 781flyingdutchman