From 7ef028c67b657239810abe3200928070ec7319d0 Mon Sep 17 00:00:00 2001 From: olly Date: Fri, 3 Jun 2016 04:35:33 -0700 Subject: [PATCH] Fix delivery of onLoadCanceled to occur before thread dies. This removes "message sent on dead thread" warnings in nearly all cases, and guarantees delivery of load cancelation to event listeners. Issue: #426 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=123957691 --- .../android/exoplayer/SingleSampleSource.java | 7 ++- .../exoplayer/chunk/ChunkTrackStream.java | 3 +- .../exoplayer/dash/DashSampleSource.java | 5 +- .../extractor/ExtractorSampleSource.java | 5 +- .../exoplayer/hls/HlsSampleSource.java | 2 +- .../exoplayer/hls/HlsTrackStreamWrapper.java | 5 +- .../SmoothStreamingSampleSource.java | 3 +- .../android/exoplayer/upstream/Loader.java | 58 +++++++++++++++---- 8 files changed, 61 insertions(+), 27 deletions(-) diff --git a/library/src/main/java/com/google/android/exoplayer/SingleSampleSource.java b/library/src/main/java/com/google/android/exoplayer/SingleSampleSource.java index c8ef102935..19c21436be 100644 --- a/library/src/main/java/com/google/android/exoplayer/SingleSampleSource.java +++ b/library/src/main/java/com/google/android/exoplayer/SingleSampleSource.java @@ -166,7 +166,6 @@ public final class SingleSampleSource implements SampleSource, TrackStream, @Override public void release() { - streamState = STREAM_STATE_END_OF_STREAM; loader.release(); } @@ -221,8 +220,10 @@ public final class SingleSampleSource implements SampleSource, TrackStream, } @Override - public void onLoadCanceled(SingleSampleSource loadable, long elapsedMs) { - maybeStartLoading(); + public void onLoadCanceled(SingleSampleSource loadable, long elapsedMs, boolean released) { + if (!released) { + maybeStartLoading(); + } } @Override diff --git a/library/src/main/java/com/google/android/exoplayer/chunk/ChunkTrackStream.java b/library/src/main/java/com/google/android/exoplayer/chunk/ChunkTrackStream.java index 18964cdb6d..aab4c68dc3 100644 --- a/library/src/main/java/com/google/android/exoplayer/chunk/ChunkTrackStream.java +++ b/library/src/main/java/com/google/android/exoplayer/chunk/ChunkTrackStream.java @@ -59,7 +59,6 @@ public class ChunkTrackStream implements TrackStream, private long pendingResetPositionUs; private boolean loadingFinished; - private boolean released; /** * @param chunkSource A {@link ChunkSource} from which chunks to load are obtained. @@ -233,7 +232,7 @@ public class ChunkTrackStream implements TrackStream, } @Override - public void onLoadCanceled(Chunk loadable, long elapsedMs) { + public void onLoadCanceled(Chunk loadable, long elapsedMs, boolean released) { eventDispatcher.loadCanceled(loadable.bytesLoaded()); if (!released) { restartFrom(pendingResetPositionUs); diff --git a/library/src/main/java/com/google/android/exoplayer/dash/DashSampleSource.java b/library/src/main/java/com/google/android/exoplayer/dash/DashSampleSource.java index dd070da8f8..2fab46d182 100644 --- a/library/src/main/java/com/google/android/exoplayer/dash/DashSampleSource.java +++ b/library/src/main/java/com/google/android/exoplayer/dash/DashSampleSource.java @@ -360,7 +360,8 @@ public final class DashSampleSource implements SampleSource { } @Override - public void onLoadCanceled(UriLoadable loadable, long elapsedMs) { + public void onLoadCanceled(UriLoadable loadable, long elapsedMs, + boolean released) { // Do nothing. } @@ -380,7 +381,7 @@ public final class DashSampleSource implements SampleSource { } @Override - public void onLoadCanceled(UriLoadable loadable, long elapsedMs) { + public void onLoadCanceled(UriLoadable loadable, long elapsedMs, boolean released) { // Do nothing. } diff --git a/library/src/main/java/com/google/android/exoplayer/extractor/ExtractorSampleSource.java b/library/src/main/java/com/google/android/exoplayer/extractor/ExtractorSampleSource.java index 2cdb6d3d69..5aafba6b44 100644 --- a/library/src/main/java/com/google/android/exoplayer/extractor/ExtractorSampleSource.java +++ b/library/src/main/java/com/google/android/exoplayer/extractor/ExtractorSampleSource.java @@ -479,7 +479,6 @@ public final class ExtractorSampleSource implements SampleSource, ExtractorOutpu for (DefaultTrackOutput sampleQueue : sampleQueues) { sampleQueue.disable(); } - enabledTrackCount = 0; loader.release(); } @@ -510,9 +509,9 @@ public final class ExtractorSampleSource implements SampleSource, ExtractorOutpu } @Override - public void onLoadCanceled(ExtractingLoadable loadable, long elapsedMs) { + public void onLoadCanceled(ExtractingLoadable loadable, long elapsedMs, boolean released) { copyLengthFromLoader(loadable); - if (enabledTrackCount > 0) { + if (!released && enabledTrackCount > 0) { restartFrom(pendingResetPositionUs); } } diff --git a/library/src/main/java/com/google/android/exoplayer/hls/HlsSampleSource.java b/library/src/main/java/com/google/android/exoplayer/hls/HlsSampleSource.java index 9dacf05b35..e56352472a 100644 --- a/library/src/main/java/com/google/android/exoplayer/hls/HlsSampleSource.java +++ b/library/src/main/java/com/google/android/exoplayer/hls/HlsSampleSource.java @@ -249,7 +249,7 @@ public final class HlsSampleSource implements SampleSource, } @Override - public void onLoadCanceled(UriLoadable loadable, long elapsedMs) { + public void onLoadCanceled(UriLoadable loadable, long elapsedMs, boolean released) { // Do nothing. } diff --git a/library/src/main/java/com/google/android/exoplayer/hls/HlsTrackStreamWrapper.java b/library/src/main/java/com/google/android/exoplayer/hls/HlsTrackStreamWrapper.java index f72495aa81..3f701b75b4 100644 --- a/library/src/main/java/com/google/android/exoplayer/hls/HlsTrackStreamWrapper.java +++ b/library/src/main/java/com/google/android/exoplayer/hls/HlsTrackStreamWrapper.java @@ -276,7 +276,6 @@ import java.util.List; sampleQueues.valueAt(i).disable(); } if (enabledTrackCount > 0) { - enabledTrackCount = 0; loadControl.unregister(this); } loader.release(); @@ -331,9 +330,9 @@ import java.util.List; } @Override - public void onLoadCanceled(Chunk loadable, long elapsedMs) { + public void onLoadCanceled(Chunk loadable, long elapsedMs, boolean released) { eventDispatcher.loadCanceled(loadable.bytesLoaded()); - if (enabledTrackCount > 0) { + if (!released && enabledTrackCount > 0) { restartFrom(pendingResetPositionUs); } } diff --git a/library/src/main/java/com/google/android/exoplayer/smoothstreaming/SmoothStreamingSampleSource.java b/library/src/main/java/com/google/android/exoplayer/smoothstreaming/SmoothStreamingSampleSource.java index d9db9fefb3..45063c2519 100644 --- a/library/src/main/java/com/google/android/exoplayer/smoothstreaming/SmoothStreamingSampleSource.java +++ b/library/src/main/java/com/google/android/exoplayer/smoothstreaming/SmoothStreamingSampleSource.java @@ -237,7 +237,8 @@ public final class SmoothStreamingSampleSource implements SampleSource, } @Override - public void onLoadCanceled(UriLoadable loadable, long elapsedMs) { + public void onLoadCanceled(UriLoadable loadable, long elapsedMs, + boolean released) { // Do nothing. } diff --git a/library/src/main/java/com/google/android/exoplayer/upstream/Loader.java b/library/src/main/java/com/google/android/exoplayer/upstream/Loader.java index ed78c71256..4e5907fc47 100644 --- a/library/src/main/java/com/google/android/exoplayer/upstream/Loader.java +++ b/library/src/main/java/com/google/android/exoplayer/upstream/Loader.java @@ -79,14 +79,24 @@ public final class Loader { /** * Invoked when a load has been canceled. + *

+ * If the {@link Loader} has not been released then there is guaranteed to exist a memory + * barrier between {@link Loadable#load()} exiting and this callback being invoked. If the + * {@link Loader} has been released then this callback may be invoked before + * {@link Loadable#load()} exits. * * @param loadable The loadable whose load has been canceled. * @param elapsedMs The elapsed time in milliseconds since loading started. + * @param released True if the load was canceled because the {@link Loader} was released. False + * otherwise. */ - void onLoadCanceled(T loadable, long elapsedMs); + void onLoadCanceled(T loadable, long elapsedMs, boolean released); /** * Invoked when a load has completed. + *

+ * There is guaranteed to exist a memory barrier between {@link Loadable#load()} exiting and + * this callback being invoked. * * @param loadable The loadable whose load has completed. * @param elapsedMs The elapsed time in milliseconds since loading started. @@ -95,6 +105,9 @@ public final class Loader { /** * Invoked when a load encounters an error. + *

+ * There is guaranteed to exist a memory barrier between {@link Loadable#load()} exiting and + * this callback being invoked. * * @param loadable The loadable whose load has encountered an error. * @param elapsedMs The elapsed time in milliseconds since loading started. @@ -179,7 +192,7 @@ public final class Loader { * This method should only be called when a load is in progress. */ public void cancelLoading() { - currentTask.cancel(); + currentTask.cancel(false); } /** @@ -189,7 +202,7 @@ public final class Loader { */ public void release() { if (currentTask != null) { - cancelLoading(); + currentTask.cancel(true); } downloadExecutorService.shutdown(); } @@ -208,6 +221,7 @@ public final class Loader { private int errorCount; private volatile Thread executorThread; + private volatile boolean released; public LoadTask(Looper looper, T loadable, Loader.Callback callback, int minRetryCount) { super(looper); @@ -233,17 +247,24 @@ public final class Loader { } } - public void cancel() { + public void cancel(boolean released) { + this.released = released; currentError = null; if (hasMessages(MSG_START)) { removeMessages(MSG_START); - sendEmptyMessage(MSG_CANCEL); + if (!released) { + sendEmptyMessage(MSG_CANCEL); + } } else { loadable.cancelLoad(); if (executorThread != null) { executorThread.interrupt(); } } + if (released) { + finish(); + callback.onLoadCanceled(loadable, SystemClock.elapsedRealtime() - startTimeMs, true); + } } @Override @@ -258,29 +279,42 @@ public final class Loader { TraceUtil.endSection(); } } - sendEmptyMessage(MSG_END_OF_SOURCE); + if (!released) { + sendEmptyMessage(MSG_END_OF_SOURCE); + } } catch (IOException e) { - obtainMessage(MSG_IO_EXCEPTION, e).sendToTarget(); + if (!released) { + obtainMessage(MSG_IO_EXCEPTION, e).sendToTarget(); + } } catch (InterruptedException e) { // The load was canceled. Assertions.checkState(loadable.isLoadCanceled()); - sendEmptyMessage(MSG_END_OF_SOURCE); + if (!released) { + sendEmptyMessage(MSG_END_OF_SOURCE); + } } catch (Exception e) { // This should never happen, but handle it anyway. Log.e(TAG, "Unexpected exception loading stream", e); - obtainMessage(MSG_IO_EXCEPTION, new UnexpectedLoaderException(e)).sendToTarget(); + if (!released) { + obtainMessage(MSG_IO_EXCEPTION, new UnexpectedLoaderException(e)).sendToTarget(); + } } catch (Error e) { // We'd hope that the platform would kill the process if an Error is thrown here, but the // executor may catch the error (b/20616433). Throw it here, but also pass and throw it from // the handler thread so that the process dies even if the executor behaves in this way. Log.e(TAG, "Unexpected error loading stream", e); - obtainMessage(MSG_FATAL_ERROR, e).sendToTarget(); + if (!released) { + obtainMessage(MSG_FATAL_ERROR, e).sendToTarget(); + } throw e; } } @Override public void handleMessage(Message msg) { + if (released) { + return; + } if (msg.what == MSG_START) { submitToExecutor(); return; @@ -291,12 +325,12 @@ public final class Loader { finish(); long elapsedMs = SystemClock.elapsedRealtime() - startTimeMs; if (loadable.isLoadCanceled()) { - callback.onLoadCanceled(loadable, elapsedMs); + callback.onLoadCanceled(loadable, elapsedMs, false); return; } switch (msg.what) { case MSG_CANCEL: - callback.onLoadCanceled(loadable, elapsedMs); + callback.onLoadCanceled(loadable, elapsedMs, false); break; case MSG_END_OF_SOURCE: callback.onLoadCompleted(loadable, elapsedMs);