From 26a6aad3e40487a571c995c051ece642907f2a7e Mon Sep 17 00:00:00 2001 From: tonihei Date: Tue, 27 Apr 2021 14:15:24 +0100 Subject: [PATCH] Misc ad handling improvements. 1. Clarify intention of getAdGroupIndexForPositionUs and getAdGroupIndexAfterPositionUs. Both methods are used for very specific but different purposes and encode the logic of which ads should be played at which time, so it's helpful to clarify this in the documentation as well. 2. Change one usage getAdGroupIndexForPositionUs to use the already existing nextAdGroupIndex. This is also more in line with the intended usage as clarified in step 1. 3. Update MediaPeriodQueueTest for updateQueuedPeriods to only look for duration changes in future periods, not in the current one, because that's not handled MediaPeriodQueue for ads and the test is just passing by chance now. Also remove wrong advancePlaying() calls that are already implicitly included in the preceding enqueueNext() call. 4. Fix a minor bug where post-roll ads are not checked whether they are played already before using them as the next ad group. Also added a test covering this case. #minor-release PiperOrigin-RevId: 370664131 --- .../google/android/exoplayer2/Timeline.java | 9 ++- .../source/ads/AdPlaybackState.java | 14 ++-- .../android/exoplayer2/MediaPeriodQueue.java | 11 ++- .../exoplayer2/MediaPeriodQueueTest.java | 80 ++++++++++++++++--- 4 files changed, 88 insertions(+), 26 deletions(-) diff --git a/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java b/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java index 1d230f927d..4d90c0a268 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java @@ -752,9 +752,10 @@ public abstract class Timeline implements Bundleable { } /** - * Returns the index of the ad group at or before {@code positionUs} in the period, if that ad - * group is unplayed. Returns {@link C#INDEX_UNSET} if the ad group at or before {@code - * positionUs} has no ads remaining to be played, or if there is no such ad group. + * Returns the index of the ad group at or before {@code positionUs} in the period that should + * be played before the content at {@code positionUs}. Returns {@link C#INDEX_UNSET} if the ad + * group at or before {@code positionUs} has no ads remaining to be played, or if there is no + * such ad group. * * @param positionUs The period position at or before which to find an ad group, in * microseconds. @@ -766,7 +767,7 @@ public abstract class Timeline implements Bundleable { /** * Returns the index of the next ad group after {@code positionUs} in the period that has ads - * remaining to be played. Returns {@link C#INDEX_UNSET} if there is no such ad group. + * that should be played. Returns {@link C#INDEX_UNSET} if there is no such ad group. * * @param positionUs The period position after which to find an ad group, in microseconds. * @return The index of the ad group, or {@link C#INDEX_UNSET}. diff --git a/library/common/src/main/java/com/google/android/exoplayer2/source/ads/AdPlaybackState.java b/library/common/src/main/java/com/google/android/exoplayer2/source/ads/AdPlaybackState.java index ac8a10f8a5..5b207492d9 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/source/ads/AdPlaybackState.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/source/ads/AdPlaybackState.java @@ -380,9 +380,9 @@ public final class AdPlaybackState implements Bundleable { } /** - * Returns the index of the ad group at or before {@code positionUs}, if that ad group is - * unplayed. Returns {@link C#INDEX_UNSET} if the ad group at or before {@code positionUs} has no - * ads remaining to be played, or if there is no such ad group. + * Returns the index of the ad group at or before {@code positionUs} that should be played before + * the content at {@code positionUs}. Returns {@link C#INDEX_UNSET} if the ad group at or before + * {@code positionUs} has no ads remaining to be played, or if there is no such ad group. * * @param positionUs The period position at or before which to find an ad group, in microseconds, * or {@link C#TIME_END_OF_SOURCE} for the end of the stream (in which case the index of any @@ -402,8 +402,8 @@ public final class AdPlaybackState implements Bundleable { } /** - * Returns the index of the next ad group after {@code positionUs} that has ads remaining to be - * played. Returns {@link C#INDEX_UNSET} if there is no such ad group. + * Returns the index of the next ad group after {@code positionUs} that should be played. Returns + * {@link C#INDEX_UNSET} if there is no such ad group. * * @param positionUs The period position after which to find an ad group, in microseconds, or * {@link C#TIME_END_OF_SOURCE} for the end of the stream (in which case there can be no ad @@ -421,8 +421,8 @@ public final class AdPlaybackState implements Bundleable { // In practice we expect there to be few ad groups so the search shouldn't be expensive. int index = 0; while (index < adGroupTimesUs.length - && adGroupTimesUs[index] != C.TIME_END_OF_SOURCE - && (positionUs >= adGroupTimesUs[index] || !adGroups[index].hasUnplayedAds())) { + && ((adGroupTimesUs[index] != C.TIME_END_OF_SOURCE && adGroupTimesUs[index] <= positionUs) + || !adGroups[index].hasUnplayedAds())) { index++; } return index < adGroupTimesUs.length ? index : C.INDEX_UNSET; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java b/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java index 7ba7589a56..d72eaa15db 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/MediaPeriodQueue.java @@ -706,10 +706,10 @@ import com.google.common.collect.ImmutableList; currentPeriodId.windowSequenceNumber); } } else { - // Play the next ad group if it's available. - int nextAdGroupIndex = period.getAdGroupIndexForPositionUs(mediaPeriodInfo.endPositionUs); - if (nextAdGroupIndex == C.INDEX_UNSET) { - // The next ad group can't be played. Play content from the previous end position instead. + // Play the next ad group if it's still available. + int adIndexInAdGroup = period.getFirstAdIndexToPlay(currentPeriodId.nextAdGroupIndex); + if (adIndexInAdGroup == period.getAdCountInAdGroup(currentPeriodId.nextAdGroupIndex)) { + // The next ad group has no ads left to play. Play content from the end position instead. return getMediaPeriodInfoForContent( timeline, currentPeriodId.periodUid, @@ -717,11 +717,10 @@ import com.google.common.collect.ImmutableList; /* requestedContentPositionUs= */ mediaPeriodInfo.durationUs, currentPeriodId.windowSequenceNumber); } - int adIndexInAdGroup = period.getFirstAdIndexToPlay(nextAdGroupIndex); return getMediaPeriodInfoForAd( timeline, currentPeriodId.periodUid, - nextAdGroupIndex, + currentPeriodId.nextAdGroupIndex, adIndexInAdGroup, /* contentPositionUs= */ mediaPeriodInfo.durationUs, currentPeriodId.windowSequenceNumber); diff --git a/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java b/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java index 5e71e41d40..8eaf476328 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/MediaPeriodQueueTest.java @@ -229,6 +229,49 @@ public final class MediaPeriodQueueTest { /* nextAdGroupIndex= */ C.INDEX_UNSET); } + @Test + public void getNextMediaPeriodInfo_withPlayedAdGroups_returnsCorrectMediaPeriodInfos() { + setupAdTimeline(/* adGroupTimesUs...= */ 0, FIRST_AD_START_TIME_US, C.TIME_END_OF_SOURCE); + setAdGroupLoaded(/* adGroupIndex= */ 0); + setAdGroupLoaded(/* adGroupIndex= */ 1); + setAdGroupLoaded(/* adGroupIndex= */ 2); + assertNextMediaPeriodInfoIsAd( + /* adGroupIndex= */ 0, AD_DURATION_US, /* contentPositionUs= */ C.TIME_UNSET); + setAdGroupPlayed(/* adGroupIndex= */ 0); + clear(); + assertGetNextMediaPeriodInfoReturnsContentMediaPeriod( + /* periodUid= */ firstPeriodUid, + /* startPositionUs= */ 0, + /* requestedContentPositionUs= */ C.TIME_UNSET, + /* endPositionUs= */ FIRST_AD_START_TIME_US, + /* durationUs= */ FIRST_AD_START_TIME_US, + /* isLastInPeriod= */ false, + /* isLastInWindow= */ false, + /* nextAdGroupIndex= */ 1); + setAdGroupPlayed(/* adGroupIndex= */ 1); + clear(); + assertGetNextMediaPeriodInfoReturnsContentMediaPeriod( + /* periodUid= */ firstPeriodUid, + /* startPositionUs= */ 0, + /* requestedContentPositionUs= */ C.TIME_UNSET, + /* endPositionUs= */ C.TIME_END_OF_SOURCE, + /* durationUs= */ CONTENT_DURATION_US, + /* isLastInPeriod= */ false, + /* isLastInWindow= */ false, + /* nextAdGroupIndex= */ 2); + setAdGroupPlayed(/* adGroupIndex= */ 2); + clear(); + assertGetNextMediaPeriodInfoReturnsContentMediaPeriod( + /* periodUid= */ firstPeriodUid, + /* startPositionUs= */ 0, + /* requestedContentPositionUs= */ C.TIME_UNSET, + /* endPositionUs= */ C.TIME_UNSET, + /* durationUs= */ CONTENT_DURATION_US, + /* isLastInPeriod= */ true, + /* isLastInWindow= */ true, + /* nextAdGroupIndex= */ C.INDEX_UNSET); + } + @Test public void getNextMediaPeriodInfo_inMultiPeriodWindow_returnsCorrectMediaPeriodInfos() { setupTimeline( @@ -269,14 +312,13 @@ public final class MediaPeriodQueueTest { setAdGroupLoaded(/* adGroupIndex= */ 0); setAdGroupLoaded(/* adGroupIndex= */ 1); enqueueNext(); // Content before first ad. - advancePlaying(); enqueueNext(); // First ad. enqueueNext(); // Content between ads. enqueueNext(); // Second ad. // Change position of second ad (= change duration of content between ads). updateAdPlaybackStateAndTimeline( - /* adGroupTimesUs...= */ FIRST_AD_START_TIME_US, SECOND_AD_START_TIME_US + 1); + /* adGroupTimesUs...= */ FIRST_AD_START_TIME_US, SECOND_AD_START_TIME_US - 1000); setAdGroupLoaded(/* adGroupIndex= */ 0); setAdGroupLoaded(/* adGroupIndex= */ 1); boolean changeHandled = @@ -294,15 +336,16 @@ public final class MediaPeriodQueueTest { setAdGroupLoaded(/* adGroupIndex= */ 0); setAdGroupLoaded(/* adGroupIndex= */ 1); enqueueNext(); // Content before first ad. - advancePlaying(); enqueueNext(); // First ad. enqueueNext(); // Content between ads. enqueueNext(); // Second ad. advanceReading(); // Reading first ad. + advanceReading(); // Reading content between ads. + advanceReading(); // Reading second ad. - // Change position of first ad (= change duration of content before first ad). + // Change position of second ad (= change duration of content between ads). updateAdPlaybackStateAndTimeline( - /* adGroupTimesUs...= */ FIRST_AD_START_TIME_US + 1, SECOND_AD_START_TIME_US); + /* adGroupTimesUs...= */ FIRST_AD_START_TIME_US, SECOND_AD_START_TIME_US - 1000); setAdGroupLoaded(/* adGroupIndex= */ 0); setAdGroupLoaded(/* adGroupIndex= */ 1); boolean changeHandled = @@ -312,7 +355,7 @@ public final class MediaPeriodQueueTest { /* maxRendererReadPositionUs= */ FIRST_AD_START_TIME_US); assertThat(changeHandled).isFalse(); - assertThat(getQueueLength()).isEqualTo(1); + assertThat(getQueueLength()).isEqualTo(3); } @Test @@ -322,7 +365,6 @@ public final class MediaPeriodQueueTest { setAdGroupLoaded(/* adGroupIndex= */ 0); setAdGroupLoaded(/* adGroupIndex= */ 1); enqueueNext(); // Content before first ad. - advancePlaying(); enqueueNext(); // First ad. enqueueNext(); // Content between ads. enqueueNext(); // Second ad. @@ -352,7 +394,6 @@ public final class MediaPeriodQueueTest { setAdGroupLoaded(/* adGroupIndex= */ 0); setAdGroupLoaded(/* adGroupIndex= */ 1); enqueueNext(); // Content before first ad. - advancePlaying(); enqueueNext(); // First ad. enqueueNext(); // Content between ads. enqueueNext(); // Second ad. @@ -382,7 +423,6 @@ public final class MediaPeriodQueueTest { setAdGroupLoaded(/* adGroupIndex= */ 0); setAdGroupLoaded(/* adGroupIndex= */ 1); enqueueNext(); // Content before first ad. - advancePlaying(); enqueueNext(); // First ad. enqueueNext(); // Content between ads. enqueueNext(); // Second ad. @@ -474,6 +514,21 @@ public final class MediaPeriodQueueTest { new RendererConfiguration[0], new ExoTrackSelection[0], /* info= */ null)); } + private void clear() { + mediaPeriodQueue.clear(); + playbackInfo = + playbackInfo.copyWithNewPosition( + mediaPeriodQueue.resolveMediaPeriodIdForAds( + mediaSourceList.createTimeline(), firstPeriodUid, /* positionUs= */ 0), + /* positionUs= */ 0, + /* requestedContentPositionUs= */ C.TIME_UNSET, + /* discontinuityStartPositionUs= */ 0, + /* totalBufferedDurationUs= */ 0, + /* trackGroups= */ null, + /* trackSelectorResult= */ null, + /* staticMetadata= */ ImmutableList.of()); + } + private MediaPeriodInfo getNextMediaPeriodInfo() { return mediaPeriodQueue.getNextMediaPeriodInfo(/* rendererPositionUs= */ 0, playbackInfo); } @@ -492,6 +547,13 @@ public final class MediaPeriodQueueTest { updateTimeline(); } + private void setAdGroupPlayed(int adGroupIndex) { + for (int i = 0; i < adPlaybackState.adGroups[adGroupIndex].count; i++) { + adPlaybackState = adPlaybackState.withPlayedAd(adGroupIndex, /* adIndexInAdGroup= */ i); + } + updateTimeline(); + } + private void setAdGroupFailedToLoad(int adGroupIndex) { adPlaybackState = adPlaybackState