From 7b5bdd3c445c5a655c3873b9176f564dc979bed3 Mon Sep 17 00:00:00 2001 From: andrewlewis Date: Wed, 14 Feb 2018 00:41:53 -0800 Subject: [PATCH] Release Extractors on the loading thread Releasing the player released the internal playback thread once renderers were released. Releasing a MediaPeriod queued a Loader.ReleaseTask on the loading thread which would post back to the playback thread. If the playback thread had been quit by the time this happened, the release task wouldn't be run. Release on the loading thread instead of the playback thread. This avoids needing to block releasing the player until the loading threads have ended, and ensures that release tasks will run eventually. As part of this change, ExtractorMediaPeriod's call to Extractor.release will now run on the loading thread (which means that all Extractor methods are called on that thread) and other cleanup in ReleaseCallback will run on the loading thread instead of the playback thread. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=185651320 --- RELEASENOTES.md | 3 ++ .../source/ExtractorMediaPeriod.java | 6 ++-- .../source/chunk/ChunkSampleStream.java | 19 +++++------- .../android/exoplayer2/upstream/Loader.java | 30 +++++-------------- .../source/dash/DashMediaPeriod.java | 7 +++-- .../source/hls/HlsSampleStreamWrapper.java | 4 +-- 6 files changed, 29 insertions(+), 40 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 320751113b..e452bd9cd2 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -99,6 +99,9 @@ ([#3796](https://github.com/google/ExoPlayer/issues/3796)). * Check `sys.display-size` on Philips ATVs ([#3807](https://github.com/google/ExoPlayer/issues/3807)). +* Release `Extractor`s on the loading thread to avoid potentially leaking + resources when the playback thread has quit by the time the loading task has + completed. ### 2.6.1 ### diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/ExtractorMediaPeriod.java b/library/core/src/main/java/com/google/android/exoplayer2/source/ExtractorMediaPeriod.java index f4f59115eb..c771188e3b 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/ExtractorMediaPeriod.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/ExtractorMediaPeriod.java @@ -179,24 +179,24 @@ import java.util.Arrays; } public void release() { - boolean releasedSynchronously = loader.release(this); - if (prepared && !releasedSynchronously) { + if (prepared) { // Discard as much as we can synchronously. We only do this if we're prepared, since otherwise // sampleQueues may still be being modified by the loading thread. for (SampleQueue sampleQueue : sampleQueues) { sampleQueue.discardToEnd(); } } + loader.release(this); handler.removeCallbacksAndMessages(null); released = true; } @Override public void onLoaderReleased() { - extractorHolder.release(); for (SampleQueue sampleQueue : sampleQueues) { sampleQueue.reset(); } + extractorHolder.release(); } @Override diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/chunk/ChunkSampleStream.java b/library/core/src/main/java/com/google/android/exoplayer2/source/chunk/ChunkSampleStream.java index e0c5d35996..7096c84c5e 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/chunk/ChunkSampleStream.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/chunk/ChunkSampleStream.java @@ -73,7 +73,7 @@ public class ChunkSampleStream implements SampleStream, S private final BaseMediaChunkOutput mediaChunkOutput; private Format primaryDownstreamTrackFormat; - private ReleaseCallback releaseCallback; + private @Nullable ReleaseCallback releaseCallback; private long pendingResetPositionUs; private long lastSeekPositionUs; /* package */ long decodeOnlyUntilPositionUs; @@ -306,20 +306,17 @@ public class ChunkSampleStream implements SampleStream, S *

This method should be called when the stream is no longer required. Either this method or * {@link #release()} can be used to release this stream. * - * @param callback A callback to be called when the release ends. Will be called synchronously - * from this method if no load is in progress, or asynchronously once the load has been - * canceled otherwise. + * @param callback An optional callback to be called on the loading thread once the loader has + * been released. */ public void release(@Nullable ReleaseCallback callback) { this.releaseCallback = callback; - boolean releasedSynchronously = loader.release(this); - if (!releasedSynchronously) { - // Discard as much as we can synchronously. - primarySampleQueue.discardToEnd(); - for (SampleQueue embeddedSampleQueue : embeddedSampleQueues) { - embeddedSampleQueue.discardToEnd(); - } + // Discard as much as we can synchronously. + primarySampleQueue.discardToEnd(); + for (SampleQueue embeddedSampleQueue : embeddedSampleQueues) { + embeddedSampleQueue.discardToEnd(); } + loader.release(this); } @Override diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/Loader.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/Loader.java index 9e495f42bf..a118f10784 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/Loader.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/Loader.java @@ -20,6 +20,7 @@ import android.os.Handler; import android.os.Looper; import android.os.Message; import android.os.SystemClock; +import android.support.annotation.Nullable; import android.util.Log; import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.TraceUtil; @@ -197,25 +198,17 @@ public final class Loader implements LoaderErrorThrower { * Releases the {@link Loader}. This method should be called when the {@link Loader} is no longer * required. * - * @param callback A callback to be called when the release ends. Will be called synchronously - * from this method if no load is in progress, or asynchronously once the load has been - * canceled otherwise. May be null. - * @return True if {@code callback} was called synchronously. False if it will be called - * asynchronously or if {@code callback} is null. + * @param callback An optional callback to be called on the loading thread once the loader has + * been released. */ - public boolean release(ReleaseCallback callback) { - boolean callbackInvoked = false; + public void release(@Nullable ReleaseCallback callback) { if (currentTask != null) { currentTask.cancel(true); - if (callback != null) { - downloadExecutorService.execute(new ReleaseTask(callback)); - } - } else if (callback != null) { - callback.onLoaderReleased(); - callbackInvoked = true; + } + if (callback != null) { + downloadExecutorService.execute(new ReleaseTask(callback)); } downloadExecutorService.shutdown(); - return callbackInvoked; } // LoaderErrorThrower implementation. @@ -419,7 +412,7 @@ public final class Loader implements LoaderErrorThrower { } - private static final class ReleaseTask extends Handler implements Runnable { + private static final class ReleaseTask implements Runnable { private final ReleaseCallback callback; @@ -429,13 +422,6 @@ public final class Loader implements LoaderErrorThrower { @Override public void run() { - if (getLooper().getThread().isAlive()) { - sendEmptyMessage(0); - } - } - - @Override - public void handleMessage(Message msg) { callback.onLoaderReleased(); } diff --git a/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/DashMediaPeriod.java b/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/DashMediaPeriod.java index d6d6ca821c..00baf15228 100644 --- a/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/DashMediaPeriod.java +++ b/library/dash/src/main/java/com/google/android/exoplayer2/source/dash/DashMediaPeriod.java @@ -153,7 +153,7 @@ import java.util.List; // ChunkSampleStream.ReleaseCallback implementation. @Override - public void onSampleStreamReleased(ChunkSampleStream stream) { + public synchronized void onSampleStreamReleased(ChunkSampleStream stream) { PlayerTrackEmsgHandler trackEmsgHandler = trackEmsgHandlerBySampleStream.remove(stream); if (trackEmsgHandler != null) { trackEmsgHandler.release(); @@ -565,7 +565,10 @@ import java.util.List; positionUs, minLoadableRetryCount, eventDispatcher); - trackEmsgHandlerBySampleStream.put(stream, trackPlayerEmsgHandler); + synchronized (this) { + // The map is also accessed on the loading thread so synchronize access. + trackEmsgHandlerBySampleStream.put(stream, trackPlayerEmsgHandler); + } return stream; } diff --git a/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/HlsSampleStreamWrapper.java b/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/HlsSampleStreamWrapper.java index 508f2f0f2f..f027ba5b05 100644 --- a/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/HlsSampleStreamWrapper.java +++ b/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/HlsSampleStreamWrapper.java @@ -386,14 +386,14 @@ import java.util.Arrays; } public void release() { - boolean releasedSynchronously = loader.release(this); - if (prepared && !releasedSynchronously) { + if (prepared) { // Discard as much as we can synchronously. We only do this if we're prepared, since otherwise // sampleQueues may still be being modified by the loading thread. for (SampleQueue sampleQueue : sampleQueues) { sampleQueue.discardToEnd(); } } + loader.release(this); handler.removeCallbacksAndMessages(null); released = true; } -- GitLab