From 3b277b281051be7b1671bd5f0d07176081a9b533 Mon Sep 17 00:00:00 2001
From: tonihei <tonihei@google.com>
Date: Wed, 16 Dec 2020 14:16:18 +0000
Subject: [PATCH] Fix unintentional new sessions after player release in stats
 listener.

PlaybackStatsListener has a method whose original intention was to be
called when the player is releaed to finish all pending sessions.
However, this also meant that later events (e.g. onVideoDecoderDisabled)
could create new sessions because the old one was already finished.

Use the new onPlayerReleased callback to implement this properly and to
fix the unintentional new session creation.

PiperOrigin-RevId: 347809527
---
 .../analytics/PlaybackStatsListener.java      | 25 +++----------------
 .../analytics/PlaybackStatsListenerTest.java  | 24 +++---------------
 2 files changed, 6 insertions(+), 43 deletions(-)

diff --git a/library/core/src/main/java/com/google/android/exoplayer2/analytics/PlaybackStatsListener.java b/library/core/src/main/java/com/google/android/exoplayer2/analytics/PlaybackStatsListener.java
index 8d3e335551..d8f4074c68 100644
--- a/library/core/src/main/java/com/google/android/exoplayer2/analytics/PlaybackStatsListener.java
+++ b/library/core/src/main/java/com/google/android/exoplayer2/analytics/PlaybackStatsListener.java
@@ -147,28 +147,6 @@ public final class PlaybackStatsListener
     return activeStatsTracker == null ? null : activeStatsTracker.build(/* isFinal= */ false);
   }
 
-  /**
-   * Finishes all pending playback sessions. Should be called when the listener is removed from the
-   * player or when the player is released.
-   */
-  public void finishAllSessions() {
-    // TODO: Add AnalyticsListener.onAttachedToPlayer and onDetachedFromPlayer to auto-release with
-    // an actual EventTime. Should also simplify other cases where the listener needs to be released
-    // separately from the player.
-    sessionManager.finishAllSessions(
-        new EventTime(
-            SystemClock.elapsedRealtime(),
-            Timeline.EMPTY,
-            /* windowIndex= */ 0,
-            /* mediaPeriodId= */ null,
-            /* eventPlaybackPositionMs= */ 0,
-            Timeline.EMPTY,
-            /* currentWindowIndex= */ 0,
-            /* currentMediaPeriodId= */ null,
-            /* currentPlaybackPositionMs= */ 0,
-            /* totalBufferedDurationMs= */ 0));
-  }
-
   // PlaybackSessionManager.Listener implementation.
 
   @Override
@@ -309,6 +287,9 @@ public final class PlaybackStatsListener
     onSeekStartedEventTime = null;
     videoFormat = null;
     audioFormat = null;
+    if (events.contains(AnalyticsListener.EVENT_PLAYER_RELEASED)) {
+      sessionManager.finishAllSessions(events.getEventTime(EVENT_PLAYER_RELEASED));
+    }
   }
 
   private void maybeAddSessions(Player player, Events events) {
diff --git a/library/core/src/test/java/com/google/android/exoplayer2/analytics/PlaybackStatsListenerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/analytics/PlaybackStatsListenerTest.java
index c4118abcc6..c736444a43 100644
--- a/library/core/src/test/java/com/google/android/exoplayer2/analytics/PlaybackStatsListenerTest.java
+++ b/library/core/src/test/java/com/google/android/exoplayer2/analytics/PlaybackStatsListenerTest.java
@@ -152,7 +152,7 @@ public final class PlaybackStatsListenerTest {
   }
 
   @Test
-  public void finishAllSessions_callsAllPendingCallbacks() throws Exception {
+  public void playerRelease_callsAllPendingCallbacks() throws Exception {
     PlaybackStatsListener.Callback callback = mock(PlaybackStatsListener.Callback.class);
     PlaybackStatsListener playbackStatsListener =
         new PlaybackStatsListener(/* keepHistory= */ true, callback);
@@ -167,7 +167,8 @@ public final class PlaybackStatsListenerTest {
     TestPlayerRunHelper.playUntilPosition(
         player, /* windowIndex= */ 0, /* positionMs= */ player.getDuration());
     runMainLooperToNextTask();
-    playbackStatsListener.finishAllSessions();
+    player.release();
+    runMainLooperToNextTask();
 
     ArgumentCaptor<AnalyticsListener.EventTime> eventTimeCaptor =
         ArgumentCaptor.forClass(AnalyticsListener.EventTime.class);
@@ -178,23 +179,4 @@ public final class PlaybackStatsListenerTest {
                 .collect(Collectors.toList()))
         .containsExactly(0, 1);
   }
-
-  @Test
-  public void finishAllSessions_doesNotCallCallbackAgainWhenSessionWouldBeAutomaticallyFinished() {
-    PlaybackStatsListener.Callback callback = mock(PlaybackStatsListener.Callback.class);
-    PlaybackStatsListener playbackStatsListener =
-        new PlaybackStatsListener(/* keepHistory= */ true, callback);
-    player.addAnalyticsListener(playbackStatsListener);
-
-    player.setMediaItem(MediaItem.fromUri("http://test.org"));
-    runMainLooperToNextTask();
-    playbackStatsListener.finishAllSessions();
-
-    // Simulate removing the playback item to ensure the session would finish if it hadn't already.
-    player.clearMediaItems();
-    runMainLooperToNextTask();
-
-    // Verify the callback was called once only.
-    verify(callback).onPlaybackStatsReady(any(), any());
-  }
 }