From 1154e8098ab079f5faf9c2cd1decc2339468c118 Mon Sep 17 00:00:00 2001 From: christosts Date: Thu, 21 May 2020 11:44:36 +0100 Subject: [PATCH] Rollback of https://github.com/google/ExoPlayer/commit/78c850a88579beb5ebf2c487a0bdd45a97d0c881 *** Original commit *** Remove set timeout on release() and setSurface() Removes the experimental methods to set a timeout when releasing the player and setting the surface. *** PiperOrigin-RevId: 312647457 --- .../google/android/exoplayer2/ExoPlayer.java | 20 +++ .../android/exoplayer2/ExoPlayerImpl.java | 23 +++- .../exoplayer2/ExoPlayerImplInternal.java | 85 ++++++++++-- .../android/exoplayer2/PlayerMessage.java | 45 +++++++ .../android/exoplayer2/PlayerMessageTest.java | 125 ++++++++++++++++++ 5 files changed, 286 insertions(+), 12 deletions(-) create mode 100644 library/core/src/test/java/com/google/android/exoplayer2/PlayerMessageTest.java diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayer.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayer.java index 89a28bd764..b4cd9a399d 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayer.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayer.java @@ -149,6 +149,8 @@ public interface ExoPlayer extends Player { private SeekParameters seekParameters; private boolean pauseAtEndOfMediaItems; private boolean buildCalled; + + private long releaseTimeoutMs; private boolean throwWhenStuckBuffering; /** @@ -213,6 +215,20 @@ public interface ExoPlayer extends Player { clock = Clock.DEFAULT; } + /** + * Set a limit on the time a call to {@link ExoPlayer#release()} can spend. If a call to {@link + * ExoPlayer#release()} takes more than {@code timeoutMs} milliseconds to complete, the player + * will raise an error via {@link Player.EventListener#onPlayerError}. + * + *

This method is experimental, and will be renamed or removed in a future release. + * + * @param timeoutMs The time limit in milliseconds, or 0 for no limit. + */ + public Builder experimental_setReleaseTimeoutMs(long timeoutMs) { + releaseTimeoutMs = timeoutMs; + return this; + } + /** * Sets whether the player should throw when it detects it's stuck buffering. * @@ -389,6 +405,10 @@ public interface ExoPlayer extends Player { pauseAtEndOfMediaItems, clock, looper); + + if (releaseTimeoutMs > 0) { + player.experimental_setReleaseTimeoutMs(releaseTimeoutMs); + } if (throwWhenStuckBuffering) { player.experimental_throwWhenStuckBuffering(); } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java index e98da39a10..26357a18dc 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java @@ -45,6 +45,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.TimeoutException; /** * An {@link ExoPlayer} implementation. Instances can be obtained from {@link ExoPlayer.Builder}. @@ -177,6 +178,20 @@ import java.util.concurrent.CopyOnWriteArrayList; internalPlayerHandler = new Handler(internalPlayer.getPlaybackLooper()); } + /** + * Set a limit on the time a call to {@link #release()} can spend. If a call to {@link #release()} + * takes more than {@code timeoutMs} milliseconds to complete, the player will raise an error via + * {@link Player.EventListener#onPlayerError}. + * + *

This method is experimental, and will be renamed or removed in a future release. It should + * only be called before the player is used. + * + * @param timeoutMs The time limit in milliseconds, or 0 for no limit. + */ + public void experimental_setReleaseTimeoutMs(long timeoutMs) { + internalPlayer.experimental_setReleaseTimeoutMs(timeoutMs); + } + /** * Configures the player to throw when it detects it's stuck buffering. * @@ -675,7 +690,13 @@ import java.util.concurrent.CopyOnWriteArrayList; Log.i(TAG, "Release " + Integer.toHexString(System.identityHashCode(this)) + " [" + ExoPlayerLibraryInfo.VERSION_SLASHY + "] [" + Util.DEVICE_DEBUG_INFO + "] [" + ExoPlayerLibraryInfo.registeredModules() + "]"); - internalPlayer.release(); + if (!internalPlayer.release()) { + notifyListeners( + listener -> + listener.onPlayerError( + ExoPlaybackException.createForUnexpected( + new RuntimeException(new TimeoutException("Player release timed out."))))); + } applicationHandler.removeCallbacksAndMessages(null); playbackInfo = getResetPlaybackInfo( diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java index 5d698b8f66..53c8a5d080 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImplInternal.java @@ -133,6 +133,8 @@ import java.util.concurrent.atomic.AtomicBoolean; private long rendererPositionUs; private int nextPendingMessageIndexHint; private boolean deliverPendingMessageAtStartPositionRequired; + + private long releaseTimeoutMs; private boolean throwWhenStuckBuffering; public ExoPlayerImplInternal( @@ -189,6 +191,10 @@ import java.util.concurrent.atomic.AtomicBoolean; } } + public void experimental_setReleaseTimeoutMs(long releaseTimeoutMs) { + this.releaseTimeoutMs = releaseTimeoutMs; + } + public void experimental_throwWhenStuckBuffering() { throwWhenStuckBuffering = true; } @@ -316,23 +322,23 @@ import java.util.concurrent.atomic.AtomicBoolean; } } - public synchronized void release() { + public synchronized boolean release() { if (released || !internalPlaybackThread.isAlive()) { - return; + return true; } + handler.sendEmptyMessage(MSG_RELEASE); - boolean wasInterrupted = false; - while (!released) { - try { - wait(); - } catch (InterruptedException e) { - wasInterrupted = true; + try { + if (releaseTimeoutMs > 0) { + waitUntilReleased(releaseTimeoutMs); + } else { + waitUntilReleased(); } - } - if (wasInterrupted) { - // Restore the interrupted status. + } catch (InterruptedException e) { Thread.currentThread().interrupt(); } + + return released; } public Looper getPlaybackLooper() { @@ -498,6 +504,63 @@ import java.util.concurrent.atomic.AtomicBoolean; // Private methods. + /** + * Blocks the current thread until {@link #releaseInternal()} is executed on the playback Thread. + * + *

If the current thread is interrupted while waiting for {@link #releaseInternal()} to + * complete, this method will delay throwing the {@link InterruptedException} to ensure that the + * underlying resources have been released, and will an {@link InterruptedException} after + * {@link #releaseInternal()} is complete. + * + * @throws {@link InterruptedException} if the current Thread was interrupted while waiting for + * {@link #releaseInternal()} to complete. + */ + private synchronized void waitUntilReleased() throws InterruptedException { + InterruptedException interruptedException = null; + while (!released) { + try { + wait(); + } catch (InterruptedException e) { + interruptedException = e; + } + } + + if (interruptedException != null) { + throw interruptedException; + } + } + + /** + * Blocks the current thread until {@link #releaseInternal()} is performed on the playback Thread + * or the specified amount of time has elapsed. + * + *

If the current thread is interrupted while waiting for {@link #releaseInternal()} to + * complete, this method will delay throwing the {@link InterruptedException} to ensure that the + * underlying resources have been released or the operation timed out, and will throw an {@link + * InterruptedException} afterwards. + * + * @param timeoutMs the time in milliseconds to wait for {@link #releaseInternal()} to complete. + * @throws {@link InterruptedException} if the current Thread was interrupted while waiting for + * {@link #releaseInternal()} to complete. + */ + private synchronized void waitUntilReleased(long timeoutMs) throws InterruptedException { + long deadlineMs = clock.elapsedRealtime() + timeoutMs; + long remainingMs = timeoutMs; + InterruptedException interruptedException = null; + while (!released && remainingMs > 0) { + try { + wait(remainingMs); + } catch (InterruptedException e) { + interruptedException = e; + } + remainingMs = deadlineMs - clock.elapsedRealtime(); + } + + if (interruptedException != null) { + throw interruptedException; + } + } + private void setState(int state) { if (playbackInfo.playbackState != state) { playbackInfo = playbackInfo.copyWithPlaybackState(state); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/PlayerMessage.java b/library/core/src/main/java/com/google/android/exoplayer2/PlayerMessage.java index 9837cb59da..be7c7ce973 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/PlayerMessage.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/PlayerMessage.java @@ -17,7 +17,10 @@ package com.google.android.exoplayer2; import android.os.Handler; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import com.google.android.exoplayer2.util.Assertions; +import com.google.android.exoplayer2.util.Clock; +import java.util.concurrent.TimeoutException; /** * Defines a player message which can be sent with a {@link Sender} and received by a {@link @@ -289,6 +292,28 @@ public final class PlayerMessage { return isDelivered; } + /** + * Blocks until after the message has been delivered or the player is no longer able to deliver + * the message or the specified waiting time elapses. + * + *

Note that this method can't be called if the current thread is the same thread used by the + * message handler set with {@link #setHandler(Handler)} as it would cause a deadlock. + * + * @param timeoutMs the maximum time to wait in milliseconds. + * @return Whether the message was delivered successfully. + * @throws IllegalStateException If this method is called before {@link #send()}. + * @throws IllegalStateException If this method is called on the same thread used by the message + * handler set with {@link #setHandler(Handler)}. + * @throws TimeoutException If the waiting time elapsed and this message has not been delivered + * and the player is still able to deliver the message. + * @throws InterruptedException If the current thread is interrupted while waiting for the message + * to be delivered. + */ + public synchronized boolean experimental_blockUntilDelivered(long timeoutMs) + throws InterruptedException, TimeoutException { + return experimental_blockUntilDelivered(timeoutMs, Clock.DEFAULT); + } + /** * Marks the message as processed. Should only be called by a {@link Sender} and may be called * multiple times. @@ -302,4 +327,24 @@ public final class PlayerMessage { isProcessed = true; notifyAll(); } + + @VisibleForTesting() + /* package */ synchronized boolean experimental_blockUntilDelivered(long timeoutMs, Clock clock) + throws InterruptedException, TimeoutException { + Assertions.checkState(isSent); + Assertions.checkState(handler.getLooper().getThread() != Thread.currentThread()); + + long deadlineMs = clock.elapsedRealtime() + timeoutMs; + long remainingMs = timeoutMs; + while (!isProcessed && remainingMs > 0) { + wait(remainingMs); + remainingMs = deadlineMs - clock.elapsedRealtime(); + } + + if (!isProcessed) { + throw new TimeoutException("Message delivery timed out."); + } + + return isDelivered; + } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/PlayerMessageTest.java b/library/core/src/test/java/com/google/android/exoplayer2/PlayerMessageTest.java new file mode 100644 index 0000000000..874a8c5a5a --- /dev/null +++ b/library/core/src/test/java/com/google/android/exoplayer2/PlayerMessageTest.java @@ -0,0 +1,125 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.android.exoplayer2; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.mockito.MockitoAnnotations.initMocks; + +import android.os.Handler; +import android.os.HandlerThread; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.android.exoplayer2.util.Clock; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; + +/** Unit test for {@link PlayerMessage}. */ +@RunWith(AndroidJUnit4.class) +public class PlayerMessageTest { + + private static final long TIMEOUT_MS = 10; + + @Mock Clock clock; + private HandlerThread handlerThread; + private PlayerMessage message; + + @Before + public void setUp() { + initMocks(this); + PlayerMessage.Sender sender = (message) -> {}; + PlayerMessage.Target target = (messageType, payload) -> {}; + handlerThread = new HandlerThread("TestHandler"); + handlerThread.start(); + Handler handler = new Handler(handlerThread.getLooper()); + message = + new PlayerMessage(sender, target, Timeline.EMPTY, /* defaultWindowIndex= */ 0, handler); + } + + @After + public void tearDown() { + handlerThread.quit(); + } + + @Test + public void experimental_blockUntilDelivered_timesOut() throws Exception { + when(clock.elapsedRealtime()).thenReturn(0L).thenReturn(TIMEOUT_MS * 2); + + try { + message.send().experimental_blockUntilDelivered(TIMEOUT_MS, clock); + fail(); + } catch (TimeoutException expected) { + } + + // Ensure experimental_blockUntilDelivered() entered the blocking loop + verify(clock, Mockito.times(2)).elapsedRealtime(); + } + + @Test + public void experimental_blockUntilDelivered_onAlreadyProcessed_succeeds() throws Exception { + when(clock.elapsedRealtime()).thenReturn(0L); + + message.send().markAsProcessed(/* isDelivered= */ true); + + assertThat(message.experimental_blockUntilDelivered(TIMEOUT_MS, clock)).isTrue(); + } + + @Test + public void experimental_blockUntilDelivered_markAsProcessedWhileBlocked_succeeds() + throws Exception { + message.send(); + + // Use a separate Thread to mark the message as processed. + CountDownLatch prepareLatch = new CountDownLatch(1); + ExecutorService executorService = Executors.newSingleThreadExecutor(); + Future future = + executorService.submit( + () -> { + prepareLatch.await(); + message.markAsProcessed(true); + return true; + }); + + when(clock.elapsedRealtime()) + .thenReturn(0L) + .then( + (invocation) -> { + // Signal the background thread to call PlayerMessage#markAsProcessed. + prepareLatch.countDown(); + return TIMEOUT_MS - 1; + }); + + try { + assertThat(message.experimental_blockUntilDelivered(TIMEOUT_MS, clock)).isTrue(); + // Ensure experimental_blockUntilDelivered() entered the blocking loop. + verify(clock, Mockito.atLeast(2)).elapsedRealtime(); + future.get(1, TimeUnit.SECONDS); + } finally { + executorService.shutdown(); + } + } +}