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

Allow DownloadManager do automatic track selection #5915

Open
tfcporciuncula opened this issue May 20, 2019 · 20 comments
Open

Allow DownloadManager do automatic track selection #5915

tfcporciuncula opened this issue May 20, 2019 · 20 comments
Assignees

Comments

@tfcporciuncula
Copy link

tfcporciuncula commented May 20, 2019

Issue description

Whenever I start a download (by calling DownloadService.sendAddDownload() see this comment) while offline, I don't see the download queued when I check the downloads in the DownloadManager (either through currentDownloads or downloadIndex). Calling isWaitingForRequirements() returns false, but as soon as the connection is back, the download starts automatically as if it was queued and waiting for requirements.

I did some debugging and was able to see within DownloadManager that notMetRequirements was correctly 1 but the downloads list was empty -- it seems that's where the issue is.

I don't think it's relevant, but just in case: I'm downloading HLS content and this is how my helper creation looks like:

DownloadHelper.forHls(
  uri,
  dataSourceFactory,
  DefaultRenderersFactory(context),
  null,
  DefaultTrackSelector.ParametersBuilder().setForceHighestSupportedBitrate(true).build()
)

Reproduction steps

I can try to reproduce this with the demo app or try to create another demo for this issue if it'd really be helpful, but I think the issue is pretty straightforward to reproduce and observe:

  • Make sure device is not connected to the internet
  • Start a download
  • Observe inconsistencies:
    • There's no downloads queued even though they'll start automatically once connection is back.
    • notMetRequirements is 1, even though isWaitingForRequirements() is false.

Link to test content

I believe this isn't relevant.

A full bug report captured from the device

Hopefully also not relevant.

Version of ExoPlayer being used

2.10.0 and 2.10.1

Device(s) and version(s) of Android being used

Samsung Note 8 and Pixel 2 emulator on API 26. It's reproducible every time.

@erdemguven
Copy link
Contributor

I can't reproduce this on the demo app.

One thing to keep in mind is DownloadService and DownloadManager methods are asynchronous. It takes some time for the internal states to change.

@tfcporciuncula
Copy link
Author

I'm sorry about my assumption that this was easily reproducible! I'll try to reproduce it on the demo app as well. I was aware about them being asynchronous but even after a while I was never able to see the items queued.

@tfcporciuncula
Copy link
Author

@erdemguven I just spent some time with the demo app and I was able to reproduce the issue.

It's a little tricky there for HLS because we show the track selection dialog before actually starting the download, but the issue is still there: if you start a download while offline, no queued download will show up in the DownloadManager until you go back online and the download start -- but in this case when you go back online the track selection dialog will show up so you can start the download.

To make things simpler, we can modify the onPrepare() method within DownloadTracker to skip the dialog:

@Override
public void onPrepared(DownloadHelper helper) {
  if (helper.getPeriodCount() == 0) {
    Log.d(TAG, "No periods found. Downloading entire stream.");
    startDownload();
    downloadHelper.release();
    return;
  }
  mappedTrackInfo = downloadHelper.getMappedTrackInfo(/* periodIndex= */ 0);
//  if (!TrackSelectionDialog.willHaveContent(mappedTrackInfo)) {
    Log.d(TAG, "No dialog content. Downloading entire stream.");
    startDownload();
    downloadHelper.release();
    return;
//  }
//  trackSelectionDialog =
//      TrackSelectionDialog.createForMappedTrackInfoAndParameters(
//          /* titleId= */ R.string.exo_download_description,
//          mappedTrackInfo,
//          /* initialParameters= */ DownloadHelper.DEFAULT_TRACK_SELECTOR_PARAMETERS,
//          /* allowAdaptiveSelections =*/ false,
//          /* allowMultipleOverrides= */ true,
//          /* onClickListener= */ this,
//          /* onDismissListener= */ this);
//  trackSelectionDialog.show(fragmentManager, /* tag= */ null);
}

With those parts commented out we can see the issue more easily. Now if you start a download while offline, nothing will happen again and no downloads will ever be queued no matter how much you wait for it. And then once you go back online, the download will start automatically.

I get the same issue with DASH, so that's probably not specific to HLS and is related to adaptive streams in general.

@tfcporciuncula
Copy link
Author

tfcporciuncula commented May 22, 2019

I just realized one of the things in my original description is wrong. I said that I'm starting the download by calling DownloadService.sendAddDownload(), but I'm actually doing that within the onPrepared() callback. I'm actually starting the download with the prepare() call on the DownloadHelper that I get by calling DownloadHelper.forHls() (just like the docs say).

And I just realized that when I'm offline, onPrepared() is never called -- onPrepareError() is called instead. So I guess DownloadManager is not queueing anything because we're never really calling DownloadService.sendAddDownload(). But it's weird that when we get back online, onPrepared() is automatically called as if that was queued and then the download starts. But from my tests this only happens for the last download I try to start while offline, so if I start two downloads while offline and go back online, only the last one will automatically start.

I hope things are more clear now, but if not just let me know how else I can help.

@erdemguven
Copy link
Contributor

Yes, I can see that behaviour with the demo app. If the device is offline a failure message is displayed. but when it becomes online the track selection dialog comes up. @tonihei can you look at this.

@erdemguven erdemguven assigned tonihei and unassigned erdemguven May 22, 2019
@tonihei
Copy link
Collaborator

tonihei commented May 22, 2019

So just to clarify: The download tracking is working as intended because the failed DownloadHelper meant that no download was ever actually started. And the sudden appearance of the download is due to the unexpected DownloadHelper.onPrepared callback.

The reason for the unexpected callback is that no one releases the DownloadHelper after onPrepareError (neither our demo app, nor your app apparently) and that's why we may get further manifest updates in the future which are successful (after coming back online). The fix is to trigger the release automatically once the preparation failed to prevent any further callbacks.

@tfcporciuncula
Copy link
Author

This doesn't seem to be the proper behavior, though. To me the download should be queued and not started because a requirement is missing (network). This doesn't seem to be consistent with how everything else works.

  • If a download is in progress and connection drops, it doesn't simply fail, it goes to the queued state.
  • If you try to start a download after the limit of parallel downloads have been reached, the download will be queued because of a missing requirement -- so it really feels that the same should happen for the network requirement.
  • If you do the same thing for a non adaptive media, then you can see the download being properly queued because of the missing requirement.

@tonihei
Copy link
Collaborator

tonihei commented May 22, 2019

That suggestion is to integrate DownloadHelper into the DownloadManager state tracking? Good idea, especially given the requirements tracking like waiting for network. Not sure how the state transitions look like in such a case. It seems we would need states like PREPARING and then a notMetRequirement along the lines of "waiting for DownloadHelper configuration".

@erdemguven What do you think about this proposal? We can use this issue to track as an enhancement.

@tfcporciuncula
Copy link
Author

I initially saw this as a bug, but I guess I wasn't seeing DownloadHelper as a separate thing. But yeah, it'd be great to have that in place. I think the notMetRequirement for this case should still be network, though, but I guess the details can be discussed along the way :)

My final goal as a consumer of the library would be to be able to support a queue out of the box with ExoPlayer, so whenever users try to start downloads while offline, everything is properly queued (and I can query whatever is queued from DownloadManager), and then once they're back online all queued downloads automatically start.

@erdemguven
Copy link
Contributor

Yes, it's a good idea to use this issue to track this feature request.

Btw, what I see in Netflix and other apps, is they just show an error when user tries to download anything without connection so they don't queue those requests.

@tonihei tonihei added enhancement and removed bug labels May 22, 2019
@tonihei tonihei changed the title Download started while offline is not being properly tracked Track DownloadHelper steps in DownloadManager May 22, 2019
@ojw28
Copy link
Contributor

ojw28 commented May 22, 2019

It behaves the way it does because it's not possible to do track selection without network connectivity, so the alternative approach doesn't work for apps designed like our demo app where user is given the option to select tracks when starting a download. Ditto for any other use cases that require showing the user something about the content to be downloaded, such as its size vs available space on the SD card.

+1 to using this to track an enhancement where we allow pushing the download straight into the manager for cases where user interaction isn't required. In the meantime, it shouldn't be difficult for an app with this kind of use case to track downloads that it hasn't managed to initialize by itself. Presumably it's just a list of URLs or content IDs. So I wouldn't consider this particularly high priority.

@tfcporciuncula
Copy link
Author

tfcporciuncula commented May 23, 2019

what I see in Netflix and other apps, is they just show an error when user tries to download anything without connection so they don't queue those requests.

Yup! We at Blinkist have always queued downloads, though, so whenever connection is back we start the downloads automatically -- it's definitely a nicer experience. When I first came across ExoPlayer 2.10 new download capabilities I really thought we'd be able to get that behavior by default with it which would be great, and that's my main motivation to try to push for this here.

it shouldn't be difficult for an app with this kind of use case to track downloads that it hasn't managed to initialize by itself.

It's definitely an easy thing to do, as it is to track downloads in general. But 2.10 made things even easier for consumers when it comes to tracking and this inconsistency just seems to be an important missing piece in the whole picture.

If I try to download something while connected to a metered connection and I have NETWORK_UNMETERED as a requirement, the downloads will be queued properly (and start automatically once I'm back on a Wi-Fi) but they won't if I'm not connected at all. If we try to abstract away the technical details and see this from a perspective of a consumer of the API, it's just inconsistent and not right. That's why I think it could deserve some priority.

@erdemguven
Copy link
Contributor

Btw, what kind of media are you downloading? If it's a progressive stream (mp4, mp3) or an adaptive stream with predefined tracks, you can skip DownloadHelper and create DownloadRequest yourself. Which would be faster and DownloadManager would queue it for you.

@erdemguven erdemguven changed the title Track DownloadHelper steps in DownloadManager Allow DownloadManager do automatic track selection May 23, 2019
@erdemguven erdemguven assigned erdemguven and unassigned tonihei May 23, 2019
@tfcporciuncula
Copy link
Author

It's an adaptive stream but it's not really predefined tracks. We're using DefaultTrackSelector.ParametersBuilder().setForceHighestSupportedBitrate(true).build(), so not sure if that's predefined enough to skip the DownloadHelper.

tonihei added a commit that referenced this issue May 23, 2019
This prevents further unexpected updates if the MediaSource happens to
finish its preparation at a later point.

Issue:#5915
PiperOrigin-RevId: 249439246
@tonihei
Copy link
Collaborator

tonihei commented May 23, 2019

Original bug (onPrepared after onPrepareError) is now fixed. You can workaround it until the next release but calling downloadHelper.release() from onPrepareError.

so not sure if that's predefined enough to skip the DownloadHelper

That definitely sounds like a use case for DownloadHelper because the track selection will take into account device capabilities etc., which is something you can't do with a manual pre-selection. However, that's also the default parameter set we are using in DownloadHelper (so no need to specify that again) and thus it should be possible to do that automatically once the network is available.

@tfcporciuncula
Copy link
Author

Thanks for fixing that!

that's also the default parameter set we are using in DownloadHelper (so no need to specify that again) and thus it should be possible to do that automatically once the network is available.

You mean doing automatically that on my end or on ExoPlayer's end?

@tonihei
Copy link
Collaborator

tonihei commented May 23, 2019

On our end, that's the enhancement tracked by this issue. So that you can add the download to the DownloadManager (or Service) and the automatic track selection using these parameters takes place once the network becomes available.

ojw28 pushed a commit that referenced this issue Jun 3, 2019
This prevents further unexpected updates if the MediaSource happens to
finish its preparation at a later point.

Issue:#5915
PiperOrigin-RevId: 249439246
@tfcporciuncula
Copy link
Author

tfcporciuncula commented Aug 21, 2019

@tonihei We're facing an interesting (and different) situation where this issue became relevant again, so I thought I'd share it here.

We allow users to start a large number of downloads at once in a batch, but we have a good maxParallelDownloads limit. However, this limit is ignored in the prepare phase of the downloads, and in the end ExoPlayer tries to prepare all of the downloads at once without checking the limit.

It'd be nice if we could add the downloads to the DownloadManager (or service) before preparation, so it'd be able to take maxParallelDownloads into account for the preparation phase as well.

@tonihei
Copy link
Collaborator

tonihei commented Aug 21, 2019

Am I right in assuming that letting DownloadManager do the track selection would solve this issue automatically? Because it would only start the process if it doesn't exceed maxParallelDownloads.

Until that is implemented, there is probably not much we can do about this because it's your code calling the DownloadHelper for all downloads in parallel at the moment. If you think there is something we should do in DownloadHelper to make that easier, please let us know!

@tonihei tonihei assigned tonihei and unassigned erdemguven Aug 21, 2019
@tfcporciuncula
Copy link
Author

tfcporciuncula commented Aug 21, 2019

Am I right in assuming that letting DownloadManager do the track selection would solve this issue automatically? Because it would only start the process if it doesn't exceed maxParallelDownloads.

Yes, I believe you're right. So I think once this issue is resolved/implemented, we'd have that solved.

Until that is implemented, there is probably not much we can do about this because it's your code calling the DownloadHelper for all downloads in parallel at the moment. If you think there is something we should do in DownloadHelper to make that easier, please let us know!

Yup, make sense. And I don't see any obvious improvement we could do on DownloadHelper right now :)

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

No branches or pull requests

4 participants