Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Download Package #213

Closed
jmshrv opened this issue Apr 10, 2022 · 32 comments · Fixed by #568
Closed

New Download Package #213

jmshrv opened this issue Apr 10, 2022 · 32 comments · Fixed by #568
Milestone

Comments

@jmshrv
Copy link
Owner

jmshrv commented Apr 10, 2022

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 jmshrv added this to the 0.7 milestone Apr 10, 2022
@drazil100
Copy link

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
Copy link

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
Copy link
Owner Author

jmshrv commented May 6, 2022

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
Copy link
Owner Author

jmshrv commented Jun 15, 2022

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
Copy link

Still, a good start! Good to hear 🙂

@jmshrv
Copy link
Owner Author

jmshrv commented Aug 15, 2022

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
Copy link
Contributor

Will transcoded downloads be included in the release?

@Chaphasilor
Copy link
Collaborator

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
Copy link
Owner Author

jmshrv commented Sep 8, 2022

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
Copy link

Fale commented Sep 9, 2022

would a local transcoding be an option?

@jmshrv
Copy link
Owner Author

jmshrv commented Sep 9, 2022

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
Copy link

Fale commented Sep 9, 2022

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
Copy link
Contributor

would a local transcoding be an option?

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

@axelsimon
Copy link

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
Copy link
Owner Author

jmshrv commented Feb 9, 2023

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

@jmshrv
Copy link
Owner Author

jmshrv commented Apr 24, 2023

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

@jmshrv
Copy link
Owner Author

jmshrv commented Apr 24, 2023

There's an interesting issue at flutter_downloader which ticks basically all of the boxes that I wanted - fluttercommunity/flutter_downloader#823

@781flyingdutchman
Copy link

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
Copy link
Owner Author

jmshrv commented Apr 27, 2023

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:

// If the song path doesn't contain the current path, assume the
// path has changed.
if (!file.path.contains(currentDocumentsDirectory.path)) {
_downloadsLogger.warning(
"Item does not contain documents directory, assuming moved.");
if (internalStorageLocation.path !=
path_helper.join(currentDocumentsDirectory.path, "songs")) {
// Append /songs to the documents directory and create the new
// song dir if it doesn't exist for some reason.
final newSongDir = Directory(
path_helper.join(currentDocumentsDirectory.path, "songs"));
_downloadsLogger.warning(
"Difference found in settings documents paths. Changing ${internalStorageLocation.path} to ${newSongDir.path} in settings.");
// Set the new path in FinampSettings.
internalStorageLocation =
await FinampSettingsHelper.resetDefaultDownloadLocation();
}
// If the song's path is not relative, make it relative. This only
// handles songs since all images will have relative paths.
if (!isPathRelative) {
downloadedSong!.path = path_helper.relative(downloadedSong.path,
from: downloadedSong.downloadLocation!.path);
downloadedSong.isPathRelative = true;
addDownloadedSong(downloadedSong);
}
if (await downloadedSong?.file.exists() ??
await downloadedImage!.file.exists()) {
_downloadsLogger
.info("Found item in new path! Everything is fine™");
return true;
} else {
_downloadsLogger.warning(
"$id not found in new path! Assuming that it was deleted before an update.");
}

@jmshrv
Copy link
Owner Author

jmshrv commented Apr 27, 2023

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
Copy link

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
Copy link
Owner Author

jmshrv commented Apr 30, 2023

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

jmshrv referenced this issue May 27, 2023
They're still coming, just not for a while
@781flyingdutchman
Copy link

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
Copy link
Owner Author

jmshrv commented May 30, 2023

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 Chaphasilor unpinned this issue Sep 24, 2023
@Chaphasilor Chaphasilor pinned this issue Dec 20, 2023
@Chaphasilor Chaphasilor linked a pull request Dec 21, 2023 that will close this issue
@Chaphasilor
Copy link
Collaborator

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
Copy link

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.

@Chaphasilor
Copy link
Collaborator

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
Copy link

781flyingdutchman commented Dec 27, 2023

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
Copy link
Collaborator

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 Chaphasilor moved this to In Progress in Finamp Redesign Jan 25, 2024
@Chaphasilor Chaphasilor moved this from In Progress to Done in Finamp Redesign Feb 20, 2024
@Chaphasilor
Copy link
Collaborator

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

@Chaphasilor Chaphasilor unpinned this issue Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants