Skip to content

Commit

Permalink
Ensure onMediaItemTransition is sent for repeats of the same item
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@tonihei @rohitjoins
tonihei authored and rohitjoins committed Oct 24, 2022
1 parent 46d5a0e commit f850206
Show file tree
Hide file tree
Showing 4 changed files with 152 additions and 59 deletions.
3 changes: 3 additions & 0 deletions RELEASENOTES.md
View file
Expand Up @@ -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`
Expand Down
26 changes: 24 additions & 2 deletions libraries/common/src/main/java/androidx/media3/common/BasePlayer.java
View file
Expand Up @@ -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. */
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -426,6 +437,17 @@ public final long getContentDuration() {
: timeline.getWindow(getCurrentMediaItemIndex(), window).getDurationMs();
}

/**
* Repeat the current media item.
*
* <p>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;
Expand Down
149 changes: 92 additions & 57 deletions libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java
View file
Expand Up @@ -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
Expand Down Expand Up @@ -663,7 +664,8 @@ public void addMediaSources(int index, List<MediaSource> mediaSources) {
/* positionDiscontinuity= */ false,
/* ignored */ DISCONTINUITY_REASON_INTERNAL,
/* ignored */ C.TIME_UNSET,
/* ignored */ C.INDEX_UNSET);
/* ignored */ C.INDEX_UNSET,
/* repeatCurrentMediaItem= */ false);
}

@Override
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -1826,7 +1803,8 @@ private void handlePlaybackInfo(ExoPlayerImplInternal.PlaybackInfoUpdate playbac
positionDiscontinuity,
pendingDiscontinuityReason,
discontinuityWindowStartPositionUs,
/* ignored */ C.INDEX_UNSET);
/* ignored */ C.INDEX_UNSET,
/* repeatCurrentMediaItem= */ false);
}
}

Expand All @@ -1840,21 +1818,24 @@ 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.
PlaybackInfo previousPlaybackInfo = this.playbackInfo;
PlaybackInfo newPlaybackInfo = playbackInfo;
this.playbackInfo = playbackInfo;

boolean timelineChanged = !previousPlaybackInfo.timeline.equals(newPlaybackInfo.timeline);
Pair<Boolean, Integer> mediaItemTransitionInfo =
evaluateMediaItemTransitionReason(
newPlaybackInfo,
previousPlaybackInfo,
positionDiscontinuity,
positionDiscontinuityReason,
!previousPlaybackInfo.timeline.equals(newPlaybackInfo.timeline));
timelineChanged,
repeatCurrentMediaItem);
boolean mediaItemTransitioned = mediaItemTransitionInfo.first;
int mediaItemTransitionReason = mediaItemTransitionInfo.second;
MediaMetadata newMediaMetadata = mediaMetadata;
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -2094,7 +2075,8 @@ private Pair<Boolean, Integer> evaluateMediaItemTransitionReason(
PlaybackInfo oldPlaybackInfo,
boolean positionDiscontinuity,
@DiscontinuityReason int positionDiscontinuityReason,
boolean timelineChanged) {
boolean timelineChanged,
boolean repeatCurrentMediaItem) {

Timeline oldTimeline = oldPlaybackInfo.timeline;
Timeline newTimeline = playbackInfo.timeline;
Expand Down Expand Up @@ -2125,11 +2107,20 @@ private Pair<Boolean, Integer> 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);
}
Expand Down Expand Up @@ -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<MediaSourceList.MediaSourceHolder> addMediaSourceHolders(
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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,
Expand Down
33 changes: 33 additions & 0 deletions libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java
View file
Expand Up @@ -8821,6 +8821,39 @@ public void onMediaItemTransition(@Nullable MediaItem mediaItem, int reason) {
player.release();
}

@Test
public void seekToNextPrevious_singleItemRepeat_notifiesMediaItemTransition() throws Exception {
List<MediaItem> reportedMediaItems = new ArrayList<>();
List<Integer> 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<MediaItem> reportedMediaItems = new ArrayList<>();
Expand Down

0 comments on commit f850206

Please sign in to comment.