From f623a9dea0afbca26a28e4a1f91998575d774864 Mon Sep 17 00:00:00 2001 From: olly Date: Fri, 12 Apr 2019 13:09:14 +0100 Subject: [PATCH] Reduce use of DownloadInternal in DownloadManager 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 --- .../android/exoplayer2/offline/Download.java | 14 +--- .../exoplayer2/offline/DownloadAction.java | 5 ++ .../exoplayer2/offline/DownloadManager.java | 75 +++++++++++-------- .../testutil/TestDownloadManagerListener.java | 2 +- 4 files changed, 49 insertions(+), 47 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java index 811df3b0d4..911d823f07 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java @@ -46,7 +46,7 @@ public final class Download { // Important: These constants are persisted into DownloadIndex. Do not change them. /** The download is waiting to be started. */ 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; /** The download is currently started. */ 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. */ public final DownloadAction action; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadAction.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadAction.java index 63916ff1a2..08e6c2aafe 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadAction.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadAction.java @@ -139,6 +139,11 @@ public final class DownloadAction implements Parcelable { id, type, newAction.uri, mergedKeys, newAction.customCacheKey, newAction.data); } + @Override + public String toString() { + return type + ":" + id; + } + @Override public boolean equals(@Nullable Object o) { if (!(o instanceof DownloadAction)) { diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadManager.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadManager.java index fe1772e829..dbc953b8c4 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadManager.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadManager.java @@ -167,7 +167,7 @@ public final class DownloadManager { // Collections that are accessed on the internal thread. private final ArrayList downloadInternals; - private final HashMap activeDownloads; + private final HashMap downloadThreads; // Mutable fields that are accessed on the main thread. private int pendingMessages; @@ -250,7 +250,7 @@ public final class DownloadManager { downloadInternals = new ArrayList<>(); downloads = new ArrayList<>(); - activeDownloads = new HashMap<>(); + downloadThreads = new HashMap<>(); listeners = new CopyOnWriteArraySet<>(); releaseLock = new Object(); @@ -568,7 +568,7 @@ public final class DownloadManager { throw new IllegalStateException(); } mainHandler - .obtainMessage(MSG_PROCESSED, processedExternalMessage ? 1 : 0, activeDownloads.size()) + .obtainMessage(MSG_PROCESSED, processedExternalMessage ? 1 : 0, downloadThreads.size()) .sendToTarget(); return true; } @@ -677,16 +677,17 @@ public final class DownloadManager { } private void onDownloadThreadStoppedInternal(DownloadThread downloadThread) { - DownloadInternal downloadInternal = downloadThread.downloadInternal; - logd("Download is stopped", downloadInternal); - activeDownloads.remove(downloadInternal); + logd("Download is stopped", downloadThread.action); + String downloadId = downloadThread.action.id; + downloadThreads.remove(downloadId); boolean tryToStartDownloads = false; - if (!downloadThread.isRemoveThread) { + if (!downloadThread.isRemove) { // If maxSimultaneousDownloads was hit, there might be a download waiting for a slot. tryToStartDownloads = simultaneousDownloads == maxSimultaneousDownloads; simultaneousDownloads--; } - downloadInternal.onDownloadThreadStopped(downloadThread.isCanceled, downloadThread.finalError); + getDownload(downloadId) + .onDownloadThreadStopped(downloadThread.isCanceled, downloadThread.finalError); if (tryToStartDownloads) { for (int i = 0; simultaneousDownloads < maxSimultaneousDownloads && i < downloadInternals.size(); @@ -697,8 +698,8 @@ public final class DownloadManager { } private void releaseInternal() { - for (DownloadInternal downloadInternal : activeDownloads.keySet()) { - stopDownloadThread(downloadInternal); + for (String downloadId : downloadThreads.keySet()) { + stopDownloadThreadInternal(downloadId); } internalThread.quit(); synchronized (releaseLock) { @@ -733,30 +734,34 @@ public final class DownloadManager { @StartThreadResults private int startDownloadThread(DownloadInternal downloadInternal) { - if (activeDownloads.containsKey(downloadInternal)) { - if (stopDownloadThread(downloadInternal)) { + DownloadAction action = downloadInternal.download.action; + String downloadId = action.id; + if (downloadThreads.containsKey(downloadId)) { + if (stopDownloadThreadInternal(downloadId)) { return START_THREAD_WAIT_DOWNLOAD_CANCELLATION; } return START_THREAD_WAIT_REMOVAL_TO_FINISH; } - if (!downloadInternal.isInRemoveState()) { + boolean isRemove = downloadInternal.isInRemoveState(); + if (!isRemove) { if (simultaneousDownloads == maxSimultaneousDownloads) { return START_THREAD_TOO_MANY_DOWNLOADS; } simultaneousDownloads++; } - DownloadThread downloadThread = new DownloadThread(downloadInternal); - activeDownloads.put(downloadInternal, downloadThread); + DownloadThread downloadThread = new DownloadThread(action, isRemove); + downloadThreads.put(downloadId, downloadThread); downloadInternal.setCounters(downloadThread.downloader.getCounters()); + downloadThread.start(); logd("Download is started", downloadInternal); return START_THREAD_SUCCEEDED; } - private boolean stopDownloadThread(DownloadInternal downloadInternal) { - DownloadThread downloadThread = activeDownloads.get(downloadInternal); - if (downloadThread != null && !downloadThread.isRemoveThread) { + private boolean stopDownloadThreadInternal(String downloadId) { + DownloadThread downloadThread = downloadThreads.get(downloadId); + if (downloadThread != null && !downloadThread.isRemove) { downloadThread.cancel(); - logd("Download is cancelled", downloadInternal); + logd("Download is cancelled", downloadThread.action); return true; } return false; @@ -800,8 +805,12 @@ public final class DownloadManager { } private static void logd(String message, DownloadInternal downloadInternal) { + logd(message, downloadInternal.download.action); + } + + private static void logd(String message, DownloadAction action) { if (DEBUG) { - logd(message + ": " + downloadInternal); + logd(message + ": " + action); } } @@ -812,14 +821,15 @@ public final class DownloadManager { } private static final class DownloadInternal { + private final DownloadManager downloadManager; private Download download; - @MonotonicNonNull @Download.FailureReason private int failureReason; // TODO: Get rid of these and use download directly. @Download.State private int state; private int manualStopReason; + @MonotonicNonNull @Download.FailureReason private int failureReason; private DownloadInternal(DownloadManager downloadManager, Download download) { this.downloadManager = downloadManager; @@ -891,7 +901,7 @@ public final class DownloadManager { } } else { if (state == STATE_DOWNLOADING || state == STATE_QUEUED) { - downloadManager.stopDownloadThread(this); + downloadManager.stopDownloadThreadInternal(download.action.id); setState(STATE_STOPPED); } } @@ -962,18 +972,17 @@ public final class DownloadManager { private class DownloadThread extends Thread { - private final DownloadInternal downloadInternal; + private final DownloadAction action; + private final boolean isRemove; private final Downloader downloader; - private final boolean isRemoveThread; private volatile boolean isCanceled; private Throwable finalError; - private DownloadThread(DownloadInternal downloadInternal) { - this.downloadInternal = downloadInternal; - this.downloader = downloaderFactory.createDownloader(downloadInternal.download.action); - this.isRemoveThread = downloadInternal.isInRemoveState(); - start(); + private DownloadThread(DownloadAction action, boolean isRemove) { + this.action = action; + this.isRemove = isRemove; + downloader = downloaderFactory.createDownloader(action); } public void cancel() { @@ -986,9 +995,9 @@ public final class DownloadManager { @Override public void run() { - logd("Download started", downloadInternal); + logd("Download started", action); try { - if (isRemoveThread) { + if (isRemove) { downloader.remove(); } else { int errorCount = 0; @@ -1001,14 +1010,14 @@ public final class DownloadManager { if (!isCanceled) { long downloadedBytes = downloader.getDownloadedBytes(); if (downloadedBytes != errorPosition) { - logd("Reset error count. downloadedBytes = " + downloadedBytes, downloadInternal); + logd("Reset error count. downloadedBytes = " + downloadedBytes, action); errorPosition = downloadedBytes; errorCount = 0; } if (++errorCount > minRetryCount) { throw e; } - logd("Download error. Retry " + errorCount, downloadInternal); + logd("Download error. Retry " + errorCount, action); Thread.sleep(getRetryDelayMillis(errorCount)); } } diff --git a/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java b/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java index 867888e91c..1677243f27 100644 --- a/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java +++ b/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java @@ -95,7 +95,7 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen public void blockUntilTasksCompleteAndThrowAnyDownloadError() throws Throwable { blockUntilTasksComplete(); if (failureReason != Download.FAILURE_REASON_NONE) { - throw new Exception("Failure reason: " + Download.getFailureString(failureReason)); + throw new Exception("Failure reason: " + failureReason); } }