From d56d94fa0af5e2132030454b22c013228e0a621c Mon Sep 17 00:00:00 2001 From: tonihei Date: Tue, 18 Oct 2022 17:08:09 +0000 Subject: [PATCH] Ensure onMediaItemTransition is sent for repeats of the same item Currently, repeating the same item (via seekNext/Previous) implicitly results in a seek to the default position of the current item, which looks exactly the same as a direct seek. As a result, we don't send onMediaItemTransition as we would for every other seekNext/Previous call. This can be fixed by explicitly marking the repeat case in the internal BasePlayer/ExoPlayerImpl methods, so that the callback can be triggered. Issue: google/ExoPlayer#10667 PiperOrigin-RevId: 481951788 (cherry picked from commit f850206c51ced023b1603aa7661dd556ee436740) --- RELEASENOTES.md | 3 + .../androidx/media3/common/BasePlayer.java | 26 ++- .../media3/exoplayer/ExoPlayerImpl.java | 149 +++++++++++------- .../media3/exoplayer/ExoPlayerTest.java | 33 ++++ 4 files changed, 152 insertions(+), 59 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index dc4cf13519..757f51d57d 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -23,6 +23,9 @@ ([#8944](https://github.com/google/ExoPlayer/issues/8944)). * Fix session tracking problem with fast seeks in `PlaybackStatsListener` ([#180](https://github.com/androidx/media/issues/180)). + * Send missing `onMediaItemTransition` callback when calling `seekToNext` + or `seekToPrevious` in a single-item playlist + ([#10667](https://github.com/google/ExoPlayer/issues/10667)). * Downloads: * Fix potential infinite loop in `ProgressiveDownloader` caused by simultaneous download and playback with the same `PriorityTaskManager` 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 9069f061bb..74b144baa3 100644 --- a/libraries/common/src/main/java/androidx/media3/common/BasePlayer.java +++ b/libraries/common/src/main/java/androidx/media3/common/BasePlayer.java @@ -22,6 +22,7 @@ import androidx.media3.common.util.UnstableApi; import androidx.media3.common.util.Util; import com.google.common.collect.ImmutableList; +import com.google.errorprone.annotations.ForOverride; import java.util.List; /** Abstract base {@link Player} which implements common implementation independent methods. */ @@ -187,7 +188,12 @@ public final void seekToPreviousWindow() { @Override public final void seekToPreviousMediaItem() { int previousMediaItemIndex = getPreviousMediaItemIndex(); - if (previousMediaItemIndex != C.INDEX_UNSET) { + if (previousMediaItemIndex == C.INDEX_UNSET) { + return; + } + if (previousMediaItemIndex == getCurrentMediaItemIndex()) { + repeatCurrentMediaItem(); + } else { seekToDefaultPosition(previousMediaItemIndex); } } @@ -254,7 +260,12 @@ public final void seekToNextWindow() { @Override public final void seekToNextMediaItem() { int nextMediaItemIndex = getNextMediaItemIndex(); - if (nextMediaItemIndex != C.INDEX_UNSET) { + if (nextMediaItemIndex == C.INDEX_UNSET) { + return; + } + if (nextMediaItemIndex == getCurrentMediaItemIndex()) { + repeatCurrentMediaItem(); + } else { seekToDefaultPosition(nextMediaItemIndex); } } @@ -426,6 +437,17 @@ public final long getContentDuration() { : timeline.getWindow(getCurrentMediaItemIndex(), window).getDurationMs(); } + /** + * Repeat the current media item. + * + *

The default implementation seeks to the default position in the current item, which can be + * overridden for additional handling. + */ + @ForOverride + protected void repeatCurrentMediaItem() { + seekToDefaultPosition(); + } + private @RepeatMode int getRepeatModeForNavigation() { @RepeatMode int repeatMode = getRepeatMode(); return repeatMode == REPEAT_MODE_ONE ? REPEAT_MODE_OFF : repeatMode; 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 bf78533492..64f1236bda 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java @@ -542,7 +542,8 @@ public void prepare() { /* positionDiscontinuity= */ false, /* ignored */ DISCONTINUITY_REASON_INTERNAL, /* ignored */ C.TIME_UNSET, - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } @Override @@ -663,7 +664,8 @@ public void addMediaSources(int index, List mediaSources) { /* positionDiscontinuity= */ false, /* ignored */ DISCONTINUITY_REASON_INTERNAL, /* ignored */ C.TIME_UNSET, - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } @Override @@ -681,7 +683,8 @@ public void removeMediaItems(int fromIndex, int toIndex) { positionDiscontinuity, DISCONTINUITY_REASON_REMOVE, /* discontinuityWindowStartPositionUs= */ getCurrentPositionUsInternal(newPlaybackInfo), - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } @Override @@ -711,7 +714,8 @@ public void moveMediaItems(int fromIndex, int toIndex, int newFromIndex) { /* positionDiscontinuity= */ false, /* ignored */ DISCONTINUITY_REASON_INTERNAL, /* ignored */ C.TIME_UNSET, - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } @Override @@ -735,7 +739,8 @@ public void setShuffleOrder(ShuffleOrder shuffleOrder) { /* positionDiscontinuity= */ false, /* ignored */ DISCONTINUITY_REASON_INTERNAL, /* ignored */ C.TIME_UNSET, - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } @Override @@ -814,47 +819,17 @@ public boolean isLoading() { return playbackInfo.isLoading; } + @Override + protected void repeatCurrentMediaItem() { + verifyApplicationThread(); + seekToInternal( + getCurrentMediaItemIndex(), /* positionMs= */ C.TIME_UNSET, /* repeatMediaItem= */ true); + } + @Override public void seekTo(int mediaItemIndex, long positionMs) { verifyApplicationThread(); - analyticsCollector.notifySeekStarted(); - Timeline timeline = playbackInfo.timeline; - if (mediaItemIndex < 0 - || (!timeline.isEmpty() && mediaItemIndex >= timeline.getWindowCount())) { - throw new IllegalSeekPositionException(timeline, mediaItemIndex, positionMs); - } - pendingOperationAcks++; - if (isPlayingAd()) { - // TODO: Investigate adding support for seeking during ads. This is complicated to do in - // general because the midroll ad preceding the seek destination must be played before the - // content position can be played, if a different ad is playing at the moment. - Log.w(TAG, "seekTo ignored because an ad is playing"); - ExoPlayerImplInternal.PlaybackInfoUpdate playbackInfoUpdate = - new ExoPlayerImplInternal.PlaybackInfoUpdate(this.playbackInfo); - playbackInfoUpdate.incrementPendingOperationAcks(1); - playbackInfoUpdateListener.onPlaybackInfoUpdate(playbackInfoUpdate); - return; - } - @Player.State - int newPlaybackState = - getPlaybackState() == Player.STATE_IDLE ? Player.STATE_IDLE : STATE_BUFFERING; - int oldMaskingMediaItemIndex = getCurrentMediaItemIndex(); - PlaybackInfo newPlaybackInfo = playbackInfo.copyWithPlaybackState(newPlaybackState); - newPlaybackInfo = - maskTimelineAndPosition( - newPlaybackInfo, - timeline, - maskWindowPositionMsOrGetPeriodPositionUs(timeline, mediaItemIndex, positionMs)); - internalPlayer.seekTo(timeline, mediaItemIndex, Util.msToUs(positionMs)); - updatePlaybackInfo( - newPlaybackInfo, - /* ignored */ TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED, - /* ignored */ PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, - /* seekProcessed= */ true, - /* positionDiscontinuity= */ true, - /* positionDiscontinuityReason= */ DISCONTINUITY_REASON_SEEK, - /* discontinuityWindowStartPositionUs= */ getCurrentPositionUsInternal(newPlaybackInfo), - oldMaskingMediaItemIndex); + seekToInternal(mediaItemIndex, positionMs, /* repeatMediaItem= */ false); } @Override @@ -895,7 +870,8 @@ public void setPlaybackParameters(PlaybackParameters playbackParameters) { /* positionDiscontinuity= */ false, /* ignored */ DISCONTINUITY_REASON_INTERNAL, /* ignored */ C.TIME_UNSET, - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } @Override @@ -1744,7 +1720,8 @@ private void stopInternal(boolean reset, @Nullable ExoPlaybackException error) { positionDiscontinuity, DISCONTINUITY_REASON_REMOVE, /* discontinuityWindowStartPositionUs= */ getCurrentPositionUsInternal(playbackInfo), - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } private int getCurrentWindowIndexInternal() { @@ -1826,7 +1803,8 @@ private void handlePlaybackInfo(ExoPlayerImplInternal.PlaybackInfoUpdate playbac positionDiscontinuity, pendingDiscontinuityReason, discontinuityWindowStartPositionUs, - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } } @@ -1840,7 +1818,8 @@ private void updatePlaybackInfo( boolean positionDiscontinuity, @DiscontinuityReason int positionDiscontinuityReason, long discontinuityWindowStartPositionUs, - int oldMaskingMediaItemIndex) { + int oldMaskingMediaItemIndex, + boolean repeatCurrentMediaItem) { // Assign playback info immediately such that all getters return the right values, but keep // snapshot of previous and new state so that listener invocations are triggered correctly. @@ -1848,13 +1827,15 @@ private void updatePlaybackInfo( PlaybackInfo newPlaybackInfo = playbackInfo; this.playbackInfo = playbackInfo; + boolean timelineChanged = !previousPlaybackInfo.timeline.equals(newPlaybackInfo.timeline); Pair mediaItemTransitionInfo = evaluateMediaItemTransitionReason( newPlaybackInfo, previousPlaybackInfo, positionDiscontinuity, positionDiscontinuityReason, - !previousPlaybackInfo.timeline.equals(newPlaybackInfo.timeline)); + timelineChanged, + repeatCurrentMediaItem); boolean mediaItemTransitioned = mediaItemTransitionInfo.first; int mediaItemTransitionReason = mediaItemTransitionInfo.second; MediaMetadata newMediaMetadata = mediaMetadata; @@ -1891,7 +1872,7 @@ private void updatePlaybackInfo( updatePriorityTaskManagerForIsLoadingChange(newPlaybackInfo.isLoading); } - if (!previousPlaybackInfo.timeline.equals(newPlaybackInfo.timeline)) { + if (timelineChanged) { listeners.queueEvent( Player.EVENT_TIMELINE_CHANGED, listener -> listener.onTimelineChanged(newPlaybackInfo.timeline, timelineChangeReason)); @@ -2094,7 +2075,8 @@ private Pair evaluateMediaItemTransitionReason( PlaybackInfo oldPlaybackInfo, boolean positionDiscontinuity, @DiscontinuityReason int positionDiscontinuityReason, - boolean timelineChanged) { + boolean timelineChanged, + boolean repeatCurrentMediaItem) { Timeline oldTimeline = oldPlaybackInfo.timeline; Timeline newTimeline = playbackInfo.timeline; @@ -2125,11 +2107,20 @@ private Pair evaluateMediaItemTransitionReason( throw new IllegalStateException(); } return new Pair<>(/* isTransitioning */ true, transitionReason); - } else if (positionDiscontinuity - && positionDiscontinuityReason == DISCONTINUITY_REASON_AUTO_TRANSITION - && oldPlaybackInfo.periodId.windowSequenceNumber - < playbackInfo.periodId.windowSequenceNumber) { - return new Pair<>(/* isTransitioning */ true, MEDIA_ITEM_TRANSITION_REASON_REPEAT); + } else { + // Only mark changes within the current item as a transition if we are repeating automatically + // or via a seek to next/previous. + if (positionDiscontinuity + && positionDiscontinuityReason == DISCONTINUITY_REASON_AUTO_TRANSITION + && oldPlaybackInfo.periodId.windowSequenceNumber + < playbackInfo.periodId.windowSequenceNumber) { + return new Pair<>(/* isTransitioning */ true, MEDIA_ITEM_TRANSITION_REASON_REPEAT); + } + if (positionDiscontinuity + && positionDiscontinuityReason == DISCONTINUITY_REASON_SEEK + && repeatCurrentMediaItem) { + return new Pair<>(/* isTransitioning */ true, MEDIA_ITEM_TRANSITION_REASON_SEEK); + } } return new Pair<>(/* isTransitioning */ false, /* mediaItemTransitionReason */ C.INDEX_UNSET); } @@ -2200,7 +2191,8 @@ private void setMediaSourcesInternal( /* positionDiscontinuity= */ positionDiscontinuity, DISCONTINUITY_REASON_REMOVE, /* discontinuityWindowStartPositionUs= */ getCurrentPositionUsInternal(newPlaybackInfo), - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } private List addMediaSourceHolders( @@ -2592,7 +2584,8 @@ private void updatePlayWhenReady( /* positionDiscontinuity= */ false, /* ignored */ DISCONTINUITY_REASON_INTERNAL, /* ignored */ C.TIME_UNSET, - /* ignored */ C.INDEX_UNSET); + /* ignored */ C.INDEX_UNSET, + /* repeatCurrentMediaItem= */ false); } private void updateWakeAndWifiLock() { @@ -2690,6 +2683,48 @@ private void updatePriorityTaskManagerForIsLoadingChange(boolean isLoading) { } } + private void seekToInternal(int mediaItemIndex, long positionMs, boolean repeatMediaItem) { + analyticsCollector.notifySeekStarted(); + Timeline timeline = playbackInfo.timeline; + if (mediaItemIndex < 0 + || (!timeline.isEmpty() && mediaItemIndex >= timeline.getWindowCount())) { + throw new IllegalSeekPositionException(timeline, mediaItemIndex, positionMs); + } + pendingOperationAcks++; + if (isPlayingAd()) { + // TODO: Investigate adding support for seeking during ads. This is complicated to do in + // general because the midroll ad preceding the seek destination must be played before the + // content position can be played, if a different ad is playing at the moment. + Log.w(TAG, "seekTo ignored because an ad is playing"); + ExoPlayerImplInternal.PlaybackInfoUpdate playbackInfoUpdate = + new ExoPlayerImplInternal.PlaybackInfoUpdate(this.playbackInfo); + playbackInfoUpdate.incrementPendingOperationAcks(1); + playbackInfoUpdateListener.onPlaybackInfoUpdate(playbackInfoUpdate); + return; + } + @Player.State + int newPlaybackState = + getPlaybackState() == Player.STATE_IDLE ? Player.STATE_IDLE : STATE_BUFFERING; + int oldMaskingMediaItemIndex = getCurrentMediaItemIndex(); + PlaybackInfo newPlaybackInfo = playbackInfo.copyWithPlaybackState(newPlaybackState); + newPlaybackInfo = + maskTimelineAndPosition( + newPlaybackInfo, + timeline, + maskWindowPositionMsOrGetPeriodPositionUs(timeline, mediaItemIndex, positionMs)); + internalPlayer.seekTo(timeline, mediaItemIndex, Util.msToUs(positionMs)); + updatePlaybackInfo( + newPlaybackInfo, + /* ignored */ TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED, + /* ignored */ PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, + /* seekProcessed= */ true, + /* positionDiscontinuity= */ true, + /* positionDiscontinuityReason= */ DISCONTINUITY_REASON_SEEK, + /* discontinuityWindowStartPositionUs= */ getCurrentPositionUsInternal(newPlaybackInfo), + oldMaskingMediaItemIndex, + repeatMediaItem); + } + private static DeviceInfo createDeviceInfo(StreamVolumeManager streamVolumeManager) { return new DeviceInfo( DeviceInfo.PLAYBACK_TYPE_LOCAL, diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java index e4e69c55a8..31a0726ef7 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java @@ -8821,6 +8821,39 @@ public void onMediaItemTransition(@Nullable MediaItem mediaItem, int reason) { player.release(); } + @Test + public void seekToNextPrevious_singleItemRepeat_notifiesMediaItemTransition() throws Exception { + List reportedMediaItems = new ArrayList<>(); + List reportedTransitionReasons = new ArrayList<>(); + MediaSource mediaSource = FakeMediaSource.createWithWindowId(/* windowId= */ new Object()); + ExoPlayer player = new TestExoPlayerBuilder(context).build(); + player.addListener( + new Listener() { + @Override + public void onMediaItemTransition(@Nullable MediaItem mediaItem, int reason) { + reportedMediaItems.add(mediaItem); + reportedTransitionReasons.add(reason); + } + }); + player.setMediaSource(mediaSource); + player.prepare(); + runUntilPlaybackState(player, Player.STATE_READY); + + player.setRepeatMode(Player.REPEAT_MODE_ALL); + player.seekToNextMediaItem(); + player.seekToPreviousMediaItem(); + player.release(); + + MediaItem expectedMediaItem = mediaSource.getMediaItem(); + assertThat(reportedMediaItems) + .containsExactly(expectedMediaItem, expectedMediaItem, expectedMediaItem); + assertThat(reportedTransitionReasons) + .containsExactly( + Player.MEDIA_ITEM_TRANSITION_REASON_PLAYLIST_CHANGED, + Player.MEDIA_ITEM_TRANSITION_REASON_SEEK, + Player.MEDIA_ITEM_TRANSITION_REASON_SEEK); + } + @Test public void repeat_notifiesMediaItemTransition() throws Exception { List reportedMediaItems = new ArrayList<>();