From 09ede4aa45e87d978d299c39bfa64e91d00f501f Mon Sep 17 00:00:00 2001 From: tonihei Date: Tue, 8 Feb 2022 10:55:32 +0000 Subject: [PATCH] Fix delayed discontinuity for failed ad insertions. We have logic to not immediately interrupt playback when an ad group fails to load and instead let the current content play and transition at the point where the ad group should have been. This logic was broken by https://github.com/androidx/media/commit/dcbdbe53417d6642f2be98c82ac941d34908bd49 because of one of the conditions used MediaPeriodId.adGroupIndex, which is always -1 for content ids. It still worked for the last ad group because the next ad group index was C.INDEX_UNSET. Fix the issue and amend the test that was meant to catch this to test the ad failures for the last ad and previous ads. Also fix the PositionInfo reported in such a case, which was also wrong. Issue: google/ExoPlayer#9929 #minor-release PiperOrigin-RevId: 427143223 --- .../media3/exoplayer/ExoPlayerImpl.java | 13 +- .../exoplayer/ExoPlayerImplInternal.java | 2 +- .../media3/exoplayer/ExoPlayerTest.java | 179 ++++++++++++++++-- 3 files changed, 166 insertions(+), 28 deletions(-) diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java index f2b132bc9c..d77fc0ac9a 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java @@ -1932,8 +1932,6 @@ import java.util.concurrent.TimeoutException; long oldPositionUs; long oldContentPositionUs; if (positionDiscontinuityReason == DISCONTINUITY_REASON_AUTO_TRANSITION) { - oldPositionUs = oldPeriod.positionInWindowUs + oldPeriod.durationUs; - oldContentPositionUs = oldPositionUs; if (oldPlaybackInfo.periodId.isAd()) { // The old position is the end of the previous ad. oldPositionUs = @@ -1941,12 +1939,15 @@ import java.util.concurrent.TimeoutException; oldPlaybackInfo.periodId.adGroupIndex, oldPlaybackInfo.periodId.adIndexInAdGroup); // The ad cue point is stored in the old requested content position. oldContentPositionUs = getRequestedContentPositionUs(oldPlaybackInfo); - } else if (oldPlaybackInfo.periodId.nextAdGroupIndex != C.INDEX_UNSET - && playbackInfo.periodId.isAd()) { - // If it's a transition from content to an ad in the same window, the old position is the - // ad cue point that is the same as current content position. + } else if (oldPlaybackInfo.periodId.nextAdGroupIndex != C.INDEX_UNSET) { + // The old position is the end of a clipped content before an ad group. Use the exact ad + // cue point as the transition position. oldPositionUs = getRequestedContentPositionUs(playbackInfo); oldContentPositionUs = oldPositionUs; + } else { + // The old position is the end of a Timeline period. Use the exact duration. + oldPositionUs = oldPeriod.positionInWindowUs + oldPeriod.durationUs; + oldContentPositionUs = oldPositionUs; } } else if (oldPlaybackInfo.periodId.isAd()) { oldPositionUs = oldPlaybackInfo.positionUs; diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java index 1bbed758c9..cc32805042 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImplInternal.java @@ -2672,7 +2672,7 @@ import java.util.concurrent.atomic.AtomicBoolean; boolean earliestCuePointIsUnchangedOrLater = periodIdWithAds.nextAdGroupIndex == C.INDEX_UNSET || (oldPeriodId.nextAdGroupIndex != C.INDEX_UNSET - && periodIdWithAds.adGroupIndex >= oldPeriodId.nextAdGroupIndex); + && periodIdWithAds.nextAdGroupIndex >= oldPeriodId.nextAdGroupIndex); // Drop update if we keep playing the same content (MediaPeriod.periodUid are identical) and // the only change is that MediaPeriodId.nextAdGroupIndex increased. This postpones a potential // discontinuity until we reach the former next ad group position. diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java index 0961f4492d..5b7802c49f 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java @@ -730,7 +730,7 @@ public final class ExoPlayerTest { } @Test - public void adGroupWithLoadErrorIsSkipped() throws Exception { + public void adGroupWithLoadError_noFurtherAdGroup_isSkipped() throws Exception { AdPlaybackState initialAdPlaybackState = FakeTimeline.createAdPlaybackState( /* adsPerAdGroup= */ 1, /* adGroupTimesUs...= */ @@ -745,11 +745,12 @@ public final class ExoPlayerTest { /* isDynamic= */ false, /* isLive= */ false, /* isPlaceholder= */ false, - /* durationUs= */ C.MICROS_PER_SECOND, + /* durationUs= */ 10 * C.MICROS_PER_SECOND, /* defaultPositionUs= */ 0, TimelineWindowDefinition.DEFAULT_WINDOW_OFFSET_IN_FIRST_PERIOD_US, initialAdPlaybackState)); - AdPlaybackState errorAdPlaybackState = initialAdPlaybackState.withAdLoadError(0, 0); + AdPlaybackState errorAdPlaybackState = + initialAdPlaybackState.withAdLoadError(/* adGroupIndex= */ 0, /* adIndexInAdGroup= */ 0); final Timeline adErrorTimeline = new FakeTimeline( new TimelineWindowDefinition( @@ -759,30 +760,166 @@ public final class ExoPlayerTest { /* isDynamic= */ false, /* isLive= */ false, /* isPlaceholder= */ false, - /* durationUs= */ C.MICROS_PER_SECOND, + /* durationUs= */ 10 * C.MICROS_PER_SECOND, /* defaultPositionUs= */ 0, TimelineWindowDefinition.DEFAULT_WINDOW_OFFSET_IN_FIRST_PERIOD_US, errorAdPlaybackState)); final FakeMediaSource fakeMediaSource = new FakeMediaSource(fakeTimeline, ExoPlayerTestRunner.VIDEO_FORMAT); - ActionSchedule actionSchedule = - new ActionSchedule.Builder(TAG) - .pause() - .waitForPlaybackState(Player.STATE_READY) - .executeRunnable(() -> fakeMediaSource.setNewSourceInfo(adErrorTimeline)) - .waitForTimelineChanged( - adErrorTimeline, /* expectedReason */ Player.TIMELINE_CHANGE_REASON_SOURCE_UPDATE) - .play() - .build(); - ExoPlayerTestRunner testRunner = - new ExoPlayerTestRunner.Builder(context) - .setMediaSources(fakeMediaSource) - .setActionSchedule(actionSchedule) - .build() - .start() - .blockUntilEnded(TIMEOUT_MS); + ExoPlayer player = new TestExoPlayerBuilder(context).build(); + Player.Listener mockListener = mock(Player.Listener.class); + player.addListener(mockListener); + + player.setMediaSource(fakeMediaSource); + player.prepare(); + runUntilPlaybackState(player, Player.STATE_READY); + fakeMediaSource.setNewSourceInfo(adErrorTimeline); + player.play(); + runUntilPlaybackState(player, Player.STATE_ENDED); + Timeline.Window window = + player.getCurrentTimeline().getWindow(/* windowIndex= */ 0, new Timeline.Window()); + Timeline.Period period = + player + .getCurrentTimeline() + .getPeriod(/* periodIndex= */ 0, new Timeline.Period(), /* setIds= */ true); + player.release(); + // There is still one discontinuity from content to content for the failed ad insertion. - testRunner.assertPositionDiscontinuityReasonsEqual(Player.DISCONTINUITY_REASON_AUTO_TRANSITION); + PositionInfo positionInfo = + new PositionInfo( + window.uid, + /* mediaItemIndex= */ 0, + window.mediaItem, + period.uid, + /* periodIndex= */ 0, + /* positionMs= */ 5_000, + /* contentPositionMs= */ 5_000, + /* adGroupIndex= */ C.INDEX_UNSET, + /* adIndexInAdGroup= */ C.INDEX_UNSET); + verify(mockListener) + .onPositionDiscontinuity( + positionInfo, positionInfo, Player.DISCONTINUITY_REASON_AUTO_TRANSITION); + } + + @Test + public void adGroupWithLoadError_withFurtherAdGroup_isSkipped() throws Exception { + AdPlaybackState initialAdPlaybackState = + FakeTimeline.createAdPlaybackState( + /* adsPerAdGroup= */ 1, /* adGroupTimesUs...= */ + TimelineWindowDefinition.DEFAULT_WINDOW_OFFSET_IN_FIRST_PERIOD_US + + 5 * C.MICROS_PER_SECOND, + TimelineWindowDefinition.DEFAULT_WINDOW_OFFSET_IN_FIRST_PERIOD_US + + 8 * C.MICROS_PER_SECOND); + Timeline fakeTimeline = + new FakeTimeline( + new TimelineWindowDefinition( + /* periodCount= */ 1, + /* id= */ 0, + /* isSeekable= */ true, + /* isDynamic= */ false, + /* isLive= */ false, + /* isPlaceholder= */ false, + /* durationUs= */ 10 * C.MICROS_PER_SECOND, + /* defaultPositionUs= */ 0, + TimelineWindowDefinition.DEFAULT_WINDOW_OFFSET_IN_FIRST_PERIOD_US, + initialAdPlaybackState)); + AdPlaybackState errorAdPlaybackState = + initialAdPlaybackState.withAdLoadError(/* adGroupIndex= */ 0, /* adIndexInAdGroup= */ 0); + final Timeline adErrorTimeline = + new FakeTimeline( + new TimelineWindowDefinition( + /* periodCount= */ 1, + /* id= */ 0, + /* isSeekable= */ true, + /* isDynamic= */ false, + /* isLive= */ false, + /* isPlaceholder= */ false, + /* durationUs= */ 10 * C.MICROS_PER_SECOND, + /* defaultPositionUs= */ 0, + TimelineWindowDefinition.DEFAULT_WINDOW_OFFSET_IN_FIRST_PERIOD_US, + errorAdPlaybackState)); + final FakeMediaSource fakeMediaSource = + new FakeMediaSource(fakeTimeline, ExoPlayerTestRunner.VIDEO_FORMAT); + ExoPlayer player = new TestExoPlayerBuilder(context).build(); + Player.Listener mockListener = mock(Player.Listener.class); + player.addListener(mockListener); + + player.setMediaSource(fakeMediaSource); + player.prepare(); + runUntilPlaybackState(player, Player.STATE_READY); + fakeMediaSource.setNewSourceInfo(adErrorTimeline); + player.play(); + runUntilPlaybackState(player, Player.STATE_ENDED); + Timeline.Window window = + player.getCurrentTimeline().getWindow(/* windowIndex= */ 0, new Timeline.Window()); + Timeline.Period period = + player + .getCurrentTimeline() + .getPeriod(/* periodIndex= */ 0, new Timeline.Period(), /* setIds= */ true); + player.release(); + + // There is still one discontinuity from content to content for the failed ad insertion and the + // normal ad transition for the successful ad insertion. + PositionInfo positionInfoFailedAd = + new PositionInfo( + window.uid, + /* mediaItemIndex= */ 0, + window.mediaItem, + period.uid, + /* periodIndex= */ 0, + /* positionMs= */ 5_000, + /* contentPositionMs= */ 5_000, + /* adGroupIndex= */ C.INDEX_UNSET, + /* adIndexInAdGroup= */ C.INDEX_UNSET); + verify(mockListener) + .onPositionDiscontinuity( + positionInfoFailedAd, + positionInfoFailedAd, + Player.DISCONTINUITY_REASON_AUTO_TRANSITION); + PositionInfo positionInfoContentAtSuccessfulAd = + new PositionInfo( + window.uid, + /* mediaItemIndex= */ 0, + window.mediaItem, + period.uid, + /* periodIndex= */ 0, + /* positionMs= */ 8_000, + /* contentPositionMs= */ 8_000, + /* adGroupIndex= */ C.INDEX_UNSET, + /* adIndexInAdGroup= */ C.INDEX_UNSET); + PositionInfo positionInfoSuccessfulAdStart = + new PositionInfo( + window.uid, + /* mediaItemIndex= */ 0, + window.mediaItem, + period.uid, + /* periodIndex= */ 0, + /* positionMs= */ 0, + /* contentPositionMs= */ 8_000, + /* adGroupIndex= */ 1, + /* adIndexInAdGroup= */ 0); + PositionInfo positionInfoSuccessfulAdEnd = + new PositionInfo( + window.uid, + /* mediaItemIndex= */ 0, + window.mediaItem, + period.uid, + /* periodIndex= */ 0, + /* positionMs= */ Util.usToMs( + period.getAdDurationUs(/* adGroupIndex= */ 1, /* adIndexInAdGroup= */ 0)), + /* contentPositionMs= */ 8_000, + /* adGroupIndex= */ 1, + /* adIndexInAdGroup= */ 0); + verify(mockListener) + .onPositionDiscontinuity( + positionInfoContentAtSuccessfulAd, + positionInfoSuccessfulAdStart, + Player.DISCONTINUITY_REASON_AUTO_TRANSITION); + verify(mockListener) + .onPositionDiscontinuity( + positionInfoSuccessfulAdEnd, + positionInfoContentAtSuccessfulAd, + Player.DISCONTINUITY_REASON_AUTO_TRANSITION); } @Test