Skip to content

Commit

Permalink
Ensure listener invocations use final state variable.
Browse files Browse the repository at this point in the history 
The MediaControllerImplBase listener invocations currently use the
class member state that can change if one of the listener method
implementations changes the state recursively.

Updating the listener invocations to use a final local variable
ensures all listeners get consistent updates.

PiperOrigin-RevId: 487503373
(cherry picked from commit 33bea39)
  • Loading branch information
@tonihei @microkatz
tonihei authored and microkatz committed Nov 10, 2022
1 parent a9eb121 commit 7f17ff6
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 70 deletions.
134 changes: 70 additions & 64 deletions libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2320,10 +2320,7 @@ void onCustomCommand(int seq, SessionCommand command, Bundle args) {
}

@SuppressWarnings("deprecation") // Implementing and calling deprecated listener method.
void onPlayerInfoChanged(
PlayerInfo newPlayerInfo,
@Player.TimelineChangeReason int timelineChangedReason,
boolean isTimelineExcluded) {
void onPlayerInfoChanged(PlayerInfo newPlayerInfo, boolean isTimelineExcluded) {
if (!isConnected()) {
return;
}
Expand All @@ -2335,169 +2332,178 @@ void onPlayerInfoChanged(
return;
}
PlayerInfo oldPlayerInfo = playerInfo;
playerInfo = newPlayerInfo;
if (isTimelineExcluded) {
playerInfo =
playerInfo.copyWithTimeline(
newPlayerInfo =
newPlayerInfo.copyWithTimeline(
pendingPlayerInfoUpdateTimeline != null
? pendingPlayerInfoUpdateTimeline
: oldPlayerInfo.timeline);
}
// Assigning class variable now so that all getters called from listeners see the updated value.
// But we need to use a local final variable to ensure listeners get consistent parameters.
playerInfo = newPlayerInfo;
PlayerInfo finalPlayerInfo = newPlayerInfo;
pendingPlayerInfoUpdateTimeline = null;
PlaybackException oldPlayerError = oldPlayerInfo.playerError;
PlaybackException playerError = playerInfo.playerError;
PlaybackException playerError = finalPlayerInfo.playerError;
boolean errorsMatch =
oldPlayerError == playerError
|| (oldPlayerError != null && oldPlayerError.errorInfoEquals(playerError));
if (!errorsMatch) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_PLAYER_ERROR,
listener -> listener.onPlayerErrorChanged(playerInfo.playerError));
if (playerInfo.playerError != null) {
listener -> listener.onPlayerErrorChanged(finalPlayerInfo.playerError));
if (finalPlayerInfo.playerError != null) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_PLAYER_ERROR,
listener -> listener.onPlayerError(playerInfo.playerError));
listener -> listener.onPlayerError(finalPlayerInfo.playerError));
}
}
MediaItem oldCurrentMediaItem = oldPlayerInfo.getCurrentMediaItem();
MediaItem currentMediaItem = playerInfo.getCurrentMediaItem();
MediaItem currentMediaItem = finalPlayerInfo.getCurrentMediaItem();
if (!Util.areEqual(oldCurrentMediaItem, currentMediaItem)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_MEDIA_ITEM_TRANSITION,
listener ->
listener.onMediaItemTransition(
currentMediaItem, playerInfo.mediaItemTransitionReason));
currentMediaItem, finalPlayerInfo.mediaItemTransitionReason));
}
if (!Util.areEqual(oldPlayerInfo.currentTracks, playerInfo.currentTracks)) {
if (!Util.areEqual(oldPlayerInfo.currentTracks, finalPlayerInfo.currentTracks)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_TRACKS_CHANGED,
listener -> listener.onTracksChanged(playerInfo.currentTracks));
listener -> listener.onTracksChanged(finalPlayerInfo.currentTracks));
}
if (!Util.areEqual(oldPlayerInfo.playbackParameters, playerInfo.playbackParameters)) {
if (!Util.areEqual(oldPlayerInfo.playbackParameters, finalPlayerInfo.playbackParameters)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_PLAYBACK_PARAMETERS_CHANGED,
listener -> listener.onPlaybackParametersChanged(playerInfo.playbackParameters));
listener -> listener.onPlaybackParametersChanged(finalPlayerInfo.playbackParameters));
}
if (oldPlayerInfo.repeatMode != playerInfo.repeatMode) {
if (oldPlayerInfo.repeatMode != finalPlayerInfo.repeatMode) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_REPEAT_MODE_CHANGED,
listener -> listener.onRepeatModeChanged(playerInfo.repeatMode));
listener -> listener.onRepeatModeChanged(finalPlayerInfo.repeatMode));
}
if (oldPlayerInfo.shuffleModeEnabled != playerInfo.shuffleModeEnabled) {
if (oldPlayerInfo.shuffleModeEnabled != finalPlayerInfo.shuffleModeEnabled) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_SHUFFLE_MODE_ENABLED_CHANGED,
listener -> listener.onShuffleModeEnabledChanged(playerInfo.shuffleModeEnabled));
listener -> listener.onShuffleModeEnabledChanged(finalPlayerInfo.shuffleModeEnabled));
}
if (!isTimelineExcluded && !Util.areEqual(oldPlayerInfo.timeline, playerInfo.timeline)) {
if (!isTimelineExcluded && !Util.areEqual(oldPlayerInfo.timeline, finalPlayerInfo.timeline)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_TIMELINE_CHANGED,
listener -> listener.onTimelineChanged(playerInfo.timeline, timelineChangedReason));
listener ->
listener.onTimelineChanged(
finalPlayerInfo.timeline, Player.TIMELINE_CHANGE_REASON_SOURCE_UPDATE));
}
if (!Util.areEqual(oldPlayerInfo.playlistMetadata, playerInfo.playlistMetadata)) {
if (!Util.areEqual(oldPlayerInfo.playlistMetadata, finalPlayerInfo.playlistMetadata)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_PLAYLIST_METADATA_CHANGED,
listener -> listener.onPlaylistMetadataChanged(playerInfo.playlistMetadata));
listener -> listener.onPlaylistMetadataChanged(finalPlayerInfo.playlistMetadata));
}
if (oldPlayerInfo.volume != playerInfo.volume) {
if (oldPlayerInfo.volume != finalPlayerInfo.volume) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_VOLUME_CHANGED,
listener -> listener.onVolumeChanged(playerInfo.volume));
listener -> listener.onVolumeChanged(finalPlayerInfo.volume));
}
if (!Util.areEqual(oldPlayerInfo.audioAttributes, playerInfo.audioAttributes)) {
if (!Util.areEqual(oldPlayerInfo.audioAttributes, finalPlayerInfo.audioAttributes)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_AUDIO_ATTRIBUTES_CHANGED,
listener -> listener.onAudioAttributesChanged(playerInfo.audioAttributes));
listener -> listener.onAudioAttributesChanged(finalPlayerInfo.audioAttributes));
}
if (!oldPlayerInfo.cueGroup.cues.equals(playerInfo.cueGroup.cues)) {
if (!oldPlayerInfo.cueGroup.cues.equals(finalPlayerInfo.cueGroup.cues)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_CUES,
listener -> listener.onCues(playerInfo.cueGroup.cues));
listener -> listener.onCues(finalPlayerInfo.cueGroup.cues));
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_CUES, listener -> listener.onCues(playerInfo.cueGroup));
/* eventFlag= */ Player.EVENT_CUES,
listener -> listener.onCues(finalPlayerInfo.cueGroup));
}
if (!Util.areEqual(oldPlayerInfo.deviceInfo, playerInfo.deviceInfo)) {
if (!Util.areEqual(oldPlayerInfo.deviceInfo, finalPlayerInfo.deviceInfo)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_DEVICE_INFO_CHANGED,
listener -> listener.onDeviceInfoChanged(playerInfo.deviceInfo));
listener -> listener.onDeviceInfoChanged(finalPlayerInfo.deviceInfo));
}
if (oldPlayerInfo.deviceVolume != playerInfo.deviceVolume
|| oldPlayerInfo.deviceMuted != playerInfo.deviceMuted) {
if (oldPlayerInfo.deviceVolume != finalPlayerInfo.deviceVolume
|| oldPlayerInfo.deviceMuted != finalPlayerInfo.deviceMuted) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_DEVICE_VOLUME_CHANGED,
listener ->
listener.onDeviceVolumeChanged(playerInfo.deviceVolume, playerInfo.deviceMuted));
listener.onDeviceVolumeChanged(
finalPlayerInfo.deviceVolume, finalPlayerInfo.deviceMuted));
}
if (oldPlayerInfo.playWhenReady != playerInfo.playWhenReady) {
if (oldPlayerInfo.playWhenReady != finalPlayerInfo.playWhenReady) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_PLAY_WHEN_READY_CHANGED,
listener ->
listener.onPlayWhenReadyChanged(
playerInfo.playWhenReady, playerInfo.playWhenReadyChangedReason));
finalPlayerInfo.playWhenReady, finalPlayerInfo.playWhenReadyChangedReason));
}
if (oldPlayerInfo.playbackSuppressionReason != playerInfo.playbackSuppressionReason) {
if (oldPlayerInfo.playbackSuppressionReason != finalPlayerInfo.playbackSuppressionReason) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_PLAYBACK_SUPPRESSION_REASON_CHANGED,
listener ->
listener.onPlaybackSuppressionReasonChanged(playerInfo.playbackSuppressionReason));
listener.onPlaybackSuppressionReasonChanged(
finalPlayerInfo.playbackSuppressionReason));
}
if (oldPlayerInfo.playbackState != playerInfo.playbackState) {
if (oldPlayerInfo.playbackState != finalPlayerInfo.playbackState) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_PLAYBACK_STATE_CHANGED,
listener -> listener.onPlaybackStateChanged(playerInfo.playbackState));
listener -> listener.onPlaybackStateChanged(finalPlayerInfo.playbackState));
}
if (oldPlayerInfo.isPlaying != playerInfo.isPlaying) {
if (oldPlayerInfo.isPlaying != finalPlayerInfo.isPlaying) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_IS_PLAYING_CHANGED,
listener -> listener.onIsPlayingChanged(playerInfo.isPlaying));
listener -> listener.onIsPlayingChanged(finalPlayerInfo.isPlaying));
}
if (oldPlayerInfo.isLoading != playerInfo.isLoading) {
if (oldPlayerInfo.isLoading != finalPlayerInfo.isLoading) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_IS_LOADING_CHANGED,
listener -> listener.onIsLoadingChanged(playerInfo.isLoading));
listener -> listener.onIsLoadingChanged(finalPlayerInfo.isLoading));
}
if (!Util.areEqual(oldPlayerInfo.videoSize, playerInfo.videoSize)) {
if (!Util.areEqual(oldPlayerInfo.videoSize, finalPlayerInfo.videoSize)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_VIDEO_SIZE_CHANGED,
listener -> listener.onVideoSizeChanged(playerInfo.videoSize));
listener -> listener.onVideoSizeChanged(finalPlayerInfo.videoSize));
}
if (!Util.areEqual(oldPlayerInfo.oldPositionInfo, playerInfo.oldPositionInfo)
|| !Util.areEqual(oldPlayerInfo.newPositionInfo, playerInfo.newPositionInfo)) {
if (!Util.areEqual(oldPlayerInfo.oldPositionInfo, finalPlayerInfo.oldPositionInfo)
|| !Util.areEqual(oldPlayerInfo.newPositionInfo, finalPlayerInfo.newPositionInfo)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_POSITION_DISCONTINUITY,
listener ->
listener.onPositionDiscontinuity(
playerInfo.oldPositionInfo,
playerInfo.newPositionInfo,
playerInfo.discontinuityReason));
finalPlayerInfo.oldPositionInfo,
finalPlayerInfo.newPositionInfo,
finalPlayerInfo.discontinuityReason));
}
if (!Util.areEqual(oldPlayerInfo.mediaMetadata, playerInfo.mediaMetadata)) {
if (!Util.areEqual(oldPlayerInfo.mediaMetadata, finalPlayerInfo.mediaMetadata)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_MEDIA_METADATA_CHANGED,
listener -> listener.onMediaMetadataChanged(playerInfo.mediaMetadata));
listener -> listener.onMediaMetadataChanged(finalPlayerInfo.mediaMetadata));
}
if (oldPlayerInfo.seekBackIncrementMs != playerInfo.seekBackIncrementMs) {
if (oldPlayerInfo.seekBackIncrementMs != finalPlayerInfo.seekBackIncrementMs) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_SEEK_BACK_INCREMENT_CHANGED,
listener -> listener.onSeekBackIncrementChanged(playerInfo.seekBackIncrementMs));
listener -> listener.onSeekBackIncrementChanged(finalPlayerInfo.seekBackIncrementMs));
}
if (oldPlayerInfo.seekForwardIncrementMs != playerInfo.seekForwardIncrementMs) {
if (oldPlayerInfo.seekForwardIncrementMs != finalPlayerInfo.seekForwardIncrementMs) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_SEEK_FORWARD_INCREMENT_CHANGED,
listener -> listener.onSeekForwardIncrementChanged(playerInfo.seekForwardIncrementMs));
listener ->
listener.onSeekForwardIncrementChanged(finalPlayerInfo.seekForwardIncrementMs));
}
if (oldPlayerInfo.maxSeekToPreviousPositionMs != newPlayerInfo.maxSeekToPreviousPositionMs) {
if (oldPlayerInfo.maxSeekToPreviousPositionMs != finalPlayerInfo.maxSeekToPreviousPositionMs) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_MAX_SEEK_TO_PREVIOUS_POSITION_CHANGED,
listener ->
listener.onMaxSeekToPreviousPositionChanged(
newPlayerInfo.maxSeekToPreviousPositionMs));
finalPlayerInfo.maxSeekToPreviousPositionMs));
}
if (!Util.areEqual(
oldPlayerInfo.trackSelectionParameters, newPlayerInfo.trackSelectionParameters)) {
oldPlayerInfo.trackSelectionParameters, finalPlayerInfo.trackSelectionParameters)) {
listeners.queueEvent(
/* eventFlag= */ Player.EVENT_TRACK_SELECTION_PARAMETERS_CHANGED,
listener ->
listener.onTrackSelectionParametersChanged(newPlayerInfo.trackSelectionParameters));
listener.onTrackSelectionParametersChanged(finalPlayerInfo.trackSelectionParameters));
}
listeners.flushEvents();
}
Expand Down
7 changes: 1 addition & 6 deletions libraries/session/src/main/java/androidx/media3/session/MediaControllerStub.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import android.os.Handler;
import android.text.TextUtils;
import androidx.annotation.Nullable;
import androidx.media3.common.Player;
import androidx.media3.common.Player.Commands;
import androidx.media3.common.util.BundleableUtil;
import androidx.media3.common.util.Log;
Expand Down Expand Up @@ -180,11 +179,7 @@ public void onPlayerInfoChanged(int seq, Bundle playerInfoBundle, boolean isTime
return;
}
dispatchControllerTaskOnHandler(
controller ->
controller.onPlayerInfoChanged(
playerInfo,
/* timelineChangedReason= */ Player.TIMELINE_CHANGE_REASON_SOURCE_UPDATE,
isTimelineExcluded));
controller -> controller.onPlayerInfoChanged(playerInfo, isTimelineExcluded));
}

@Override
Expand Down
38 changes: 38 additions & 0 deletions ...ion_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3300,6 +3300,44 @@ public void onEvents(Player player, Player.Events events) {
assertThat(getEventsAsList(eventsRef.get())).containsExactly(Player.EVENT_RENDERED_FIRST_FRAME);
}

@Test
public void recursiveChangesFromListeners_reportConsistentValuesForAllListeners()
throws Exception {
// We add two listeners to the controller. The first stops the player as soon as it's ready and
// both record the state change events they receive.
MediaController controller = controllerTestRule.createController(remoteSession.getToken());
List<Integer> listener1States = new ArrayList<>();
List<Integer> listener2States = new ArrayList<>();
CountDownLatch latch = new CountDownLatch(4);
Player.Listener listener1 =
new Player.Listener() {
@Override
public void onPlaybackStateChanged(@Player.State int playbackState) {
listener1States.add(playbackState);
if (playbackState == Player.STATE_READY) {
controller.stop();
}
latch.countDown();
}
};
Player.Listener listener2 =
new Player.Listener() {
@Override
public void onPlaybackStateChanged(@Player.State int playbackState) {
listener2States.add(playbackState);
latch.countDown();
}
};
controller.addListener(listener1);
controller.addListener(listener2);

remoteSession.getMockPlayer().notifyPlaybackStateChanged(Player.STATE_READY);

assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue();
assertThat(listener1States).containsExactly(Player.STATE_READY, Player.STATE_IDLE).inOrder();
assertThat(listener2States).containsExactly(Player.STATE_READY, Player.STATE_IDLE).inOrder();
}

private void testControllerAfterSessionIsClosed(String id) throws Exception {
MediaController controller = controllerTestRule.createController(remoteSession.getToken());
// This causes the session service to die.
Expand Down

0 comments on commit 7f17ff6

Please sign in to comment.