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

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

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<Player.EventListener> listeners;
   private final Timeline.Window window;
   private final Timeline.Period period;
+  private final ArrayDeque<PlaybackInfoUpdate> 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;
+      updatePlaybackInfo(
-      for (Player.EventListener listener : listeners) {
+          playbackInfo,
-        listener.onPlayerStateChanged(playWhenReady, playbackInfo.playbackState);
+          /* 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,19 +653,94 @@ 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 seekProcessed,
-    boolean timelineOrManifestChanged =
+      boolean playWhenReadyChanged) {
-        playbackInfo.timeline != newPlaybackInfo.timeline
+    boolean isRunningRecursiveListenerNotification = !pendingPlaybackInfoUpdates.isEmpty();
-            || playbackInfo.manifest != newPlaybackInfo.manifest;
+    pendingPlaybackInfoUpdates.addLast(
-    boolean playbackStateChanged = playbackInfo.playbackState != newPlaybackInfo.playbackState;
+        new PlaybackInfoUpdate(
-    boolean isLoadingChanged = playbackInfo.isLoading != newPlaybackInfo.isLoading;
+            playbackInfo,
-    boolean trackSelectorResultChanged =
+            /* previousPlaybackInfo= */ this.playbackInfo,
-        playbackInfo.trackSelectorResult != newPlaybackInfo.trackSelectorResult;
+            listeners,
-    playbackInfo = newPlaybackInfo;
+            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;
+    }
+    while (!pendingPlaybackInfoUpdates.isEmpty()) {
+      pendingPlaybackInfoUpdates.peekFirst().notifyListeners();
+      pendingPlaybackInfoUpdates.removeFirst();
+    }
+  }
+
+  private long playbackInfoPositionUsToWindowPositionMs(long positionUs) {
+    long positionMs = C.usToMs(positionUs);
+    if (!playbackInfo.periodId.isAd()) {
+      playbackInfo.timeline.getPeriod(playbackInfo.periodId.periodIndex, period);
+      positionMs += period.getPositionInWindowMs();
+    }
+    return positionMs;
+  }
+
+  private boolean shouldMaskPosition() {
+    return playbackInfo.timeline.isEmpty() || pendingOperationAcks > 0;
+  }
+
+  private static final class PlaybackInfoUpdate {
+
+    private final PlaybackInfo playbackInfo;
+    private final Set<Player.EventListener> 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<Player.EventListener> 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(
@@ -679,7 +764,7 @@ import java.util.concurrent.CopyOnWriteArraySet;
           listener.onLoadingChanged(playbackInfo.isLoading);
         }
       }
-    if (playbackStateChanged) {
+      if (playbackStateOrPlayWhenReadyChanged) {
         for (Player.EventListener listener : listeners) {
           listener.onPlayerStateChanged(playWhenReady, playbackInfo.playbackState);
         }
@@ -690,17 +775,5 @@ import java.util.concurrent.CopyOnWriteArraySet;
         }
       }
     }
-
-  private long playbackInfoPositionUsToWindowPositionMs(long positionUs) {
-    long positionMs = C.usToMs(positionUs);
-    if (!playbackInfo.periodId.isAd()) {
-      playbackInfo.timeline.getPeriod(playbackInfo.periodId.periodIndex, period);
-      positionMs += period.getPositionInWindowMs();
-    }
-    return positionMs;
-  }
-
-  private boolean shouldMaskPosition() {
-    return playbackInfo.timeline.isEmpty() || pendingOperationAcks > 0;
   }
 }

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<Player> playerReference = new AtomicReference<>();
+    final List<Integer> eventListener1States = new ArrayList<>();
+    final List<Integer> 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<Player> playerReference = new AtomicReference<>();
+    final List<Boolean> eventListenerPlayWhenReady = new ArrayList<>();
+    final List<Integer> 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) {