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

Add in app download functionality #1404

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

Conversation

7ritn
Copy link

@7ritn 7ritn commented Jun 3, 2024

This PR internalizes the download process.

Changes
Downloads are stored inside the app directory including thumbnail and external subtitle files. MediaSource data is downloaded as well and stored inside the Room Database.
A new Downloads fragment is added to show available downloads. Downloads are played back using the Exoplayer.

What's left
This is not a complete list

  • better download overview
  • add functionality to remove downloads
  • add button in main menu to get to downloads (currently only available from server selection)
  • overall polish

Notes
There is still lots of work to do, but it is already working. I new to Android development, so I am really sorry if something is not correct. Furthermore I implement this functionality as part of my Master's degree, so even if for whatever reason this PR is rejected, I will still complete the project to my best ability. I sorry that I changed some ExoPlayer code even though, it is currently reworked, but due to time constraints, I could not wait.

Thanks for any feedback :)

@jellyfin-bot jellyfin-bot added this to the v2.7.0 milestone Jun 3, 2024
@7ritn
Copy link
Author

7ritn commented Jun 4, 2024

Updated download page so it looks somewhat nice. Also added functionality to delete downloads.

@7ritn
Copy link
Author

7ritn commented Jun 6, 2024

So I would call the PR in a beta state. Feature wise, I think it is already at a good point. Adding the view downloads button requires modifying the web app. If something is missing I can still add it, but otherwise next step would be the polish.

@rcv11x
Copy link

rcv11x commented Jun 6, 2024

Thank you very much for this PR, it looks very good, I have a few questions to ask you:

1 - This is to download media on the device and watch them locally without internet?
2 - When you download the media are they compressed so that they take up less space on the device or are they downloaded with the actual size they occupy? ❤️

@7ritn
Copy link
Author

7ritn commented Jun 6, 2024

Thank you very much for this PR, it looks very good, I have a few questions to ask you:
1 - This is to download media on the device and watch them locally without internet?

Yes. Currently downloads are saved using Android's built-in DownloadManager. They are saved in for example the Downloads folder so one can playback the file using VLC. This PR adds the functionality to save the download to the app internal storage making playback from within the app possible.

2 - When you download the media are they compressed so that they take up less space on the device or are they downloaded with the actual size they occupy? ❤️

Video files are usually already very well compressed through the used codecs, adding a conventional lossless compression such as zstandard would barely reduce the size. The next step after this PR could adding functionality to support downloads in different bitrates, enabling smaller downloads.

@7ritn
Copy link
Author

7ritn commented Jun 24, 2024

While thinking about implementing the ability to download transcoded media (streams), I realized it would be better if ExoPlayer would handle downloading, since it natively supports HLS streams, making future efforts easier. So I rewrote the download logic to utilize ExoPlayers in-buildt download capabilities.

Oh yeah this also makes regular video playback use the downloade / cached files if available.

gradle/libs.versions.toml Fixed Show fixed Hide fixed
gradle/libs.versions.toml Fixed Show fixed Hide fixed
gradle/libs.versions.toml Fixed Show fixed Hide fixed
gradle/libs.versions.toml Fixed Show fixed Hide fixed
gradle/libs.versions.toml Fixed Show fixed Hide fixed
<string name="view_downloads">View Downloads</string>
<string name="available_downloads">Available downloads</string>
<string name="downloads">Downloads</string>
<string name="downloaded">Downloaded %1$s</string>

Check warning

Code scanning / Android Lint

Unused resources Warning

The resource R.string.downloaded appears to be unused
<string name="available_downloads">Available downloads</string>
<string name="downloads">Downloads</string>
<string name="downloaded">Downloaded %1$s</string>
<string name="download_finished">Download Finished</string>

Check warning

Code scanning / Android Lint

Unused resources Warning

The resource R.string.download_finished appears to be unused
<string name="downloaded">Downloaded %1$s</string>
<string name="download_finished">Download Finished</string>
<string name="download_notifications_description">Download notifications</string>
<string name="download_failed">Download failed</string>

Check warning

Code scanning / Android Lint

Unused resources Warning

The resource R.string.download_failed appears to be unused
<string name="download_finished">Download Finished</string>
<string name="download_notifications_description">Download notifications</string>
<string name="download_failed">Download failed</string>
<string name="failed_subs">Failed to download external subtitles</string>

Check warning

Code scanning / Android Lint

Unused resources Warning

The resource R.string.failed_subs appears to be unused
<string name="download_failed">Download failed</string>
<string name="failed_subs">Failed to download external subtitles</string>
<string name="failed_thumbnail">Failed to download thumbnail</string>
<string name="failed_media">Failed to download media</string>

Check warning

Code scanning / Android Lint

Unused resources Warning

The resource R.string.failed_media appears to be unused
@7ritn 7ritn marked this pull request as ready for review June 26, 2024 08:30
@satmandu
Copy link

I would love to test this if you want to post some builds...

I have a flight coming up. 😅

Copy link

@neBM neBM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much of the detekt issues blocking the PR seems like quick fixes except for one :/
Looks like we're the unlucky PR that needs that final param and hit the limit! 😆

I've thought of a potential solution but it's by no means perfect.

@satmandu
Copy link

Any chance we could get the workflows approved?

@satmandu
Copy link

It would also be nice to be able to select a player app for the files that get listed under downloads.

For instance, downloaded HDR videos may not play through the built-in player, but may play ok with VLC...

@SubhrajyotiSen
Copy link

This patch should help resolve the detekt failures:

---
 .../java/org/jellyfin/mobile/downloads/DownloadsAdapter.kt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/app/src/main/java/org/jellyfin/mobile/downloads/DownloadsAdapter.kt b/app/src/main/java/org/jellyfin/mobile/downloads/DownloadsAdapter.kt
index 41e4c1ae..f258b9e5 100644
--- a/app/src/main/java/org/jellyfin/mobile/downloads/DownloadsAdapter.kt
+++ b/app/src/main/java/org/jellyfin/mobile/downloads/DownloadsAdapter.kt
@@ -37,7 +37,12 @@ class DownloadsAdapter(
             val mediaItem: BaseItemDto? = downloadEntity.mediaSource.item
             binding.textViewName.text = downloadEntity.mediaSource.name
             binding.textViewDescription.text = when {
-                mediaItem?.seriesName != null -> context.getString(R.string.tv_show_desc, mediaItem.seriesName, mediaItem.parentIndexNumber, mediaItem.indexNumber)
+                mediaItem?.seriesName != null -> context.getString(
+                    R.string.tv_show_desc,
+                    mediaItem.seriesName,
+                    mediaItem.parentIndexNumber,
+                    mediaItem.indexNumber,
+                )
                 mediaItem?.productionYear != null -> mediaItem.productionYear.toString()
                 else -> downloadEntity.mediaSource.id
             }
--

@7ritn
Copy link
Author

7ritn commented Jul 16, 2024

Detekt now runs sucessfully

@7ritn
Copy link
Author

7ritn commented Jul 16, 2024

It would also be nice to be able to select a player app for the files that get listed under downloads.

For instance, downloaded HDR videos may not play through the built-in player, but may play ok with VLC...

This is not possible since we do not have access to the files when using ExoPlayer

@satmandu
Copy link

If a video is downloaded, playing the video when connected to the network tries to stream the file from the server, instead of preferring the locally downloaded file.

Would it be useful to be able to change that?

@7ritn
Copy link
Author

7ritn commented Jul 22, 2024

For me the cached version is used when playing the video by clicking on the item in the web view. Requirement for this to happen is to use the ExoPlayer in the settings. Can you check if you have selected the ExoPlayer and not the Web Player?

@satmandu
Copy link

For me the cached version is used when playing the video by clicking on the item in the web view. Requirement for this to happen is to use the ExoPlayer in the settings. Can you check if you have selected the ExoPlayer and not the Web Player?

Ah yes! It was reset to web player when I updated to my locally built version!

@satmandu
Copy link

Maybe some sort of if cached && player_not_set_to_exoplayer then notify user logic wold be warranted? (Or offer to use exoplayer to play the file?)

@7ritn
Copy link
Author

7ritn commented Jul 23, 2024 via email

@neBM
Copy link

neBM commented Jul 30, 2024

I've been daily driving the latest build. I've noticed the Download All functionality for an entire TV show appears to successfully download one or two eps only to not download any of the remaining eps.

If someone can trial this behaviour to test this isn't just my env, that would be great. Otherwise, this might be worth patching.

I have yet to test this against other media groups such as download all for a season.

@satmandu
Copy link

Also spent the last week using a build of this.

I would note that when viewing a cached video offline from the downloads folder, that the watched state/progress doesn't get uploaded back to the jellyfin server once a connection is re-established.

Should I make that a separate feature request?

@7ritn
Copy link
Author

7ritn commented Jul 30, 2024

I've been daily driving the latest build. I've noticed the Download All functionality for an entire TV show appears to successfully download one or two eps only to not download any of the remaining eps.

If someone can trial this behaviour to test this isn't just my env, that would be great. Otherwise, this might be worth patching.

I have yet to test this against other media groups such as download all for a season.

I'll look into this

Also spent the last week using a build of this.

I would note that when viewing a cached video offline from the downloads folder, that the watched state/progress doesn't get uploaded back to the jellyfin server once a connection is re-established.

Should I make that a separate feature request?

Yeah. I am aware of this. This can be added later if you want. Would need to remember the relevant server instance and also verify if play sessions are remembered that long so progress reports can be still made

@neBM
Copy link

neBM commented Jul 30, 2024

Also spent the last week using a build of this.

I would note that when viewing a cached video offline from the downloads folder, that the watched state/progress doesn't get uploaded back to the jellyfin server once a connection is re-established.

Should I make that a separate feature request?

Good catch, @satmandu!

Personally, I feel that the purpose of this PR: "To add download functionality", has been met. In addition to this, we already have a fairly large PR and I fear for keeping this branch up to date if/when potentially other large PRs get merged upstream.

+1 for doing this in a separate PR 👍

@satmandu

This comment was marked as off-topic.

@kimonmeier

This comment was marked as off-topic.

@7ritn
Copy link
Author

7ritn commented Aug 18, 2024

The commit I just pushed should fix the problems of dropping downloads when multiple downloads are enqueued. The problem was the small size of the event buffer and the policy to drop old events. Remaining problem is the a bit freaky download notification, when downloading files at the same time.

@7ritn
Copy link
Author

7ritn commented Aug 18, 2024

Would need to update the getForegroundNotification to consider all pending downloads when building the notification ugh

@kimonmeier
Copy link

kimonmeier commented Aug 19, 2024

I tested the whole pr for a bit and i found a strange bug.

Somehow the replacement of the DefaultDataSource with the CacheDataSource in the AppModule, breaks the change in bitrate during playback. If you change the bitrate, the player reports "source error".

playerFailed [eventTime=22.98, mediaPos=395.34, window=0, period=0, errorCode=ERROR_CODE_PARSING_MANIFEST_MALFORMED
  com.google.android.exoplayer2.ExoPlaybackException: Source error
      at com.google.android.exoplayer2.ExoPlayerImplInternal.handleIoException(ExoPlayerImplInternal.java:684)
      at com.google.android.exoplayer2.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:654)
      at android.os.Handler.dispatchMessage(Handler.java:102)
      at android.os.Looper.loopOnce(Looper.java:224)
      at android.os.Looper.loop(Looper.java:318)
      at android.os.HandlerThread.run(HandlerThread.java:67)
  Caused by: com.google.android.exoplayer2.ParserException: Input does not start with the #EXTM3U header.{contentIsMalformed=true, dataType=4}
      at com.google.android.exoplayer2.source.hls.playlist.HlsPlaylistParser.parse(HlsPlaylistParser.java:268)
      at com.google.android.exoplayer2.source.hls.playlist.HlsPlaylistParser.parse(HlsPlaylistParser.java:75)
      at com.google.android.exoplayer2.upstream.ParsingLoadable.load(ParsingLoadable.java:181)
      at com.google.android.exoplayer2.upstream.Loader$LoadTask.run(Loader.java:420)
      at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
      at java.lang.Thread.run(Thread.java:1012)
]

If you debug into the exoplayer source code you see that in com.google.android.exoplayer2.source.hls.playlist.HlsPlaylistParser in the method checkPlaylistHeader. That the header isn't correct anymore. The char codes are completly different and if you read the whole line you get a malformed header.
Output of Download via DefaultDataSource:
#EXTM3U ... and more
Output of Download via CacheDataSource:
�Eߣ�B��matroskaB���B����S�g W�t��M�t�M��S���I�fS����M��S���T�kS����M��S���S�kS���W�1�M��S���C�pS��

Did i configure something wrong or is it indeed a bug?

Ps: To fix it on my device, I just replaced the CacheDataSource with the DefaultDataSource

@7ritn
Copy link
Author

7ritn commented Aug 19, 2024

I just tried to replicate this bug. It only seems to appear on some bitrates i.e. 360p. But on higher bitrates, I do not get the exception.

@7ritn
Copy link
Author

7ritn commented Aug 20, 2024

Commit #7ce600c fixes the HLS content source error. Problem was that the exoplayer cache cached videos when playing them back, this was not intended and let to complication. The commit makes the cache read-only.

@7ritn
Copy link
Author

7ritn commented Aug 22, 2024

Commit #6db98b7 improves download notification when multiple downloads are in progress. This should fix all current outstanding problems.

@felixschndr
Copy link

Would it be possible for someone to take a look at this PR and review it please? That'd be great ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants