From bf5b52e288059f7a7ef37a4c6f0c4092835d3db1 Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 24 Apr 2020 16:02:35 +0100 Subject: [PATCH] Make sure finishAllSessions() can be called without removing listener Currently, this method is only supposed to be called before removing the listener from the player or when releasing the player. If called at other times, it will throw an exception later when a playback session is ended automatically. issue:#7193 PiperOrigin-RevId: 308254993 --- .../DefaultPlaybackSessionManager.java | 14 ++++ .../analytics/PlaybackSessionManager.java | 8 ++ .../analytics/PlaybackStatsListener.java | 5 +- .../DefaultPlaybackSessionManagerTest.java | 26 +++++++ .../analytics/PlaybackStatsListenerTest.java | 73 ++++++++++++++++++- 5 files changed, 121 insertions(+), 5 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/analytics/DefaultPlaybackSessionManager.java b/library/core/src/main/java/com/google/android/exoplayer2/analytics/DefaultPlaybackSessionManager.java index e9baef37ba..04536bb6c1 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/analytics/DefaultPlaybackSessionManager.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/analytics/DefaultPlaybackSessionManager.java @@ -202,6 +202,20 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag } } + @Override + public void finishAllSessions(EventTime eventTime) { + currentSessionId = null; + Iterator iterator = sessions.values().iterator(); + while (iterator.hasNext()) { + SessionDescriptor session = iterator.next(); + iterator.remove(); + if (session.isCreated && listener != null) { + listener.onSessionFinished( + eventTime, session.sessionId, /* automaticTransitionToNextPlayback= */ false); + } + } + } + private SessionDescriptor getOrAddSession( int windowIndex, @Nullable MediaPeriodId mediaPeriodId) { // There should only be one matching session if mediaPeriodId is non-null. If mediaPeriodId is diff --git a/library/core/src/main/java/com/google/android/exoplayer2/analytics/PlaybackSessionManager.java b/library/core/src/main/java/com/google/android/exoplayer2/analytics/PlaybackSessionManager.java index 53d63e23fc..7045779125 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/analytics/PlaybackSessionManager.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/analytics/PlaybackSessionManager.java @@ -117,4 +117,12 @@ public interface PlaybackSessionManager { * @param reason The {@link DiscontinuityReason}. */ void handlePositionDiscontinuity(EventTime eventTime, @DiscontinuityReason int reason); + + /** + * Finishes all existing sessions and calls their respective {@link + * Listener#onSessionFinished(EventTime, String, boolean)} callback. + * + * @param eventTime The event time at which sessions are finished. + */ + void finishAllSessions(EventTime eventTime); } 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 6c086afc92..0524f4d3b1 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 @@ -150,7 +150,6 @@ public final class PlaybackStatsListener // 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. - HashMap trackerCopy = new HashMap<>(playbackStatsTrackers); EventTime dummyEventTime = new EventTime( SystemClock.elapsedRealtime(), @@ -160,9 +159,7 @@ public final class PlaybackStatsListener /* eventPlaybackPositionMs= */ 0, /* currentPlaybackPositionMs= */ 0, /* totalBufferedDurationMs= */ 0); - for (String session : trackerCopy.keySet()) { - onSessionFinished(dummyEventTime, session, /* automaticTransition= */ false); - } + sessionManager.finishAllSessions(dummyEventTime); } // PlaybackSessionManager.Listener implementation. diff --git a/library/core/src/test/java/com/google/android/exoplayer2/analytics/DefaultPlaybackSessionManagerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/analytics/DefaultPlaybackSessionManagerTest.java index d0a9a496f1..b24135152e 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/analytics/DefaultPlaybackSessionManagerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/analytics/DefaultPlaybackSessionManagerTest.java @@ -21,6 +21,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -1058,6 +1059,31 @@ public final class DefaultPlaybackSessionManagerTest { verify(mockListener, never()).onSessionActive(any(), eq(adSessionId2)); } + @Test + public void finishAllSessions_callsOnSessionFinishedForAllCreatedSessions() { + Timeline timeline = new FakeTimeline(/* windowCount= */ 4); + EventTime eventTimeWindow0 = + createEventTime(timeline, /* windowIndex= */ 0, /* mediaPeriodId= */ null); + EventTime eventTimeWindow2 = + createEventTime(timeline, /* windowIndex= */ 2, /* mediaPeriodId= */ null); + // Actually create sessions for window 0 and 2. + sessionManager.updateSessions(eventTimeWindow0); + sessionManager.updateSessions(eventTimeWindow2); + // Query information about session for window 1, but don't create it. + sessionManager.getSessionForMediaPeriodId( + timeline, + new MediaPeriodId( + timeline.getPeriod(/* periodIndex= */ 1, new Timeline.Period(), /* setIds= */ true).uid, + /* windowSequenceNumber= */ 123)); + verify(mockListener, times(2)).onSessionCreated(any(), anyString()); + + EventTime finishEventTime = + createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId= */ null); + sessionManager.finishAllSessions(finishEventTime); + + verify(mockListener, times(2)).onSessionFinished(eq(finishEventTime), anyString(), eq(false)); + } + private static EventTime createEventTime( Timeline timeline, int windowIndex, @Nullable MediaPeriodId mediaPeriodId) { return new EventTime( 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 695e1f4557..c6d4a597ed 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 @@ -16,7 +16,14 @@ package com.google.android.exoplayer2.analytics; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import android.os.SystemClock; import androidx.annotation.Nullable; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.exoplayer2.Player; @@ -42,7 +49,7 @@ public final class PlaybackStatsListenerTest { private static final Timeline TEST_TIMELINE = new FakeTimeline(/* windowCount= */ 1); private static final AnalyticsListener.EventTime TEST_EVENT_TIME = new AnalyticsListener.EventTime( - /* realtimeMs= */ 700, + /* realtimeMs= */ 500, TEST_TIMELINE, /* windowIndex= */ 0, new MediaSource.MediaPeriodId( @@ -119,4 +126,68 @@ public final class PlaybackStatsListenerTest { assertThat(playbackStats).isNotNull(); assertThat(playbackStats.endedCount).isEqualTo(1); } + + @Test + public void finishedSession_callsCallback() { + PlaybackStatsListener.Callback callback = mock(PlaybackStatsListener.Callback.class); + PlaybackStatsListener playbackStatsListener = + new PlaybackStatsListener(/* keepHistory= */ true, callback); + + // Create session with an event and finish it by simulating removal from playlist. + playbackStatsListener.onPlaybackStateChanged(TEST_EVENT_TIME, Player.STATE_BUFFERING); + verify(callback, never()).onPlaybackStatsReady(any(), any()); + playbackStatsListener.onTimelineChanged( + EMPTY_TIMELINE_EVENT_TIME, Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED); + + verify(callback).onPlaybackStatsReady(eq(TEST_EVENT_TIME), any()); + } + + @Test + public void finishAllSessions_callsAllPendingCallbacks() { + AnalyticsListener.EventTime eventTimeWindow0 = + new AnalyticsListener.EventTime( + /* realtimeMs= */ 0, + Timeline.EMPTY, + /* windowIndex= */ 0, + /* mediaPeriodId= */ null, + /* eventPlaybackPositionMs= */ 0, + /* currentPlaybackPositionMs= */ 0, + /* totalBufferedDurationMs= */ 0); + AnalyticsListener.EventTime eventTimeWindow1 = + new AnalyticsListener.EventTime( + /* realtimeMs= */ 0, + Timeline.EMPTY, + /* windowIndex= */ 1, + /* mediaPeriodId= */ null, + /* eventPlaybackPositionMs= */ 0, + /* currentPlaybackPositionMs= */ 0, + /* totalBufferedDurationMs= */ 0); + PlaybackStatsListener.Callback callback = mock(PlaybackStatsListener.Callback.class); + PlaybackStatsListener playbackStatsListener = + new PlaybackStatsListener(/* keepHistory= */ true, callback); + playbackStatsListener.onPlaybackStateChanged(eventTimeWindow0, Player.STATE_BUFFERING); + playbackStatsListener.onPlaybackStateChanged(eventTimeWindow1, Player.STATE_BUFFERING); + + playbackStatsListener.finishAllSessions(); + + verify(callback, times(2)).onPlaybackStatsReady(any(), any()); + verify(callback).onPlaybackStatsReady(eq(eventTimeWindow0), any()); + verify(callback).onPlaybackStatsReady(eq(eventTimeWindow1), any()); + } + + @Test + public void finishAllSessions_doesNotCallCallbackAgainWhenSessionWouldBeAutomaticallyFinished() { + PlaybackStatsListener.Callback callback = mock(PlaybackStatsListener.Callback.class); + PlaybackStatsListener playbackStatsListener = + new PlaybackStatsListener(/* keepHistory= */ true, callback); + playbackStatsListener.onPlaybackStateChanged(TEST_EVENT_TIME, Player.STATE_BUFFERING); + SystemClock.setCurrentTimeMillis(TEST_EVENT_TIME.realtimeMs + 100); + + playbackStatsListener.finishAllSessions(); + // Simulate removing the playback item to ensure the session would finish if it hadn't already. + playbackStatsListener.onTimelineChanged( + EMPTY_TIMELINE_EVENT_TIME, Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED); + + verify(callback).onPlaybackStatsReady(any(), any()); + } }