From 55154ad475eb3f498fcf31a7430bf6354b526fdb Mon Sep 17 00:00:00 2001 From: Matthias Tamegger Date: Fri, 12 Apr 2019 10:49:48 +0200 Subject: [PATCH 01/11] Allow hex format tags when parsing url templates --- .../android/exoplayer2/source/dash/manifest/UrlTemplate.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/UrlTemplate.java b/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/UrlTemplate.java index a7ce7eb9a0..370db184b0 100644 --- a/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/UrlTemplate.java +++ b/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/UrlTemplate.java @@ -139,7 +139,7 @@ public final class UrlTemplate { String formatTag = DEFAULT_FORMAT_TAG; if (formatTagIndex != -1) { formatTag = identifier.substring(formatTagIndex); - if (!formatTag.endsWith("d")) { + if (!formatTag.endsWith("d") && !formatTag.endsWith("x")) { formatTag += "d"; } identifier = identifier.substring(0, formatTagIndex); From eed5d957d87d44cb9c716f1a4c80f39ad2a6a442 Mon Sep 17 00:00:00 2001 From: olly Date: Wed, 1 May 2019 19:19:02 +0100 Subject: [PATCH 02/11] Rework DownloadManager to fix remaining TODOs - Removed DownloadInternal and its sometimes-out-of-sync duplicate state - Fixed downloads being in STOPPED rather than QUEUED state when the manager is paused - Fixed setMaxParallelDownloads to start/stop downloads if necessary when the value changes - Fixed isWaitingForRequirements PiperOrigin-RevId: 246164845 --- .../offline/ActionFileUpgradeUtil.java | 9 +- .../offline/DefaultDownloadIndex.java | 24 +- .../exoplayer2/offline/DownloadManager.java | 836 +++++++++--------- .../offline/WritableDownloadIndex.java | 7 + .../offline/ActionFileUpgradeUtilTest.java | 14 +- .../offline/DownloadManagerTest.java | 54 +- 6 files changed, 473 insertions(+), 471 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/ActionFileUpgradeUtil.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/ActionFileUpgradeUtil.java index 975fc10b93..baf47772ab 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/ActionFileUpgradeUtil.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/ActionFileUpgradeUtil.java @@ -67,11 +67,12 @@ public final class ActionFileUpgradeUtil { if (actionFile.exists()) { boolean success = false; try { + long nowMs = System.currentTimeMillis(); for (DownloadRequest request : actionFile.load()) { if (downloadIdProvider != null) { request = request.copyWithId(downloadIdProvider.getId(request)); } - mergeRequest(request, downloadIndex, addNewDownloadsAsCompleted); + mergeRequest(request, downloadIndex, addNewDownloadsAsCompleted, nowMs); } success = true; } finally { @@ -93,13 +94,13 @@ public final class ActionFileUpgradeUtil { /* package */ static void mergeRequest( DownloadRequest request, DefaultDownloadIndex downloadIndex, - boolean addNewDownloadAsCompleted) + boolean addNewDownloadAsCompleted, + long nowMs) throws IOException { Download download = downloadIndex.getDownload(request.id); if (download != null) { - download = DownloadManager.mergeRequest(download, request, download.stopReason); + download = DownloadManager.mergeRequest(download, request, download.stopReason, nowMs); } else { - long nowMs = System.currentTimeMillis(); download = new Download( request, diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java index 252c058b88..06f308d1e9 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java @@ -69,7 +69,9 @@ public final class DefaultDownloadIndex implements WritableDownloadIndex { private static final int COLUMN_INDEX_BYTES_DOWNLOADED = 13; private static final String WHERE_ID_EQUALS = COLUMN_ID + " = ?"; - private static final String WHERE_STATE_TERMINAL = + private static final String WHERE_STATE_IS_DOWNLOADING = + COLUMN_STATE + " = " + Download.STATE_DOWNLOADING; + private static final String WHERE_STATE_IS_TERMINAL = getStateQuery(Download.STATE_COMPLETED, Download.STATE_FAILED); private static final String[] COLUMNS = @@ -218,6 +220,19 @@ public final class DefaultDownloadIndex implements WritableDownloadIndex { } } + @Override + public void setDownloadingStatesToQueued() throws DatabaseIOException { + ensureInitialized(); + try { + ContentValues values = new ContentValues(); + values.put(COLUMN_STATE, Download.STATE_QUEUED); + SQLiteDatabase writableDatabase = databaseProvider.getWritableDatabase(); + writableDatabase.update(tableName, values, WHERE_STATE_IS_DOWNLOADING, /* whereArgs= */ null); + } catch (SQLException e) { + throw new DatabaseIOException(e); + } + } + @Override public void setStopReason(int stopReason) throws DatabaseIOException { ensureInitialized(); @@ -225,7 +240,7 @@ public final class DefaultDownloadIndex implements WritableDownloadIndex { ContentValues values = new ContentValues(); values.put(COLUMN_STOP_REASON, stopReason); SQLiteDatabase writableDatabase = databaseProvider.getWritableDatabase(); - writableDatabase.update(tableName, values, WHERE_STATE_TERMINAL, /* whereArgs= */ null); + writableDatabase.update(tableName, values, WHERE_STATE_IS_TERMINAL, /* whereArgs= */ null); } catch (SQLException e) { throw new DatabaseIOException(e); } @@ -239,7 +254,10 @@ public final class DefaultDownloadIndex implements WritableDownloadIndex { values.put(COLUMN_STOP_REASON, stopReason); SQLiteDatabase writableDatabase = databaseProvider.getWritableDatabase(); writableDatabase.update( - tableName, values, WHERE_STATE_TERMINAL + " AND " + WHERE_ID_EQUALS, new String[] {id}); + tableName, + values, + WHERE_STATE_IS_TERMINAL + " AND " + WHERE_ID_EQUALS, + new String[] {id}); } catch (SQLException e) { throw new DatabaseIOException(e); } 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 8502a56ea7..b528d91759 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 @@ -31,7 +31,6 @@ import android.os.Handler; import android.os.HandlerThread; import android.os.Looper; import android.os.Message; -import androidx.annotation.IntDef; import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.database.DatabaseProvider; @@ -46,14 +45,11 @@ import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.Util; import java.io.IOException; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.concurrent.CopyOnWriteArraySet; -import org.checkerframework.checker.nullness.qual.MonotonicNonNull; /** * Manages downloads. @@ -125,8 +121,7 @@ public final class DownloadManager { // Messages posted to the main handler. private static final int MSG_INITIALIZED = 0; private static final int MSG_PROCESSED = 1; - private static final int MSG_DOWNLOAD_CHANGED = 2; - private static final int MSG_DOWNLOAD_REMOVED = 3; + private static final int MSG_DOWNLOAD_UPDATE = 2; // Messages posted to the background handler. private static final int MSG_INITIALIZE = 0; @@ -141,31 +136,14 @@ public final class DownloadManager { private static final int MSG_CONTENT_LENGTH_CHANGED = 9; private static final int MSG_RELEASE = 10; - @Retention(RetentionPolicy.SOURCE) - @IntDef({ - START_THREAD_SUCCEEDED, - START_THREAD_WAIT_REMOVAL_TO_FINISH, - START_THREAD_WAIT_DOWNLOAD_CANCELLATION, - START_THREAD_TOO_MANY_DOWNLOADS - }) - private @interface StartThreadResults {} - - private static final int START_THREAD_SUCCEEDED = 0; - private static final int START_THREAD_WAIT_REMOVAL_TO_FINISH = 1; - private static final int START_THREAD_WAIT_DOWNLOAD_CANCELLATION = 2; - private static final int START_THREAD_TOO_MANY_DOWNLOADS = 3; - private static final String TAG = "DownloadManager"; - private static final boolean DEBUG = false; private final Context context; private final WritableDownloadIndex downloadIndex; private final Handler mainHandler; private final InternalHandler internalHandler; private final RequirementsWatcher.Listener requirementsListener; - private final CopyOnWriteArraySet listeners; - private final ArrayList downloads; private int pendingMessages; private int activeTaskCount; @@ -174,6 +152,7 @@ public final class DownloadManager { private int maxParallelDownloads; private int minRetryCount; private int notMetRequirements; + private List downloads; private RequirementsWatcher requirementsWatcher; /** @@ -205,11 +184,13 @@ public final class DownloadManager { Context context, WritableDownloadIndex downloadIndex, DownloaderFactory downloaderFactory) { this.context = context.getApplicationContext(); this.downloadIndex = downloadIndex; + maxParallelDownloads = DEFAULT_MAX_PARALLEL_DOWNLOADS; minRetryCount = DEFAULT_MIN_RETRY_COUNT; downloadsPaused = true; - downloads = new ArrayList<>(); + downloads = Collections.emptyList(); listeners = new CopyOnWriteArraySet<>(); + requirementsListener = this::onRequirementsStateChanged; requirementsWatcher = new RequirementsWatcher(context, requirementsListener, DEFAULT_REQUIREMENTS); @@ -253,8 +234,14 @@ public final class DownloadManager { * reason that the {@link #getRequirements() Requirements} are not met. */ public boolean isWaitingForRequirements() { - // TODO: Fix this to return the right thing. - return !downloads.isEmpty(); + if (!downloadsPaused && notMetRequirements != 0) { + for (int i = 0; i < downloads.size(); i++) { + if (downloads.get(i).state == STATE_QUEUED) { + return true; + } + } + } + return false; } /** @@ -362,7 +349,7 @@ public final class DownloadManager { * #getDownloadIndex()} instead. */ public List getCurrentDownloads() { - return Collections.unmodifiableList(new ArrayList<>(downloads)); + return downloads; } /** Returns whether downloads are currently paused. */ @@ -475,10 +462,10 @@ public final class DownloadManager { } mainHandler.removeCallbacksAndMessages(/* token= */ null); // Reset state. + downloads = Collections.emptyList(); pendingMessages = 0; activeTaskCount = 0; initialized = false; - downloads.clear(); } } @@ -508,13 +495,9 @@ public final class DownloadManager { List downloads = (List) message.obj; onInitialized(downloads); break; - case MSG_DOWNLOAD_CHANGED: - Download state = (Download) message.obj; - onDownloadChanged(state); - break; - case MSG_DOWNLOAD_REMOVED: - state = (Download) message.obj; - onDownloadRemoved(state); + case MSG_DOWNLOAD_UPDATE: + DownloadUpdate update = (DownloadUpdate) message.obj; + onDownloadUpdate(update); break; case MSG_PROCESSED: int processedMessageCount = message.arg1; @@ -529,32 +512,23 @@ public final class DownloadManager { private void onInitialized(List downloads) { initialized = true; - this.downloads.addAll(downloads); + this.downloads = Collections.unmodifiableList(downloads); for (Listener listener : listeners) { listener.onInitialized(DownloadManager.this); } } - private void onDownloadChanged(Download download) { - int downloadIndex = getDownloadIndex(download.request.id); - if (download.isTerminalState()) { - if (downloadIndex != C.INDEX_UNSET) { - downloads.remove(downloadIndex); + private void onDownloadUpdate(DownloadUpdate update) { + downloads = Collections.unmodifiableList(update.downloads); + Download updatedDownload = update.download; + if (update.isRemove) { + for (Listener listener : listeners) { + listener.onDownloadRemoved(this, updatedDownload); } - } else if (downloadIndex != C.INDEX_UNSET) { - downloads.set(downloadIndex, download); } else { - downloads.add(download); - } - for (Listener listener : listeners) { - listener.onDownloadChanged(this, download); - } - } - - private void onDownloadRemoved(Download download) { - downloads.remove(getDownloadIndex(download.request.id)); - for (Listener listener : listeners) { - listener.onDownloadRemoved(this, download); + for (Listener listener : listeners) { + listener.onDownloadChanged(this, updatedDownload); + } } } @@ -568,18 +542,14 @@ public final class DownloadManager { } } - private int getDownloadIndex(String id) { - for (int i = 0; i < downloads.size(); i++) { - if (downloads.get(i).request.id.equals(id)) { - return i; - } - } - return C.INDEX_UNSET; - } - /* package */ static Download mergeRequest( - Download download, DownloadRequest request, int stopReason) { + Download download, DownloadRequest request, int stopReason, long nowMs) { @Download.State int state = download.state; + // Treat the merge as creating a new download if we're currently removing the existing one, or + // if the existing download is in a terminal state. Else treat the merge as updating the + // existing download. + long startTimeMs = + state == STATE_REMOVING || download.isTerminalState() ? nowMs : download.startTimeMs; if (state == STATE_REMOVING || state == STATE_RESTARTING) { state = STATE_RESTARTING; } else if (stopReason != STOP_REASON_NONE) { @@ -587,8 +557,6 @@ public final class DownloadManager { } else { state = STATE_QUEUED; } - long nowMs = System.currentTimeMillis(); - long startTimeMs = download.isTerminalState() ? nowMs : download.startTimeMs; return new Download( download.request.copyWithMergedRequest(request), state, @@ -599,40 +567,6 @@ public final class DownloadManager { FAILURE_REASON_NONE); } - private static Download copyWithState(Download download, @Download.State int state) { - return new Download( - download.request, - state, - download.startTimeMs, - /* updateTimeMs= */ System.currentTimeMillis(), - download.contentLength, - download.stopReason, - FAILURE_REASON_NONE, - download.progress); - } - - private static void logd(String message) { - if (DEBUG) { - Log.d(TAG, message); - } - } - - private static void logd(String message, DownloadInternal downloadInternal) { - logd(message, downloadInternal.download.request); - } - - private static void logd(String message, DownloadRequest request) { - if (DEBUG) { - logd(message + ": " + request); - } - } - - private static void logdFlags(String message, int flags) { - if (DEBUG) { - logd(message + ": " + Integer.toBinaryString(flags)); - } - } - private static final class InternalHandler extends Handler { public boolean released; @@ -641,15 +575,14 @@ public final class DownloadManager { private final WritableDownloadIndex downloadIndex; private final DownloaderFactory downloaderFactory; private final Handler mainHandler; - private final ArrayList downloadInternals; + private final ArrayList downloads; private final HashMap activeTasks; - // Mutable fields that are accessed on the internal thread. @Requirements.RequirementFlags private int notMetRequirements; private boolean downloadsPaused; private int maxParallelDownloads; private int minRetryCount; - private int parallelDownloads; + private int activeDownloadTaskCount; public InternalHandler( HandlerThread thread, @@ -667,7 +600,7 @@ public final class DownloadManager { this.maxParallelDownloads = maxParallelDownloads; this.minRetryCount = minRetryCount; this.downloadsPaused = downloadsPaused; - downloadInternals = new ArrayList<>(); + downloads = new ArrayList<>(); activeTasks = new HashMap<>(); } @@ -732,70 +665,91 @@ public final class DownloadManager { private void initialize(int notMetRequirements) { this.notMetRequirements = notMetRequirements; - ArrayList loadedStates = new ArrayList<>(); - try (DownloadCursor cursor = - downloadIndex.getDownloads( - STATE_QUEUED, STATE_STOPPED, STATE_DOWNLOADING, STATE_REMOVING, STATE_RESTARTING)) { + DownloadCursor cursor = null; + try { + downloadIndex.setDownloadingStatesToQueued(); + cursor = + downloadIndex.getDownloads( + STATE_QUEUED, STATE_STOPPED, STATE_DOWNLOADING, STATE_REMOVING, STATE_RESTARTING); while (cursor.moveToNext()) { - loadedStates.add(cursor.getDownload()); + downloads.add(cursor.getDownload()); } - logd("Downloads are loaded."); - } catch (Throwable e) { - Log.e(TAG, "Download state loading failed.", e); - loadedStates.clear(); - } - for (Download download : loadedStates) { - addDownloadForState(download); - } - logd("Downloads are created."); - mainHandler.obtainMessage(MSG_INITIALIZED, loadedStates).sendToTarget(); - for (int i = 0; i < downloadInternals.size(); i++) { - downloadInternals.get(i).start(); + } catch (IOException e) { + Log.e(TAG, "Failed to load index.", e); + downloads.clear(); + } finally { + Util.closeQuietly(cursor); } + // A copy must be used for the message to ensure that subsequent changes to the downloads list + // are not visible to the main thread when it processes the message. + ArrayList downloadsForMessage = new ArrayList<>(downloads); + mainHandler.obtainMessage(MSG_INITIALIZED, downloadsForMessage).sendToTarget(); + syncTasks(); } private void setDownloadsPaused(boolean downloadsPaused) { this.downloadsPaused = downloadsPaused; - for (int i = 0; i < downloadInternals.size(); i++) { - downloadInternals.get(i).updateStopState(); - } + syncTasks(); } private void setNotMetRequirements(@Requirements.RequirementFlags int notMetRequirements) { this.notMetRequirements = notMetRequirements; - logdFlags("Not met requirements are changed", notMetRequirements); - for (int i = 0; i < downloadInternals.size(); i++) { - downloadInternals.get(i).updateStopState(); - } + syncTasks(); } private void setStopReason(@Nullable String id, int stopReason) { - if (id != null) { - DownloadInternal downloadInternal = getDownload(id); - if (downloadInternal != null) { - logd("download stop reason is set to : " + stopReason, downloadInternal); - downloadInternal.setStopReason(stopReason); - return; + if (id == null) { + for (int i = 0; i < downloads.size(); i++) { + setStopReason(downloads.get(i), stopReason); + } + try { + // Set the stop reason for downloads in terminal states as well. + downloadIndex.setStopReason(stopReason); + } catch (IOException e) { + Log.e(TAG, "Failed to set manual stop reason", e); } } else { - for (int i = 0; i < downloadInternals.size(); i++) { - downloadInternals.get(i).setStopReason(stopReason); + Download download = getDownload(id, /* loadFromIndex= */ false); + if (download != null) { + setStopReason(download, stopReason); + } else { + try { + // Set the stop reason if the download is in a terminal state. + downloadIndex.setStopReason(id, stopReason); + } catch (IOException e) { + Log.e(TAG, "Failed to set manual stop reason: " + id, e); + } } } - try { - if (id != null) { - downloadIndex.setStopReason(id, stopReason); - } else { - downloadIndex.setStopReason(stopReason); + syncTasks(); + } + + private void setStopReason(Download download, int stopReason) { + if (stopReason == STOP_REASON_NONE) { + if (download.state == STATE_STOPPED) { + putDownloadWithState(download, STATE_QUEUED); } - } catch (IOException e) { - Log.e(TAG, "setStopReason failed", e); + } else if (stopReason != download.stopReason) { + @Download.State int state = download.state; + if (state == STATE_QUEUED || state == STATE_DOWNLOADING) { + state = STATE_STOPPED; + } + putDownload( + new Download( + download.request, + state, + download.startTimeMs, + /* updateTimeMs= */ System.currentTimeMillis(), + download.contentLength, + stopReason, + FAILURE_REASON_NONE, + download.progress)); } } private void setMaxParallelDownloads(int maxParallelDownloads) { this.maxParallelDownloads = maxParallelDownloads; - // TODO: Start or stop downloads if necessary. + syncTasks(); } private void setMinRetryCount(int minRetryCount) { @@ -803,77 +757,44 @@ public final class DownloadManager { } private void addDownload(DownloadRequest request, int stopReason) { - DownloadInternal downloadInternal = getDownload(request.id); - if (downloadInternal != null) { - downloadInternal.addRequest(request, stopReason); - logd("Request is added to existing download", downloadInternal); + Download download = getDownload(request.id, /* loadFromIndex= */ true); + long nowMs = System.currentTimeMillis(); + if (download != null) { + putDownload(mergeRequest(download, request, stopReason, nowMs)); } else { - Download download = loadDownload(request.id); - if (download == null) { - long nowMs = System.currentTimeMillis(); - download = - new Download( - request, - stopReason != Download.STOP_REASON_NONE ? STATE_STOPPED : STATE_QUEUED, - /* startTimeMs= */ nowMs, - /* updateTimeMs= */ nowMs, - /* contentLength= */ C.LENGTH_UNSET, - stopReason, - Download.FAILURE_REASON_NONE); - logd("Download state is created for " + request.id); - } else { - download = mergeRequest(download, request, stopReason); - logd("Download state is loaded for " + request.id); - } - addDownloadForState(download); + putDownload( + new Download( + request, + stopReason != STOP_REASON_NONE ? STATE_STOPPED : STATE_QUEUED, + /* startTimeMs= */ nowMs, + /* updateTimeMs= */ nowMs, + /* contentLength= */ C.LENGTH_UNSET, + stopReason, + FAILURE_REASON_NONE)); } + syncTasks(); } private void removeDownload(String id) { - DownloadInternal downloadInternal = getDownload(id); - if (downloadInternal != null) { - downloadInternal.remove(); - } else { - Download download = loadDownload(id); - if (download != null) { - addDownloadForState(copyWithState(download, STATE_REMOVING)); - } else { - logd("Can't remove download. No download with id: " + id); - } + Download download = getDownload(id, /* loadFromIndex= */ true); + if (download == null) { + Log.e(TAG, "Failed to remove nonexistent download: " + id); + return; } - } - - private void onTaskStopped(Task task) { - logd("Task is stopped", task.request); - String downloadId = task.request.id; - activeTasks.remove(downloadId); - boolean tryToStartDownloads = false; - if (!task.isRemove) { - // If maxParallelDownloads was hit, there might be a download waiting for a slot. - tryToStartDownloads = parallelDownloads == maxParallelDownloads; - parallelDownloads--; - } - getDownload(downloadId).onTaskStopped(task.isCanceled, task.finalError); - if (tryToStartDownloads) { - for (int i = 0; - parallelDownloads < maxParallelDownloads && i < downloadInternals.size(); - i++) { - downloadInternals.get(i).start(); - } - } - } - - private void onContentLengthChanged(Task task) { - String downloadId = task.request.id; - getDownload(downloadId).setContentLength(task.contentLength); + putDownloadWithState(download, STATE_REMOVING); + syncTasks(); } private void release() { for (Task task : activeTasks.values()) { task.cancel(/* released= */ true); } - activeTasks.clear(); - downloadInternals.clear(); + try { + downloadIndex.setDownloadingStatesToQueued(); + } catch (IOException e) { + Log.e(TAG, "Failed to update index.", e); + } + downloads.clear(); thread.quit(); synchronized (this) { released = true; @@ -881,261 +802,293 @@ public final class DownloadManager { } } - private void onDownloadChanged(DownloadInternal downloadInternal, Download download) { - logd("Download state is changed", downloadInternal); + // Start and cancel tasks based on the current download and manager states. + + private void syncTasks() { + int accumulatingDownloadTaskCount = 0; + for (int i = 0; i < downloads.size(); i++) { + Download download = downloads.get(i); + Task activeTask = activeTasks.get(download.request.id); + switch (download.state) { + case STATE_STOPPED: + syncStoppedDownload(activeTask); + break; + case STATE_QUEUED: + activeTask = syncQueuedDownload(activeTask, download); + break; + case STATE_DOWNLOADING: + activeTask = Assertions.checkNotNull(activeTask); + syncDownloadingDownload(activeTask, download, accumulatingDownloadTaskCount); + break; + case STATE_REMOVING: + case STATE_RESTARTING: + syncRemovingDownload(activeTask, download); + break; + case STATE_COMPLETED: + case STATE_FAILED: + default: + throw new IllegalStateException(); + } + if (activeTask != null && !activeTask.isRemove) { + accumulatingDownloadTaskCount++; + } + } + } + + private void syncStoppedDownload(@Nullable Task activeTask) { + if (activeTask != null) { + // We have a task, which must be a download task. Cancel it. + Assertions.checkState(!activeTask.isRemove); + activeTask.cancel(/* released= */ false); + } + } + + private Task syncQueuedDownload(@Nullable Task activeTask, Download download) { + if (activeTask != null) { + // We have a task, which must be a download task. If the download state is queued we need to + // cancel it and start a new one, since a new request has been merged into the download. + Assertions.checkState(!activeTask.isRemove); + activeTask.cancel(/* released= */ false); + return activeTask; + } + + if (!canDownloadsRun() || activeDownloadTaskCount >= maxParallelDownloads) { + return null; + } + + // We can start a download task. + download = putDownloadWithState(download, STATE_DOWNLOADING); + Downloader downloader = downloaderFactory.createDownloader(download.request); + activeTask = + new Task( + download.request, + downloader, + download.progress, + /* isRemove= */ false, + minRetryCount, + /* internalHandler= */ this); + activeTasks.put(download.request.id, activeTask); + activeDownloadTaskCount++; + activeTask.start(); + return activeTask; + } + + private void syncDownloadingDownload( + Task activeTask, Download download, int accumulatingDownloadTaskCount) { + Assertions.checkState(!activeTask.isRemove); + if (!canDownloadsRun() || accumulatingDownloadTaskCount >= maxParallelDownloads) { + putDownloadWithState(download, STATE_QUEUED); + activeTask.cancel(/* released= */ false); + } + } + + private void syncRemovingDownload(@Nullable Task activeTask, Download download) { + if (activeTask != null) { + if (!activeTask.isRemove) { + // Cancel the downloading task. + activeTask.cancel(/* released= */ false); + } + // The activeTask is either a remove task, or a downloading task that we just cancelled. In + // the latter case we need to wait for the task to stop before we start a remove task. + return; + } + + // We can start a remove task. + Downloader downloader = downloaderFactory.createDownloader(download.request); + activeTask = + new Task( + download.request, + downloader, + download.progress, + /* isRemove= */ true, + minRetryCount, + /* internalHandler= */ this); + activeTasks.put(download.request.id, activeTask); + activeTask.start(); + } + + // Task event processing. + + private void onContentLengthChanged(Task task) { + String downloadId = task.request.id; + long contentLength = task.contentLength; + Download download = getDownload(downloadId, /* loadFromIndex= */ false); + if (contentLength == download.contentLength || contentLength == C.LENGTH_UNSET) { + return; + } + putDownload( + new Download( + download.request, + download.state, + download.startTimeMs, + /* updateTimeMs= */ System.currentTimeMillis(), + contentLength, + download.stopReason, + download.failureReason, + download.progress)); + } + + private void onTaskStopped(Task task) { + String downloadId = task.request.id; + activeTasks.remove(downloadId); + + boolean isRemove = task.isRemove; + if (!isRemove) { + activeDownloadTaskCount--; + } + + if (task.isCanceled) { + syncTasks(); + return; + } + + Throwable finalError = task.finalError; + if (finalError != null) { + Log.e(TAG, "Task failed: " + task.request + ", " + isRemove, finalError); + } + + Download download = + Assertions.checkNotNull(getDownload(downloadId, /* loadFromIndex= */ false)); + switch (download.state) { + case STATE_DOWNLOADING: + Assertions.checkState(!isRemove); + onDownloadTaskStopped(download, finalError); + break; + case STATE_REMOVING: + case STATE_RESTARTING: + Assertions.checkState(isRemove); + onRemoveTaskStopped(download); + break; + case STATE_QUEUED: + case STATE_STOPPED: + case STATE_COMPLETED: + case STATE_FAILED: + default: + throw new IllegalStateException(); + } + + syncTasks(); + } + + private void onDownloadTaskStopped(Download download, @Nullable Throwable finalError) { + download = + new Download( + download.request, + finalError == null ? STATE_COMPLETED : STATE_FAILED, + download.startTimeMs, + /* updateTimeMs= */ System.currentTimeMillis(), + download.contentLength, + download.stopReason, + finalError == null ? FAILURE_REASON_NONE : FAILURE_REASON_UNKNOWN, + download.progress); + // The download is now in a terminal state, so should not be in the downloads list. + downloads.remove(getDownloadIndex(download.request.id)); + // We still need to update the download index and main thread. try { downloadIndex.putDownload(download); } catch (IOException e) { - Log.e(TAG, "Failed to update index", e); + Log.e(TAG, "Failed to update index.", e); } - if (downloadInternal.state == STATE_COMPLETED || downloadInternal.state == STATE_FAILED) { - downloadInternals.remove(downloadInternal); - } - mainHandler.obtainMessage(MSG_DOWNLOAD_CHANGED, download).sendToTarget(); + DownloadUpdate update = + new DownloadUpdate(download, /* isRemove= */ false, new ArrayList<>(downloads)); + mainHandler.obtainMessage(MSG_DOWNLOAD_UPDATE, update).sendToTarget(); } - private void onDownloadRemoved(DownloadInternal downloadInternal, Download download) { - logd("Download is removed", downloadInternal); - try { - downloadIndex.removeDownload(download.request.id); - } catch (IOException e) { - Log.e(TAG, "Failed to remove from index", e); - } - downloadInternals.remove(downloadInternal); - mainHandler.obtainMessage(MSG_DOWNLOAD_REMOVED, download).sendToTarget(); - } - - @StartThreadResults - private int startTask(DownloadInternal downloadInternal) { - DownloadRequest request = downloadInternal.download.request; - String downloadId = request.id; - if (activeTasks.containsKey(downloadId)) { - if (stopDownloadTask(downloadId)) { - return START_THREAD_WAIT_DOWNLOAD_CANCELLATION; + private void onRemoveTaskStopped(Download download) { + if (download.state == STATE_RESTARTING) { + putDownloadWithState( + download, download.stopReason == STOP_REASON_NONE ? STATE_QUEUED : STATE_STOPPED); + syncTasks(); + } else { + int removeIndex = getDownloadIndex(download.request.id); + downloads.remove(removeIndex); + try { + downloadIndex.removeDownload(download.request.id); + } catch (IOException e) { + Log.e(TAG, "Failed to remove from database"); } - return START_THREAD_WAIT_REMOVAL_TO_FINISH; + DownloadUpdate update = + new DownloadUpdate(download, /* isRemove= */ true, new ArrayList<>(downloads)); + mainHandler.obtainMessage(MSG_DOWNLOAD_UPDATE, update).sendToTarget(); } - boolean isRemove = downloadInternal.isInRemoveState(); - if (!isRemove) { - if (parallelDownloads == maxParallelDownloads) { - return START_THREAD_TOO_MANY_DOWNLOADS; - } - parallelDownloads++; - } - Downloader downloader = downloaderFactory.createDownloader(request); - DownloadProgress downloadProgress = downloadInternal.download.progress; - Task task = - new Task( - request, - downloader, - downloadProgress, - isRemove, - minRetryCount, - /* internalHandler= */ this); - activeTasks.put(downloadId, task); - task.start(); - logd("Task is started", downloadInternal); - return START_THREAD_SUCCEEDED; } - private boolean stopDownloadTask(String downloadId) { - Task task = activeTasks.get(downloadId); - if (task != null && !task.isRemove) { - task.cancel(/* released= */ false); - logd("Task is cancelled", task.request); - return true; - } - return false; - } + // Helper methods. - @Nullable - private DownloadInternal getDownload(String id) { - for (int i = 0; i < downloadInternals.size(); i++) { - DownloadInternal downloadInternal = downloadInternals.get(i); - if (downloadInternal.download.request.id.equals(id)) { - return downloadInternal; - } - } - return null; - } - - private Download loadDownload(String id) { - try { - return downloadIndex.getDownload(id); - } catch (IOException e) { - Log.e(TAG, "loadDownload failed", e); - } - return null; - } - - private void addDownloadForState(Download download) { - DownloadInternal downloadInternal = new DownloadInternal(this, download); - downloadInternals.add(downloadInternal); - logd("Download is added", downloadInternal); - downloadInternal.initialize(); - } - - private boolean canStartDownloads() { + private boolean canDownloadsRun() { return !downloadsPaused && notMetRequirements == 0; } - } - private static final class DownloadInternal { - - private final InternalHandler internalHandler; - - private Download download; - - // TODO: Get rid of these and use download directly. - @Download.State private int state; - private long contentLength; - private int stopReason; - @MonotonicNonNull @Download.FailureReason private int failureReason; - - private DownloadInternal(InternalHandler internalHandler, Download download) { - this.internalHandler = internalHandler; - this.download = download; - state = download.state; - contentLength = download.contentLength; - stopReason = download.stopReason; - failureReason = download.failureReason; - } - - private void initialize() { - initialize(download.state); - } - - public void addRequest(DownloadRequest newRequest, int stopReason) { - download = mergeRequest(download, newRequest, stopReason); - initialize(); - } - - public void remove() { - initialize(STATE_REMOVING); - } - - public Download getUpdatedDownload() { - download = + 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( new Download( download.request, state, download.startTimeMs, /* updateTimeMs= */ System.currentTimeMillis(), - contentLength, - stopReason, - state != STATE_FAILED ? FAILURE_REASON_NONE : failureReason, - download.progress); + download.contentLength, + /* stopReason= */ 0, + FAILURE_REASON_NONE, + download.progress)); + } + + private Download putDownload(Download download) { + // Downloads in terminal states shouldn't be in the downloads list. + Assertions.checkState(download.state != STATE_COMPLETED && download.state != STATE_FAILED); + int changedIndex = getDownloadIndex(download.request.id); + if (changedIndex == C.INDEX_UNSET) { + downloads.add(download); + Collections.sort(downloads, InternalHandler::compareStartTimes); + } else { + boolean needsSort = download.startTimeMs != downloads.get(changedIndex).startTimeMs; + downloads.set(changedIndex, download); + if (needsSort) { + Collections.sort(downloads, InternalHandler::compareStartTimes); + } + } + try { + downloadIndex.putDownload(download); + } catch (IOException e) { + Log.e(TAG, "Failed to update index.", e); + } + DownloadUpdate update = + new DownloadUpdate(download, /* isRemove= */ false, new ArrayList<>(downloads)); + mainHandler.obtainMessage(MSG_DOWNLOAD_UPDATE, update).sendToTarget(); return download; } - public boolean isIdle() { - return state != STATE_DOWNLOADING && state != STATE_REMOVING && state != STATE_RESTARTING; - } - - @Override - public String toString() { - return download.request.id + ' ' + Download.getStateString(state); - } - - public void start() { - if (state == STATE_QUEUED || state == STATE_DOWNLOADING) { - startOrQueue(); - } else if (isInRemoveState()) { - internalHandler.startTask(this); + @Nullable + private Download getDownload(String id, boolean loadFromIndex) { + int index = getDownloadIndex(id); + if (index != C.INDEX_UNSET) { + return downloads.get(index); } - } - - public void setStopReason(int stopReason) { - this.stopReason = stopReason; - updateStopState(); - } - - public boolean isInRemoveState() { - return state == STATE_REMOVING || state == STATE_RESTARTING; - } - - public void setContentLength(long contentLength) { - if (this.contentLength == contentLength) { - return; - } - this.contentLength = contentLength; - internalHandler.onDownloadChanged(this, getUpdatedDownload()); - } - - private void updateStopState() { - Download oldDownload = download; - if (canStart()) { - if (state == STATE_STOPPED) { - startOrQueue(); - } - } else { - if (state == STATE_DOWNLOADING || state == STATE_QUEUED) { - internalHandler.stopDownloadTask(download.request.id); - setState(STATE_STOPPED); + if (loadFromIndex) { + try { + return downloadIndex.getDownload(id); + } catch (IOException e) { + Log.e(TAG, "Failed to load download: " + id, e); } } - if (oldDownload == download) { - internalHandler.onDownloadChanged(this, getUpdatedDownload()); - } + return null; } - private void initialize(int initialState) { - // Don't notify listeners with initial state until we make sure we don't switch to another - // state immediately. - state = initialState; - if (isInRemoveState()) { - internalHandler.startTask(this); - } else if (canStart()) { - startOrQueue(); - } else { - setState(STATE_STOPPED); - } - if (state == initialState) { - internalHandler.onDownloadChanged(this, getUpdatedDownload()); - } - } - - private boolean canStart() { - return internalHandler.canStartDownloads() && stopReason == STOP_REASON_NONE; - } - - private void startOrQueue() { - Assertions.checkState(!isInRemoveState()); - @StartThreadResults int result = internalHandler.startTask(this); - Assertions.checkState(result != START_THREAD_WAIT_REMOVAL_TO_FINISH); - if (result == START_THREAD_SUCCEEDED || result == START_THREAD_WAIT_DOWNLOAD_CANCELLATION) { - setState(STATE_DOWNLOADING); - } else { - setState(STATE_QUEUED); - } - } - - private void setState(@Download.State int newState) { - if (state != newState) { - state = newState; - internalHandler.onDownloadChanged(this, getUpdatedDownload()); - } - } - - private void onTaskStopped(boolean isCanceled, @Nullable Throwable error) { - if (isIdle()) { - return; - } - if (isCanceled) { - internalHandler.startTask(this); - } else if (state == STATE_REMOVING) { - internalHandler.onDownloadRemoved(this, getUpdatedDownload()); - } else if (state == STATE_RESTARTING) { - initialize(STATE_QUEUED); - } else { // STATE_DOWNLOADING - if (error != null) { - Log.e(TAG, "Download failed: " + download.request.id, error); - failureReason = FAILURE_REASON_UNKNOWN; - setState(STATE_FAILED); - } else { - setState(STATE_COMPLETED); + private int getDownloadIndex(String id) { + for (int i = 0; i < downloads.size(); i++) { + Download download = downloads.get(i); + if (download.request.id.equals(id)) { + return i; } } + return C.INDEX_UNSET; + } + + private static int compareStartTimes(Download first, Download second) { + return Util.compareLong(first.startTimeMs, second.startTimeMs); } } @@ -1177,16 +1130,17 @@ public final class DownloadManager { // download manager whilst cancellation is ongoing. internalHandler = null; } - isCanceled = true; - downloader.cancel(); - interrupt(); + if (!isCanceled) { + isCanceled = true; + downloader.cancel(); + interrupt(); + } } // Methods running on download thread. @Override public void run() { - logd("Download started", request); try { if (isRemove) { downloader.remove(); @@ -1201,14 +1155,12 @@ public final class DownloadManager { if (!isCanceled) { long bytesDownloaded = downloadProgress.bytesDownloaded; if (bytesDownloaded != errorPosition) { - logd("Reset error count. bytesDownloaded = " + bytesDownloaded, request); errorPosition = bytesDownloaded; errorCount = 0; } if (++errorCount > minRetryCount) { throw e; } - logd("Download error. Retry " + errorCount, request); Thread.sleep(getRetryDelayMillis(errorCount)); } } @@ -1240,4 +1192,18 @@ public final class DownloadManager { return Math.min((errorCount - 1) * 1000, 5000); } } + + private static final class DownloadUpdate { + + private final Download download; + private final boolean isRemove; + + private final List downloads; + + public DownloadUpdate(Download download, boolean isRemove, List downloads) { + this.download = download; + this.isRemove = isRemove; + this.downloads = downloads; + } + } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/WritableDownloadIndex.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/WritableDownloadIndex.java index 00b08dc76a..ae634f8544 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/WritableDownloadIndex.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/WritableDownloadIndex.java @@ -37,6 +37,13 @@ public interface WritableDownloadIndex extends DownloadIndex { */ void removeDownload(String id) throws IOException; + /** + * Sets all {@link Download#STATE_DOWNLOADING} states to {@link Download#STATE_QUEUED}. + * + * @throws IOException If an error occurs updating the state. + */ + void setDownloadingStatesToQueued() throws IOException; + /** * Sets the stop reason of the downloads in a terminal state ({@link Download#STATE_COMPLETED}, * {@link Download#STATE_FAILED}). diff --git a/library/core/src/test/java/com/google/android/exoplayer2/offline/ActionFileUpgradeUtilTest.java b/library/core/src/test/java/com/google/android/exoplayer2/offline/ActionFileUpgradeUtilTest.java index b5dbe41521..17c1b57f37 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/offline/ActionFileUpgradeUtilTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/offline/ActionFileUpgradeUtilTest.java @@ -38,6 +38,8 @@ import org.junit.runner.RunWith; @RunWith(AndroidJUnit4.class) public class ActionFileUpgradeUtilTest { + private static final long NOW_MS = 1234; + private File tempFile; private ExoDatabaseProvider databaseProvider; private DefaultDownloadIndex downloadIndex; @@ -113,7 +115,7 @@ public class ActionFileUpgradeUtilTest { data); ActionFileUpgradeUtil.mergeRequest( - request, downloadIndex, /* addNewDownloadAsCompleted= */ false); + request, downloadIndex, /* addNewDownloadAsCompleted= */ false, NOW_MS); assertDownloadIndexContainsRequest(request, Download.STATE_QUEUED); } @@ -141,9 +143,9 @@ public class ActionFileUpgradeUtilTest { /* customCacheKey= */ "key123", new byte[] {5, 4, 3, 2, 1}); ActionFileUpgradeUtil.mergeRequest( - request1, downloadIndex, /* addNewDownloadAsCompleted= */ false); + request1, downloadIndex, /* addNewDownloadAsCompleted= */ false, NOW_MS); ActionFileUpgradeUtil.mergeRequest( - request2, downloadIndex, /* addNewDownloadAsCompleted= */ false); + request2, downloadIndex, /* addNewDownloadAsCompleted= */ false, NOW_MS); Download download = downloadIndex.getDownload(request2.id); assertThat(download).isNotNull(); @@ -178,16 +180,16 @@ public class ActionFileUpgradeUtilTest { /* customCacheKey= */ "key123", new byte[] {5, 4, 3, 2, 1}); ActionFileUpgradeUtil.mergeRequest( - request1, downloadIndex, /* addNewDownloadAsCompleted= */ false); + request1, downloadIndex, /* addNewDownloadAsCompleted= */ false, NOW_MS); // Merging existing download, keeps it queued. ActionFileUpgradeUtil.mergeRequest( - request1, downloadIndex, /* addNewDownloadAsCompleted= */ true); + request1, downloadIndex, /* addNewDownloadAsCompleted= */ true, NOW_MS); assertThat(downloadIndex.getDownload(request1.id).state).isEqualTo(Download.STATE_QUEUED); // New download is merged as completed. ActionFileUpgradeUtil.mergeRequest( - request2, downloadIndex, /* addNewDownloadAsCompleted= */ true); + request2, downloadIndex, /* addNewDownloadAsCompleted= */ true, NOW_MS); assertThat(downloadIndex.getDownload(request2.id).state).isEqualTo(Download.STATE_COMPLETED); } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java index 01c73caf46..a3df647efe 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java @@ -61,6 +61,8 @@ public class DownloadManagerTest { private static final int APP_STOP_REASON = 1; /** The minimum number of times a task must be retried before failing. */ private static final int MIN_RETRY_COUNT = 3; + /** Dummy value for the current time. */ + private static final long NOW_MS = 1234; private Uri uri1; private Uri uri2; @@ -132,6 +134,7 @@ public class DownloadManagerTest { task.assertCompleted(); runner.assertCreatedDownloaderCount(1); downloadManagerListener.blockUntilTasksCompleteAndThrowAnyDownloadError(); + assertThat(downloadManager.getCurrentDownloads()).isEmpty(); } @Test @@ -143,6 +146,7 @@ public class DownloadManagerTest { task.assertRemoved(); runner.assertCreatedDownloaderCount(2); downloadManagerListener.blockUntilTasksCompleteAndThrowAnyDownloadError(); + assertThat(downloadManager.getCurrentDownloads()).isEmpty(); } @Test @@ -158,6 +162,7 @@ public class DownloadManagerTest { downloader.assertReleased().assertStartCount(MIN_RETRY_COUNT + 1); runner.getTask().assertFailed(); downloadManagerListener.blockUntilTasksComplete(); + assertThat(downloadManager.getCurrentDownloads()).isEmpty(); } @Test @@ -174,6 +179,7 @@ public class DownloadManagerTest { downloader.assertReleased().assertStartCount(MIN_RETRY_COUNT + 1); runner.getTask().assertCompleted(); downloadManagerListener.blockUntilTasksComplete(); + assertThat(downloadManager.getCurrentDownloads()).isEmpty(); } @Test @@ -341,7 +347,7 @@ public class DownloadManagerTest { } @Test - public void getTasks_returnTasks() { + public void getCurrentDownloads_returnsCurrentDownloads() { TaskWrapper task1 = new DownloadRunner(uri1).postDownloadRequest().getTask(); TaskWrapper task2 = new DownloadRunner(uri2).postDownloadRequest().getTask(); TaskWrapper task3 = @@ -370,13 +376,11 @@ public class DownloadManagerTest { runOnMainThread(() -> downloadManager.pauseDownloads()); - // TODO: This should be assertQueued. Fix implementation and update test. - runner1.getTask().assertStopped(); + runner1.getTask().assertQueued(); // remove requests aren't stopped. runner2.getDownloader(1).unblock().assertReleased(); - // TODO: This should be assertQueued. Fix implementation and update test. - runner2.getTask().assertStopped(); + runner2.getTask().assertQueued(); // Although remove2 is finished, download2 doesn't start. runner2.getDownloader(2).assertDoesNotStart(); @@ -397,7 +401,7 @@ public class DownloadManagerTest { } @Test - public void manuallyStopAndResumeSingleDownload() throws Throwable { + public void setAndClearSingleDownloadStopReason() throws Throwable { DownloadRunner runner = new DownloadRunner(uri1).postDownloadRequest(); TaskWrapper task = runner.getTask(); @@ -415,7 +419,7 @@ public class DownloadManagerTest { } @Test - public void manuallyStoppedDownloadCanBeCancelled() throws Throwable { + public void setSingleDownloadStopReasonThenRemove_removesDownload() throws Throwable { DownloadRunner runner = new DownloadRunner(uri1).postDownloadRequest(); TaskWrapper task = runner.getTask(); @@ -433,7 +437,7 @@ public class DownloadManagerTest { } @Test - public void manuallyStoppedSingleDownload_doesNotAffectOthers() throws Throwable { + public void setSingleDownloadStopReason_doesNotAffectOtherDownloads() throws Throwable { DownloadRunner runner1 = new DownloadRunner(uri1); DownloadRunner runner2 = new DownloadRunner(uri2); DownloadRunner runner3 = new DownloadRunner(uri3); @@ -455,21 +459,22 @@ public class DownloadManagerTest { } @Test - public void mergeRequest_removingDownload_becomesRestarting() { + public void mergeRequest_removing_becomesRestarting() { DownloadRequest downloadRequest = createDownloadRequest(); DownloadBuilder downloadBuilder = new DownloadBuilder(downloadRequest).setState(Download.STATE_REMOVING); Download download = downloadBuilder.build(); Download mergedDownload = - DownloadManager.mergeRequest(download, downloadRequest, download.stopReason); + DownloadManager.mergeRequest(download, downloadRequest, download.stopReason, NOW_MS); - Download expectedDownload = downloadBuilder.setState(Download.STATE_RESTARTING).build(); - assertEqualIgnoringTimeFields(mergedDownload, expectedDownload); + Download expectedDownload = + downloadBuilder.setStartTimeMs(NOW_MS).setState(Download.STATE_RESTARTING).build(); + assertEqualIgnoringUpdateTime(mergedDownload, expectedDownload); } @Test - public void mergeRequest_failedDownload_becomesQueued() { + public void mergeRequest_failed_becomesQueued() { DownloadRequest downloadRequest = createDownloadRequest(); DownloadBuilder downloadBuilder = new DownloadBuilder(downloadRequest) @@ -478,18 +483,19 @@ public class DownloadManagerTest { Download download = downloadBuilder.build(); Download mergedDownload = - DownloadManager.mergeRequest(download, downloadRequest, download.stopReason); + DownloadManager.mergeRequest(download, downloadRequest, download.stopReason, NOW_MS); Download expectedDownload = downloadBuilder + .setStartTimeMs(NOW_MS) .setState(Download.STATE_QUEUED) .setFailureReason(Download.FAILURE_REASON_NONE) .build(); - assertEqualIgnoringTimeFields(mergedDownload, expectedDownload); + assertEqualIgnoringUpdateTime(mergedDownload, expectedDownload); } @Test - public void mergeRequest_stoppedDownload_staysStopped() { + public void mergeRequest_stopped_staysStopped() { DownloadRequest downloadRequest = createDownloadRequest(); DownloadBuilder downloadBuilder = new DownloadBuilder(downloadRequest) @@ -498,13 +504,13 @@ public class DownloadManagerTest { Download download = downloadBuilder.build(); Download mergedDownload = - DownloadManager.mergeRequest(download, downloadRequest, download.stopReason); + DownloadManager.mergeRequest(download, downloadRequest, download.stopReason, NOW_MS); - assertEqualIgnoringTimeFields(mergedDownload, download); + assertEqualIgnoringUpdateTime(mergedDownload, download); } @Test - public void mergeRequest_stopReasonSetButNotStopped_becomesStopped() { + public void mergeRequest_completedWithStopReason_becomesStopped() { DownloadRequest downloadRequest = createDownloadRequest(); DownloadBuilder downloadBuilder = new DownloadBuilder(downloadRequest) @@ -513,10 +519,11 @@ public class DownloadManagerTest { Download download = downloadBuilder.build(); Download mergedDownload = - DownloadManager.mergeRequest(download, downloadRequest, download.stopReason); + DownloadManager.mergeRequest(download, downloadRequest, download.stopReason, NOW_MS); - Download expectedDownload = downloadBuilder.setState(Download.STATE_STOPPED).build(); - assertEqualIgnoringTimeFields(mergedDownload, expectedDownload); + Download expectedDownload = + downloadBuilder.setStartTimeMs(NOW_MS).setState(Download.STATE_STOPPED).build(); + assertEqualIgnoringUpdateTime(mergedDownload, expectedDownload); } private void setUpDownloadManager(final int maxParallelDownloads) throws Exception { @@ -554,9 +561,10 @@ public class DownloadManagerTest { dummyMainThread.runTestOnMainThread(r); } - private static void assertEqualIgnoringTimeFields(Download download, Download that) { + private static void assertEqualIgnoringUpdateTime(Download download, Download that) { assertThat(download.request).isEqualTo(that.request); assertThat(download.state).isEqualTo(that.state); + assertThat(download.startTimeMs).isEqualTo(that.startTimeMs); assertThat(download.contentLength).isEqualTo(that.contentLength); assertThat(download.failureReason).isEqualTo(that.failureReason); assertThat(download.stopReason).isEqualTo(that.stopReason); From ea0efa74641fb688ece3e170a0ecfe42a3f3ffe5 Mon Sep 17 00:00:00 2001 From: olly Date: Wed, 1 May 2019 20:06:11 +0100 Subject: [PATCH 03/11] Periodically persist progress to index whilst downloading PiperOrigin-RevId: 246173972 --- .../exoplayer2/offline/DownloadManager.java | 37 +++++++++++++++---- .../exoplayer2/offline/DownloadService.java | 4 +- 2 files changed, 32 insertions(+), 9 deletions(-) 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 b528d91759..3e0375718b 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 @@ -134,7 +134,8 @@ public final class DownloadManager { private static final int MSG_REMOVE_DOWNLOAD = 7; private static final int MSG_TASK_STOPPED = 8; private static final int MSG_CONTENT_LENGTH_CHANGED = 9; - private static final int MSG_RELEASE = 10; + private static final int MSG_UPDATE_PROGRESS = 10; + private static final int MSG_RELEASE = 11; private static final String TAG = "DownloadManager"; @@ -569,6 +570,8 @@ public final class DownloadManager { private static final class InternalHandler extends Handler { + private static final int UPDATE_PROGRESS_INTERVAL_MS = 5000; + public boolean released; private final HandlerThread thread; @@ -650,11 +653,13 @@ public final class DownloadManager { case MSG_CONTENT_LENGTH_CHANGED: task = (Task) message.obj; onContentLengthChanged(task); - processedExternalMessage = false; // This message is posted internally. - break; + return; // No need to post back to mainHandler. + case MSG_UPDATE_PROGRESS: + updateProgress(); + return; // No need to post back to mainHandler. case MSG_RELEASE: release(); - return; // Don't post back to mainHandler on release. + return; // No need to post back to mainHandler. default: throw new IllegalStateException(); } @@ -868,7 +873,9 @@ public final class DownloadManager { minRetryCount, /* internalHandler= */ this); activeTasks.put(download.request.id, activeTask); - activeDownloadTaskCount++; + if (activeDownloadTaskCount++ == 0) { + sendEmptyMessageDelayed(MSG_UPDATE_PROGRESS, UPDATE_PROGRESS_INTERVAL_MS); + } activeTask.start(); return activeTask; } @@ -933,8 +940,8 @@ public final class DownloadManager { activeTasks.remove(downloadId); boolean isRemove = task.isRemove; - if (!isRemove) { - activeDownloadTaskCount--; + if (!isRemove && --activeDownloadTaskCount == 0) { + removeMessages(MSG_UPDATE_PROGRESS); } if (task.isCanceled) { @@ -1013,6 +1020,22 @@ public final class DownloadManager { } } + // Progress updates. + + private void updateProgress() { + for (int i = 0; i < downloads.size(); i++) { + Download download = downloads.get(i); + if (download.state == STATE_DOWNLOADING) { + try { + downloadIndex.putDownload(download); + } catch (IOException e) { + Log.e(TAG, "Failed to update index.", e); + } + } + } + sendEmptyMessageDelayed(MSG_UPDATE_PROGRESS, UPDATE_PROGRESS_INTERVAL_MS); + } + // Helper methods. private boolean canDownloadsRun() { diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java index ee00cf3d5f..ce9087c6c8 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java @@ -683,7 +683,7 @@ public abstract class DownloadService extends Service { // Do nothing. } - private void notifyDownloadChange(Download download) { + private void notifyDownloadChanged(Download download) { onDownloadChanged(download); if (foregroundNotificationUpdater != null) { if (download.state == Download.STATE_DOWNLOADING @@ -834,7 +834,7 @@ public abstract class DownloadService extends Service { @Override public void onDownloadChanged(DownloadManager downloadManager, Download download) { if (downloadService != null) { - downloadService.notifyDownloadChange(download); + downloadService.notifyDownloadChanged(download); } } From de7c62a915076dd8363054acb0377c43601d794a Mon Sep 17 00:00:00 2001 From: olly Date: Wed, 1 May 2019 20:50:44 +0100 Subject: [PATCH 04/11] Remove unnecessary logging As justification for why we should not have this type of logging, it would scale up to about 13K LOC, 1800 Strings, and 36K (after pro-guarding - in the case of the demo app) if we did it through the whole code base*. It makes the code messier to read, and in most cases doesn't add significant value. Note: I left the Scheduler logging because it logs interactions with some awkward library components outside of ExoPlayer, so is perhaps a bit more justified. * This is a bit unfair since realistically we wouldn't ever add lots of logging into trivial classes. But I think it is fair to say that the deltas would be non-negligible. PiperOrigin-RevId: 246181421 --- .../jobdispatcher/JobDispatcherScheduler.java | 1 + .../android/exoplayer2/offline/Download.java | 22 --------------- .../exoplayer2/offline/DownloadService.java | 27 +++++-------------- .../scheduler/PlatformScheduler.java | 1 + .../exoplayer2/scheduler/Requirements.java | 12 --------- .../scheduler/RequirementsWatcher.java | 23 ---------------- .../exoplayer2/scheduler/Scheduler.java | 2 -- .../testutil/TestDownloadManagerListener.java | 22 +-------------- 8 files changed, 10 insertions(+), 100 deletions(-) diff --git a/extensions/jobdispatcher/src/main/java/com/google/android/exoplayer2/ext/jobdispatcher/JobDispatcherScheduler.java b/extensions/jobdispatcher/src/main/java/com/google/android/exoplayer2/ext/jobdispatcher/JobDispatcherScheduler.java index 790f5ca4e5..d79dead0d7 100644 --- a/extensions/jobdispatcher/src/main/java/com/google/android/exoplayer2/ext/jobdispatcher/JobDispatcherScheduler.java +++ b/extensions/jobdispatcher/src/main/java/com/google/android/exoplayer2/ext/jobdispatcher/JobDispatcherScheduler.java @@ -57,6 +57,7 @@ import com.google.android.exoplayer2.util.Util; */ public final class JobDispatcherScheduler implements Scheduler { + private static final boolean DEBUG = false; private static final String TAG = "JobDispatcherScheduler"; private static final String KEY_SERVICE_ACTION = "service_action"; private static final String KEY_SERVICE_PACKAGE = "service_package"; 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 00d81b392c..97dff8394e 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 @@ -81,28 +81,6 @@ public final class Download { /** The download isn't stopped. */ public static final int STOP_REASON_NONE = 0; - /** Returns the state string for the given state value. */ - public static String getStateString(@State int state) { - switch (state) { - case STATE_QUEUED: - return "QUEUED"; - case STATE_STOPPED: - return "STOPPED"; - case STATE_DOWNLOADING: - return "DOWNLOADING"; - case STATE_COMPLETED: - return "COMPLETED"; - case STATE_FAILED: - return "FAILED"; - case STATE_REMOVING: - return "REMOVING"; - case STATE_RESTARTING: - return "RESTARTING"; - default: - throw new IllegalStateException(); - } - } - /** The download request. */ public final DownloadRequest request; /** The state of the download. */ diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java index ce9087c6c8..fdd7163a2c 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java @@ -127,18 +127,18 @@ public abstract class DownloadService extends Service { public static final String KEY_DOWNLOAD_REQUEST = "download_request"; /** - * Key for the content id in {@link #ACTION_SET_STOP_REASON} and {@link #ACTION_REMOVE_DOWNLOAD} - * intents. + * Key for the {@link String} content id in {@link #ACTION_SET_STOP_REASON} and {@link + * #ACTION_REMOVE_DOWNLOAD} intents. */ public static final String KEY_CONTENT_ID = "content_id"; /** - * Key for the stop reason in {@link #ACTION_SET_STOP_REASON} and {@link #ACTION_ADD_DOWNLOAD} - * intents. + * Key for the integer stop reason in {@link #ACTION_SET_STOP_REASON} and {@link + * #ACTION_ADD_DOWNLOAD} intents. */ public static final String KEY_STOP_REASON = "stop_reason"; - /** Key for the requirements in {@link #ACTION_SET_REQUIREMENTS} intents. */ + /** Key for the {@link Requirements} in {@link #ACTION_SET_REQUIREMENTS} intents. */ public static final String KEY_REQUIREMENTS = "requirements"; /** @@ -155,7 +155,6 @@ public abstract class DownloadService extends Service { public static final long DEFAULT_FOREGROUND_NOTIFICATION_UPDATE_INTERVAL = 1000; private static final String TAG = "DownloadService"; - private static final boolean DEBUG = false; // Keep DownloadManagerListeners for each DownloadService as long as there are downloads (and the // process is running). This allows DownloadService to restart when there's no scheduler. @@ -506,7 +505,6 @@ public abstract class DownloadService extends Service { @Override public void onCreate() { - logd("onCreate"); if (channelId != null) { NotificationUtil.createNotificationChannel( this, channelId, channelNameResourceId, NotificationUtil.IMPORTANCE_LOW); @@ -541,7 +539,6 @@ public abstract class DownloadService extends Service { if (intentAction == null) { intentAction = ACTION_INIT; } - logd("onStartCommand action: " + intentAction + " startId: " + startId); switch (intentAction) { case ACTION_INIT: case ACTION_RESTART: @@ -573,7 +570,7 @@ public abstract class DownloadService extends Service { if (!intent.hasExtra(KEY_STOP_REASON)) { Log.e(TAG, "Ignored SET_STOP_REASON: Missing " + KEY_STOP_REASON + " extra"); } else { - int stopReason = intent.getIntExtra(KEY_STOP_REASON, Download.STOP_REASON_NONE); + int stopReason = intent.getIntExtra(KEY_STOP_REASON, /* defaultValue= */ 0); downloadManager.setStopReason(contentId, stopReason); } break; @@ -598,13 +595,11 @@ public abstract class DownloadService extends Service { @Override public void onTaskRemoved(Intent rootIntent) { - logd("onTaskRemoved rootIntent: " + rootIntent); taskRemoved = true; } @Override public void onDestroy() { - logd("onDestroy"); isDestroyed = true; DownloadManagerHelper downloadManagerHelper = downloadManagerListeners.get(getClass()); boolean unschedule = !downloadManager.isWaitingForRequirements(); @@ -713,16 +708,8 @@ public abstract class DownloadService extends Service { } if (Util.SDK_INT < 28 && taskRemoved) { // See [Internal: b/74248644]. stopSelf(); - logd("stopSelf()"); } else { - boolean stopSelfResult = stopSelfResult(lastStartId); - logd("stopSelf(" + lastStartId + ") result: " + stopSelfResult); - } - } - - private void logd(String message) { - if (DEBUG) { - Log.d(TAG, message); + stopSelfResult(lastStartId); } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/PlatformScheduler.java b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/PlatformScheduler.java index fc8e8b61a5..8572c9c7ca 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/PlatformScheduler.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/PlatformScheduler.java @@ -44,6 +44,7 @@ import com.google.android.exoplayer2.util.Util; @TargetApi(21) public final class PlatformScheduler implements Scheduler { + private static final boolean DEBUG = false; private static final String TAG = "PlatformScheduler"; private static final String KEY_SERVICE_ACTION = "service_action"; private static final String KEY_SERVICE_PACKAGE = "service_package"; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Requirements.java b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Requirements.java index babc4e49fb..30cf452572 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Requirements.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Requirements.java @@ -27,7 +27,6 @@ import android.os.Parcel; import android.os.Parcelable; import android.os.PowerManager; import androidx.annotation.IntDef; -import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.Util; import java.lang.annotation.Documented; import java.lang.annotation.Retention; @@ -56,8 +55,6 @@ public final class Requirements implements Parcelable { /** Requirement that the device is charging. */ public static final int DEVICE_CHARGING = 1 << 3; - private static final String TAG = "Requirements"; - @RequirementFlags private final int requirements; /** @param requirements A combination of requirement flags. */ @@ -135,7 +132,6 @@ public final class Requirements implements Parcelable { if (networkInfo == null || !networkInfo.isConnected() || !isInternetConnectivityValidated(connectivityManager)) { - logd("No network info, connection or connectivity."); return requirements & (NETWORK | NETWORK_UNMETERED); } @@ -172,7 +168,6 @@ public final class Requirements implements Parcelable { } Network activeNetwork = connectivityManager.getActiveNetwork(); if (activeNetwork == null) { - logd("No active network."); return false; } NetworkCapabilities networkCapabilities = @@ -180,16 +175,9 @@ public final class Requirements implements Parcelable { boolean validated = networkCapabilities == null || !networkCapabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED); - logd("Network capability validated: " + validated); return !validated; } - private static void logd(String message) { - if (Scheduler.DEBUG) { - Log.d(TAG, message); - } - } - @Override public boolean equals(Object o) { if (this == o) { diff --git a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/RequirementsWatcher.java b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/RequirementsWatcher.java index d2ad357ff6..f0d0f37cdf 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/RequirementsWatcher.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/RequirementsWatcher.java @@ -28,7 +28,6 @@ import android.os.Handler; import android.os.Looper; import android.os.PowerManager; import androidx.annotation.RequiresApi; -import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.Util; /** @@ -53,8 +52,6 @@ public final class RequirementsWatcher { @Requirements.RequirementFlags int notMetRequirements); } - private static final String TAG = "RequirementsWatcher"; - private final Context context; private final Listener listener; private final Requirements requirements; @@ -75,7 +72,6 @@ public final class RequirementsWatcher { this.listener = listener; this.requirements = requirements; handler = new Handler(Util.getLooper()); - logd(this + " created"); } /** @@ -110,7 +106,6 @@ public final class RequirementsWatcher { } receiver = new DeviceStatusChangeReceiver(); context.registerReceiver(receiver, filter, null, handler); - logd(this + " started"); return notMetRequirements; } @@ -121,7 +116,6 @@ public final class RequirementsWatcher { if (networkCallback != null) { unregisterNetworkCallback(); } - logd(this + " stopped"); } /** Returns watched {@link Requirements}. */ @@ -129,14 +123,6 @@ public final class RequirementsWatcher { return requirements; } - @Override - public String toString() { - if (!Scheduler.DEBUG) { - return super.toString(); - } - return "RequirementsWatcher{" + requirements + '}'; - } - @TargetApi(23) private void registerNetworkCallbackV23() { ConnectivityManager connectivityManager = @@ -163,22 +149,14 @@ public final class RequirementsWatcher { int notMetRequirements = requirements.getNotMetRequirements(context); if (this.notMetRequirements != notMetRequirements) { this.notMetRequirements = notMetRequirements; - logd("notMetRequirements has changed: " + notMetRequirements); listener.onRequirementsStateChanged(this, notMetRequirements); } } - private static void logd(String message) { - if (Scheduler.DEBUG) { - Log.d(TAG, message); - } - } - private class DeviceStatusChangeReceiver extends BroadcastReceiver { @Override public void onReceive(Context context, Intent intent) { if (!isInitialStickyBroadcast()) { - logd(RequirementsWatcher.this + " received " + intent.getAction()); checkRequirements(); } } @@ -200,7 +178,6 @@ public final class RequirementsWatcher { handler.post( () -> { if (networkCallback != null) { - logd(RequirementsWatcher.this + " NetworkCallback"); checkRequirements(); } }); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Scheduler.java b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Scheduler.java index 1b225d9a4d..b5a6f40424 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Scheduler.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Scheduler.java @@ -22,8 +22,6 @@ import android.content.Intent; /** Schedules a service to be started in the foreground when some {@link Requirements} are met. */ public interface Scheduler { - /* package */ boolean DEBUG = false; - /** * Schedules a service to be started in the foreground when some {@link Requirements} are met. * Anything that was previously scheduled will be canceled. 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 9d6223b8b1..4c334992b5 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 @@ -22,9 +22,7 @@ import android.os.ConditionVariable; import com.google.android.exoplayer2.offline.Download; import com.google.android.exoplayer2.offline.Download.State; import com.google.android.exoplayer2.offline.DownloadManager; -import java.util.ArrayList; import java.util.HashMap; -import java.util.Locale; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -138,7 +136,6 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen } private void assertStateInternal(String taskId, int expectedState, int timeoutMs) { - ArrayList receivedStates = new ArrayList<>(); while (true) { Integer state = null; try { @@ -150,25 +147,8 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen if (expectedState == state) { return; } - receivedStates.add(state); } else { - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < receivedStates.size(); i++) { - if (i > 0) { - sb.append(','); - } - int receivedState = receivedStates.get(i); - String receivedStateString = - receivedState == STATE_REMOVED ? "REMOVED" : Download.getStateString(receivedState); - sb.append(receivedStateString); - } - fail( - String.format( - Locale.US, - "for download (%s) expected:<%s> but was:<%s>", - taskId, - Download.getStateString(expectedState), - sb)); + fail("Didn't receive expected state: " + expectedState); } } } From 5992699d91406e8e1d289e565f95a15e3b40a11b Mon Sep 17 00:00:00 2001 From: tonihei Date: Thu, 2 May 2019 10:37:31 +0100 Subject: [PATCH 05/11] Post-submit fixes for https://github.com/google/ExoPlayer/commit/eed5d957d87d44cb9c716f1a4c80f39ad2a6a442. One wrong return value, a useless assignment, unusual visibility of private class fields and some nullability issues. PiperOrigin-RevId: 246282995 --- .../exoplayer2/offline/DownloadManager.java | 34 ++++++++++++------- .../google/android/exoplayer2/util/Util.java | 4 +-- 2 files changed, 23 insertions(+), 15 deletions(-) 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 3e0375718b..e8b7eaf9b2 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 @@ -31,6 +31,7 @@ import android.os.Handler; import android.os.HandlerThread; import android.os.Looper; import android.os.Message; +import androidx.annotation.CheckResult; import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.database.DatabaseProvider; @@ -192,12 +193,9 @@ public final class DownloadManager { downloads = Collections.emptyList(); listeners = new CopyOnWriteArraySet<>(); - requirementsListener = this::onRequirementsStateChanged; - requirementsWatcher = - new RequirementsWatcher(context, requirementsListener, DEFAULT_REQUIREMENTS); - notMetRequirements = requirementsWatcher.start(); - - mainHandler = new Handler(Util.getLooper(), this::handleMainMessage); + @SuppressWarnings("methodref.receiver.bound.invalid") + Handler mainHandler = Util.createHandler(this::handleMainMessage); + this.mainHandler = mainHandler; HandlerThread internalThread = new HandlerThread("DownloadManager file i/o"); internalThread.start(); internalHandler = @@ -210,6 +208,13 @@ public final class DownloadManager { minRetryCount, downloadsPaused); + @SuppressWarnings("methodref.receiver.bound.invalid") + RequirementsWatcher.Listener requirementsListener = this::onRequirementsStateChanged; + this.requirementsListener = requirementsListener; + requirementsWatcher = + new RequirementsWatcher(context, requirementsListener, DEFAULT_REQUIREMENTS); + notMetRequirements = requirementsWatcher.start(); + pendingMessages = 1; internalHandler .obtainMessage(MSG_INITIALIZE, notMetRequirements, /* unused */ 0) @@ -822,7 +827,7 @@ public final class DownloadManager { activeTask = syncQueuedDownload(activeTask, download); break; case STATE_DOWNLOADING: - activeTask = Assertions.checkNotNull(activeTask); + Assertions.checkNotNull(activeTask); syncDownloadingDownload(activeTask, download, accumulatingDownloadTaskCount); break; case STATE_REMOVING: @@ -848,6 +853,8 @@ public final class DownloadManager { } } + @Nullable + @CheckResult private Task syncQueuedDownload(@Nullable Task activeTask, Download download) { if (activeTask != null) { // We have a task, which must be a download task. If the download state is queued we need to @@ -919,7 +926,8 @@ public final class DownloadManager { private void onContentLengthChanged(Task task) { String downloadId = task.request.id; long contentLength = task.contentLength; - Download download = getDownload(downloadId, /* loadFromIndex= */ false); + Download download = + Assertions.checkNotNull(getDownload(downloadId, /* loadFromIndex= */ false)); if (contentLength == download.contentLength || contentLength == C.LENGTH_UNSET) { return; } @@ -1125,7 +1133,7 @@ public final class DownloadManager { private volatile InternalHandler internalHandler; private volatile boolean isCanceled; - private Throwable finalError; + @Nullable private Throwable finalError; private long contentLength; @@ -1145,6 +1153,7 @@ public final class DownloadManager { contentLength = C.LENGTH_UNSET; } + @SuppressWarnings("nullness:assignment.type.incompatible") public void cancel(boolean released) { if (released) { // Download threads are GC roots for as long as they're running. The time taken for @@ -1218,10 +1227,9 @@ public final class DownloadManager { private static final class DownloadUpdate { - private final Download download; - private final boolean isRemove; - - private final List downloads; + public final Download download; + public final boolean isRemove; + public final List downloads; public DownloadUpdate(Download download, boolean isRemove, List downloads) { this.download = download; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/util/Util.java b/library/core/src/main/java/com/google/android/exoplayer2/util/Util.java index c05486bedf..97bcb68708 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/util/Util.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/util/Util.java @@ -390,7 +390,7 @@ public final class Util { * * @param dataSource The {@link DataSource} to close. */ - public static void closeQuietly(DataSource dataSource) { + public static void closeQuietly(@Nullable DataSource dataSource) { try { if (dataSource != null) { dataSource.close(); @@ -406,7 +406,7 @@ public final class Util { * * @param closeable The {@link Closeable} to close. */ - public static void closeQuietly(Closeable closeable) { + public static void closeQuietly(@Nullable Closeable closeable) { try { if (closeable != null) { closeable.close(); From ad5948f3ed62941acf6a8e39ce43a4001c53a255 Mon Sep 17 00:00:00 2001 From: aquilescanta Date: Thu, 2 May 2019 11:41:06 +0100 Subject: [PATCH 06/11] Fix SmoothStreaming links NOTE: Streams are working on ExoPlayer but querying them from other platforms yields "bad request". The new links: + Match Microsoft's test server. + Allow querying from clients other than ExoPlayer, like curl. PiperOrigin-RevId: 246289755 --- demos/main/src/main/assets/media.exolist.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/demos/main/src/main/assets/media.exolist.json b/demos/main/src/main/assets/media.exolist.json index c2acf3990b..bcb3ef4ad1 100644 --- a/demos/main/src/main/assets/media.exolist.json +++ b/demos/main/src/main/assets/media.exolist.json @@ -330,11 +330,11 @@ "samples": [ { "name": "Super speed", - "uri": "https://playready.directtaps.net/smoothstreaming/SSWSS720H264/SuperSpeedway_720.ism" + "uri": "https://playready.directtaps.net/smoothstreaming/SSWSS720H264/SuperSpeedway_720.ism/Manifest" }, { "name": "Super speed (PlayReady)", - "uri": "https://playready.directtaps.net/smoothstreaming/SSWSS720H264PR/SuperSpeedway_720.ism", + "uri": "https://playready.directtaps.net/smoothstreaming/SSWSS720H264PR/SuperSpeedway_720.ism/Manifest", "drm_scheme": "playready" } ] From 76184deb21af9ea058bcdf0cb35b28441e419c6e Mon Sep 17 00:00:00 2001 From: olly Date: Thu, 2 May 2019 17:35:30 +0100 Subject: [PATCH 07/11] Minor download documentation tweaks PiperOrigin-RevId: 246333281 --- .../android/exoplayer2/offline/DefaultDownloaderFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloaderFactory.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloaderFactory.java index ca20c769dc..d8126d4736 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloaderFactory.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloaderFactory.java @@ -112,7 +112,7 @@ public class DefaultDownloaderFactory implements DownloaderFactory { .getConstructor(Uri.class, List.class, DownloaderConstructorHelper.class); } catch (NoSuchMethodException e) { // The downloader is present, but the expected constructor is missing. - throw new RuntimeException("DASH downloader constructor missing", e); + throw new RuntimeException("Downloader constructor missing", e); } } // LINT.ThenChange(../../../../../../../../proguard-rules.txt) From 1292a35ec65f23a1ed9c453d25ef4e132390d9f3 Mon Sep 17 00:00:00 2001 From: olly Date: Fri, 3 May 2019 13:14:38 +0100 Subject: [PATCH 08/11] Add a couple of assertions to DownloadManager set methods PiperOrigin-RevId: 246491511 --- .../google/android/exoplayer2/offline/DownloadManager.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 e8b7eaf9b2..3bf03dd3e8 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 @@ -306,9 +306,10 @@ public final class DownloadManager { /** * Sets the maximum number of parallel downloads. * - * @param maxParallelDownloads The maximum number of parallel downloads. + * @param maxParallelDownloads The maximum number of parallel downloads. Must be greater than 0. */ public void setMaxParallelDownloads(int maxParallelDownloads) { + Assertions.checkArgument(maxParallelDownloads > 0); if (this.maxParallelDownloads == maxParallelDownloads) { return; } @@ -334,6 +335,7 @@ public final class DownloadManager { * @param minRetryCount The minimum number of times that a download will be retried. */ public void setMinRetryCount(int minRetryCount) { + Assertions.checkArgument(minRetryCount >= 0); if (this.minRetryCount == minRetryCount) { return; } From b53ac32b8c35cb62ede7df920d3d5b0015edad5e Mon Sep 17 00:00:00 2001 From: olly Date: Sun, 5 May 2019 17:25:48 +0100 Subject: [PATCH 09/11] Prevent CachedContentIndex.idToKey from growing without bound PiperOrigin-RevId: 246727723 --- .../upstream/cache/CachedContentIndex.java | 43 +++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java index 98ae6fa6f5..3749ecfc9a 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java @@ -89,6 +89,8 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; * efficiently when the index is next stored. */ private final SparseBooleanArray removedIds; + /** Tracks ids that are new since the index was last stored. */ + private final SparseBooleanArray newIds; private Storage storage; @Nullable private Storage previousStorage; @@ -150,6 +152,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; keyToContent = new HashMap<>(); idToKey = new SparseArray<>(); removedIds = new SparseBooleanArray(); + newIds = new SparseBooleanArray(); Storage databaseStorage = databaseProvider != null ? new DatabaseStorage(databaseProvider) : null; Storage legacyStorage = @@ -206,6 +209,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; idToKey.remove(removedIds.keyAt(i)); } removedIds.clear(); + newIds.clear(); } /** @@ -250,11 +254,19 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; CachedContent cachedContent = keyToContent.get(key); if (cachedContent != null && cachedContent.isEmpty() && !cachedContent.isLocked()) { keyToContent.remove(key); - storage.onRemove(cachedContent); - // Keep an entry in idToKey to stop the id from being reused until the index is next stored. - idToKey.put(cachedContent.id, /* value= */ null); - // Track that the entry should be removed from idToKey when the index is next stored. - removedIds.put(cachedContent.id, /* value= */ true); + int id = cachedContent.id; + boolean neverStored = newIds.get(id); + storage.onRemove(cachedContent, neverStored); + if (neverStored) { + // The id can be reused immediately. + idToKey.remove(id); + newIds.delete(id); + } else { + // Keep an entry in idToKey to stop the id from being reused until the index is next stored, + // and add an entry to removedIds to track that it should be removed when this does happen. + idToKey.put(id, /* value= */ null); + removedIds.put(id, /* value= */ true); + } } } @@ -297,8 +309,9 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; private CachedContent addNew(String key) { int id = getNewId(idToKey); CachedContent cachedContent = new CachedContent(id, key); - keyToContent.put(cachedContent.key, cachedContent); - idToKey.put(cachedContent.id, cachedContent.key); + keyToContent.put(key, cachedContent); + idToKey.put(id, key); + newIds.put(id, true); storage.onUpdate(cachedContent); return cachedContent; } @@ -435,7 +448,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; /** * Ensures incremental changes to the index since the initial {@link #initialize(long)} or last * {@link #storeFully(HashMap)} are persisted. The storage will have been notified of all such - * changes via {@link #onUpdate(CachedContent)} and {@link #onRemove(CachedContent)}. + * changes via {@link #onUpdate(CachedContent)} and {@link #onRemove(CachedContent, boolean)}. * * @param content The key to content map to persist. * @throws IOException If an error occurs persisting the index. @@ -453,8 +466,10 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; * Called when a {@link CachedContent} is removed. * * @param cachedContent The removed {@link CachedContent}. + * @param neverStored True if the {@link CachedContent} was added more recently than when the + * index was last stored. */ - void onRemove(CachedContent cachedContent); + void onRemove(CachedContent cachedContent, boolean neverStored); } /** {@link Storage} implementation that uses an {@link AtomicFile}. */ @@ -540,7 +555,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; } @Override - public void onRemove(CachedContent cachedContent) { + public void onRemove(CachedContent cachedContent, boolean neverStored) { changed = true; } @@ -856,8 +871,12 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; } @Override - public void onRemove(CachedContent cachedContent) { - pendingUpdates.put(cachedContent.id, null); + public void onRemove(CachedContent cachedContent, boolean neverStored) { + if (neverStored) { + pendingUpdates.delete(cachedContent.id); + } else { + pendingUpdates.put(cachedContent.id, null); + } } private Cursor getCursor() { From 1ef49ed205956122bff013c8359d1bce828125fc Mon Sep 17 00:00:00 2001 From: olly Date: Sun, 5 May 2019 17:42:06 +0100 Subject: [PATCH 10/11] Fix broken Javadoc PiperOrigin-RevId: 246728418 --- .../java/com/google/android/exoplayer2/util/GlUtil.java | 4 ++-- .../android/exoplayer2/ui/spherical/CanvasRenderer.java | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/util/GlUtil.java b/library/core/src/main/java/com/google/android/exoplayer2/util/GlUtil.java index 915e855d23..7fc46dc363 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/util/GlUtil.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/util/GlUtil.java @@ -51,7 +51,7 @@ public final class GlUtil { } /** - * Builds a GL shader program from vertex & fragment shader code. + * Builds a GL shader program from vertex and fragment shader code. * * @param vertexCode GLES20 vertex shader program as arrays of strings. Strings are joined by * adding a new line character in between each of them. @@ -64,7 +64,7 @@ public final class GlUtil { } /** - * Builds a GL shader program from vertex & fragment shader code. + * Builds a GL shader program from vertex and fragment shader code. * * @param vertexCode GLES20 vertex shader program. * @param fragmentCode GLES20 fragment shader program. diff --git a/library/ui/src/main/java/com/google/android/exoplayer2/ui/spherical/CanvasRenderer.java b/library/ui/src/main/java/com/google/android/exoplayer2/ui/spherical/CanvasRenderer.java index fdd59101e7..3d7e57bbd2 100644 --- a/library/ui/src/main/java/com/google/android/exoplayer2/ui/spherical/CanvasRenderer.java +++ b/library/ui/src/main/java/com/google/android/exoplayer2/ui/spherical/CanvasRenderer.java @@ -72,7 +72,7 @@ public final class CanvasRenderer { "}" }; - // The quad has 2 triangles built from 4 total vertices. Each vertex has 3 position & 2 texture + // The quad has 2 triangles built from 4 total vertices. Each vertex has 3 position and 2 texture // coordinates. private static final int POSITION_COORDS_PER_VERTEX = 3; private static final int TEXTURE_COORDS_PER_VERTEX = 2; @@ -253,8 +253,8 @@ public final class CanvasRenderer { * Translates an orientation into pixel coordinates on the canvas. * *

This is a minimal hit detection system that works for this quad because it has no model - * matrix. All the math is based on the fact that its size & distance are hard-coded into this - * class. For a more complex 3D mesh, a general bounding box & ray collision system would be + * matrix. All the math is based on the fact that its size and distance are hard-coded into this + * class. For a more complex 3D mesh, a general bounding box and ray collision system would be * required. * * @param yaw Yaw of the orientation in radians. @@ -287,7 +287,7 @@ public final class CanvasRenderer { return null; } // Convert from the polar coordinates of the controller to the rectangular coordinates of the - // View. Note the negative yaw & pitch used to generate Android-compliant x & y coordinates. + // View. Note the negative yaw and pitch used to generate Android-compliant x and y coordinates. float clickXPixel = (float) (widthPixel - clickXUnit * widthPixel / widthUnit); float clickYPixel = (float) (heightPixel - clickYUnit * heightPixel / heightUnit); return new PointF(clickXPixel, clickYPixel); From f4968e1a8e39563603a4b8528d8e826ee8c504ca Mon Sep 17 00:00:00 2001 From: olly Date: Sun, 5 May 2019 17:58:33 +0100 Subject: [PATCH 11/11] Update translations PiperOrigin-RevId: 246729123 --- library/ui/src/main/res/values-af/strings.xml | 2 +- library/ui/src/main/res/values-cs/strings.xml | 2 +- library/ui/src/main/res/values-hi/strings.xml | 4 ++-- library/ui/src/main/res/values-hy/strings.xml | 2 +- library/ui/src/main/res/values-ja/strings.xml | 2 +- library/ui/src/main/res/values-ka/strings.xml | 2 +- library/ui/src/main/res/values-sw/strings.xml | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-) diff --git a/library/ui/src/main/res/values-af/strings.xml b/library/ui/src/main/res/values-af/strings.xml index 9e0fc245fc..8a983c543a 100644 --- a/library/ui/src/main/res/values-af/strings.xml +++ b/library/ui/src/main/res/values-af/strings.xml @@ -21,7 +21,7 @@ Verwyder tans aflaaie Video Oudio - SMS + Teks Geen Outo Onbekend diff --git a/library/ui/src/main/res/values-cs/strings.xml b/library/ui/src/main/res/values-cs/strings.xml index 8c73c01d74..1568074f9f 100644 --- a/library/ui/src/main/res/values-cs/strings.xml +++ b/library/ui/src/main/res/values-cs/strings.xml @@ -21,7 +21,7 @@ Odstraňování staženého obsahu Videa Zvuk - SMS + Text Žádné Automaticky Neznámé diff --git a/library/ui/src/main/res/values-hi/strings.xml b/library/ui/src/main/res/values-hi/strings.xml index da606cd166..8ba92054ff 100644 --- a/library/ui/src/main/res/values-hi/strings.xml +++ b/library/ui/src/main/res/values-hi/strings.xml @@ -31,8 +31,8 @@ सराउंड साउंड 5.1 सराउंड साउंड 7.1 सराउंड साउंड - वैकल्पिक - अतिरिक्त + विकल्प + सप्लिमेंट्री कमेंट्री सबटाइटल %1$.2f एमबीपीएस diff --git a/library/ui/src/main/res/values-hy/strings.xml b/library/ui/src/main/res/values-hy/strings.xml index 11a9124f54..04a2aeb140 100644 --- a/library/ui/src/main/res/values-hy/strings.xml +++ b/library/ui/src/main/res/values-hy/strings.xml @@ -35,6 +35,6 @@ Լրացուցիչ Մեկնաբանություններ Ենթագրեր - %1$.2f մբ/վ + %1$.2f Մբիթ/վ %1$s, %2$s diff --git a/library/ui/src/main/res/values-ja/strings.xml b/library/ui/src/main/res/values-ja/strings.xml index aef5a12a96..b4158736a8 100644 --- a/library/ui/src/main/res/values-ja/strings.xml +++ b/library/ui/src/main/res/values-ja/strings.xml @@ -21,7 +21,7 @@ ダウンロードを削除しています 動画 音声 - SMS + 文字 なし 自動 不明 diff --git a/library/ui/src/main/res/values-ka/strings.xml b/library/ui/src/main/res/values-ka/strings.xml index f7b8272bcc..13ceaaf51f 100644 --- a/library/ui/src/main/res/values-ka/strings.xml +++ b/library/ui/src/main/res/values-ka/strings.xml @@ -21,7 +21,7 @@ მიმდინარეობს ჩამოტვირთვების ამოშლა ვიდეო აუდიო - SMS + ტექსტი არცერთი ავტომატური უცნობი diff --git a/library/ui/src/main/res/values-sw/strings.xml b/library/ui/src/main/res/values-sw/strings.xml index af58d417d6..1cdd325278 100644 --- a/library/ui/src/main/res/values-sw/strings.xml +++ b/library/ui/src/main/res/values-sw/strings.xml @@ -21,7 +21,7 @@ Inaondoa vipakuliwa Video Sauti - SMS + Maandishi Hamna Otomatiki Haijulikani