From 09096d6fbf5532728ab675d2d1ccefc3daa0bb03 Mon Sep 17 00:00:00 2001 From: olly Date: Tue, 20 Apr 2021 17:05:26 +0100 Subject: [PATCH] Rollback of https://github.com/google/ExoPlayer/commit/e60609e34447553474cdfc86b2dee5462316b10c *** Original commit *** Prevent creation of new sessions if the Timeline is empty. We currently create sessions based on the placeholder window index. This shouldn't be needed as we now set a non-empty timeline as soon as the first MediaItem is added to the playlist. Once this check is part of the session manager, we can also remove the equivalent workarounds from the various code integrations. *** PiperOrigin-RevId: 369452067 --- .../DefaultPlaybackSessionManager.java | 9 ---- .../analytics/PlaybackStatsListener.java | 10 ++-- .../DefaultPlaybackSessionManagerTest.java | 46 +++++++------------ .../analytics/PlaybackStatsListenerTest.java | 10 ++-- 4 files changed, 26 insertions(+), 49 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 7d4d6087dc..2cc8456e12 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 @@ -105,10 +105,6 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag @Override public synchronized void updateSessions(EventTime eventTime) { Assertions.checkNotNull(listener); - if (eventTime.timeline.isEmpty()) { - // Don't try to create new sessions for empty timelines. - return; - } @Nullable SessionDescriptor currentSession = sessions.get(currentSessionId); if (eventTime.mediaPeriodId != null && currentSession != null) { // If we receive an event associated with a media period, then it needs to be either part of @@ -238,11 +234,6 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag @RequiresNonNull("listener") private void updateCurrentSession(EventTime eventTime) { - if (eventTime.timeline.isEmpty()) { - // Clear current session if the Timeline is empty. - currentSessionId = null; - return; - } @Nullable SessionDescriptor previousSessionDescriptor = sessions.get(currentSessionId); SessionDescriptor currentSessionDescriptor = getOrAddSession(eventTime.windowIndex, eventTime.mediaPeriodId); 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 9465bd2387..8b9967addb 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 @@ -240,7 +240,7 @@ public final class PlaybackStatsListener if (events.size() == 0) { return; } - maybeAddSessions(events); + maybeAddSessions(player, events); for (String session : playbackStatsTrackers.keySet()) { Pair eventTimeAndBelongsToPlayback = findBestEventTime(events, session); PlaybackStatsTracker tracker = playbackStatsTrackers.get(session); @@ -281,15 +281,17 @@ public final class PlaybackStatsListener } } - private void maybeAddSessions(Events events) { + private void maybeAddSessions(Player player, Events events) { + boolean isCompletelyIdle = + player.getCurrentTimeline().isEmpty() && player.getPlaybackState() == Player.STATE_IDLE; for (int i = 0; i < events.size(); i++) { @EventFlags int event = events.get(i); EventTime eventTime = events.getEventTime(event); if (event == EVENT_TIMELINE_CHANGED) { sessionManager.updateSessionsWithTimelineChange(eventTime); - } else if (event == EVENT_POSITION_DISCONTINUITY) { + } else if (!isCompletelyIdle && event == EVENT_POSITION_DISCONTINUITY) { sessionManager.updateSessionsWithDiscontinuity(eventTime, discontinuityReason); - } else { + } else if (!isCompletelyIdle) { sessionManager.updateSessions(eventTime); } } 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 930a835cec..72998db0e0 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 @@ -59,16 +59,6 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.setListener(mockListener); } - @Test - public void updatesSession_withEmptyTimeline_doesNotCreateNewSession() { - EventTime eventTime = - createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId */ null); - - sessionManager.updateSessions(eventTime); - - verifyNoMoreInteractions(mockListener); - } - @Test public void updateSessions_withoutMediaPeriodId_createsNewSession() { Timeline timeline = new FakeTimeline(); @@ -496,6 +486,8 @@ public final class DefaultPlaybackSessionManagerTest { @Test public void belongsToSession_withSameWindowIndex_returnsTrue() { + EventTime eventTime = + createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId= */ null); Timeline timeline = new FakeTimeline(); EventTime eventTimeWithTimeline = createEventTime(timeline, /* windowIndex= */ 0, /* mediaPeriodId= */ null); @@ -504,10 +496,11 @@ public final class DefaultPlaybackSessionManagerTest { timeline.getUidOfPeriod(/* periodIndex= */ 0), /* windowSequenceNumber= */ 0); EventTime eventTimeWithMediaPeriodId = createEventTime(timeline, /* windowIndex= */ 0, mediaPeriodId); - sessionManager.updateSessions(eventTimeWithTimeline); + sessionManager.updateSessions(eventTime); ArgumentCaptor sessionId = ArgumentCaptor.forClass(String.class); - verify(mockListener).onSessionCreated(eq(eventTimeWithTimeline), sessionId.capture()); + verify(mockListener).onSessionCreated(eq(eventTime), sessionId.capture()); + assertThat(sessionManager.belongsToSession(eventTime, sessionId.getValue())).isTrue(); assertThat(sessionManager.belongsToSession(eventTimeWithTimeline, sessionId.getValue())) .isTrue(); assertThat(sessionManager.belongsToSession(eventTimeWithMediaPeriodId, sessionId.getValue())) @@ -516,11 +509,11 @@ public final class DefaultPlaybackSessionManagerTest { @Test public void belongsToSession_withOtherWindowIndex_returnsFalse() { - Timeline timeline = new FakeTimeline(/* windowCount= */ 2); EventTime eventTime = - createEventTime(timeline, /* windowIndex= */ 0, /* mediaPeriodId= */ null); + createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId= */ null); EventTime eventTimeOtherWindow = - createEventTime(timeline, /* windowIndex= */ 1, /* mediaPeriodId= */ null); + createEventTime(Timeline.EMPTY, /* windowIndex= */ 1, /* mediaPeriodId= */ null); + Timeline timeline = new FakeTimeline(/* windowCount= */ 2); MediaPeriodId mediaPeriodId = new MediaPeriodId( timeline.getUidOfPeriod(/* periodIndex= */ 1), /* windowSequenceNumber= */ 1); @@ -592,18 +585,18 @@ public final class DefaultPlaybackSessionManagerTest { } @Test - public void timelineUpdate_toEmpty_finishesAllSessionsAndDoesNotCreateNewSessions() { - Timeline timeline = new FakeTimeline(/* windowCount= */ 2); + public void initialTimelineUpdate_finishesAllSessionsOutsideTimeline() { EventTime eventTime1 = - createEventTime(timeline, /* windowIndex= */ 0, /* mediaPeriodId= */ null); + createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId= */ null); EventTime eventTime2 = - createEventTime(timeline, /* windowIndex= */ 1, /* mediaPeriodId= */ null); + createEventTime(Timeline.EMPTY, /* windowIndex= */ 1, /* mediaPeriodId= */ null); sessionManager.updateSessions(eventTime1); sessionManager.updateSessions(eventTime2); + Timeline timeline = new FakeTimeline(); + EventTime newTimelineEventTime = + createEventTime(timeline, /* windowIndex= */ 0, /* mediaPeriodId= */ null); - EventTime eventTimeWithEmptyTimeline = - createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId= */ null); - sessionManager.updateSessionsWithTimelineChange(eventTimeWithEmptyTimeline); + sessionManager.updateSessionsWithTimelineChange(newTimelineEventTime); ArgumentCaptor sessionId1 = ArgumentCaptor.forClass(String.class); ArgumentCaptor sessionId2 = ArgumentCaptor.forClass(String.class); @@ -612,19 +605,14 @@ public final class DefaultPlaybackSessionManagerTest { verify(mockListener).onSessionActive(eventTime1, sessionId1.getValue()); verify(mockListener) .onSessionFinished( - eventTimeWithEmptyTimeline, - sessionId1.getValue(), - /* automaticTransitionToNextPlayback= */ false); - verify(mockListener) - .onSessionFinished( - eventTimeWithEmptyTimeline, + newTimelineEventTime, sessionId2.getValue(), /* automaticTransitionToNextPlayback= */ false); verifyNoMoreInteractions(mockListener); } @Test - public void timelineUpdate_resolvesWindowIndices() { + public void dynamicTimelineUpdate_resolvesWindowIndices() { Timeline initialTimeline = new FakeTimeline( new TimelineWindowDefinition(/* periodCount= */ 2, /* id= */ 100), 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 668ed5f556..0fa1a4a85b 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 @@ -22,7 +22,6 @@ 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 static org.mockito.Mockito.verifyNoMoreInteractions; import androidx.annotation.Nullable; import androidx.test.core.app.ApplicationProvider; @@ -78,18 +77,15 @@ public final class PlaybackStatsListenerTest { } @Test - public void stateChangeEvent_toEndedWithEmptyTimeline_doesNotCreateInitialPlaybackStats() - throws Exception { - PlaybackStatsListener.Callback callback = mock(PlaybackStatsListener.Callback.class); + public void stateChangeEvent_toNonIdle_createsInitialPlaybackStats() throws Exception { PlaybackStatsListener playbackStatsListener = - new PlaybackStatsListener(/* keepHistory= */ true, callback); + new PlaybackStatsListener(/* keepHistory= */ true, /* callback= */ null); player.addAnalyticsListener(playbackStatsListener); player.prepare(); runUntilPendingCommandsAreFullyHandled(player); - assertThat(playbackStatsListener.getPlaybackStats()).isNull(); - verifyNoMoreInteractions(callback); + assertThat(playbackStatsListener.getPlaybackStats()).isNotNull(); } @Test