From c7004d43af743a97f8b66ab5da96c20a15127ae7 Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 15 May 2023 14:19:03 +0100 Subject: [PATCH] Add consistency check to sending and receiving position updates The periodic updates are only meant to happen while we are in the same period or ad. This was already guaranteed except for two cases: 1. The Player in a session has updated its state without yet calling its listeners 2. The session scheduled a PlayerInfo update that hasn't been sent yet ... and in both cases, the following happened: - The change updated the mediaItemIndex to an index that didn't exist in a previous Timeline known to the Controller - One of the period position updates happened to be sent at exactly this time This problem can be avoided by only scheduling the update if we are still in the same period/ad and haven't scheduled a normal PlayerInfo update already. Since new MediaControllers may still connect to old sessons with this bug, we need an equivalent change on the controller side to ignore such buggy updates. PiperOrigin-RevId: 532089328 (cherry picked from commit 96dd0ae5837a5fd82d7407623bedb5fd4d1e9252) --- RELEASENOTES.md | 3 ++ .../session/MediaControllerImplBase.java | 7 +++ .../media3/session/MediaSessionImpl.java | 19 ++++++-- .../androidx/media3/session/MediaUtils.java | 13 ++++++ .../media3/session/MediaControllerTest.java | 46 +++++++++++++++++++ 5 files changed, 85 insertions(+), 3 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 02807708b8..a7fc67516a 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -7,6 +7,9 @@ implement playback resumption with media button events sent by, for example, a Bluetooth headset ([#167](https://github.com/androidx/media/issues/167)). + * Fix bug where a combined `Timeline` and position update in a + `MediaSession` may cause a `MediaController` to throw an + `IllegalStateException`. * Remove deprecated symbols: * Remove two deprecated `SimpleCache` constructors, use a non-deprecated constructor that takes a `DatabaseProvider` instead for better 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 f1128e8969..5a4b33be2b 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java @@ -2613,6 +2613,13 @@ import org.checkerframework.checker.nullness.qual.NonNull; private void updateSessionPositionInfoIfNeeded(SessionPositionInfo sessionPositionInfo) { if (pendingMaskingSequencedFutureNumbers.isEmpty() && playerInfo.sessionPositionInfo.eventTimeMs < sessionPositionInfo.eventTimeMs) { + if (!MediaUtils.areSessionPositionInfosInSamePeriodOrAd( + sessionPositionInfo, playerInfo.sessionPositionInfo)) { + // MediaSessionImpl before version 1.0.2 has a bug that may send position info updates for + // new periods too early. Ignore these updates to avoid an inconsistent state (see + // [internal b/277301159]). + return; + } playerInfo = playerInfo.copyWithSessionPositionInfo(sessionPositionInfo); } } 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 db50614290..74d660bbf7 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java @@ -807,7 +807,16 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; } } SessionPositionInfo sessionPositionInfo = playerWrapper.createSessionPositionInfoForBundling(); - dispatchOnPeriodicSessionPositionInfoChanged(sessionPositionInfo); + if (!onPlayerInfoChangedHandler.hasPendingPlayerInfoChangedUpdate() + && MediaUtils.areSessionPositionInfosInSamePeriodOrAd( + sessionPositionInfo, playerInfo.sessionPositionInfo)) { + // Send a periodic position info only if a PlayerInfo update is not already already pending + // and the player state is still corresponding to the currently known PlayerInfo. Both + // conditions will soon trigger a new PlayerInfo update with the latest position info anyway + // and we also don't want to send a new position info early if the corresponding Timeline + // update hasn't been sent yet (see [internal b/277301159]). + dispatchOnPeriodicSessionPositionInfoChanged(sessionPositionInfo); + } schedulePeriodicSessionPositionInfoChanges(); } @@ -1362,11 +1371,15 @@ import org.checkerframework.checker.nullness.qual.MonotonicNonNull; } } + public boolean hasPendingPlayerInfoChangedUpdate() { + return hasMessages(MSG_PLAYER_INFO_CHANGED); + } + public void sendPlayerInfoChangedMessage(boolean excludeTimeline, boolean excludeTracks) { this.excludeTimeline = this.excludeTimeline && excludeTimeline; this.excludeTracks = this.excludeTracks && excludeTracks; - if (!onPlayerInfoChangedHandler.hasMessages(MSG_PLAYER_INFO_CHANGED)) { - onPlayerInfoChangedHandler.sendEmptyMessage(MSG_PLAYER_INFO_CHANGED); + if (!hasMessages(MSG_PLAYER_INFO_CHANGED)) { + sendEmptyMessage(MSG_PLAYER_INFO_CHANGED); } } } 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 0fda511e7c..a15d5fe0c3 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaUtils.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaUtils.java @@ -1476,6 +1476,19 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; } } + /** + * Returns whether the two provided {@link SessionPositionInfo} describe a position in the same + * period or ad. + */ + public static boolean areSessionPositionInfosInSamePeriodOrAd( + SessionPositionInfo info1, SessionPositionInfo info2) { + // TODO: b/259220235 - Use UIDs instead of mediaItemIndex and periodIndex + return info1.positionInfo.mediaItemIndex == info2.positionInfo.mediaItemIndex + && info1.positionInfo.periodIndex == info2.positionInfo.periodIndex + && info1.positionInfo.adGroupIndex == info2.positionInfo.adGroupIndex + && info1.positionInfo.adIndexInAdGroup == info2.positionInfo.adIndexInAdGroup; + } + private static byte[] convertToByteArray(Bitmap bitmap) throws IOException { try (ByteArrayOutputStream stream = new ByteArrayOutputStream()) { bitmap.compress(Bitmap.CompressFormat.PNG, /* ignored */ 0, stream); diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java index 4a4c01db4f..5405c0f6d7 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java @@ -34,6 +34,7 @@ import android.app.PendingIntent; import android.content.Context; import android.os.Bundle; import android.os.RemoteException; +import androidx.annotation.Nullable; import androidx.media3.common.AudioAttributes; import androidx.media3.common.C; import androidx.media3.common.Format; @@ -1064,6 +1065,51 @@ public class MediaControllerTest { assertThat(bufferedPositionAfterDelay.get()).isNotEqualTo(testBufferedPosition); } + @Test + public void + getCurrentMediaItemIndex_withPeriodicUpdateOverlappingTimelineChanges_updatesIndexCorrectly() + throws Exception { + Bundle playerConfig = + new RemoteMediaSession.MockPlayerConfigBuilder() + .setPlayWhenReady(true) + .setPlaybackState(Player.STATE_READY) + .build(); + remoteSession.setPlayer(playerConfig); + MediaController controller = controllerTestRule.createController(remoteSession.getToken()); + ArrayList transitionMediaItemIndices = new ArrayList<>(); + controller.addListener( + new Player.Listener() { + @Override + public void onMediaItemTransition(@Nullable MediaItem mediaItem, int reason) { + transitionMediaItemIndices.add(controller.getCurrentMediaItemIndex()); + } + }); + + // Intentionally trigger update often to ensure there is a likely overlap with Timeline updates. + remoteSession.setSessionPositionUpdateDelayMs(1L); + // Trigger many timeline and position updates that are incompatible with any previous updates. + for (int i = 1; i <= 100; i++) { + remoteSession.getMockPlayer().createAndSetFakeTimeline(/* windowCount= */ i); + remoteSession.getMockPlayer().setCurrentMediaItemIndex(i - 1); + remoteSession + .getMockPlayer() + .notifyTimelineChanged(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED); + remoteSession + .getMockPlayer() + .notifyMediaItemTransition( + /* index= */ i - 1, Player.MEDIA_ITEM_TRANSITION_REASON_PLAYLIST_CHANGED); + } + PollingCheck.waitFor(TIMEOUT_MS, () -> transitionMediaItemIndices.size() == 100); + + ImmutableList.Builder expectedMediaItemIndices = ImmutableList.builder(); + for (int i = 0; i < 100; i++) { + expectedMediaItemIndices.add(i); + } + assertThat(transitionMediaItemIndices) + .containsExactlyElementsIn(expectedMediaItemIndices.build()) + .inOrder(); + } + @Test public void getContentBufferedPosition_byDefault_returnsZero() throws Exception { MediaController controller = controllerTestRule.createController(remoteSession.getToken());