loliball/media
1
0
Fork 0
mirror of https://github.com/androidx/media.git synced 2025-04-30 06:46:50 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

Reduce use of DownloadInternal in DownloadManager

Browse Source 
The only state DownloadInternal holds is duplicate of state contained
in Download, but can confusingly be temporarily out of sync. This is
error prone because it's easy to use the wrong variable (e.g.
download.state vs state). DownloadInternals methods are called into
and call out into DownloadManager, which makes some code paths that
are quite hard to follow.

It's possible to simplify DownloadManager quite a lot by removing the
duplicated state in DownloadInternal, at which point DownloadInternal's
methods flatten into DownloadManager, which can just hold an internal
list of Downloads directly.

This is a first step, which makes it clear that DownloadThready only
needs its immutable DownloadAction + an isRemove flag.

PiperOrigin-RevId: 243245288
This commit is contained in:
olly 2019-04-12 13:09:14 +01:00 committed by Oliver Woodman
parent bd841b18f2
commit f623a9dea0
4 changed files with 49 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java
View File

@ -46,7 +46,7 @@ public final class Download {
// Important: These constants are persisted into DownloadIndex. Do not change them. // Important: These constants are persisted into DownloadIndex. Do not change them.
/** The download is waiting to be started. */ /** The download is waiting to be started. */
public static final int STATE_QUEUED = 0; public static final int STATE_QUEUED = 0;
/** The download is manually stopped for the reason specified by {@link #manualStopReason}. */ /** The download is stopped for a specified {@link #manualStopReason}. */
public static final int STATE_STOPPED = 1; public static final int STATE_STOPPED = 1;
/** The download is currently started. */ /** The download is currently started. */
public static final int STATE_DOWNLOADING = 2; public static final int STATE_DOWNLOADING = 2;
@ -94,18 +94,6 @@ public final class Download {
} }
} }
/** Returns the failure string for the given failure reason value. */
public static String getFailureString(@FailureReason int failureReason) {
switch (failureReason) {
case FAILURE_REASON_NONE:
return "NO_REASON";
case FAILURE_REASON_UNKNOWN:
return "UNKNOWN_REASON";
default:
throw new IllegalStateException();
}
}
/** The download action. */ /** The download action. */
public final DownloadAction action; public final DownloadAction action;

5
library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadAction.java
View File

@ -139,6 +139,11 @@ public final class DownloadAction implements Parcelable {
id, type, newAction.uri, mergedKeys, newAction.customCacheKey, newAction.data); id, type, newAction.uri, mergedKeys, newAction.customCacheKey, newAction.data);
} }
@Override
public String toString() {
return type + ":" + id;
}
@Override @Override
public boolean equals(@Nullable Object o) { public boolean equals(@Nullable Object o) {
if (!(o instanceof DownloadAction)) { if (!(o instanceof DownloadAction)) {

75
library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadManager.java
View File

@ -167,7 +167,7 @@ public final class DownloadManager {
// Collections that are accessed on the internal thread. // Collections that are accessed on the internal thread.
private final ArrayList<DownloadInternal> downloadInternals; private final ArrayList<DownloadInternal> downloadInternals;
private final HashMap<DownloadInternal, DownloadThread> activeDownloads; private final HashMap<String, DownloadThread> downloadThreads;
// Mutable fields that are accessed on the main thread. // Mutable fields that are accessed on the main thread.
private int pendingMessages; private int pendingMessages;
@ -250,7 +250,7 @@ public final class DownloadManager {
downloadInternals = new ArrayList<>(); downloadInternals = new ArrayList<>();
downloads = new ArrayList<>(); downloads = new ArrayList<>();
activeDownloads = new HashMap<>(); downloadThreads = new HashMap<>();
listeners = new CopyOnWriteArraySet<>(); listeners = new CopyOnWriteArraySet<>();
releaseLock = new Object(); releaseLock = new Object();
@ -568,7 +568,7 @@ public final class DownloadManager {
throw new IllegalStateException(); throw new IllegalStateException();
} }
mainHandler mainHandler
.obtainMessage(MSG_PROCESSED, processedExternalMessage ? 1 : 0, activeDownloads.size()) .obtainMessage(MSG_PROCESSED, processedExternalMessage ? 1 : 0, downloadThreads.size())
.sendToTarget(); .sendToTarget();
return true; return true;
} }
@ -677,16 +677,17 @@ public final class DownloadManager {
} }
private void onDownloadThreadStoppedInternal(DownloadThread downloadThread) { private void onDownloadThreadStoppedInternal(DownloadThread downloadThread) {
DownloadInternal downloadInternal = downloadThread.downloadInternal; logd("Download is stopped", downloadThread.action);
logd("Download is stopped", downloadInternal); String downloadId = downloadThread.action.id;
activeDownloads.remove(downloadInternal); downloadThreads.remove(downloadId);
boolean tryToStartDownloads = false; boolean tryToStartDownloads = false;
if (!downloadThread.isRemoveThread) { if (!downloadThread.isRemove) {
// If maxSimultaneousDownloads was hit, there might be a download waiting for a slot. // If maxSimultaneousDownloads was hit, there might be a download waiting for a slot.
tryToStartDownloads = simultaneousDownloads == maxSimultaneousDownloads; tryToStartDownloads = simultaneousDownloads == maxSimultaneousDownloads;
simultaneousDownloads--; simultaneousDownloads--;
} }
downloadInternal.onDownloadThreadStopped(downloadThread.isCanceled, downloadThread.finalError); getDownload(downloadId)
.onDownloadThreadStopped(downloadThread.isCanceled, downloadThread.finalError);
if (tryToStartDownloads) { if (tryToStartDownloads) {
for (int i = 0; for (int i = 0;
simultaneousDownloads < maxSimultaneousDownloads && i < downloadInternals.size(); simultaneousDownloads < maxSimultaneousDownloads && i < downloadInternals.size();
@ -697,8 +698,8 @@ public final class DownloadManager {
} }
private void releaseInternal() { private void releaseInternal() {
for (DownloadInternal downloadInternal : activeDownloads.keySet()) { for (String downloadId : downloadThreads.keySet()) {
stopDownloadThread(downloadInternal); stopDownloadThreadInternal(downloadId);
} }
internalThread.quit(); internalThread.quit();
synchronized (releaseLock) { synchronized (releaseLock) {
@ -733,30 +734,34 @@ public final class DownloadManager {
@StartThreadResults @StartThreadResults
private int startDownloadThread(DownloadInternal downloadInternal) { private int startDownloadThread(DownloadInternal downloadInternal) {
if (activeDownloads.containsKey(downloadInternal)) { DownloadAction action = downloadInternal.download.action;
if (stopDownloadThread(downloadInternal)) { String downloadId = action.id;
if (downloadThreads.containsKey(downloadId)) {
if (stopDownloadThreadInternal(downloadId)) {
return START_THREAD_WAIT_DOWNLOAD_CANCELLATION; return START_THREAD_WAIT_DOWNLOAD_CANCELLATION;
} }
return START_THREAD_WAIT_REMOVAL_TO_FINISH; return START_THREAD_WAIT_REMOVAL_TO_FINISH;
} }
if (!downloadInternal.isInRemoveState()) { boolean isRemove = downloadInternal.isInRemoveState();
if (!isRemove) {
if (simultaneousDownloads == maxSimultaneousDownloads) { if (simultaneousDownloads == maxSimultaneousDownloads) {
return START_THREAD_TOO_MANY_DOWNLOADS; return START_THREAD_TOO_MANY_DOWNLOADS;
} }
simultaneousDownloads++; simultaneousDownloads++;
} }
DownloadThread downloadThread = new DownloadThread(downloadInternal); DownloadThread downloadThread = new DownloadThread(action, isRemove);
activeDownloads.put(downloadInternal, downloadThread); downloadThreads.put(downloadId, downloadThread);
downloadInternal.setCounters(downloadThread.downloader.getCounters()); downloadInternal.setCounters(downloadThread.downloader.getCounters());
downloadThread.start();
logd("Download is started", downloadInternal); logd("Download is started", downloadInternal);
return START_THREAD_SUCCEEDED; return START_THREAD_SUCCEEDED;
} }
private boolean stopDownloadThread(DownloadInternal downloadInternal) { private boolean stopDownloadThreadInternal(String downloadId) {
DownloadThread downloadThread = activeDownloads.get(downloadInternal); DownloadThread downloadThread = downloadThreads.get(downloadId);
if (downloadThread != null && !downloadThread.isRemoveThread) { if (downloadThread != null && !downloadThread.isRemove) {
downloadThread.cancel(); downloadThread.cancel();
logd("Download is cancelled", downloadInternal); logd("Download is cancelled", downloadThread.action);
return true; return true;
} }
return false; return false;
@ -800,8 +805,12 @@ public final class DownloadManager {
} }
private static void logd(String message, DownloadInternal downloadInternal) { private static void logd(String message, DownloadInternal downloadInternal) {
logd(message, downloadInternal.download.action);
}
private static void logd(String message, DownloadAction action) {
if (DEBUG) { if (DEBUG) {
logd(message + ": " + downloadInternal); logd(message + ": " + action);
} }
} }
@ -812,14 +821,15 @@ public final class DownloadManager {
} }
private static final class DownloadInternal { private static final class DownloadInternal {
private final DownloadManager downloadManager; private final DownloadManager downloadManager;
private Download download; private Download download;
@MonotonicNonNull @Download.FailureReason private int failureReason;
// TODO: Get rid of these and use download directly. // TODO: Get rid of these and use download directly.
@Download.State private int state; @Download.State private int state;
private int manualStopReason; private int manualStopReason;
@MonotonicNonNull @Download.FailureReason private int failureReason;
private DownloadInternal(DownloadManager downloadManager, Download download) { private DownloadInternal(DownloadManager downloadManager, Download download) {
this.downloadManager = downloadManager; this.downloadManager = downloadManager;
@ -891,7 +901,7 @@ public final class DownloadManager {
} }
} else { } else {
if (state == STATE_DOWNLOADING || state == STATE_QUEUED) { if (state == STATE_DOWNLOADING || state == STATE_QUEUED) {
downloadManager.stopDownloadThread(this); downloadManager.stopDownloadThreadInternal(download.action.id);
setState(STATE_STOPPED); setState(STATE_STOPPED);
} }
} }
@ -962,18 +972,17 @@ public final class DownloadManager {
private class DownloadThread extends Thread { private class DownloadThread extends Thread {
private final DownloadInternal downloadInternal; private final DownloadAction action;
private final boolean isRemove;
private final Downloader downloader; private final Downloader downloader;
private final boolean isRemoveThread;
private volatile boolean isCanceled; private volatile boolean isCanceled;
private Throwable finalError; private Throwable finalError;
private DownloadThread(DownloadInternal downloadInternal) { private DownloadThread(DownloadAction action, boolean isRemove) {
this.downloadInternal = downloadInternal; this.action = action;
this.downloader = downloaderFactory.createDownloader(downloadInternal.download.action); this.isRemove = isRemove;
this.isRemoveThread = downloadInternal.isInRemoveState(); downloader = downloaderFactory.createDownloader(action);
start();
} }
public void cancel() { public void cancel() {
@ -986,9 +995,9 @@ public final class DownloadManager {
@Override @Override
public void run() { public void run() {
logd("Download started", downloadInternal); logd("Download started", action);
try { try {
if (isRemoveThread) { if (isRemove) {
downloader.remove(); downloader.remove();
} else { } else {
int errorCount = 0; int errorCount = 0;
@ -1001,14 +1010,14 @@ public final class DownloadManager {
if (!isCanceled) { if (!isCanceled) {
long downloadedBytes = downloader.getDownloadedBytes(); long downloadedBytes = downloader.getDownloadedBytes();
if (downloadedBytes != errorPosition) { if (downloadedBytes != errorPosition) {
logd("Reset error count. downloadedBytes = " + downloadedBytes, downloadInternal); logd("Reset error count. downloadedBytes = " + downloadedBytes, action);
errorPosition = downloadedBytes; errorPosition = downloadedBytes;
errorCount = 0; errorCount = 0;
} }
if (++errorCount > minRetryCount) { if (++errorCount > minRetryCount) {
throw e; throw e;
} }
logd("Download error. Retry " + errorCount, downloadInternal); logd("Download error. Retry " + errorCount, action);
Thread.sleep(getRetryDelayMillis(errorCount)); Thread.sleep(getRetryDelayMillis(errorCount));
} }
} }

2
testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java
View File

@ -95,7 +95,7 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen
public void blockUntilTasksCompleteAndThrowAnyDownloadError() throws Throwable { public void blockUntilTasksCompleteAndThrowAnyDownloadError() throws Throwable {
blockUntilTasksComplete(); blockUntilTasksComplete();
if (failureReason != Download.FAILURE_REASON_NONE) { if (failureReason != Download.FAILURE_REASON_NONE) {
throw new Exception("Failure reason: " + Download.getFailureString(failureReason)); throw new Exception("Failure reason: " + failureReason);
} }
} }