From 11257ecacbb6f9ee4a17f96d05b8536a3bc6a4db Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 15 Apr 2024 02:36:16 -0700 Subject: [PATCH] Forward presumed no-op seeks to handler methods in (Simple)BasePlayer Some seek operations are currently filtered in the base classes if the target index is not explicitly specified and the implicitly assumed operation doesn't have an obvious target index. (Example: calling seekToNext() at the last item in a playlist) This is too opinionated because the actual player implementation using this base class may be able to handle this seek request (e.g. by adding new items on the fly or using other logic to select the most suitable next item). This can be solved by forwarding all seek requests to the respective handler methods even if they are a presumed no-op. Also clarify the Javadoc that the provided index is just an assumption if it wasn't provided explicitly in the method call. PiperOrigin-RevId: 624887116 --- RELEASENOTES.md | 4 + .../java/androidx/media3/cast/CastPlayer.java | 3 + .../androidx/media3/common/BasePlayer.java | 24 +++++- .../media3/common/SimpleBasePlayer.java | 51 ++++++++----- .../media3/common/BasePlayerTest.java | 73 +++++++++++++++++++ .../media3/common/SimpleBasePlayerTest.java | 56 ++++++++++++++ .../media3/exoplayer/ExoPlayerImpl.java | 5 +- 7 files changed, 194 insertions(+), 22 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 90695c3c0e..2e898d948b 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -3,6 +3,10 @@ ### Unreleased changes * Common Library: + * Forward presumed no-op seek calls to the protected `BasePlayer.seekTo` + and `SimpleBasePlayer.handleSeek` methods instead of ignoring them. If + you are implementing these methods in a custom player, you may need to + handle these additional calls with `mediaItemIndex == C.INDEX_UNSET`. * ExoPlayer: * Add `reset` to `BasePreloadManager` to release all the holding sources while keep the preload manager instance. diff --git a/libraries/cast/src/main/java/androidx/media3/cast/CastPlayer.java b/libraries/cast/src/main/java/androidx/media3/cast/CastPlayer.java index 884169151e..f18fde331c 100644 --- a/libraries/cast/src/main/java/androidx/media3/cast/CastPlayer.java +++ b/libraries/cast/src/main/java/androidx/media3/cast/CastPlayer.java @@ -417,6 +417,9 @@ public final class CastPlayer extends BasePlayer { long positionMs, @Player.Command int seekCommand, boolean isRepeatingCurrentItem) { + if (mediaItemIndex == C.INDEX_UNSET) { + return; + } checkArgument(mediaItemIndex >= 0); if (!currentTimeline.isEmpty() && mediaItemIndex >= currentTimeline.getWindowCount()) { return; diff --git a/libraries/common/src/main/java/androidx/media3/common/BasePlayer.java b/libraries/common/src/main/java/androidx/media3/common/BasePlayer.java index 7910d474a8..31036bf9fb 100644 --- a/libraries/common/src/main/java/androidx/media3/common/BasePlayer.java +++ b/libraries/common/src/main/java/androidx/media3/common/BasePlayer.java @@ -197,12 +197,15 @@ public abstract class BasePlayer implements Player { public final void seekToPrevious() { Timeline timeline = getCurrentTimeline(); if (timeline.isEmpty() || isPlayingAd()) { + ignoreSeek(Player.COMMAND_SEEK_TO_PREVIOUS); return; } boolean hasPreviousMediaItem = hasPreviousMediaItem(); if (isCurrentMediaItemLive() && !isCurrentMediaItemSeekable()) { if (hasPreviousMediaItem) { seekToPreviousMediaItemInternal(Player.COMMAND_SEEK_TO_PREVIOUS); + } else { + ignoreSeek(Player.COMMAND_SEEK_TO_PREVIOUS); } } else if (hasPreviousMediaItem && getCurrentPosition() <= getMaxSeekToPreviousPosition()) { seekToPreviousMediaItemInternal(Player.COMMAND_SEEK_TO_PREVIOUS); @@ -261,12 +264,15 @@ public abstract class BasePlayer implements Player { public final void seekToNext() { Timeline timeline = getCurrentTimeline(); if (timeline.isEmpty() || isPlayingAd()) { + ignoreSeek(Player.COMMAND_SEEK_TO_NEXT); return; } if (hasNextMediaItem()) { seekToNextMediaItemInternal(Player.COMMAND_SEEK_TO_NEXT); } else if (isCurrentMediaItemLive() && isCurrentMediaItemDynamic()) { seekToDefaultPositionInternal(getCurrentMediaItemIndex(), Player.COMMAND_SEEK_TO_NEXT); + } else { + ignoreSeek(Player.COMMAND_SEEK_TO_NEXT); } } @@ -287,9 +293,13 @@ public abstract class BasePlayer implements Player { /** * Seeks to a position in the specified {@link MediaItem}. * - * @param mediaItemIndex The index of the {@link MediaItem}. + * @param mediaItemIndex The index of the {@link MediaItem}. If the original seek operation did + * not directly specify an index, this is the most likely implied index based on the available + * player state. If the implied action is to do nothing, this will be {@link C#INDEX_UNSET}. * @param positionMs The seek position in the specified {@link MediaItem} in milliseconds, or - * {@link C#TIME_UNSET} to seek to the media item's default position. + * {@link C#TIME_UNSET} to seek to the media item's default position. If the original seek + * operation did not directly specify a position, this is the most likely implied position + * based on the available player state. * @param seekCommand The {@link Player.Command} used to trigger the seek. * @param isRepeatingCurrentItem Whether this seeks repeats the current item. */ @@ -459,6 +469,14 @@ public abstract class BasePlayer implements Player { return repeatMode == REPEAT_MODE_ONE ? REPEAT_MODE_OFF : repeatMode; } + private void ignoreSeek(@Player.Command int seekCommand) { + seekTo( + /* mediaItemIndex= */ C.INDEX_UNSET, + /* positionMs= */ C.TIME_UNSET, + seekCommand, + /* isRepeatingCurrentItem= */ false); + } + private void seekToCurrentItem(long positionMs, @Player.Command int seekCommand) { seekTo( getCurrentMediaItemIndex(), positionMs, seekCommand, /* isRepeatingCurrentItem= */ false); @@ -485,6 +503,7 @@ public abstract class BasePlayer implements Player { private void seekToNextMediaItemInternal(@Player.Command int seekCommand) { int nextMediaItemIndex = getNextMediaItemIndex(); if (nextMediaItemIndex == C.INDEX_UNSET) { + ignoreSeek(seekCommand); return; } if (nextMediaItemIndex == getCurrentMediaItemIndex()) { @@ -497,6 +516,7 @@ public abstract class BasePlayer implements Player { private void seekToPreviousMediaItemInternal(@Player.Command int seekCommand) { int previousMediaItemIndex = getPreviousMediaItemIndex(); if (previousMediaItemIndex == C.INDEX_UNSET) { + ignoreSeek(seekCommand); return; } if (previousMediaItemIndex == getCurrentMediaItemIndex()) { diff --git a/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java b/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java index a46ff5ddba..0c6b6e12b3 100644 --- a/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java +++ b/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java @@ -2352,19 +2352,24 @@ public abstract class SimpleBasePlayer extends BasePlayer { @Player.Command int seekCommand, boolean isRepeatingCurrentItem) { verifyApplicationThreadAndInitState(); - checkArgument(mediaItemIndex >= 0); + checkArgument(mediaItemIndex == C.INDEX_UNSET || mediaItemIndex >= 0); // Use a local copy to ensure the lambda below uses the current state value. State state = this.state; - if (!shouldHandleCommand(seekCommand) - || isPlayingAd() - || (!state.playlist.isEmpty() && mediaItemIndex >= state.playlist.size())) { + if (!shouldHandleCommand(seekCommand)) { return; } + boolean ignoreSeekForPlaceholderState = + mediaItemIndex == C.INDEX_UNSET + || isPlayingAd() + || (!state.playlist.isEmpty() && mediaItemIndex >= state.playlist.size()); updateStateForPendingOperation( /* pendingOperation= */ handleSeek(mediaItemIndex, positionMs, seekCommand), /* placeholderStateSupplier= */ () -> - getStateWithNewPlaylistAndPosition(state, state.playlist, mediaItemIndex, positionMs), - /* seeked= */ true, + ignoreSeekForPlaceholderState + ? state + : getStateWithNewPlaylistAndPosition( + state, state.playlist, mediaItemIndex, positionMs), + /* forceSeekDiscontinuity= */ !ignoreSeekForPlaceholderState, isRepeatingCurrentItem); } @@ -2921,7 +2926,7 @@ public abstract class SimpleBasePlayer extends BasePlayer { return; } updateStateAndInformListeners( - getState(), /* seeked= */ false, /* isRepeatingCurrentItem= */ false); + getState(), /* forceSeekDiscontinuity= */ false, /* isRepeatingCurrentItem= */ false); } /** @@ -3344,10 +3349,13 @@ public abstract class SimpleBasePlayer extends BasePlayer { *

Will only be called if the appropriate {@link Player.Command}, for example {@link * Player#COMMAND_SEEK_TO_MEDIA_ITEM} or {@link Player#COMMAND_SEEK_TO_NEXT}, is available. * - * @param mediaItemIndex The media item index to seek to. The index is in the range 0 <= {@code - * mediaItemIndex} < {@code mediaItems.size()}. + * @param mediaItemIndex The media item index to seek to. If the original seek operation did not + * directly specify an index, this is the most likely implied index based on the available + * player state. If the implied action is to do nothing, this will be {@link C#INDEX_UNSET}. * @param positionMs The position in milliseconds to start playback from, or {@link C#TIME_UNSET} - * to start at the default position in the media item. + * to start at the default position in the media item. If the original seek operation did not + * directly specify a position, this is the most likely implied position based on the + * available player state. * @param seekCommand The {@link Player.Command} used to trigger the seek. * @return A {@link ListenableFuture} indicating the completion of all immediate {@link State} * changes caused by this call. @@ -3385,7 +3393,7 @@ public abstract class SimpleBasePlayer extends BasePlayer { @SuppressWarnings("deprecation") // Calling deprecated listener methods. @RequiresNonNull("state") private void updateStateAndInformListeners( - State newState, boolean seeked, boolean isRepeatingCurrentItem) { + State newState, boolean forceSeekDiscontinuity, boolean isRepeatingCurrentItem) { State previousState = state; // Assign new state immediately such that all getters return the right values, but use a // snapshot of the previous and new state so that listener invocations are triggered correctly. @@ -3407,7 +3415,8 @@ public abstract class SimpleBasePlayer extends BasePlayer { MediaMetadata previousMediaMetadata = getMediaMetadataInternal(previousState); MediaMetadata newMediaMetadata = getMediaMetadataInternal(newState); int positionDiscontinuityReason = - getPositionDiscontinuityReason(previousState, newState, seeked, window, period); + getPositionDiscontinuityReason( + previousState, newState, forceSeekDiscontinuity, window, period); boolean timelineChanged = !previousState.timeline.equals(newState.timeline); int mediaItemTransitionReason = getMediaItemTransitionReason( @@ -3618,7 +3627,7 @@ public abstract class SimpleBasePlayer extends BasePlayer { updateStateForPendingOperation( pendingOperation, placeholderStateSupplier, - /* seeked= */ false, + /* forceSeekDiscontinuity= */ false, /* isRepeatingCurrentItem= */ false); } @@ -3626,22 +3635,26 @@ public abstract class SimpleBasePlayer extends BasePlayer { private void updateStateForPendingOperation( ListenableFuture pendingOperation, Supplier placeholderStateSupplier, - boolean seeked, + boolean forceSeekDiscontinuity, boolean isRepeatingCurrentItem) { if (pendingOperation.isDone() && pendingOperations.isEmpty()) { - updateStateAndInformListeners(getState(), seeked, isRepeatingCurrentItem); + updateStateAndInformListeners(getState(), forceSeekDiscontinuity, isRepeatingCurrentItem); } else { pendingOperations.add(pendingOperation); State suggestedPlaceholderState = placeholderStateSupplier.get(); updateStateAndInformListeners( - getPlaceholderState(suggestedPlaceholderState), seeked, isRepeatingCurrentItem); + getPlaceholderState(suggestedPlaceholderState), + forceSeekDiscontinuity, + isRepeatingCurrentItem); pendingOperation.addListener( () -> { castNonNull(state); // Already checked by method @RequiresNonNull pre-condition. pendingOperations.remove(pendingOperation); if (pendingOperations.isEmpty() && !released) { updateStateAndInformListeners( - getState(), /* seeked= */ false, /* isRepeatingCurrentItem= */ false); + getState(), + /* forceSeekDiscontinuity= */ false, + /* isRepeatingCurrentItem= */ false); } }, this::postOrRunOnApplicationHandler); @@ -3740,14 +3753,14 @@ public abstract class SimpleBasePlayer extends BasePlayer { private static int getPositionDiscontinuityReason( State previousState, State newState, - boolean seeked, + boolean forceSeekDiscontinuity, Timeline.Window window, Timeline.Period period) { if (newState.hasPositionDiscontinuity) { // We were asked to report a discontinuity. return newState.positionDiscontinuityReason; } - if (seeked) { + if (forceSeekDiscontinuity) { return Player.DISCONTINUITY_REASON_SEEK; } if (previousState.playlist.isEmpty()) { diff --git a/libraries/common/src/test/java/androidx/media3/common/BasePlayerTest.java b/libraries/common/src/test/java/androidx/media3/common/BasePlayerTest.java index 4f3c677f66..a47f52c4fd 100644 --- a/libraries/common/src/test/java/androidx/media3/common/BasePlayerTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/BasePlayerTest.java @@ -119,6 +119,32 @@ public class BasePlayerTest { /* isRepeatingCurrentItem= */ false); } + @Test + public void seekToNext_withoutNextItem_forwardsCallWithUnsetMediaItemIndex() { + BasePlayer player = + spy( + new TestBasePlayer() { + @Override + public Timeline getCurrentTimeline() { + return new FakeTimeline(/* windowCount= */ 3); + } + + @Override + public int getCurrentMediaItemIndex() { + return 2; + } + }); + + player.seekToNext(); + + verify(player) + .seekTo( + /* mediaItemIndex= */ C.INDEX_UNSET, + /* positionMs= */ C.TIME_UNSET, + Player.COMMAND_SEEK_TO_NEXT, + /* isRepeatingCurrentItem= */ false); + } + @Test public void seekToNextMediaItem_usesCommandSeekToNextMediaItem() { BasePlayer player = @@ -140,6 +166,32 @@ public class BasePlayerTest { /* isRepeatingCurrentItem= */ false); } + @Test + public void seekToNextMediaItem_withoutNextItem_forwardsCallWithUnsetMediaItemIndex() { + BasePlayer player = + spy( + new TestBasePlayer() { + @Override + public Timeline getCurrentTimeline() { + return new FakeTimeline(/* windowCount= */ 3); + } + + @Override + public int getCurrentMediaItemIndex() { + return 2; + } + }); + + player.seekToNextMediaItem(); + + verify(player) + .seekTo( + /* mediaItemIndex= */ C.INDEX_UNSET, + /* positionMs= */ C.TIME_UNSET, + Player.COMMAND_SEEK_TO_NEXT_MEDIA_ITEM, + /* isRepeatingCurrentItem= */ false); + } + @Test public void seekForward_usesCommandSeekForward() { BasePlayer player = @@ -223,6 +275,27 @@ public class BasePlayerTest { /* isRepeatingCurrentItem= */ false); } + @Test + public void seekToPreviousMediaItem_withoutPreviousItem_forwardsCallWithUnsetMediaItemIndex() { + BasePlayer player = + spy( + new TestBasePlayer() { + @Override + public int getCurrentMediaItemIndex() { + return 0; + } + }); + + player.seekToPreviousMediaItem(); + + verify(player) + .seekTo( + /* mediaItemIndex= */ C.INDEX_UNSET, + /* positionMs= */ C.TIME_UNSET, + Player.COMMAND_SEEK_TO_PREVIOUS_MEDIA_ITEM, + /* isRepeatingCurrentItem= */ false); + } + @Test public void seekBack_usesCommandSeekBack() { BasePlayer player = diff --git a/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java b/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java index 37f91ea27e..26d33ff606 100644 --- a/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java @@ -8013,6 +8013,62 @@ public class SimpleBasePlayerTest { verifyNoMoreInteractions(listener); } + @SuppressWarnings("deprecation") // Verifying deprecated listener calls. + @Test + public void seekTo_asyncHandlingForNoImpliedActionSeeks_usesCurrentStateAsPlaceholderState() { + MediaItem mediaItem = new MediaItem.Builder().setMediaId("2").build(); + State state = + new State.Builder() + .setAvailableCommands(new Commands.Builder().addAllCommands().build()) + .setPlaylist( + ImmutableList.of( + new SimpleBasePlayer.MediaItemData.Builder(/* uid= */ 1) + .setMediaItem(mediaItem) + .build(), + new SimpleBasePlayer.MediaItemData.Builder(/* uid= */ 2).build())) + .setCurrentMediaItemIndex(1) + .build(); + // Change updated state to see a difference to the placeholder state. + State updatedState = + state + .buildUpon() + .setCurrentMediaItemIndex(0) + .setPositionDiscontinuity( + Player.DISCONTINUITY_REASON_SEEK, /* discontinuityPositionMs= */ 0) + .build(); + SettableFuture future = SettableFuture.create(); + SimpleBasePlayer player = + new SimpleBasePlayer(Looper.myLooper()) { + @Override + protected State getState() { + return future.isDone() ? updatedState : state; + } + + @Override + protected ListenableFuture handleSeek( + int mediaItemIndex, long positionMs, @Player.Command int seekCommand) { + return future; + } + }; + Listener listener = mock(Listener.class); + player.addListener(listener); + + player.seekToNext(); + + // Verify placeholder state is the same as before with no listener updates. + assertThat(player.getCurrentMediaItemIndex()).isEqualTo(1); + verifyNoMoreInteractions(listener); + + future.set(null); + + // Verify actual state update. + assertThat(player.getCurrentMediaItemIndex()).isEqualTo(0); + verify(listener).onPositionDiscontinuity(Player.DISCONTINUITY_REASON_SEEK); + verify(listener).onPositionDiscontinuity(any(), any(), eq(Player.DISCONTINUITY_REASON_SEEK)); + verify(listener).onMediaItemTransition(mediaItem, Player.MEDIA_ITEM_TRANSITION_REASON_SEEK); + verifyNoMoreInteractions(listener); + } + @Test public void seekTo_withoutAvailableCommandForSeekToMediaItem_isNotForwarded() { State state = diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java index 9d4f6f5c87..8a2b6466d9 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java @@ -890,12 +890,15 @@ import java.util.concurrent.TimeoutException; @Player.Command int seekCommand, boolean isRepeatingCurrentItem) { verifyApplicationThread(); + if (mediaItemIndex == C.INDEX_UNSET) { + return; + } checkArgument(mediaItemIndex >= 0); - analyticsCollector.notifySeekStarted(); Timeline timeline = playbackInfo.timeline; if (!timeline.isEmpty() && mediaItemIndex >= timeline.getWindowCount()) { return; } + analyticsCollector.notifySeekStarted(); pendingOperationAcks++; if (isPlayingAd()) { // TODO: Investigate adding support for seeking during ads. This is complicated to do in