Skip to content

Commit

Permalink
Avoid position jumping back when controller replaces the current item
Browse files Browse the repository at this point in the history 
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: #951

PiperOrigin-RevId: 611417539
(cherry picked from commit 1bdc58d)
  • Loading branch information
@tianyif @SheenaChhabra
tianyif authored and SheenaChhabra committed Apr 2, 2024
1 parent 1d2116c commit cf9ff4d
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 11 deletions.
6 changes: 5 additions & 1 deletion RELEASENOTES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@
([#966](https://github.com/androidx/media/issues/966)).
* Effect:
* Improved PQ to SDR tone-mapping by converting color spaces.
* UI:
* 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
([#988](https://github.com/androidx/media/issues/988)).
Expand Down
91 changes: 82 additions & 9 deletions libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -970,7 +970,9 @@ private void addMediaItemsInternal(int index, List<MediaItem> mediaItems) {
}
// 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 =
Expand All @@ -983,8 +985,23 @@ private void addMediaItemsInternal(int index, List<MediaItem> mediaItems) {
/* mediaItemTransitionReason= */ mediaItemTransitionReason);
}

private static PlayerInfo maskPlaybackInfoForAddedItems(
PlayerInfo playerInfo, int index, List<MediaItem> 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<MediaItem> mediaItems,
long currentPositionMs,
long currentContentPositionMs) {
Timeline oldTimeline = playerInfo.timeline;
List<Window> newWindows = new ArrayList<>();
List<Period> newPeriods = new ArrayList<>();
Expand Down Expand Up @@ -1017,6 +1034,8 @@ private static PlayerInfo maskPlaybackInfoForAddedItems(
newTimeline,
newMediaItemIndex,
newPeriodIndex,
currentPositionMs,
currentContentPositionMs,
Player.DISCONTINUITY_REASON_INTERNAL);
}

Expand Down Expand Up @@ -1066,7 +1085,14 @@ private void removeMediaItemsInternal(int fromIndex, int toIndex) {
}
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;
Expand All @@ -1082,8 +1108,30 @@ private void removeMediaItemsInternal(int fromIndex, int toIndex) {
: 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<Window> newWindows = new ArrayList<>();
List<Period> newPeriods = new ArrayList<>();
Expand Down Expand Up @@ -1141,6 +1189,16 @@ private static PlayerInfo maskPlayerInfoForRemovedItems(
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();
Expand Down Expand Up @@ -1182,6 +1240,8 @@ private static PlayerInfo maskPlayerInfoForRemovedItems(
newTimeline,
newMediaItemIndex,
newPeriodIndex,
currentPositionMs,
currentContentPositionMs,
Player.DISCONTINUITY_REASON_REMOVE);
}

Expand Down Expand Up @@ -1290,8 +1350,17 @@ private void replaceMediaItemsInternal(int fromIndex, int toIndex, List<MediaIte
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;
Expand Down Expand Up @@ -2111,6 +2180,8 @@ private void moveMediaItemsInternal(int fromIndex, int toIndex, int newIndex) {
newTimeline,
newWindowIndex,
newPeriodIndex,
getCurrentPosition(),
getContentPosition(),
Player.DISCONTINUITY_REASON_INTERNAL);

updatePlayerInfo(
Expand Down Expand Up @@ -2972,6 +3043,8 @@ private static PlayerInfo maskTimelineAndPositionInfo(
Timeline timeline,
int newMediaItemIndex,
int newPeriodIndex,
long newPositionMs,
long newContentPositionMs,
int discontinuityReason) {
PositionInfo newPositionInfo =
new PositionInfo(
Expand All @@ -2980,8 +3053,8 @@ private static PlayerInfo maskTimelineAndPositionInfo(
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(
Expand Down
3 changes: 2 additions & 1 deletion libraries/session/src/main/java/androidx/media3/session/MediaUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,8 @@ public static long getUpdatedCurrentPositionMs(
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;
Expand Down
13 changes: 13 additions & 0 deletions ...current/src/androidTest/java/androidx/media3/session/MediaControllerStateMaskingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2348,6 +2348,7 @@ public void removeMediaItems_currentItemRemoved() throws Exception {
createMediaItems(firstMediaId, secondMediaId, thirdMediaId)))
.setCurrentMediaItemIndex(initialMediaItemIndex)
.setCurrentPeriodIndex(initialMediaItemIndex)
.setCurrentPosition(2000L)
.build();
remoteSession.setPlayer(playerConfig);

Expand Down Expand Up @@ -2409,6 +2410,7 @@ public void onEvents(Player player, Player.Events events) {
/* 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,
Expand Down Expand Up @@ -3439,12 +3441,14 @@ public void replaceMediaItems_replacingCurrentItem_correctMasking() throws Excep
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<Timeline> newTimelineRef = new AtomicReference<>();
AtomicReference<Player.Events> onEventsRef = new AtomicReference<>();
AtomicReference<PositionInfo> newPositionInfoRef = new AtomicReference<>();
Player.Listener listener =
new Player.Listener() {
@Override
Expand All @@ -3458,6 +3462,14 @@ public void onEvents(Player player, Player.Events events) {
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();
Expand All @@ -3478,6 +3490,7 @@ public void onEvents(Player player, Player.Events events) {
Player.EVENT_TIMELINE_CHANGED,
Player.EVENT_POSITION_DISCONTINUITY,
Player.EVENT_MEDIA_ITEM_TRANSITION);
assertThat(newPositionInfoRef.get().positionMs).isEqualTo(2000L);
}

@Test
Expand Down

0 comments on commit cf9ff4d

Please sign in to comment.