From e250fe6277b31c35f91500285bf3c705ccbc7c9a Mon Sep 17 00:00:00 2001 From: tonihei Date: Thu, 23 Apr 2020 19:59:10 +0100 Subject: [PATCH] Make sure not to create new playback sessions while still IDLE. The first session should only be created once we have the media items and/or called prepare. Otherwise the first session is created with an EventTime having an empty timeline making it less useful. issue:#7193 PiperOrigin-RevId: 308100555 --- .../analytics/PlaybackStatsListener.java | 43 ++++++++------ .../analytics/PlaybackStatsListenerTest.java | 56 ++++++++++++++++++- 2 files changed, 80 insertions(+), 19 deletions(-) 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 97805da0fd..c312696176 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 @@ -245,7 +245,7 @@ public final class PlaybackStatsListener @Override public void onPlaybackStateChanged(EventTime eventTime, @Player.State int state) { playbackState = state; - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { boolean belongsToPlayback = sessionManager.belongsToSession(eventTime, session); playbackStatsTrackers @@ -258,7 +258,7 @@ public final class PlaybackStatsListener public void onPlayWhenReadyChanged( EventTime eventTime, boolean playWhenReady, @Player.PlayWhenReadyChangeReason int reason) { this.playWhenReady = playWhenReady; - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { boolean belongsToPlayback = sessionManager.belongsToSession(eventTime, session); playbackStatsTrackers @@ -271,7 +271,7 @@ public final class PlaybackStatsListener public void onPlaybackSuppressionReasonChanged( EventTime eventTime, int playbackSuppressionReason) { isSuppressed = playbackSuppressionReason != Player.PLAYBACK_SUPPRESSION_REASON_NONE; - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { boolean belongsToPlayback = sessionManager.belongsToSession(eventTime, session); playbackStatsTrackers @@ -283,7 +283,7 @@ public final class PlaybackStatsListener @Override public void onTimelineChanged(EventTime eventTime, int reason) { sessionManager.handleTimelineUpdate(eventTime); - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onPositionDiscontinuity(eventTime); @@ -294,7 +294,7 @@ public final class PlaybackStatsListener @Override public void onPositionDiscontinuity(EventTime eventTime, int reason) { sessionManager.handlePositionDiscontinuity(eventTime, reason); - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onPositionDiscontinuity(eventTime); @@ -304,7 +304,7 @@ public final class PlaybackStatsListener @Override public void onSeekStarted(EventTime eventTime) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { boolean belongsToPlayback = sessionManager.belongsToSession(eventTime, session); playbackStatsTrackers.get(session).onSeekStarted(eventTime, belongsToPlayback); @@ -314,7 +314,7 @@ public final class PlaybackStatsListener @Override public void onSeekProcessed(EventTime eventTime) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { boolean belongsToPlayback = sessionManager.belongsToSession(eventTime, session); playbackStatsTrackers.get(session).onSeekProcessed(eventTime, belongsToPlayback); @@ -324,7 +324,7 @@ public final class PlaybackStatsListener @Override public void onPlayerError(EventTime eventTime, ExoPlaybackException error) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onFatalError(eventTime, error); @@ -335,7 +335,7 @@ public final class PlaybackStatsListener @Override public void onPlaybackSpeedChanged(EventTime eventTime, float playbackSpeed) { this.playbackSpeed = playbackSpeed; - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (PlaybackStatsTracker tracker : playbackStatsTrackers.values()) { tracker.onPlaybackSpeedChanged(eventTime, playbackSpeed); } @@ -344,7 +344,7 @@ public final class PlaybackStatsListener @Override public void onTracksChanged( EventTime eventTime, TrackGroupArray trackGroups, TrackSelectionArray trackSelections) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onTracksChanged(eventTime, trackSelections); @@ -355,7 +355,7 @@ public final class PlaybackStatsListener @Override public void onLoadStarted( EventTime eventTime, LoadEventInfo loadEventInfo, MediaLoadData mediaLoadData) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onLoadStarted(eventTime); @@ -365,7 +365,7 @@ public final class PlaybackStatsListener @Override public void onDownstreamFormatChanged(EventTime eventTime, MediaLoadData mediaLoadData) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onDownstreamFormatChanged(eventTime, mediaLoadData); @@ -380,7 +380,7 @@ public final class PlaybackStatsListener int height, int unappliedRotationDegrees, float pixelWidthHeightRatio) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onVideoSizeChanged(eventTime, width, height); @@ -391,7 +391,7 @@ public final class PlaybackStatsListener @Override public void onBandwidthEstimate( EventTime eventTime, int totalLoadTimeMs, long totalBytesLoaded, long bitrateEstimate) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onBandwidthData(totalLoadTimeMs, totalBytesLoaded); @@ -402,7 +402,7 @@ public final class PlaybackStatsListener @Override public void onAudioUnderrun( EventTime eventTime, int bufferSize, long bufferSizeMs, long elapsedSinceLastFeedMs) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onAudioUnderrun(); @@ -412,7 +412,7 @@ public final class PlaybackStatsListener @Override public void onDroppedVideoFrames(EventTime eventTime, int droppedFrames, long elapsedMs) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onDroppedVideoFrames(droppedFrames); @@ -427,7 +427,7 @@ public final class PlaybackStatsListener MediaLoadData mediaLoadData, IOException error, boolean wasCanceled) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onNonFatalError(eventTime, error); @@ -437,7 +437,7 @@ public final class PlaybackStatsListener @Override public void onDrmSessionManagerError(EventTime eventTime, Exception error) { - sessionManager.updateSessions(eventTime); + maybeAddSession(eventTime); for (String session : playbackStatsTrackers.keySet()) { if (sessionManager.belongsToSession(eventTime, session)) { playbackStatsTrackers.get(session).onNonFatalError(eventTime, error); @@ -445,6 +445,13 @@ public final class PlaybackStatsListener } } + private void maybeAddSession(EventTime eventTime) { + boolean isCompletelyIdle = eventTime.timeline.isEmpty() && playbackState == Player.STATE_IDLE; + if (!isCompletelyIdle) { + sessionManager.updateSessions(eventTime); + } + } + /** Tracker for playback stats of a single playback. */ private static final class PlaybackStatsTracker { 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 f08d63400d..695e1f4557 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 @@ -21,6 +21,8 @@ import androidx.annotation.Nullable; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.exoplayer2.Player; import com.google.android.exoplayer2.Timeline; +import com.google.android.exoplayer2.source.MediaSource; +import com.google.android.exoplayer2.testutil.FakeTimeline; import org.junit.Test; import org.junit.runner.RunWith; @@ -28,7 +30,7 @@ import org.junit.runner.RunWith; @RunWith(AndroidJUnit4.class) public final class PlaybackStatsListenerTest { - private static final AnalyticsListener.EventTime TEST_EVENT_TIME = + private static final AnalyticsListener.EventTime EMPTY_TIMELINE_EVENT_TIME = new AnalyticsListener.EventTime( /* realtimeMs= */ 500, Timeline.EMPTY, @@ -37,6 +39,58 @@ public final class PlaybackStatsListenerTest { /* eventPlaybackPositionMs= */ 0, /* currentPlaybackPositionMs= */ 0, /* totalBufferedDurationMs= */ 0); + private static final Timeline TEST_TIMELINE = new FakeTimeline(/* windowCount= */ 1); + private static final AnalyticsListener.EventTime TEST_EVENT_TIME = + new AnalyticsListener.EventTime( + /* realtimeMs= */ 700, + TEST_TIMELINE, + /* windowIndex= */ 0, + new MediaSource.MediaPeriodId( + TEST_TIMELINE.getPeriod( + /* periodIndex= */ 0, new Timeline.Period(), /* setIds= */ true) + .uid, + /* windowSequenceNumber= */ 42), + /* eventPlaybackPositionMs= */ 123, + /* currentPlaybackPositionMs= */ 123, + /* totalBufferedDurationMs= */ 456); + + @Test + public void events_duringInitialIdleState_dontCreateNewPlaybackStats() { + PlaybackStatsListener playbackStatsListener = + new PlaybackStatsListener(/* keepHistory= */ true, /* callback= */ null); + + playbackStatsListener.onPositionDiscontinuity( + EMPTY_TIMELINE_EVENT_TIME, Player.DISCONTINUITY_REASON_SEEK); + playbackStatsListener.onPlaybackSpeedChanged( + EMPTY_TIMELINE_EVENT_TIME, /* playbackSpeed= */ 2.0f); + playbackStatsListener.onPlayWhenReadyChanged( + EMPTY_TIMELINE_EVENT_TIME, + /* playWhenReady= */ true, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST); + + assertThat(playbackStatsListener.getPlaybackStats()).isNull(); + } + + @Test + public void stateChangeEvent_toNonIdle_createsInitialPlaybackStats() { + PlaybackStatsListener playbackStatsListener = + new PlaybackStatsListener(/* keepHistory= */ true, /* callback= */ null); + + playbackStatsListener.onPlaybackStateChanged(EMPTY_TIMELINE_EVENT_TIME, Player.STATE_BUFFERING); + + assertThat(playbackStatsListener.getPlaybackStats()).isNotNull(); + } + + @Test + public void timelineChangeEvent_toNonEmpty_createsInitialPlaybackStats() { + PlaybackStatsListener playbackStatsListener = + new PlaybackStatsListener(/* keepHistory= */ true, /* callback= */ null); + + playbackStatsListener.onTimelineChanged( + TEST_EVENT_TIME, Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED); + + assertThat(playbackStatsListener.getPlaybackStats()).isNotNull(); + } @Test public void playback_withKeepHistory_updatesStats() {