From 9dec0d308b8e7d8b0e764ef2d9d705ef0d990107 Mon Sep 17 00:00:00 2001 From: jbibik Date: Tue, 13 Jun 2023 15:46:46 +0100 Subject: [PATCH] Notify listeners of error changes when masking in `MediaControllerImplBase` Currently, the implementation of `MediaControllerImplBase` differs from `ExoPlayerImpl`. The listeners of the former are notified of player error changes only in `onPlayerInfoChanged` and not `updatePlayerInfo` (masking method). Whereas `ExoPlayerImpl` has one unified method - `updatePlaybackInfo` - which sends the events to all the available listeners. This change fixes the lack of 2 particular callbacks - `onPlayerErrorChanged` and `onPlayerError`, however, there might be more differences. Ideally, there should be a unified method for oldPlayerInfo/newPlayerInfo comparison-update-notify-listeners flow. Issue: androidx/media#449 #minor-release PiperOrigin-RevId: 539961618 (cherry picked from commit 4b5a4577905d86ec6e24a92a088a65689c7be901) --- .../session/MediaControllerImplBase.java | 23 +++++++++++++++---- .../MediaControllerStateMaskingTest.java | 20 +++++++++++----- 2 files changed, 33 insertions(+), 10 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 a19f86929a..ff4dd3e092 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java @@ -421,13 +421,13 @@ import org.checkerframework.checker.nullness.qual.NonNull; (iSession, seq) -> iSession.prepare(controllerStub, seq)); if (playerInfo.playbackState == Player.STATE_IDLE) { - PlayerInfo playerInfo = + PlayerInfo newPlayerInfo = this.playerInfo.copyWithPlaybackState( this.playerInfo.timeline.isEmpty() ? Player.STATE_ENDED : Player.STATE_BUFFERING, /* playerError= */ null); updatePlayerInfo( - playerInfo, + newPlayerInfo, /* ignored */ Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED, /* ignored */ Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, /* positionDiscontinuity= */ false, @@ -2152,11 +2152,11 @@ import org.checkerframework.checker.nullness.qual.NonNull; // Update position and then stop estimating until a new positionInfo arrives from the player. maybeUpdateCurrentPositionMs(); lastSetPlayWhenReadyCalledTimeMs = SystemClock.elapsedRealtime(); - PlayerInfo playerInfo = + PlayerInfo newPlayerInfo = this.playerInfo.copyWithPlayWhenReady( playWhenReady, playWhenReadyChangeReason, playbackSuppressionReason); updatePlayerInfo( - playerInfo, + newPlayerInfo, /* ignored */ Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED, playWhenReadyChangeReason, /* positionDiscontinuity= */ false, @@ -2199,6 +2199,21 @@ import org.checkerframework.checker.nullness.qual.NonNull; /* eventFlag= */ Player.EVENT_TIMELINE_CHANGED, listener -> listener.onTimelineChanged(newPlayerInfo.timeline, timelineChangeReason)); } + PlaybackException oldPlayerError = oldPlayerInfo.playerError; + PlaybackException newPlayerError = newPlayerInfo.playerError; + boolean errorsMatch = + oldPlayerError == newPlayerError + || (oldPlayerError != null && oldPlayerError.errorInfoEquals(newPlayerError)); + if (!errorsMatch) { + listeners.queueEvent( + /* eventFlag= */ Player.EVENT_PLAYER_ERROR, + listener -> listener.onPlayerErrorChanged(newPlayerError)); + if (newPlayerError != null) { + listeners.queueEvent( + /* eventFlag= */ Player.EVENT_PLAYER_ERROR, + listener -> listener.onPlayerError(newPlayerError)); + } + } if (oldPlayerInfo.playbackState != newPlayerInfo.playbackState) { listeners.queueEvent( /* eventFlag= */ Player.EVENT_PLAYBACK_STATE_CHANGED, 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 8d0561352f..04a5b7cbbc 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 @@ -630,9 +630,10 @@ public class MediaControllerStateMaskingTest { remoteSession.setPlayer(playerConfig); MediaController controller = controllerTestRule.createController(remoteSession.getToken()); - CountDownLatch latch = new CountDownLatch(2); + CountDownLatch latch = new CountDownLatch(3); AtomicInteger playbackStateFromCallbackRef = new AtomicInteger(); AtomicReference onEventsRef = new AtomicReference<>(); + AtomicReference playerErrorFromCallbackRef = new AtomicReference<>(); Player.Listener listener = new Player.Listener() { @Override @@ -646,25 +647,32 @@ public class MediaControllerStateMaskingTest { onEventsRef.set(events); latch.countDown(); } + + @Override + public void onPlayerErrorChanged(@Nullable PlaybackException error) { + playerErrorFromCallbackRef.set(error); + latch.countDown(); + } }; threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener)); AtomicInteger playbackStateFromGetterRef = new AtomicInteger(); - AtomicReference playerErrorRef = new AtomicReference<>(); + AtomicReference playerErrorFromGetterRef = new AtomicReference<>(); threadTestRule .getHandler() .postAndSync( () -> { controller.prepare(); playbackStateFromGetterRef.set(controller.getPlaybackState()); - playerErrorRef.set(controller.getPlayerError()); + playerErrorFromGetterRef.set(controller.getPlayerError()); }); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playbackStateFromCallbackRef.get()).isEqualTo(testPlaybackState); - assertThat(getEventsAsList(onEventsRef.get())) - .containsExactly(Player.EVENT_PLAYBACK_STATE_CHANGED); assertThat(playbackStateFromGetterRef.get()).isEqualTo(testPlaybackState); - assertThat(playerErrorRef.get()).isNull(); + assertThat(playerErrorFromGetterRef.get()).isNull(); + assertThat(playerErrorFromCallbackRef.get()).isNull(); + assertThat(getEventsAsList(onEventsRef.get())) + .containsExactly(Player.EVENT_PLAYBACK_STATE_CHANGED, Player.EVENT_PLAYER_ERROR); } @Test