From dee83cc7db73e5a07b4addc9325810643c9ec13d Mon Sep 17 00:00:00 2001 From: bachinger Date: Wed, 27 Apr 2022 11:48:01 +0100 Subject: [PATCH] Update session position info on timeline change This fixes an inconsistent state of the `PlayerInfo` when the index of the playing media item is changed by a playlist modification. In this inconsistent state, calling `Playerinfo.getCurrentMediaItem` can produce an `ArrayIndexOutOfBoundException` (see stack trace in GH issue). This change takes the following measurements: - always update sessionPosition and timeline of the PlayerInfo together in `MediaSessionImpl.PlayerListener` where the PlayerInfo originates from - add an assertion to avoid building a `PlayerInfo` instance in an inconsistent state - reduce the window of opportunity for concurrent access to `mediaSessionImpl.playerInfo` when dispatching player info changes in `MediaSessionImpl` Issue: androidx/media#51 PiperOrigin-RevId: 444812661 --- RELEASENOTES.md | 3 ++ .../session/MediaControllerImplLegacy.java | 9 +++-- .../media3/session/MediaSessionImpl.java | 11 ++++--- .../androidx/media3/session/PlayerInfo.java | 33 +++++++++++++------ 4 files changed, 39 insertions(+), 17 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index c517c8eeb5..02fc46b970 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -80,6 +80,9 @@ * Session: * Fix NPE in MediaControllerImplLegacy ([#59](https://github.com/androidx/media/pull/59)) + * Update session position info on timeline + change([#51](https://github.com/androidx/media/issues/51)) + * Data sources: * Rename `DummyDataSource` to `PlaceHolderDataSource`. * Remove deprecated symbols: diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java index 577986f772..4de254cd50 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java @@ -753,7 +753,8 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; int newCurrentMediaItemIndex = calculateCurrentItemIndexAfterAddItems(currentMediaItemIndex, index, mediaItems.size()); PlayerInfo maskedPlayerInfo = - controllerInfo.playerInfo.copyWithTimeline(newQueueTimeline, newCurrentMediaItemIndex); + controllerInfo.playerInfo.copyWithTimelineAndMediaItemIndex( + newQueueTimeline, newCurrentMediaItemIndex); ControllerInfo maskedControllerInfo = new ControllerInfo( maskedPlayerInfo, @@ -801,7 +802,8 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; + " new current item"); } PlayerInfo maskedPlayerInfo = - controllerInfo.playerInfo.copyWithTimeline(newQueueTimeline, newCurrentMediaItemIndex); + controllerInfo.playerInfo.copyWithTimelineAndMediaItemIndex( + newQueueTimeline, newCurrentMediaItemIndex); ControllerInfo maskedControllerInfo = new ControllerInfo( @@ -861,7 +863,8 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; QueueTimeline newQueueTimeline = queueTimeline.copyWithMovedMediaItems(fromIndex, toIndex, newIndex); PlayerInfo maskedPlayerInfo = - controllerInfo.playerInfo.copyWithTimeline(newQueueTimeline, newCurrentMediaItemIndex); + controllerInfo.playerInfo.copyWithTimelineAndMediaItemIndex( + newQueueTimeline, newCurrentMediaItemIndex); ControllerInfo maskedControllerInfo = new ControllerInfo( diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java index 4ff9f34cd5..07caa4e5ef 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java @@ -366,7 +366,7 @@ import org.checkerframework.checker.initialization.qual.Initialized; (controller, seq) -> controller.sendCustomCommand(seq, command, args)); } - private void dispatchOnPlayerInfoChanged(boolean excludeTimeline) { + private void dispatchOnPlayerInfoChanged(PlayerInfo playerInfo, boolean excludeTimeline) { List controllers = sessionStub.getConnectedControllersManager().getConnectedControllers(); @@ -910,7 +910,9 @@ import org.checkerframework.checker.initialization.qual.Initialized; if (player == null) { return; } - session.playerInfo = session.playerInfo.copyWithTimeline(timeline); + session.playerInfo = + session.playerInfo.copyWithTimelineAndSessionPositionInfo( + timeline, player.createSessionPositionInfoForBundling()); session.onPlayerInfoChangedHandler.sendPlayerInfoChangedMessage(/* excludeTimeline= */ false); session.dispatchRemoteControllerTaskToLegacyStub( (callback, seq) -> callback.onTimelineChanged(seq, timeline, reason)); @@ -1177,9 +1179,10 @@ import org.checkerframework.checker.initialization.qual.Initialized; public void handleMessage(Message msg) { if (msg.what == MSG_PLAYER_INFO_CHANGED) { playerInfo = - playerInfo.copyWithSessionPositionInfo( + playerInfo.copyWithTimelineAndSessionPositionInfo( + getPlayerWrapper().getCurrentTimeline(), getPlayerWrapper().createSessionPositionInfoForBundling()); - dispatchOnPlayerInfoChanged(excludeTimeline); + dispatchOnPlayerInfoChanged(playerInfo, excludeTimeline); excludeTimeline = true; } else { throw new IllegalStateException("Invalid message what=" + msg.what); diff --git a/libraries/session/src/main/java/androidx/media3/session/PlayerInfo.java b/libraries/session/src/main/java/androidx/media3/session/PlayerInfo.java index 342796e7cc..e8375699d8 100644 --- a/libraries/session/src/main/java/androidx/media3/session/PlayerInfo.java +++ b/libraries/session/src/main/java/androidx/media3/session/PlayerInfo.java @@ -44,6 +44,7 @@ import androidx.media3.common.Timeline.Window; import androidx.media3.common.TrackSelectionParameters; import androidx.media3.common.VideoSize; import androidx.media3.common.text.Cue; +import androidx.media3.common.util.Assertions; import androidx.media3.common.util.BundleableUtil; import com.google.common.collect.ImmutableList; import java.lang.annotation.Documented; @@ -271,6 +272,9 @@ import java.util.List; } public PlayerInfo build() { + Assertions.checkState( + timeline.isEmpty() + || sessionPositionInfo.positionInfo.mediaItemIndex < timeline.getWindowCount()); return new PlayerInfo( playerError, mediaItemTransitionReason, @@ -344,7 +348,7 @@ import java.util.List; MediaMetadata.EMPTY, /* seekBackIncrementMs= */ 0, /* seekForwardIncrementMs= */ 0, - /* maxSeekToPreviousPosition= */ 0, + /* maxSeekToPreviousPositionMs= */ 0, TrackSelectionParameters.DEFAULT_WITHOUT_CONTEXT); @Nullable public final PlaybackException playerError; @@ -429,11 +433,6 @@ import java.util.List; return new Builder(this).setPlayerError(playerError).build(); } - @CheckResult - public PlayerInfo copyWithSessionPositionInfo(SessionPositionInfo sessionPositionInfo) { - return new Builder(this).setSessionPositionInfo(sessionPositionInfo).build(); - } - @CheckResult public PlayerInfo copyWithPlaybackState( @Player.State int playbackState, @Nullable PlaybackException playerError) { @@ -454,6 +453,11 @@ import java.util.List; return new Builder(this).setIsLoading(isLoading).build(); } + @CheckResult + public PlayerInfo copyWithPlaybackParameters(PlaybackParameters playbackParameters) { + return new Builder(this).setPlaybackParameters(playbackParameters).build(); + } + @CheckResult public PlayerInfo copyWithPositionInfos( PositionInfo oldPositionInfo, @@ -467,8 +471,8 @@ import java.util.List; } @CheckResult - public PlayerInfo copyWithPlaybackParameters(PlaybackParameters playbackParameters) { - return new Builder(this).setPlaybackParameters(playbackParameters).build(); + public PlayerInfo copyWithSessionPositionInfo(SessionPositionInfo sessionPositionInfo) { + return new Builder(this).setSessionPositionInfo(sessionPositionInfo).build(); } @CheckResult @@ -477,14 +481,23 @@ import java.util.List; } @CheckResult - public PlayerInfo copyWithTimeline(Timeline timeline, int windowIndex) { + public PlayerInfo copyWithTimelineAndSessionPositionInfo( + Timeline timeline, SessionPositionInfo sessionPositionInfo) { + return new Builder(this) + .setTimeline(timeline) + .setSessionPositionInfo(sessionPositionInfo) + .build(); + } + + @CheckResult + public PlayerInfo copyWithTimelineAndMediaItemIndex(Timeline timeline, int mediaItemIndex) { return new Builder(this) .setTimeline(timeline) .setSessionPositionInfo( new SessionPositionInfo( new PositionInfo( sessionPositionInfo.positionInfo.windowUid, - windowIndex, + mediaItemIndex, sessionPositionInfo.positionInfo.mediaItem, sessionPositionInfo.positionInfo.periodUid, sessionPositionInfo.positionInfo.periodIndex,