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)
This commit is contained in:
tonihei 2023-05-15 14:19:03 +01:00 committed by Tofunmi Adigun-Hameed
parent 5722e6472a
commit c7004d43af
5 changed files with 85 additions and 3 deletions

View File

@ -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

View File

@ -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);
}
}

View File

@ -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);
}
}
}

View File

@ -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);

View File

@ -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<Integer> 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<Integer> 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());