Skip to content

Commit

Permalink
Fix DownloadManager assertion failure
Browse files Browse the repository at this point in the history 
Issue: #8419
#minor-release
PiperOrigin-RevId: 350134719
  • Loading branch information
@ojw28
ojw28 committed Jan 5, 2021
1 parent a9a150a commit 6d2d2e3
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 24 deletions.
20 changes: 12 additions & 8 deletions RELEASENOTES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,23 @@
* DRM:
* Fix playback failure when switching from PlayReady protected content to
Widevine or Clearkey protected content in a playlist.
* Downloads:
* Fix crash in `DownloadManager` that could occur when adding a stopped
download with the same ID as a download currently being removed
([#8419](https://github.com/google/ExoPlayer/issues/8419)).
* Analytics:
* Pass a `DecoderReuseEvaluation` to `AnalyticsListener`'s
`onVideoInputFormatChanged` and `onAudioInputFormatChanged` methods. The
`DecoderReuseEvaluation` indicates whether it was possible to re-use an
existing decoder instance for the new format, and if not then the
reasons why.
* Text:
* Gracefully handle null-terminated subtitle content in Matroska
containers.
* Fix CEA-708 anchor positioning
([#1807](https://github.com/google/ExoPlayer/issues/1807)).
* Metadata retriever:
* Parse Google Photos HEIC motion photos metadata.
* IMA extension:
* Add support for playback of ads in playlists
([#3750](https://github.com/google/ExoPlayer/issues/3750)).
Expand All @@ -83,16 +94,9 @@
([#8290](https://github.com/google/ExoPlayer/issues/8290)).
* Add `ImaAdsLoader.Builder.setEnableContinuousPlayback` for setting
whether to request ads for continuous playback.
* Metadata retriever:
* Parse Google Photos HEIC motion photos metadata.
* FFmpeg extension:
* Link the FFmpeg library statically, saving 350KB in binary size on
average.
* Text:
* Gracefully handle null-terminated subtitle content in Matroska
containers.
* Fix CEA-708 anchor positioning
([#1807](https://github.com/google/ExoPlayer/issues/1807)).
* OkHttp extension:
* Add `OkHttpDataSource.Factory` and deprecate `OkHttpDataSourceFactory`.
* Cronet extension:
Expand Down Expand Up @@ -225,7 +229,7 @@
([#7378](https://github.com/google/ExoPlayer/issues/7378)).
* Downloads: Fix issue retrying progressive downloads, which could also result
in a crash in `DownloadManager.InternalHandler.onContentLengthChanged`
([#8078](https://github.com/google/ExoPlayer/issues/8078).
([#8078](https://github.com/google/ExoPlayer/issues/8078)).
* HLS: Fix crash affecting chunkful preparation of master playlists that start
with an I-FRAME only variant
([#8025](https://github.com/google/ExoPlayer/issues/8025)).
Expand Down
34 changes: 18 additions & 16 deletions library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -856,7 +856,7 @@ private void setStopReason(@Nullable String id, int stopReason) {
private void setStopReason(Download download, int stopReason) {
if (stopReason == STOP_REASON_NONE) {
if (download.state == STATE_STOPPED) {
putDownloadWithState(download, STATE_QUEUED);
putDownloadWithState(download, STATE_QUEUED, STOP_REASON_NONE);
}
} else if (stopReason != download.stopReason) {
@Download.State int state = download.state;
Expand Down Expand Up @@ -910,7 +910,7 @@ private void removeDownload(String id) {
Log.e(TAG, "Failed to remove nonexistent download: " + id);
return;
}
putDownloadWithState(download, STATE_REMOVING);
putDownloadWithState(download, STATE_REMOVING, STOP_REASON_NONE);
syncTasks();
}

Expand All @@ -924,10 +924,11 @@ private void removeAllDownloads() {
Log.e(TAG, "Failed to load downloads.");
}
for (int i = 0; i < downloads.size(); i++) {
downloads.set(i, copyDownloadWithState(downloads.get(i), STATE_REMOVING));
downloads.set(i, copyDownloadWithState(downloads.get(i), STATE_REMOVING, STOP_REASON_NONE));
}
for (int i = 0; i < terminalDownloads.size(); i++) {
downloads.add(copyDownloadWithState(terminalDownloads.get(i), STATE_REMOVING));
downloads.add(
copyDownloadWithState(terminalDownloads.get(i), STATE_REMOVING, STOP_REASON_NONE));
}
Collections.sort(downloads, InternalHandler::compareStartTimes);
try {
Expand Down Expand Up @@ -1019,7 +1020,7 @@ private Task syncQueuedDownload(@Nullable Task activeTask, Download download) {
}

// We can start a download task.
download = putDownloadWithState(download, STATE_DOWNLOADING);
download = putDownloadWithState(download, STATE_DOWNLOADING, STOP_REASON_NONE);
Downloader downloader = downloaderFactory.createDownloader(download.request);
activeTask =
new Task(
Expand All @@ -1041,7 +1042,7 @@ private void syncDownloadingDownload(
Task activeTask, Download download, int accumulatingDownloadTaskCount) {
Assertions.checkState(!activeTask.isRemove);
if (!canDownloadsRun() || accumulatingDownloadTaskCount >= maxParallelDownloads) {
putDownloadWithState(download, STATE_QUEUED);
putDownloadWithState(download, STATE_QUEUED, STOP_REASON_NONE);
activeTask.cancel(/* released= */ false);
}
}
Expand Down Expand Up @@ -1161,8 +1162,9 @@ private void onDownloadTaskStopped(Download download, @Nullable Exception finalE

private void onRemoveTaskStopped(Download download) {
if (download.state == STATE_RESTARTING) {
putDownloadWithState(
download, download.stopReason == STOP_REASON_NONE ? STATE_QUEUED : STATE_STOPPED);
@Download.State
int state = download.stopReason == STOP_REASON_NONE ? STATE_QUEUED : STATE_STOPPED;
putDownloadWithState(download, state, download.stopReason);
syncTasks();
} else {
int removeIndex = getDownloadIndex(download.request.id);
Expand Down Expand Up @@ -1204,12 +1206,11 @@ private boolean canDownloadsRun() {
return !downloadsPaused && notMetRequirements == 0;
}

private Download putDownloadWithState(Download download, @Download.State int state) {
// Downloads in terminal states shouldn't be in the downloads list. This method cannot be used
// to set STATE_STOPPED either, because it doesn't have a stopReason argument.
Assertions.checkState(
state != STATE_COMPLETED && state != STATE_FAILED && state != STATE_STOPPED);
return putDownload(copyDownloadWithState(download, state));
private Download putDownloadWithState(
Download download, @Download.State int state, int stopReason) {
// Downloads in terminal states shouldn't be in the downloads list.
Assertions.checkState(state != STATE_COMPLETED && state != STATE_FAILED);
return putDownload(copyDownloadWithState(download, state, stopReason));
}

private Download putDownload(Download download) {
Expand Down Expand Up @@ -1267,14 +1268,15 @@ private int getDownloadIndex(String id) {
return C.INDEX_UNSET;
}

private static Download copyDownloadWithState(Download download, @Download.State int state) {
private static Download copyDownloadWithState(
Download download, @Download.State int state, int stopReason) {
return new Download(
download.request,
state,
download.startTimeMs,
/* updateTimeMs= */ System.currentTimeMillis(),
download.contentLength,
/* stopReason= */ 0,
stopReason,
FAILURE_REASON_NONE,
download.progress);
}
Expand Down
58 changes: 58 additions & 0 deletions library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,64 @@ public void getCurrentDownloads_returnsCurrentDownloads() throws Throwable {
assertThat(download2.state).isEqualTo(Download.STATE_REMOVING);
}

@Test
public void addDownload_whilstRemovingWithStopReason_addsStartedDownload() throws Throwable {
runOnMainThread(
() -> downloadManager.addDownload(createDownloadRequest(ID1), /* stopReason= */ 1234));

postRemoveRequest(ID1);
FakeDownloader downloadRemover = getDownloaderAt(0);
downloadRemover.assertRemoveStarted();

// Re-add the download without a stop reason.
postDownloadRequest(ID1);

downloadRemover.finish();

FakeDownloader downloader = getDownloaderAt(1);
downloader.finish();

assertDownloadIndexSize(1);
// We expect one downloader for the removal, and one for when the download was re-added.
assertDownloaderCount(2);
// The download has completed, and so is no longer current.
assertCurrentDownloadCount(0);

Download download = postGetDownloadIndex().getDownload(ID1);
assertThat(download.state).isEqualTo(Download.STATE_COMPLETED);
assertThat(download.stopReason).isEqualTo(0);
}

/** Test for https://github.com/google/ExoPlayer/issues/8419 */
@Test
public void addDownloadWithStopReason_whilstRemoving_addsStoppedDownload() throws Throwable {
postDownloadRequest(ID1);
getDownloaderAt(0).finish();

postRemoveRequest(ID1);
FakeDownloader downloadRemover = getDownloaderAt(1);
downloadRemover.assertRemoveStarted();

// Re-add the download with a stop reason.
runOnMainThread(
() -> downloadManager.addDownload(createDownloadRequest(ID1), /* stopReason= */ 1234));

downloadRemover.finish();

assertDownloadIndexSize(1);
// We expect one downloader for the initial download, and one for the removal. A downloader
// should not be created when the download is re-added, since a stop reason is specified.
assertDownloaderCount(2);
// The download isn't completed, and is therefore still current.
assertCurrentDownloadCount(1);

List<Download> downloads = postGetCurrentDownloads();
Download download = downloads.get(0);
assertThat(download.request.id).isEqualTo(ID1);
assertThat(download.state).isEqualTo(Download.STATE_STOPPED);
assertThat(download.stopReason).isEqualTo(1234);
}

@Test
public void mergeRequest_removing_becomesRestarting() {
DownloadRequest downloadRequest = createDownloadRequest(ID1);
Expand Down

0 comments on commit 6d2d2e3

Please sign in to comment.