From dddc6026e90a1b8587f40d37272565782eef9949 Mon Sep 17 00:00:00 2001 From: tonihei Date: Wed, 9 Nov 2022 17:29:31 +0000 Subject: [PATCH] Wait with PlayerInfo updates until all pending operations are done Accepting a PlayerInfo while the MediaController is masking its state means we are reverting all masking changes we've made earlier. This only makes sense if the update already contains the masked operation. If multiple operations are in flight (or are sent from the session while they are in flight), we need to wait until all of them are handled before accepting new updates. In cases where a new update from the session excludes the Timeline and the masked state is incompatible with the new update, we also risk an exception if we accept the update too early. PiperOrigin-RevId: 487266899 (cherry picked from commit 0b4ba3e3a6130253b801ddc231501168efce8901) --- .../session/MediaControllerImplBase.java | 39 +++++++-- .../MediaControllerStateMaskingTest.java | 79 +++++++++++++++++++ .../session/MediaSessionProviderService.java | 9 +++ 3 files changed, 121 insertions(+), 6 deletions(-) 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 41e1f5a97a..68fcbf3576 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java @@ -47,6 +47,7 @@ import android.view.SurfaceHolder; import android.view.SurfaceView; import android.view.TextureView; import androidx.annotation.Nullable; +import androidx.collection.ArraySet; import androidx.media3.common.AdPlaybackState; import androidx.media3.common.AudioAttributes; import androidx.media3.common.BundleListRetriever; @@ -110,6 +111,7 @@ import org.checkerframework.checker.nullness.qual.NonNull; private final SurfaceCallback surfaceCallback; private final ListenerSet listeners; private final FlushCommandQueueHandler flushCommandQueueHandler; + private final ArraySet pendingMaskingSequencedFutureNumbers; @Nullable private SessionToken connectedToken; @Nullable private SessionServiceConnection serviceConnection; @@ -127,6 +129,7 @@ import org.checkerframework.checker.nullness.qual.NonNull; @Nullable private IMediaSession iSession; private long lastReturnedCurrentPositionMs; private long lastSetPlayWhenReadyCalledTimeMs; + @Nullable private Timeline pendingPlayerInfoUpdateTimeline; public MediaControllerImplBase( Context context, @@ -154,6 +157,7 @@ import org.checkerframework.checker.nullness.qual.NonNull; this.context = context; sequencedFutureManager = new SequencedFutureManager(); controllerStub = new MediaControllerStub(this); + pendingMaskingSequencedFutureNumbers = new ArraySet<>(); this.token = token; this.connectionHints = connectionHints; deathRecipient = @@ -303,7 +307,7 @@ import org.checkerframework.checker.nullness.qual.NonNull; if (command != Player.COMMAND_SET_VIDEO_SURFACE) { flushCommandQueueHandler.sendFlushCommandQueueMessage(); } - return dispatchRemoteSessionTask(iSession, task); + return dispatchRemoteSessionTask(iSession, task, /* addToPendingMaskingOperations= */ true); } private ListenableFuture dispatchRemoteSessionTaskWithSessionCommand( @@ -326,17 +330,22 @@ import org.checkerframework.checker.nullness.qual.NonNull; sessionCommand != null ? getSessionInterfaceWithSessionCommandIfAble(sessionCommand) : getSessionInterfaceWithSessionCommandIfAble(commandCode), - task); + task, + /* addToPendingMaskingOperations= */ false); } private ListenableFuture dispatchRemoteSessionTask( - IMediaSession iSession, RemoteSessionTask task) { + IMediaSession iSession, RemoteSessionTask task, boolean addToPendingMaskingOperations) { if (iSession != null) { SequencedFutureManager.SequencedFuture result = sequencedFutureManager.createSequencedFuture( new SessionResult(SessionResult.RESULT_INFO_SKIPPED)); try { - task.run(iSession, result.getSequenceNumber()); + int sequenceNumber = result.getSequenceNumber(); + task.run(iSession, sequenceNumber); + if (addToPendingMaskingOperations) { + pendingMaskingSequencedFutureNumbers.add(sequenceNumber); + } } catch (RemoteException e) { Log.w(TAG, "Cannot connect to the service or the session is gone", e); result.set(new SessionResult(SessionResult.RESULT_ERROR_SESSION_DISCONNECTED)); @@ -2223,7 +2232,12 @@ import org.checkerframework.checker.nullness.qual.NonNull; } void setFutureResult(int seq, T futureResult) { + // Don't set the future result on the application looper so that the result can be obtained by a + // blocking future.get() on the application looper. But post a message to remove the pending + // masking operation on the application looper to ensure it's executed in order with other + // updates sent to the application looper. sequencedFutureManager.setFutureResult(seq, futureResult); + getInstance().runOnApplicationLooper(() -> pendingMaskingSequencedFutureNumbers.remove(seq)); } void onConnected(ConnectionState result) { @@ -2313,11 +2327,23 @@ import org.checkerframework.checker.nullness.qual.NonNull; if (!isConnected()) { return; } + if (!pendingMaskingSequencedFutureNumbers.isEmpty()) { + // We are still waiting for all pending masking operations to be handled. + if (!isTimelineExcluded) { + pendingPlayerInfoUpdateTimeline = newPlayerInfo.timeline; + } + return; + } PlayerInfo oldPlayerInfo = playerInfo; playerInfo = newPlayerInfo; if (isTimelineExcluded) { - playerInfo = playerInfo.copyWithTimeline(oldPlayerInfo.timeline); + playerInfo = + playerInfo.copyWithTimeline( + pendingPlayerInfoUpdateTimeline != null + ? pendingPlayerInfoUpdateTimeline + : oldPlayerInfo.timeline); } + pendingPlayerInfoUpdateTimeline = null; PlaybackException oldPlayerError = oldPlayerInfo.playerError; PlaybackException playerError = playerInfo.playerError; boolean errorsMatch = @@ -2568,7 +2594,8 @@ import org.checkerframework.checker.nullness.qual.NonNull; } private void updateSessionPositionInfoIfNeeded(SessionPositionInfo sessionPositionInfo) { - if (playerInfo.sessionPositionInfo.eventTimeMs < sessionPositionInfo.eventTimeMs) { + if (pendingMaskingSequencedFutureNumbers.isEmpty() + && playerInfo.sessionPositionInfo.eventTimeMs < sessionPositionInfo.eventTimeMs) { playerInfo = playerInfo.copyWithSessionPositionInfo(sessionPositionInfo); } } 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 e7ffef7880..7e7f18dc17 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 @@ -2878,6 +2878,85 @@ public class MediaControllerStateMaskingTest { /* testPreviousMediaItemIndex= */ C.INDEX_UNSET); } + @Test + public void incompatibleUpdatesDuringMasking_areOnlyReportedOnceAllPendingUpdatesAreResolved() + throws Exception { + // Test setup: + // 1. Report a discontinuity from item 0 to item 1 in the session. + // 2. Before (1) can be handled by the controller, remove item 1. + // Expectation: + // - Session: State is updated to ENDED as the current item is removed. + // - Controller: Discontinuity is only reported after the state is fully resolved + // = The discontinuity is only reported once we also report the state change to ENDED. + Timeline timeline = MediaTestUtils.createTimeline(/* windowCount= */ 2); + remoteSession.getMockPlayer().setTimeline(timeline); + remoteSession + .getMockPlayer() + .notifyTimelineChanged(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED); + remoteSession.getMockPlayer().setCurrentMediaItemIndex(0); + MediaController controller = controllerTestRule.createController(remoteSession.getToken()); + CountDownLatch positionDiscontinuityReported = new CountDownLatch(1); + AtomicBoolean reportedStateChangeToEndedAtSameTimeAsDiscontinuity = new AtomicBoolean(); + Player.Listener listener = + new Player.Listener() { + @Override + public void onEvents(Player player, Player.Events events) { + if (events.contains(Player.EVENT_POSITION_DISCONTINUITY)) { + if (events.contains(Player.EVENT_PLAYBACK_STATE_CHANGED) + && player.getPlaybackState() == Player.STATE_ENDED) { + reportedStateChangeToEndedAtSameTimeAsDiscontinuity.set(true); + } + positionDiscontinuityReported.countDown(); + } + } + }; + threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener)); + + // Step 1: Report a discontinuity from item 0 to item 1 in the session. + PositionInfo oldPositionInfo = + new PositionInfo( + /* windowUid= */ timeline.getWindow(/* windowIndex= */ 0, new Window()).uid, + /* mediaItemIndex= */ 0, + MediaItem.EMPTY, + /* periodUid= */ timeline.getPeriod( + /* periodIndex= */ 0, new Period(), /* setIds= */ true) + .uid, + /* periodIndex= */ 0, + /* positionMs= */ 10_000, + /* contentPositionMs= */ 10_000, + /* adGroupIndex= */ C.INDEX_UNSET, + /* adIndexInAdGroup= */ C.INDEX_UNSET); + PositionInfo newPositionInfo = + new PositionInfo( + /* windowUid= */ timeline.getWindow(/* windowIndex= */ 1, new Window()).uid, + /* mediaItemIndex= */ 1, + MediaItem.EMPTY, + /* periodUid= */ timeline.getPeriod( + /* periodIndex= */ 1, new Period(), /* setIds= */ true) + .uid, + /* periodIndex= */ 1, + /* positionMs= */ 0, + /* contentPositionMs= */ 0, + /* adGroupIndex= */ C.INDEX_UNSET, + /* adIndexInAdGroup= */ C.INDEX_UNSET); + remoteSession.getMockPlayer().setCurrentMediaItemIndex(1); + remoteSession + .getMockPlayer() + .notifyPositionDiscontinuity( + oldPositionInfo, newPositionInfo, Player.DISCONTINUITY_REASON_AUTO_TRANSITION); + // Step 2: Before step 1 can be handled by the controller, remove item 1. + threadTestRule.getHandler().postAndSync(() -> controller.removeMediaItem(/* index= */ 1)); + remoteSession.getMockPlayer().setCurrentMediaItemIndex(0); + remoteSession.getMockPlayer().setTimeline(MediaTestUtils.createTimeline(/* windowCount= */ 1)); + remoteSession.getMockPlayer().notifyPlaybackStateChanged(Player.STATE_ENDED); + remoteSession + .getMockPlayer() + .notifyTimelineChanged(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED); + + assertThat(positionDiscontinuityReported.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + assertThat(reportedStateChangeToEndedAtSameTimeAsDiscontinuity.get()).isTrue(); + } + private void assertMoveMediaItems( int initialMediaItemCount, int initialMediaItemIndex, diff --git a/libraries/test_session_current/src/main/java/androidx/media3/session/MediaSessionProviderService.java b/libraries/test_session_current/src/main/java/androidx/media3/session/MediaSessionProviderService.java index 2f91a3a286..fa312abd31 100644 --- a/libraries/test_session_current/src/main/java/androidx/media3/session/MediaSessionProviderService.java +++ b/libraries/test_session_current/src/main/java/androidx/media3/session/MediaSessionProviderService.java @@ -785,6 +785,13 @@ public class MediaSessionProviderService extends Service { MediaSession session = sessionMap.get(sessionId); MockPlayer player = (MockPlayer) session.getPlayer(); player.timeline = Timeline.CREATOR.fromBundle(timelineBundle); + List mediaItems = new ArrayList<>(); + for (int i = 0; i < player.timeline.getWindowCount(); i++) { + mediaItems.add( + player.timeline.getWindow(/* windowIndex= */ i, new Timeline.Window()).mediaItem); + } + player.mediaItems.clear(); + player.mediaItems.addAll(mediaItems); }); } @@ -800,6 +807,8 @@ public class MediaSessionProviderService extends Service { mediaItems.add( MediaTestUtils.createMediaItem(TestUtils.getMediaIdInFakeTimeline(windowIndex))); } + player.mediaItems.clear(); + player.mediaItems.addAll(mediaItems); player.timeline = new PlaylistTimeline(mediaItems); }); }