From e0191ddded1754cc31b025493970240aeb08a46e Mon Sep 17 00:00:00 2001 From: tonihei Date: Tue, 20 Jun 2023 12:27:23 +0100 Subject: [PATCH] Fix spurious sessions created for events after the playlist is cleared Some events may arrive after the playlist is cleared (e.g. load cancellation). In this case, the DefaultPlaybackSessionManager may create a new session for the already removed item. We already have checks in place that ignore events with old windowSequenceNumbers, but these checks only work if the current session is set (i.e. the playlist is non-empty). The fix is to add the same check for empty playlists by keeping note of the last removed window sequence number. PiperOrigin-RevId: 541870812 --- RELEASENOTES.md | 2 + .../DefaultPlaybackSessionManager.java | 61 ++++++---- .../DefaultPlaybackSessionManagerTest.java | 104 ++++++++++++++++++ 3 files changed, 147 insertions(+), 20 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 2757b9d6819..d558de8583e 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -28,6 +28,8 @@ * Fix seeking issues in AC4 streams caused by not identifying decode-only samples correctly ([#11000](https://github.com/google/ExoPlayer/issues/11000)). + * Fix issue in `PlaybackStatsListener` where spurious `PlaybackStats` are + created after the playlist is cleared. * Transformer: * Parse EXIF rotation data for image inputs. * Track Selection: diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManager.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManager.java index b0af78bf618..cc4b7792c59 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManager.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManager.java @@ -15,6 +15,7 @@ */ package androidx.media3.exoplayer.analytics; +import static androidx.media3.common.util.Assertions.checkNotNull; import static java.lang.Math.max; import android.util.Base64; @@ -23,7 +24,6 @@ import androidx.media3.common.Player; import androidx.media3.common.Player.DiscontinuityReason; import androidx.media3.common.Timeline; -import androidx.media3.common.util.Assertions; import androidx.media3.common.util.UnstableApi; import androidx.media3.common.util.Util; import androidx.media3.exoplayer.analytics.AnalyticsListener.EventTime; @@ -59,6 +59,7 @@ public final class DefaultPlaybackSessionManager implements PlaybackSessionManag private @MonotonicNonNull Listener listener; private Timeline currentTimeline; @Nullable private String currentSessionId; + private long lastRemovedCurrentWindowSequenceNumber; /** * Creates session manager with a {@link #DEFAULT_SESSION_ID_GENERATOR} to generate session ids. @@ -79,6 +80,7 @@ public DefaultPlaybackSessionManager(Supplier sessionIdGenerator) { period = new Timeline.Period(); sessions = new HashMap<>(); currentTimeline = Timeline.EMPTY; + lastRemovedCurrentWindowSequenceNumber = -1; } @Override @@ -105,22 +107,22 @@ public synchronized boolean belongsToSession(EventTime eventTime, String session @Override public synchronized void updateSessions(EventTime eventTime) { - Assertions.checkNotNull(listener); + 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 - // 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) { + if (eventTime.mediaPeriodId != null) { + if (eventTime.mediaPeriodId.windowSequenceNumber < getMinWindowSequenceNumber()) { + // Ignore event because it is part of a past window that has already been finished. + return; + } + @Nullable SessionDescriptor currentSession = sessions.get(currentSessionId); + if (currentSession != null + && currentSession.windowSequenceNumber == C.INDEX_UNSET + && currentSession.windowIndex != eventTime.windowIndex) { + // Ignore events for anything other than the current window before the first media period + // has been created. return; } } @@ -173,7 +175,7 @@ public synchronized void updateSessions(EventTime eventTime) { @Override public synchronized void updateSessionsWithTimelineChange(EventTime eventTime) { - Assertions.checkNotNull(listener); + checkNotNull(listener); Timeline previousTimeline = currentTimeline; currentTimeline = eventTime.timeline; Iterator iterator = sessions.values().iterator(); @@ -184,7 +186,7 @@ public synchronized void updateSessionsWithTimelineChange(EventTime eventTime) { iterator.remove(); if (session.isCreated) { if (session.sessionId.equals(currentSessionId)) { - currentSessionId = null; + clearCurrentSession(session); } listener.onSessionFinished( eventTime, session.sessionId, /* automaticTransitionToNextPlayback= */ false); @@ -197,7 +199,7 @@ public synchronized void updateSessionsWithTimelineChange(EventTime eventTime) { @Override public synchronized void updateSessionsWithDiscontinuity( EventTime eventTime, @DiscontinuityReason int reason) { - Assertions.checkNotNull(listener); + checkNotNull(listener); boolean hasAutomaticTransition = reason == Player.DISCONTINUITY_REASON_AUTO_TRANSITION; Iterator iterator = sessions.values().iterator(); while (iterator.hasNext()) { @@ -209,7 +211,7 @@ public synchronized void updateSessionsWithDiscontinuity( boolean isAutomaticTransition = hasAutomaticTransition && isRemovingCurrentSession && session.isActive; if (isRemovingCurrentSession) { - currentSessionId = null; + clearCurrentSession(session); } listener.onSessionFinished(eventTime, session.sessionId, isAutomaticTransition); } @@ -226,7 +228,9 @@ public synchronized String getActiveSessionId() { @Override public synchronized void finishAllSessions(EventTime eventTime) { - currentSessionId = null; + if (currentSessionId != null) { + clearCurrentSession(checkNotNull(sessions.get(currentSessionId))); + } Iterator iterator = sessions.values().iterator(); while (iterator.hasNext()) { SessionDescriptor session = iterator.next(); @@ -242,7 +246,9 @@ public synchronized void finishAllSessions(EventTime eventTime) { private void updateCurrentSession(EventTime eventTime) { if (eventTime.timeline.isEmpty()) { // Clear current session if the Timeline is empty. - currentSessionId = null; + if (currentSessionId != null) { + clearCurrentSession(checkNotNull(sessions.get(currentSessionId))); + } return; } @Nullable SessionDescriptor previousSessionDescriptor = sessions.get(currentSessionId); @@ -271,6 +277,20 @@ private void updateCurrentSession(EventTime eventTime) { } } + private void clearCurrentSession(SessionDescriptor currentSession) { + if (currentSession.windowSequenceNumber != C.INDEX_UNSET) { + lastRemovedCurrentWindowSequenceNumber = currentSession.windowSequenceNumber; + } + currentSessionId = null; + } + + private long getMinWindowSequenceNumber() { + @Nullable SessionDescriptor currentSession = sessions.get(currentSessionId); + return currentSession != null && currentSession.windowSequenceNumber != C.INDEX_UNSET + ? currentSession.windowSequenceNumber + : lastRemovedCurrentWindowSequenceNumber + 1; + } + private SessionDescriptor getOrAddSession( int windowIndex, @Nullable MediaPeriodId mediaPeriodId) { // There should only be one matching session if mediaPeriodId is non-null. If mediaPeriodId is @@ -375,7 +395,8 @@ public void maybeSetWindowSequenceNumber( int eventWindowIndex, @Nullable MediaPeriodId eventMediaPeriodId) { if (windowSequenceNumber == C.INDEX_UNSET && eventWindowIndex == windowIndex - && eventMediaPeriodId != null) { + && eventMediaPeriodId != null + && eventMediaPeriodId.windowSequenceNumber >= getMinWindowSequenceNumber()) { // Set window sequence number for this session as soon as we have one. windowSequenceNumber = eventMediaPeriodId.windowSequenceNumber; } diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManagerTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManagerTest.java index 5c6d0d16c5e..ba045002f72 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManagerTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManagerTest.java @@ -482,6 +482,110 @@ public void updateSessions_withoutMediaPeriodId_afterSessionForMediaPeriodId_ret verify(mockListener, never()).onSessionActive(any(), eq(adSessionId2)); } + @Test + public void + updateSession_withNewMediaPeriodIdOfSameWindowAfterTimelineUpdateToEmpty_createsNewSession() { + Timeline timeline = new FakeTimeline(/* windowCount= */ 2); + EventTime eventTimeBeforeClear = + createEventTime( + timeline, + /* windowIndex= */ 0, + new MediaPeriodId( + timeline.getUidOfPeriod(/* periodIndex= */ 0), /* windowSequenceNumber= */ 0)); + sessionManager.updateSessions(eventTimeBeforeClear); + EventTime eventTimeWithEmptyTimeline = + createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId= */ null); + sessionManager.updateSessionsWithTimelineChange(eventTimeWithEmptyTimeline); + EventTime newEventTime = + createEventTime( + timeline, + /* windowIndex= */ 0, + new MediaPeriodId( + timeline.getUidOfPeriod(/* periodIndex= */ 0), /* windowSequenceNumber= */ 1)); + + sessionManager.updateSessions(newEventTime); + + ArgumentCaptor sessionId1 = ArgumentCaptor.forClass(String.class); + ArgumentCaptor sessionId2 = ArgumentCaptor.forClass(String.class); + verify(mockListener).onSessionCreated(eq(eventTimeBeforeClear), sessionId1.capture()); + verify(mockListener).onSessionActive(eventTimeBeforeClear, sessionId1.getValue()); + verify(mockListener) + .onSessionFinished( + eventTimeWithEmptyTimeline, + sessionId1.getValue(), + /* automaticTransitionToNextPlayback= */ false); + verify(mockListener).onSessionCreated(eq(newEventTime), sessionId2.capture()); + verify(mockListener).onSessionActive(newEventTime, sessionId2.getValue()); + verifyNoMoreInteractions(mockListener); + } + + @Test + public void + updateSession_withOldMediaPeriodIdOfSameWindowAfterTimelineUpdateToEmpty_doesNotCreateNewSession() { + Timeline timeline = new FakeTimeline(/* windowCount= */ 2); + EventTime eventTimeBeforeClear = + createEventTime( + timeline, + /* windowIndex= */ 0, + new MediaPeriodId( + timeline.getUidOfPeriod(/* periodIndex= */ 0), /* windowSequenceNumber= */ 0)); + sessionManager.updateSessions(eventTimeBeforeClear); + EventTime eventTimeWithEmptyTimeline = + createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId= */ null); + sessionManager.updateSessionsWithTimelineChange(eventTimeWithEmptyTimeline); + EventTime newEventTime = + createEventTime( + timeline, + /* windowIndex= */ 0, + new MediaPeriodId( + timeline.getUidOfPeriod(/* periodIndex= */ 0), /* windowSequenceNumber= */ 0)); + + sessionManager.updateSessions(newEventTime); + + ArgumentCaptor sessionId = ArgumentCaptor.forClass(String.class); + verify(mockListener).onSessionCreated(eq(eventTimeBeforeClear), sessionId.capture()); + verify(mockListener).onSessionActive(eventTimeBeforeClear, sessionId.getValue()); + verify(mockListener) + .onSessionFinished( + eventTimeWithEmptyTimeline, + sessionId.getValue(), + /* automaticTransitionToNextPlayback= */ false); + verifyNoMoreInteractions(mockListener); + } + + @Test + public void + updateSession_ofSameWindowWithoutMediaPeriodIdAfterTimelineUpdateToEmpty_createsNewSession() { + Timeline timeline = new FakeTimeline(/* windowCount= */ 2); + EventTime eventTimeBeforeClear = + createEventTime( + timeline, + /* windowIndex= */ 0, + new MediaPeriodId( + timeline.getUidOfPeriod(/* periodIndex= */ 0), /* windowSequenceNumber= */ 0)); + sessionManager.updateSessions(eventTimeBeforeClear); + EventTime eventTimeWithEmptyTimeline = + createEventTime(Timeline.EMPTY, /* windowIndex= */ 0, /* mediaPeriodId= */ null); + sessionManager.updateSessionsWithTimelineChange(eventTimeWithEmptyTimeline); + EventTime newEventTime = + createEventTime(timeline, /* windowIndex= */ 0, /* mediaPeriodId= */ null); + + sessionManager.updateSessions(newEventTime); + + ArgumentCaptor sessionId1 = ArgumentCaptor.forClass(String.class); + ArgumentCaptor sessionId2 = ArgumentCaptor.forClass(String.class); + verify(mockListener).onSessionCreated(eq(eventTimeBeforeClear), sessionId1.capture()); + verify(mockListener).onSessionActive(eventTimeBeforeClear, sessionId1.getValue()); + verify(mockListener) + .onSessionFinished( + eventTimeWithEmptyTimeline, + sessionId1.getValue(), + /* automaticTransitionToNextPlayback= */ false); + verify(mockListener).onSessionCreated(eq(newEventTime), sessionId2.capture()); + verify(mockListener).onSessionActive(newEventTime, sessionId2.getValue()); + verifyNoMoreInteractions(mockListener); + } + @Test public void getSessionForMediaPeriodId_returnsValue_butDoesNotCreateSession() { Timeline timeline = new FakeTimeline();