Skip to content

Commit

Permalink
Fix spurious sessions created for events after the playlist is cleared
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@tonihei @icbaker
tonihei authored and icbaker committed Jun 20, 2023
1 parent 71f7322 commit e0191dd
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 20 deletions.
2 changes: 2 additions & 0 deletions RELEASENOTES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
61 changes: 41 additions & 20 deletions ...ayer/src/main/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -79,6 +80,7 @@ public DefaultPlaybackSessionManager(Supplier<String> sessionIdGenerator) {
period = new Timeline.Period();
sessions = new HashMap<>();
currentTimeline = Timeline.EMPTY;
lastRemovedCurrentWindowSequenceNumber = -1;
}

@Override
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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<SessionDescriptor> iterator = sessions.values().iterator();
Expand All @@ -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);
Expand All @@ -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<SessionDescriptor> iterator = sessions.values().iterator();
while (iterator.hasNext()) {
Expand All @@ -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);
}
Expand All @@ -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<SessionDescriptor> iterator = sessions.values().iterator();
while (iterator.hasNext()) {
SessionDescriptor session = iterator.next();
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down
104 changes: 104 additions & 0 deletions .../src/test/java/androidx/media3/exoplayer/analytics/DefaultPlaybackSessionManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> sessionId1 = ArgumentCaptor.forClass(String.class);
ArgumentCaptor<String> 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<String> 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<String> sessionId1 = ArgumentCaptor.forClass(String.class);
ArgumentCaptor<String> 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();
Expand Down

0 comments on commit e0191dd

Please sign in to comment.