From 0912fd1d74c413918f4feaf5c1e504e458bbba4c Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 25 May 2018 04:02:53 -0700 Subject: [PATCH] Serialize recursive listener notifications. When the player state is changed from an event listener callback, we may get recursive listener notifications. These recursions can produce a wrong order, skip or duplicate updates, and send different notifications to different listeners. This change serializes listener notifications by clustering all update data in a helper data class and adding the updates to a queue which can be handled in a loop on the outer layer of the recursion. As playWhenReady updates also reference the current playbackInfo, we need to redirect the listener notifcations for setPlayWhenReady to the same queue. Issue:#4276 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=198031431 --- RELEASENOTES.md | 3 +- .../android/exoplayer2/ExoPlayerImpl.java | 169 +++++++++++++----- .../android/exoplayer2/ExoPlayerTest.java | 99 ++++++++++ 3 files changed, 222 insertions(+), 49 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 0b17cb5251..670d6299ec 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -2,7 +2,8 @@ ### dev-v2 (not yet released) ### -* Coming soon +* Fix inconsistent `Player.EventListener` invocations for recursive player state + changes ([#4276](https://github.com/google/ExoPlayer/issues/4276)). ### 2.8.1 ### diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java index 4125a203a6..9a9577c50a 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java @@ -33,8 +33,10 @@ import com.google.android.exoplayer2.trackselection.TrackSelectorResult; import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.Clock; import com.google.android.exoplayer2.util.Util; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.List; +import java.util.Set; import java.util.concurrent.CopyOnWriteArraySet; /** @@ -53,6 +55,7 @@ import java.util.concurrent.CopyOnWriteArraySet; private final CopyOnWriteArraySet listeners; private final Timeline.Window window; private final Timeline.Period period; + private final ArrayDeque pendingPlaybackInfoUpdates; private boolean playWhenReady; private @RepeatMode int repeatMode; @@ -112,6 +115,7 @@ import java.util.concurrent.CopyOnWriteArraySet; /* startPositionUs= */ 0, TrackGroupArray.EMPTY, emptyTrackSelectorResult); + pendingPlaybackInfoUpdates = new ArrayDeque<>(); internalPlayer = new ExoPlayerImplInternal( renderers, @@ -185,7 +189,8 @@ import java.util.concurrent.CopyOnWriteArraySet; /* positionDiscontinuity= */ false, /* ignored */ DISCONTINUITY_REASON_INTERNAL, TIMELINE_CHANGE_REASON_RESET, - /* seekProcessed= */ false); + /* seekProcessed= */ false, + /* playWhenReadyChanged= */ false); } @Override @@ -193,10 +198,13 @@ import java.util.concurrent.CopyOnWriteArraySet; if (this.playWhenReady != playWhenReady) { this.playWhenReady = playWhenReady; internalPlayer.setPlayWhenReady(playWhenReady); - PlaybackInfo playbackInfo = this.playbackInfo; - for (Player.EventListener listener : listeners) { - listener.onPlayerStateChanged(playWhenReady, playbackInfo.playbackState); - } + updatePlaybackInfo( + playbackInfo, + /* positionDiscontinuity= */ false, + /* ignored */ DISCONTINUITY_REASON_INTERNAL, + /* ignored */ TIMELINE_CHANGE_REASON_RESET, + /* seekProcessed= */ false, + /* playWhenReadyChanged= */ true); } } @@ -352,7 +360,8 @@ import java.util.concurrent.CopyOnWriteArraySet; /* positionDiscontinuity= */ false, /* ignored */ DISCONTINUITY_REASON_INTERNAL, TIMELINE_CHANGE_REASON_RESET, - /* seekProcessed= */ false); + /* seekProcessed= */ false, + /* playWhenReadyChanged= */ false); } @Override @@ -615,7 +624,8 @@ import java.util.concurrent.CopyOnWriteArraySet; positionDiscontinuity, positionDiscontinuityReason, timelineChangeReason, - seekProcessed); + seekProcessed, + /* playWhenReadyChanged= */ false); } } @@ -643,51 +653,33 @@ import java.util.concurrent.CopyOnWriteArraySet; } private void updatePlaybackInfo( - PlaybackInfo newPlaybackInfo, + PlaybackInfo playbackInfo, boolean positionDiscontinuity, @Player.DiscontinuityReason int positionDiscontinuityReason, @Player.TimelineChangeReason int timelineChangeReason, - boolean seekProcessed) { - boolean timelineOrManifestChanged = - playbackInfo.timeline != newPlaybackInfo.timeline - || playbackInfo.manifest != newPlaybackInfo.manifest; - boolean playbackStateChanged = playbackInfo.playbackState != newPlaybackInfo.playbackState; - boolean isLoadingChanged = playbackInfo.isLoading != newPlaybackInfo.isLoading; - boolean trackSelectorResultChanged = - playbackInfo.trackSelectorResult != newPlaybackInfo.trackSelectorResult; - playbackInfo = newPlaybackInfo; - if (timelineOrManifestChanged || timelineChangeReason == TIMELINE_CHANGE_REASON_PREPARED) { - for (Player.EventListener listener : listeners) { - listener.onTimelineChanged( - playbackInfo.timeline, playbackInfo.manifest, timelineChangeReason); - } + boolean seekProcessed, + boolean playWhenReadyChanged) { + boolean isRunningRecursiveListenerNotification = !pendingPlaybackInfoUpdates.isEmpty(); + pendingPlaybackInfoUpdates.addLast( + new PlaybackInfoUpdate( + playbackInfo, + /* previousPlaybackInfo= */ this.playbackInfo, + listeners, + trackSelector, + positionDiscontinuity, + positionDiscontinuityReason, + timelineChangeReason, + seekProcessed, + playWhenReady, + playWhenReadyChanged)); + // Assign playback info immediately such that all getters return the right values. + this.playbackInfo = playbackInfo; + if (isRunningRecursiveListenerNotification) { + return; } - if (positionDiscontinuity) { - for (Player.EventListener listener : listeners) { - listener.onPositionDiscontinuity(positionDiscontinuityReason); - } - } - if (trackSelectorResultChanged) { - trackSelector.onSelectionActivated(playbackInfo.trackSelectorResult.info); - for (Player.EventListener listener : listeners) { - listener.onTracksChanged( - playbackInfo.trackGroups, playbackInfo.trackSelectorResult.selections); - } - } - if (isLoadingChanged) { - for (Player.EventListener listener : listeners) { - listener.onLoadingChanged(playbackInfo.isLoading); - } - } - if (playbackStateChanged) { - for (Player.EventListener listener : listeners) { - listener.onPlayerStateChanged(playWhenReady, playbackInfo.playbackState); - } - } - if (seekProcessed) { - for (Player.EventListener listener : listeners) { - listener.onSeekProcessed(); - } + while (!pendingPlaybackInfoUpdates.isEmpty()) { + pendingPlaybackInfoUpdates.peekFirst().notifyListeners(); + pendingPlaybackInfoUpdates.removeFirst(); } } @@ -703,4 +695,85 @@ import java.util.concurrent.CopyOnWriteArraySet; private boolean shouldMaskPosition() { return playbackInfo.timeline.isEmpty() || pendingOperationAcks > 0; } + + private static final class PlaybackInfoUpdate { + + private final PlaybackInfo playbackInfo; + private final Set listeners; + private final TrackSelector trackSelector; + private final boolean positionDiscontinuity; + private final @Player.DiscontinuityReason int positionDiscontinuityReason; + private final @Player.TimelineChangeReason int timelineChangeReason; + private final boolean seekProcessed; + private final boolean playWhenReady; + private final boolean playbackStateOrPlayWhenReadyChanged; + private final boolean timelineOrManifestChanged; + private final boolean isLoadingChanged; + private final boolean trackSelectorResultChanged; + + public PlaybackInfoUpdate( + PlaybackInfo playbackInfo, + PlaybackInfo previousPlaybackInfo, + Set listeners, + TrackSelector trackSelector, + boolean positionDiscontinuity, + @Player.DiscontinuityReason int positionDiscontinuityReason, + @Player.TimelineChangeReason int timelineChangeReason, + boolean seekProcessed, + boolean playWhenReady, + boolean playWhenReadyChanged) { + this.playbackInfo = playbackInfo; + this.listeners = listeners; + this.trackSelector = trackSelector; + this.positionDiscontinuity = positionDiscontinuity; + this.positionDiscontinuityReason = positionDiscontinuityReason; + this.timelineChangeReason = timelineChangeReason; + this.seekProcessed = seekProcessed; + this.playWhenReady = playWhenReady; + playbackStateOrPlayWhenReadyChanged = + playWhenReadyChanged || previousPlaybackInfo.playbackState != playbackInfo.playbackState; + timelineOrManifestChanged = + previousPlaybackInfo.timeline != playbackInfo.timeline + || previousPlaybackInfo.manifest != playbackInfo.manifest; + isLoadingChanged = previousPlaybackInfo.isLoading != playbackInfo.isLoading; + trackSelectorResultChanged = + previousPlaybackInfo.trackSelectorResult != playbackInfo.trackSelectorResult; + } + + public void notifyListeners() { + if (timelineOrManifestChanged || timelineChangeReason == TIMELINE_CHANGE_REASON_PREPARED) { + for (Player.EventListener listener : listeners) { + listener.onTimelineChanged( + playbackInfo.timeline, playbackInfo.manifest, timelineChangeReason); + } + } + if (positionDiscontinuity) { + for (Player.EventListener listener : listeners) { + listener.onPositionDiscontinuity(positionDiscontinuityReason); + } + } + if (trackSelectorResultChanged) { + trackSelector.onSelectionActivated(playbackInfo.trackSelectorResult.info); + for (Player.EventListener listener : listeners) { + listener.onTracksChanged( + playbackInfo.trackGroups, playbackInfo.trackSelectorResult.selections); + } + } + if (isLoadingChanged) { + for (Player.EventListener listener : listeners) { + listener.onLoadingChanged(playbackInfo.isLoading); + } + } + if (playbackStateOrPlayWhenReadyChanged) { + for (Player.EventListener listener : listeners) { + listener.onPlayerStateChanged(playWhenReady, playbackInfo.playbackState); + } + } + if (seekProcessed) { + for (Player.EventListener listener : listeners) { + listener.onSeekProcessed(); + } + } + } + } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java index 0df854cddb..c05f8914f5 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java @@ -1980,6 +1980,105 @@ public final class ExoPlayerTest { .inOrder(); } + @Test + public void testRecursivePlayerChangesReportConsistentValuesForAllListeners() throws Exception { + // We add two listeners to the player. The first stops the player as soon as it's ready and both + // record the state change events they receive. + final AtomicReference playerReference = new AtomicReference<>(); + final List eventListener1States = new ArrayList<>(); + final List eventListener2States = new ArrayList<>(); + final EventListener eventListener1 = + new DefaultEventListener() { + @Override + public void onPlayerStateChanged(boolean playWhenReady, int playbackState) { + eventListener1States.add(playbackState); + if (playbackState == Player.STATE_READY) { + playerReference.get().stop(/* reset= */ true); + } + } + }; + final EventListener eventListener2 = + new DefaultEventListener() { + @Override + public void onPlayerStateChanged(boolean playWhenReady, int playbackState) { + eventListener2States.add(playbackState); + } + }; + ActionSchedule actionSchedule = + new ActionSchedule.Builder("testRecursivePlayerChanges") + .executeRunnable( + new PlayerRunnable() { + @Override + public void run(SimpleExoPlayer player) { + playerReference.set(player); + player.addListener(eventListener1); + player.addListener(eventListener2); + } + }) + .build(); + new ExoPlayerTestRunner.Builder() + .setActionSchedule(actionSchedule) + .build() + .start() + .blockUntilEnded(TIMEOUT_MS); + + assertThat(eventListener1States) + .containsExactly(Player.STATE_BUFFERING, Player.STATE_READY, Player.STATE_IDLE) + .inOrder(); + assertThat(eventListener2States) + .containsExactly(Player.STATE_BUFFERING, Player.STATE_READY, Player.STATE_IDLE) + .inOrder(); + } + + @Test + public void testRecursivePlayerChangesAreReportedInCorrectOrder() throws Exception { + // The listener stops the player as soon as it's ready (which should report a timeline and state + // change) and sets playWhenReady to false when the timeline callback is received. + final AtomicReference playerReference = new AtomicReference<>(); + final List eventListenerPlayWhenReady = new ArrayList<>(); + final List eventListenerStates = new ArrayList<>(); + final EventListener eventListener = + new DefaultEventListener() { + @Override + public void onTimelineChanged(Timeline timeline, Object manifest, int reason) { + if (timeline.isEmpty()) { + playerReference.get().setPlayWhenReady(/* playWhenReady= */ false); + } + } + + @Override + public void onPlayerStateChanged(boolean playWhenReady, int playbackState) { + eventListenerPlayWhenReady.add(playWhenReady); + eventListenerStates.add(playbackState); + if (playbackState == Player.STATE_READY) { + playerReference.get().stop(/* reset= */ true); + } + } + }; + ActionSchedule actionSchedule = + new ActionSchedule.Builder("testRecursivePlayerChanges") + .executeRunnable( + new PlayerRunnable() { + @Override + public void run(SimpleExoPlayer player) { + playerReference.set(player); + player.addListener(eventListener); + } + }) + .build(); + new ExoPlayerTestRunner.Builder() + .setActionSchedule(actionSchedule) + .build() + .start() + .blockUntilEnded(TIMEOUT_MS); + + assertThat(eventListenerStates) + .containsExactly( + Player.STATE_BUFFERING, Player.STATE_READY, Player.STATE_IDLE, Player.STATE_IDLE) + .inOrder(); + assertThat(eventListenerPlayWhenReady).containsExactly(true, true, true, false).inOrder(); + } + // Internal methods. private static ActionSchedule.Builder addSurfaceSwitch(ActionSchedule.Builder builder) {