From c0ee312a307164b84822d1b84e875e8e3f0c15ef Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 13 Jan 2020 14:52:14 +0000 Subject: [PATCH] Simplify keeping track of current id in DefaultPlaybackSessionManager We currently have a currentMediaPeriodId and an activeSessionId that are more or less tracking the same thing unless the current media period isn't "active" yet. Simplify this logic by using a single currentSessionId field and the respective isActive flag of this session. Also move all session creation and activation code in the same method to make it easier to reason about the code. This change also fixes a subtle bug where events after a seek to a new window are not ignored as they should. PiperOrigin-RevId: 289432181 --- .../DefaultPlaybackSessionManager.java | 92 ++++++++++--------- .../DefaultPlaybackSessionManagerTest.java | 14 +++ 2 files changed, 61 insertions(+), 45 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 44f8c10afe..cede01e199 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 @@ -29,7 +29,6 @@ import java.util.HashMap; import java.util.Iterator; import java.util.Random; import org.checkerframework.checker.nullness.qual.MonotonicNonNull; -import org.checkerframework.checker.nullness.qual.RequiresNonNull; /** * Default {@link PlaybackSessionManager} which instantiates a new session for each window in the @@ -48,8 +47,7 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag private @MonotonicNonNull Listener listener; private Timeline currentTimeline; - @Nullable private MediaPeriodId currentMediaPeriodId; - @Nullable private String activeSessionId; + @Nullable private String currentSessionId; /** Creates session manager. */ public DefaultPlaybackSessionManager() { @@ -83,22 +81,34 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag @Override public synchronized void updateSessions(EventTime eventTime) { - boolean isObviouslyFinished = - eventTime.mediaPeriodId != null - && currentMediaPeriodId != null - && eventTime.mediaPeriodId.windowSequenceNumber - < currentMediaPeriodId.windowSequenceNumber; - if (!isObviouslyFinished) { - SessionDescriptor descriptor = - getOrAddSession(eventTime.windowIndex, eventTime.mediaPeriodId); - if (!descriptor.isCreated) { - descriptor.isCreated = true; - Assertions.checkNotNull(listener).onSessionCreated(eventTime, descriptor.sessionId); - if (activeSessionId == null) { - updateActiveSession(eventTime, descriptor); - } + Assertions.checkNotNull(listener); + @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 + // the current window if it's the first created media period, or a window that will be played + // in the future. Otherwise, we know that it belongs to a session that was already finished + // and we can ignore the event. + boolean isAlreadyFinished = + currentSession.windowSequenceNumber == C.INDEX_UNSET + ? currentSession.windowIndex != eventTime.windowIndex + : eventTime.mediaPeriodId.windowSequenceNumber < currentSession.windowSequenceNumber; + if (isAlreadyFinished) { + return; } } + SessionDescriptor eventSession = + getOrAddSession(eventTime.windowIndex, eventTime.mediaPeriodId); + if (currentSessionId == null) { + currentSessionId = eventSession.sessionId; + } + if (!eventSession.isCreated) { + eventSession.isCreated = true; + listener.onSessionCreated(eventTime, eventSession.sessionId); + } + if (eventSession.sessionId.equals(currentSessionId) && !eventSession.isActive) { + eventSession.isActive = true; + listener.onSessionActive(eventTime, eventSession.sessionId); + } } @Override @@ -112,8 +122,8 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag if (!session.tryResolvingToNewTimeline(previousTimeline, currentTimeline)) { iterator.remove(); if (session.isCreated) { - if (session.sessionId.equals(activeSessionId)) { - activeSessionId = null; + if (session.sessionId.equals(currentSessionId)) { + currentSessionId = null; } listener.onSessionFinished( eventTime, session.sessionId, /* automaticTransitionToNextPlayback= */ false); @@ -136,36 +146,41 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag if (session.isFinishedAtEventTime(eventTime)) { iterator.remove(); if (session.isCreated) { - boolean isRemovingActiveSession = session.sessionId.equals(activeSessionId); - boolean isAutomaticTransition = hasAutomaticTransition && isRemovingActiveSession; - if (isRemovingActiveSession) { - activeSessionId = null; + boolean isRemovingCurrentSession = session.sessionId.equals(currentSessionId); + boolean isAutomaticTransition = + hasAutomaticTransition && isRemovingCurrentSession && session.isActive; + if (isRemovingCurrentSession) { + currentSessionId = null; } listener.onSessionFinished(eventTime, session.sessionId, isAutomaticTransition); } } } - SessionDescriptor activeSessionDescriptor = + @Nullable SessionDescriptor previousSessionDescriptor = sessions.get(currentSessionId); + SessionDescriptor currentSessionDescriptor = getOrAddSession(eventTime.windowIndex, eventTime.mediaPeriodId); + currentSessionId = currentSessionDescriptor.sessionId; if (eventTime.mediaPeriodId != null && eventTime.mediaPeriodId.isAd() - && (currentMediaPeriodId == null - || currentMediaPeriodId.windowSequenceNumber + && (previousSessionDescriptor == null + || previousSessionDescriptor.windowSequenceNumber != eventTime.mediaPeriodId.windowSequenceNumber - || currentMediaPeriodId.adGroupIndex != eventTime.mediaPeriodId.adGroupIndex - || currentMediaPeriodId.adIndexInAdGroup != eventTime.mediaPeriodId.adIndexInAdGroup)) { + || previousSessionDescriptor.adMediaPeriodId == null + || previousSessionDescriptor.adMediaPeriodId.adGroupIndex + != eventTime.mediaPeriodId.adGroupIndex + || previousSessionDescriptor.adMediaPeriodId.adIndexInAdGroup + != eventTime.mediaPeriodId.adIndexInAdGroup)) { // New ad playback started. Find corresponding content session and notify ad playback started. MediaPeriodId contentMediaPeriodId = new MediaPeriodId( eventTime.mediaPeriodId.periodUid, eventTime.mediaPeriodId.windowSequenceNumber); SessionDescriptor contentSession = getOrAddSession(eventTime.windowIndex, contentMediaPeriodId); - if (contentSession.isCreated && activeSessionDescriptor.isCreated) { + if (contentSession.isCreated && currentSessionDescriptor.isCreated) { listener.onAdPlaybackStarted( - eventTime, contentSession.sessionId, activeSessionDescriptor.sessionId); + eventTime, contentSession.sessionId, currentSessionDescriptor.sessionId); } } - updateActiveSession(eventTime, activeSessionDescriptor); } private SessionDescriptor getOrAddSession( @@ -199,18 +214,6 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag return bestMatch; } - @RequiresNonNull("listener") - private void updateActiveSession(EventTime eventTime, SessionDescriptor sessionDescriptor) { - currentMediaPeriodId = eventTime.mediaPeriodId; - if (sessionDescriptor.isCreated) { - activeSessionId = sessionDescriptor.sessionId; - if (!sessionDescriptor.isActive) { - sessionDescriptor.isActive = true; - listener.onSessionActive(eventTime, sessionDescriptor.sessionId); - } - } - } - private static String generateSessionId() { byte[] randomBytes = new byte[SESSION_ID_LENGTH]; RANDOM.nextBytes(randomBytes); @@ -284,8 +287,7 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag int eventWindowIndex, @Nullable MediaPeriodId eventMediaPeriodId) { if (windowSequenceNumber == C.INDEX_UNSET && eventWindowIndex == windowIndex - && eventMediaPeriodId != null - && !eventMediaPeriodId.isAd()) { + && eventMediaPeriodId != null) { // Set window sequence number for this session as soon as we have one. windowSequenceNumber = eventMediaPeriodId.windowSequenceNumber; } 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 f0b18b4a20..00e93c0782 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 @@ -501,6 +501,7 @@ public final class DefaultPlaybackSessionManagerTest { createEventTime(timeline, /* windowIndex= */ 0, /* mediaPeriodId= */ null); sessionManager.handleTimelineUpdate(newTimelineEventTime); + sessionManager.updateSessions(newTimelineEventTime); ArgumentCaptor sessionId1 = ArgumentCaptor.forClass(String.class); ArgumentCaptor sessionId2 = ArgumentCaptor.forClass(String.class); @@ -657,6 +658,7 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.handlePositionDiscontinuity( eventTime2, Player.DISCONTINUITY_REASON_PERIOD_TRANSITION); + sessionManager.updateSessions(eventTime2); verify(mockListener).onSessionCreated(eq(eventTime1), anyString()); verify(mockListener).onSessionActive(eq(eventTime1), anyString()); @@ -688,6 +690,7 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.handlePositionDiscontinuity( eventTime2, Player.DISCONTINUITY_REASON_PERIOD_TRANSITION); + sessionManager.updateSessions(eventTime2); verify(mockListener).onSessionCreated(eventTime1, sessionId1); verify(mockListener).onSessionActive(eventTime1, sessionId1); @@ -722,6 +725,7 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.getSessionForMediaPeriodId(timeline, eventTime2.mediaPeriodId); sessionManager.handlePositionDiscontinuity(eventTime2, Player.DISCONTINUITY_REASON_SEEK); + sessionManager.updateSessions(eventTime2); verify(mockListener).onSessionCreated(eventTime1, sessionId1); verify(mockListener).onSessionActive(eventTime1, sessionId1); @@ -748,6 +752,7 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.updateSessions(eventTime2); sessionManager.handlePositionDiscontinuity(eventTime2, Player.DISCONTINUITY_REASON_SEEK); + sessionManager.updateSessions(eventTime2); verify(mockListener, never()).onSessionFinished(any(), anyString(), anyBoolean()); } @@ -790,6 +795,7 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.getSessionForMediaPeriodId(timeline, eventTime2.mediaPeriodId); sessionManager.handlePositionDiscontinuity(eventTime3, Player.DISCONTINUITY_REASON_SEEK); + sessionManager.updateSessions(eventTime3); verify(mockListener).onSessionCreated(eventTime1, sessionId1); verify(mockListener).onSessionActive(eventTime1, sessionId1); @@ -851,6 +857,7 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.handlePositionDiscontinuity( contentEventTime, Player.DISCONTINUITY_REASON_AD_INSERTION); + sessionManager.updateSessions(contentEventTime); verify(mockListener).onSessionCreated(adEventTime1, adSessionId1); verify(mockListener).onSessionActive(adEventTime1, adSessionId1); @@ -858,6 +865,8 @@ public final class DefaultPlaybackSessionManagerTest { verify(mockListener) .onSessionFinished( contentEventTime, adSessionId1, /* automaticTransitionToNextPlayback= */ true); + verify(mockListener).onSessionCreated(eq(contentEventTime), anyString()); + verify(mockListener).onSessionActive(eq(contentEventTime), anyString()); verifyNoMoreInteractions(mockListener); } @@ -908,6 +917,7 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.handlePositionDiscontinuity( adEventTime1, Player.DISCONTINUITY_REASON_AD_INSERTION); + sessionManager.updateSessions(adEventTime1); verify(mockListener, never()).onSessionFinished(any(), anyString(), anyBoolean()); } @@ -964,7 +974,9 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.handlePositionDiscontinuity( adEventTime1, Player.DISCONTINUITY_REASON_AD_INSERTION); + sessionManager.updateSessions(adEventTime1); sessionManager.handlePositionDiscontinuity(adEventTime2, Player.DISCONTINUITY_REASON_SEEK); + sessionManager.updateSessions(adEventTime2); verify(mockListener).onSessionCreated(eq(contentEventTime), anyString()); verify(mockListener).onSessionActive(eq(contentEventTime), anyString()); @@ -1034,8 +1046,10 @@ public final class DefaultPlaybackSessionManagerTest { sessionManager.updateSessions(adEventTime1); sessionManager.handlePositionDiscontinuity( adEventTime1, Player.DISCONTINUITY_REASON_AD_INSERTION); + sessionManager.updateSessions(adEventTime1); sessionManager.handlePositionDiscontinuity( contentEventTime2, Player.DISCONTINUITY_REASON_AD_INSERTION); + sessionManager.updateSessions(contentEventTime2); String adSessionId2 = sessionManager.getSessionForMediaPeriodId(adTimeline, adEventTime2.mediaPeriodId);