From 1bdc58de0be73c14e1b5fdffab64d69da470ffb3 Mon Sep 17 00:00:00 2001 From: tianyifeng Date: Thu, 29 Feb 2024 04:01:01 -0800 Subject: [PATCH] Avoid position jumping back when controller replaces the current item When the controller replaces the current item, the masking position will be changed to the default position of the new item for a short while, before the correct position comes from the session. This will interrupt the current position fetched from the controller when the playback doesn't interrupted by the item replacing. Issue: androidx/media#951 #minor-release PiperOrigin-RevId: 611417539 --- RELEASENOTES.md | 3 + .../session/MediaControllerImplBase.java | 91 +++++++++++++++++-- .../androidx/media3/session/MediaUtils.java | 3 +- .../MediaControllerStateMaskingTest.java | 13 +++ 4 files changed, 100 insertions(+), 10 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 32d6035e66..099b89af6e 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -49,6 +49,9 @@ * Muxers: * IMA extension: * Session: + * Fix issue where the current position jumps back when the controller + replaces the current item + ([#951](https://github.com/androidx/media/issues/951)). * UI: * Fallback to include audio track language name if `Locale` cannot identify a display name diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java index e4c5cfcae8..9e590dbe61 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java @@ -970,7 +970,9 @@ import org.checkerframework.checker.nullness.qual.NonNull; } // Add media items to the end of the timeline if the index exceeds the window count. index = min(index, playerInfo.timeline.getWindowCount()); - PlayerInfo newPlayerInfo = maskPlaybackInfoForAddedItems(playerInfo, index, mediaItems); + PlayerInfo newPlayerInfo = + maskPlayerInfoForAddedItems( + playerInfo, index, mediaItems, getCurrentPosition(), getContentPosition()); @Nullable @Player.MediaItemTransitionReason Integer mediaItemTransitionReason = @@ -983,8 +985,23 @@ import org.checkerframework.checker.nullness.qual.NonNull; /* mediaItemTransitionReason= */ mediaItemTransitionReason); } - private static PlayerInfo maskPlaybackInfoForAddedItems( - PlayerInfo playerInfo, int index, List mediaItems) { + /** + * Returns a masking {@link PlayerInfo} for the added {@linkplain MediaItem media items} with the + * provided information. + * + * @param playerInfo The {@link PlayerInfo} that the new masking {@link PlayerInfo} is based on. + * @param index The index at which the {@linkplain MediaItem media items} are added. + * @param mediaItems The {@linkplain MediaItem media items} added. + * @param currentPositionMs The current position in milliseconds. + * @param currentContentPositionMs The current content position in milliseconds. + * @return A masking {@link PlayerInfo}. + */ + private static PlayerInfo maskPlayerInfoForAddedItems( + PlayerInfo playerInfo, + int index, + List mediaItems, + long currentPositionMs, + long currentContentPositionMs) { Timeline oldTimeline = playerInfo.timeline; List newWindows = new ArrayList<>(); List newPeriods = new ArrayList<>(); @@ -1017,6 +1034,8 @@ import org.checkerframework.checker.nullness.qual.NonNull; newTimeline, newMediaItemIndex, newPeriodIndex, + currentPositionMs, + currentContentPositionMs, Player.DISCONTINUITY_REASON_INTERNAL); } @@ -1066,7 +1085,14 @@ import org.checkerframework.checker.nullness.qual.NonNull; } boolean wasCurrentItemRemoved = getCurrentMediaItemIndex() >= fromIndex && getCurrentMediaItemIndex() < toIndex; - PlayerInfo newPlayerInfo = maskPlayerInfoForRemovedItems(playerInfo, fromIndex, toIndex); + PlayerInfo newPlayerInfo = + maskPlayerInfoForRemovedItems( + playerInfo, + fromIndex, + toIndex, + /* isReplacingItems= */ false, + getCurrentPosition(), + getContentPosition()); boolean didMediaItemTransitionHappen = playerInfo.sessionPositionInfo.positionInfo.mediaItemIndex >= fromIndex && playerInfo.sessionPositionInfo.positionInfo.mediaItemIndex < toIndex; @@ -1082,8 +1108,30 @@ import org.checkerframework.checker.nullness.qual.NonNull; : null); } + /** + * Returns a masking {@link PlayerInfo} for the removed {@linkplain MediaItem media items} with + * the provided information. + * + * @param playerInfo The {@link PlayerInfo} that the new masking {@link PlayerInfo} is based on. + * @param fromIndex The index at which to start removing media items (inclusive). + * @param toIndex The index of the first item to be kept (exclusive). + * @param isReplacingItems A boolean indicating whether the media items are removed due to + * replacing. + * @param currentPositionMs The current position in milliseconds. This value will be used in the + * new masking {@link PlayerInfo} if the removal of the media items doesn't affect the current + * playback position. + * @param currentContentPositionMs The current content position in milliseconds. This value will + * be used in the new masking {@link PlayerInfo} if the removal of the media items doesn't + * affect the current playback position. + * @return A masking {@link PlayerInfo}. + */ private static PlayerInfo maskPlayerInfoForRemovedItems( - PlayerInfo playerInfo, int fromIndex, int toIndex) { + PlayerInfo playerInfo, + int fromIndex, + int toIndex, + boolean isReplacingItems, + long currentPositionMs, + long currentContentPositionMs) { Timeline oldTimeline = playerInfo.timeline; List newWindows = new ArrayList<>(); List newPeriods = new ArrayList<>(); @@ -1141,6 +1189,16 @@ import org.checkerframework.checker.nullness.qual.NonNull; newPositionInfo, SessionPositionInfo.DEFAULT, Player.DISCONTINUITY_REASON_REMOVE); + } else if (isReplacingItems) { + newPlayerInfo = + maskTimelineAndPositionInfo( + playerInfo, + newTimeline, + newMediaItemIndex, + newPeriodIndex, + currentPositionMs, + currentContentPositionMs, + Player.DISCONTINUITY_REASON_REMOVE); } else { Window newWindow = newTimeline.getWindow(newMediaItemIndex, new Window()); long defaultPositionMs = newWindow.getDefaultPositionMs(); @@ -1182,6 +1240,8 @@ import org.checkerframework.checker.nullness.qual.NonNull; newTimeline, newMediaItemIndex, newPeriodIndex, + currentPositionMs, + currentContentPositionMs, Player.DISCONTINUITY_REASON_REMOVE); } @@ -1290,8 +1350,17 @@ import org.checkerframework.checker.nullness.qual.NonNull; return; } toIndex = min(toIndex, playlistSize); - PlayerInfo newPlayerInfo = maskPlaybackInfoForAddedItems(playerInfo, toIndex, mediaItems); - newPlayerInfo = maskPlayerInfoForRemovedItems(newPlayerInfo, fromIndex, toIndex); + PlayerInfo newPlayerInfo = + maskPlayerInfoForAddedItems( + playerInfo, toIndex, mediaItems, getCurrentPosition(), getContentPosition()); + newPlayerInfo = + maskPlayerInfoForRemovedItems( + newPlayerInfo, + fromIndex, + toIndex, + /* isReplacingItems= */ true, + getCurrentPosition(), + getContentPosition()); boolean wasCurrentItemReplaced = playerInfo.sessionPositionInfo.positionInfo.mediaItemIndex >= fromIndex && playerInfo.sessionPositionInfo.positionInfo.mediaItemIndex < toIndex; @@ -2111,6 +2180,8 @@ import org.checkerframework.checker.nullness.qual.NonNull; newTimeline, newWindowIndex, newPeriodIndex, + getCurrentPosition(), + getContentPosition(), Player.DISCONTINUITY_REASON_INTERNAL); updatePlayerInfo( @@ -2972,6 +3043,8 @@ import org.checkerframework.checker.nullness.qual.NonNull; Timeline timeline, int newMediaItemIndex, int newPeriodIndex, + long newPositionMs, + long newContentPositionMs, int discontinuityReason) { PositionInfo newPositionInfo = new PositionInfo( @@ -2980,8 +3053,8 @@ import org.checkerframework.checker.nullness.qual.NonNull; timeline.getWindow(newMediaItemIndex, new Window()).mediaItem, /* periodUid= */ null, newPeriodIndex, - playerInfo.sessionPositionInfo.positionInfo.positionMs, - playerInfo.sessionPositionInfo.positionInfo.contentPositionMs, + newPositionMs, + newContentPositionMs, playerInfo.sessionPositionInfo.positionInfo.adGroupIndex, playerInfo.sessionPositionInfo.positionInfo.adIndexInAdGroup); return maskTimelineAndPositionInfo( diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaUtils.java b/libraries/session/src/main/java/androidx/media3/session/MediaUtils.java index 5a3a224ef2..bae4127da8 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaUtils.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaUtils.java @@ -244,7 +244,8 @@ import java.util.List; long lastSetPlayWhenReadyCalledTimeMs, long timeDiffMs) { boolean receivedUpdatedPositionInfo = - lastSetPlayWhenReadyCalledTimeMs < playerInfo.sessionPositionInfo.eventTimeMs; + playerInfo.sessionPositionInfo.equals(SessionPositionInfo.DEFAULT) + || (lastSetPlayWhenReadyCalledTimeMs < playerInfo.sessionPositionInfo.eventTimeMs); if (!playerInfo.isPlaying) { if (receivedUpdatedPositionInfo || currentPositionMs == C.TIME_UNSET) { return playerInfo.sessionPositionInfo.positionInfo.positionMs; diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerStateMaskingTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerStateMaskingTest.java index 9accbf2e3b..972f08330a 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerStateMaskingTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerStateMaskingTest.java @@ -2348,6 +2348,7 @@ public class MediaControllerStateMaskingTest { createMediaItems(firstMediaId, secondMediaId, thirdMediaId))) .setCurrentMediaItemIndex(initialMediaItemIndex) .setCurrentPeriodIndex(initialMediaItemIndex) + .setCurrentPosition(2000L) .build(); remoteSession.setPlayer(playerConfig); @@ -2409,6 +2410,7 @@ public class MediaControllerStateMaskingTest { /* testLastPeriodIndex= */ testCurrentMediaItemIndex); assertThat(newMediaItemRef.get().mediaId).isEqualTo(testCurrentMediaId); assertThat(newPositionInfoRef.get().mediaItemIndex).isEqualTo(testCurrentMediaItemIndex); + assertThat(newPositionInfoRef.get().positionMs).isEqualTo(0L); assertThat(getEventsAsList(onEventsRef.get())) .containsExactly( Player.EVENT_TIMELINE_CHANGED, @@ -3439,12 +3441,14 @@ public class MediaControllerStateMaskingTest { new RemoteMediaSession.MockPlayerConfigBuilder() .setTimeline(MediaTestUtils.createTimeline(3)) .setCurrentMediaItemIndex(1) + .setCurrentPosition(2000L) .build(); remoteSession.setPlayer(playerConfig); MediaController controller = controllerTestRule.createController(remoteSession.getToken()); CountDownLatch latch = new CountDownLatch(2); AtomicReference newTimelineRef = new AtomicReference<>(); AtomicReference onEventsRef = new AtomicReference<>(); + AtomicReference newPositionInfoRef = new AtomicReference<>(); Player.Listener listener = new Player.Listener() { @Override @@ -3458,6 +3462,14 @@ public class MediaControllerStateMaskingTest { onEventsRef.set(events); latch.countDown(); } + + @Override + public void onPositionDiscontinuity( + PositionInfo oldPosition, + PositionInfo newPosition, + @Player.DiscontinuityReason int reason) { + newPositionInfoRef.set(newPosition); + } }; threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener)); AtomicInteger currentMediaItemIndexRef = new AtomicInteger(); @@ -3478,6 +3490,7 @@ public class MediaControllerStateMaskingTest { Player.EVENT_TIMELINE_CHANGED, Player.EVENT_POSITION_DISCONTINUITY, Player.EVENT_MEDIA_ITEM_TRANSITION); + assertThat(newPositionInfoRef.get().positionMs).isEqualTo(2000L); } @Test