Enable detach surface timeout by default.

Experiments showed the timeout is beneficial to avoid ANRs and
we can thus enable the feature by default.

Also add configuration to set the timeout if required.

Issue: #5887
PiperOrigin-RevId: 335652506

RELEASENOTES.md

@@ -12,8 +12,11 @@
         ([#4463](https://github.com/google/ExoPlayer/issues/4463)).
     *   Add a getter and callback for static metadata to the player
         ([#7266](https://github.com/google/ExoPlayer/issues/7266)).
-    *   Timeout on release to prevent ANRs if the underlying platform call
+    *   Time out on release to prevent ANRs if the underlying platform call
         is stuck ([#4352](https://github.com/google/ExoPlayer/issues/4352)).
+    *   Time out when detaching a surface to prevent ANRs if the underlying
+        platform call is stuck
+        ([#5887](https://github.com/google/ExoPlayer/issues/5887)).
 *   Track selection:
     *   Add option to specify multiple preferred audio or text languages.
 *   Data sources:

library/core/src/main/java/com/google/android/exoplayer2/ExoPlaybackException.java

@@ -83,16 +83,17 @@ public final class ExoPlaybackException extends Exception {
 
   /**
    * The operation which produced the timeout error. One of {@link #TIMEOUT_OPERATION_RELEASE},
-   * {@link #TIMEOUT_OPERATION_SET_FOREGROUND_MODE} or {@link #TIMEOUT_OPERATION_UNDEFINED}. Note
-   * that new operations may be added in the future and error handling should handle unknown
-   * operation values.
+   * {@link #TIMEOUT_OPERATION_SET_FOREGROUND_MODE}, {@link #TIMEOUT_OPERATION_DETACH_SURFACE} or
+   * {@link #TIMEOUT_OPERATION_UNDEFINED}. Note that new operations may be added in the future and
+   * error handling should handle unknown operation values.
    */
   @Documented
   @Retention(RetentionPolicy.SOURCE)
   @IntDef({
     TIMEOUT_OPERATION_UNDEFINED,
     TIMEOUT_OPERATION_RELEASE,
-    TIMEOUT_OPERATION_SET_FOREGROUND_MODE
+    TIMEOUT_OPERATION_SET_FOREGROUND_MODE,
+    TIMEOUT_OPERATION_DETACH_SURFACE
   })
   public @interface TimeoutOperation {}
 
@@ -102,6 +103,8 @@ public final class ExoPlaybackException extends Exception {
   public static final int TIMEOUT_OPERATION_RELEASE = 1;
   /** The error occurred in {@link ExoPlayer#setForegroundMode}. */
   public static final int TIMEOUT_OPERATION_SET_FOREGROUND_MODE = 2;
+  /** The error occurred while detaching a surface from the player. */
+  public static final int TIMEOUT_OPERATION_DETACH_SURFACE = 3;
 
   /** If {@link #type} is {@link #TYPE_RENDERER}, this is the name of the renderer. */
   @Nullable public final String rendererName;

library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java

@@ -656,18 +656,28 @@ public void setForegroundMode(boolean foregroundMode) {
     if (this.foregroundMode != foregroundMode) {
       this.foregroundMode = foregroundMode;
       if (!internalPlayer.setForegroundMode(foregroundMode)) {
-        notifyListeners(
-            listener ->
-                listener.onPlayerError(
-                    ExoPlaybackException.createForTimeout(
-                        new TimeoutException("Setting foreground mode timed out."),
-                        ExoPlaybackException.TIMEOUT_OPERATION_SET_FOREGROUND_MODE)));
+        stop(
+            /* reset= */ false,
+            ExoPlaybackException.createForTimeout(
+                new TimeoutException("Setting foreground mode timed out."),
+                ExoPlaybackException.TIMEOUT_OPERATION_SET_FOREGROUND_MODE));
       }
     }
   }
 
   @Override
   public void stop(boolean reset) {
+    stop(reset, /* error= */ null);
+  }
+
+  /**
+   * Stops the player.
+   *
+   * @param reset Whether the playlist should be cleared and whether the playback position and
+   *     playback error should be reset.
+   * @param error An optional {@link ExoPlaybackException} to set.
+   */
+  public void stop(boolean reset, @Nullable ExoPlaybackException error) {
     PlaybackInfo playbackInfo;
     if (reset) {
       playbackInfo =
@@ -680,6 +690,9 @@ public void stop(boolean reset) {
       playbackInfo.totalBufferedDurationUs = 0;
     }
     playbackInfo = playbackInfo.copyWithPlaybackState(Player.STATE_IDLE);
+    if (error != null) {
+      playbackInfo = playbackInfo.copyWithPlaybackError(error);
+    }
     pendingOperationAcks++;
     internalPlayer.stop();
     updatePlaybackInfo(

library/core/src/main/java/com/google/android/exoplayer2/PlayerMessage.java

@@ -269,6 +269,20 @@ public synchronized boolean isCanceled() {
     return isCanceled;
   }
 
+  /**
+   * Marks the message as processed. Should only be called by a {@link Sender} and may be called
+   * multiple times.
+   *
+   * @param isDelivered Whether the message has been delivered to its target. The message is
+   *     considered as being delivered when this method has been called with {@code isDelivered} set
+   *     to true at least once.
+   */
+  public synchronized void markAsProcessed(boolean isDelivered) {
+    this.isDelivered |= isDelivered;
+    isProcessed = true;
+    notifyAll();
+  }
+
   /**
    * Blocks until after the message has been delivered or the player is no longer able to deliver
    * the message.
@@ -292,44 +306,30 @@ public synchronized boolean blockUntilDelivered() throws InterruptedException {
     return isDelivered;
   }
 
-  /**
-   * Marks the message as processed. Should only be called by a {@link Sender} and may be called
-   * multiple times.
-   *
-   * @param isDelivered Whether the message has been delivered to its target. The message is
-   *     considered as being delivered when this method has been called with {@code isDelivered} set
-   *     to true at least once.
-   */
-  public synchronized void markAsProcessed(boolean isDelivered) {
-    this.isDelivered |= isDelivered;
-    isProcessed = true;
-    notifyAll();
-  }
-
   /**
    * 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.
+   * the message or the specified timeout elapsed.
    *
    * <p>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.
+   * @param timeoutMs The timeout 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 TimeoutException If the {@code timeoutMs} 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 experimentalBlockUntilDelivered(long timeoutMs)
+  public synchronized boolean blockUntilDelivered(long timeoutMs)
       throws InterruptedException, TimeoutException {
-    return experimentalBlockUntilDelivered(timeoutMs, Clock.DEFAULT);
+    return blockUntilDelivered(timeoutMs, Clock.DEFAULT);
   }
 
   @VisibleForTesting()
-  /* package */ synchronized boolean experimentalBlockUntilDelivered(long timeoutMs, Clock clock)
+  /* package */ synchronized boolean blockUntilDelivered(long timeoutMs, Clock clock)
       throws InterruptedException, TimeoutException {
     Assertions.checkState(isSent);
     Assertions.checkState(handler.getLooper().getThread() != Thread.currentThread());

library/core/src/main/java/com/google/android/exoplayer2/SimpleExoPlayer.java

@@ -67,6 +67,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.concurrent.TimeoutException;
 
 /**
  * An {@link ExoPlayer} implementation that uses default {@link Renderer} components. Instances can
@@ -80,6 +81,9 @@ public class SimpleExoPlayer extends BasePlayer
         Player.MetadataComponent,
         Player.DeviceComponent {
 
+  /** The default timeout for detaching a surface from the player, in milliseconds. */
+  public static final long DEFAULT_DETACH_SURFACE_TIMEOUT_MS = 2_000;
+
   /** @deprecated Use {@link com.google.android.exoplayer2.video.VideoListener}. */
   @Deprecated
   public interface VideoListener extends com.google.android.exoplayer2.video.VideoListener {}
@@ -111,6 +115,7 @@ public static final class Builder {
     private boolean useLazyPreparation;
     private SeekParameters seekParameters;
     private long releaseTimeoutMs;
+    private long detachSurfaceTimeoutMs;
     private boolean pauseAtEndOfMediaItems;
     private boolean throwWhenStuckBuffering;
     private boolean buildCalled;
@@ -145,6 +150,7 @@ public static final class Builder {
      *   <li>{@code useLazyPreparation}: {@code true}
      *   <li>{@link SeekParameters}: {@link SeekParameters#DEFAULT}
      *   <li>{@code releaseTimeoutMs}: {@link ExoPlayer#DEFAULT_RELEASE_TIMEOUT_MS}
+     *   <li>{@code detachSurfaceTimeoutMs}: {@link #DEFAULT_DETACH_SURFACE_TIMEOUT_MS}
      *   <li>{@code pauseAtEndOfMediaItems}: {@code false}
      *   <li>{@link Clock}: {@link Clock#DEFAULT}
      * </ul>
@@ -243,6 +249,7 @@ public Builder(
       clock = Clock.DEFAULT;
       throwWhenStuckBuffering = true;
       releaseTimeoutMs = ExoPlayer.DEFAULT_RELEASE_TIMEOUT_MS;
+      detachSurfaceTimeoutMs = DEFAULT_DETACH_SURFACE_TIMEOUT_MS;
     }
 
     /**
@@ -476,6 +483,23 @@ public Builder setReleaseTimeoutMs(long releaseTimeoutMs) {
       return this;
     }
 
+    /**
+     * Sets a timeout for detaching a surface from the player.
+     *
+     * <p>If detaching a surface or replacing a surface takes more than {@code
+     * detachSurfaceTimeoutMs} to complete, the player will report an error via {@link
+     * Player.EventListener#onPlayerError}.
+     *
+     * @param detachSurfaceTimeoutMs The timeout for detaching a surface, in milliseconds.
+     * @return This builder.
+     * @throws IllegalStateException If {@link #build()} has already been called.
+     */
+    public Builder setDetachSurfaceTimeoutMs(long detachSurfaceTimeoutMs) {
+      Assertions.checkState(!buildCalled);
+      this.detachSurfaceTimeoutMs = detachSurfaceTimeoutMs;
+      return this;
+    }
+
     /**
      * Sets whether to pause playback at the end of each media item.
      *
@@ -557,6 +581,7 @@ public SimpleExoPlayer build() {
   private final StreamVolumeManager streamVolumeManager;
   private final WakeLockManager wakeLockManager;
   private final WifiLockManager wifiLockManager;
+  private final long detachSurfaceTimeoutMs;
 
   @Nullable private Format videoFormat;
   @Nullable private Format audioFormat;
@@ -617,6 +642,7 @@ protected SimpleExoPlayer(Builder builder) {
     audioAttributes = builder.audioAttributes;
     videoScalingMode = builder.videoScalingMode;
     skipSilenceEnabled = builder.skipSilenceEnabled;
+    detachSurfaceTimeoutMs = builder.detachSurfaceTimeoutMs;
     componentListener = new ComponentListener();
     videoListeners = new CopyOnWriteArraySet<>();
     audioListeners = new CopyOnWriteArraySet<>();
@@ -2019,10 +2045,16 @@ private void setVideoSurfaceInternal(@Nullable Surface surface, boolean ownsSurf
       // We're replacing a surface. Block to ensure that it's not accessed after the method returns.
       try {
         for (PlayerMessage message : messages) {
-          message.blockUntilDelivered();
+          message.blockUntilDelivered(detachSurfaceTimeoutMs);
         }
       } catch (InterruptedException e) {
         Thread.currentThread().interrupt();
+      } catch (TimeoutException e) {
+        player.stop(
+            /* reset= */ false,
+            ExoPlaybackException.createForTimeout(
+                new TimeoutException("Detaching surface timed out."),
+                ExoPlaybackException.TIMEOUT_OPERATION_DETACH_SURFACE));
       }
       // If we created the previous surface, we are responsible for releasing it.
       if (this.ownsSurface) {

library/core/src/test/java/com/google/android/exoplayer2/PlayerMessageTest.java

@@ -17,7 +17,7 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static java.util.concurrent.TimeUnit.SECONDS;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.assertThrows;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.mockito.MockitoAnnotations.initMocks;
@@ -66,31 +66,27 @@ public void tearDown() {
   }
 
   @Test
-  public void experimentalBlockUntilDelivered_timesOut() throws Exception {
+  public void blockUntilDelivered_timesOut() throws Exception {
     when(clock.elapsedRealtime()).thenReturn(0L).thenReturn(TIMEOUT_MS * 2);
 
-    try {
-      message.send().experimentalBlockUntilDelivered(TIMEOUT_MS, clock);
-      fail();
-    } catch (TimeoutException expected) {
-    }
+    assertThrows(
+        TimeoutException.class, () -> message.send().blockUntilDelivered(TIMEOUT_MS, clock));
 
-    // Ensure experimentalBlockUntilDelivered() entered the blocking loop
+    // Ensure blockUntilDelivered() entered the blocking loop.
     verify(clock, Mockito.times(2)).elapsedRealtime();
   }
 
   @Test
-  public void experimentalBlockUntilDelivered_onAlreadyProcessed_succeeds() throws Exception {
+  public void blockUntilDelivered_onAlreadyProcessed_succeeds() throws Exception {
     when(clock.elapsedRealtime()).thenReturn(0L);
 
     message.send().markAsProcessed(/* isDelivered= */ true);
 
-    assertThat(message.experimentalBlockUntilDelivered(TIMEOUT_MS, clock)).isTrue();
+    assertThat(message.blockUntilDelivered(TIMEOUT_MS, clock)).isTrue();
   }
 
   @Test
-  public void experimentalBlockUntilDelivered_markAsProcessedWhileBlocked_succeeds()
-      throws Exception {
+  public void blockUntilDelivered_markAsProcessedWhileBlocked_succeeds() throws Exception {
     message.send();
 
     // Use a separate Thread to mark the message as processed.
@@ -114,8 +110,8 @@ public void experimentalBlockUntilDelivered_markAsProcessedWhileBlocked_succeeds
             });
 
     try {
-      assertThat(message.experimentalBlockUntilDelivered(TIMEOUT_MS, clock)).isTrue();
-      // Ensure experimentalBlockUntilDelivered() entered the blocking loop.
+      assertThat(message.blockUntilDelivered(TIMEOUT_MS, clock)).isTrue();
+      // Ensure blockUntilDelivered() entered the blocking loop.
       verify(clock, Mockito.atLeast(2)).elapsedRealtime();
       future.get(1, SECONDS);
     } finally {