Updates code to replace mp4 downloads and use hls streams instead #554
Conversation
|
@sammyd I seem to remember there was a reason we didn't support HLS downloads from the start - can you remember it? |
|
@0xTim the primary reason was that I didn't realise it was a thing. The secondary reason is that it means we can no longer gate downloads on the backend. i.e. anybody that downloads the source for the app would be able to tweak it to download videos they have access to view. I've decided that this is a much lower risk than first thought, and we should standardise on HLS for simplicity, and the benefits we get with subtitles. I'll take a look through this PR as well, but since you have a lot more experience with HLS downloading, if you could review it that'd be great! Thanks (= |
|
@VegetarianZombie Is this ready for review? There's a fair bit of commented out code and the tests aren't passing |
|
@0xTim When I submitted this, we were migrating off Auth0 so the failed I ran into were login issues. Those appeared to have been corrected. I also deleted the deprecated tests (commented out). There is one test that is inconsistently failing: testDownloadQueueStreamRespectsTheMaxLimit Sometimes this passes when given enough time. Other times I get it to pass by running all the DownloadQueueManagerTests. I'm having a little difficulty making heads or tails of this one. 1 @sammyd, do you have any insight on this test? |
|
I've merged #557. I'm gonna merge development into this to see if the tests are fixed. |
| @@ -85,7 +85,7 @@ class Service { | |||
| func prepare<R: Request>(request: R, | |||
| parameters: [Parameter]?) -> URLRequest? { | |||
| let pathURL = networkClient.environment.baseURL.appendingPathComponent(request.path) | |||
|
|
|||
| print("pathURL: \(pathURL)") | |||
VegetarianZombie
changed the title
|
Hi @0xTim - I removed the print statement and found a few others. Should be all set now. |
| @@ -150,7 +150,7 @@ final class VideoPlaybackViewModel { | |||
| } | |||
| // If we're online then, that's all good | |||
| if sessionController.sessionState == .online { | |||
There was a problem hiding this comment.
If we're commenting out this return we should just remove the whole if block
There was a problem hiding this comment.
Hi Tim,
I went added that code back in. As for the SD vs HD quality options, I have those in place. For SD streams, I'm downloading the streams at a lower bitrate (250,000). The SD file sizes ended up being half the size as the HD files.
|
@VegetarianZombie final task before this is merged 😞 the conflicts need to be resolved @sammyd I think this is good to go - there's nothing we need to prepare on the backend for everyone downloading HLS streams etc going forward is there? I'm assuming not but worth asking in case the MP4 allotment has special capacity etc |
|
@VegetarianZombie scrap that, the merge conflict was dead simple so I fixed it |
|
@0xTim nothing to change on the backend, no. We'll want to test this once we get a TestFlight release, cos it removes the need for the API_KEY—and no longer gates downloads on the backend. I will try to fix CI very soon so that we can start having TestFlight releases again. It's just a bit of a coordination nightmare to actually log in to ASC. |
|
@sammyd do you want me to hold off merging until Testflight is fixed? |
|
nah...go for it. I'll retrigger once I fix it |
This pull request removes the mp4 downloads and uses HLS downloads instead. It creates AVAssetDownload tasks instead of standard download task. It also requires a new way to calculate progress. The pull request gets rid of the DownloadVideoRequest and replaces it with the DownloadStreamVideoRequest which is designed to handle hls streams. I also had to remove the code for download resuming as that isn't handled by hls downloads.