diff --git a/RELEASENOTES.md b/RELEASENOTES.md index a9c94ee336..c27b5e75ed 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -47,6 +47,8 @@ * `writeSampleData()` API now uses muxer specific `BufferInfo` class instead of `MediaCodec.BufferInfo`. * IMA extension: + * Fix a bug where a load error in one ad may accidentally invalidate + another ad group. * Session: * UI: * Downloads: diff --git a/libraries/exoplayer_ima/src/main/java/androidx/media3/exoplayer/ima/AdTagLoader.java b/libraries/exoplayer_ima/src/main/java/androidx/media3/exoplayer/ima/AdTagLoader.java index b1e03fc159..e080a3a22b 100644 --- a/libraries/exoplayer_ima/src/main/java/androidx/media3/exoplayer/ima/AdTagLoader.java +++ b/libraries/exoplayer_ima/src/main/java/androidx/media3/exoplayer/ima/AdTagLoader.java @@ -801,9 +801,9 @@ import java.util.Objects; private void resumeContentInternal() { if (imaAdInfo != null) { - // Remove any pending timeout tasks as CONTENT_RESUME_REQUESTED may occur instead of loadAd. + // Mark current ad group as skipped if it hasn't finished yet. This could for example happen + // after a load timeout where we receive CONTENT_RESUME_REQUESTED instead of loadAd. // See [Internal: b/330750756]. - handler.removeCallbacks(adLoadTimeoutRunnable); adPlaybackState = adPlaybackState.withSkippedAdGroup(checkNotNull(imaAdInfo).adGroupIndex); updateAdPlaybackState(); } @@ -844,6 +844,9 @@ import java.util.Objects; if (adGroupIndex == C.INDEX_UNSET) { return false; } + if (adGroupIndex >= adPlaybackState.adGroupCount) { + return true; + } AdPlaybackState.AdGroup adGroup = adPlaybackState.getAdGroup(adGroupIndex); int adIndexInAdGroup = player.getCurrentAdIndexInAdGroup(); if (adGroup.count == C.LENGTH_UNSET || adGroup.count <= adIndexInAdGroup) { @@ -972,12 +975,6 @@ import java.util.Objects; // timeout after its media load timeout. return; } - if (player != null - && player.getCurrentAdGroupIndex() == adGroupIndex - && player.getCurrentAdIndexInAdGroup() == adIndexInAdGroup) { - // Loaded ad info the player is currently waiting for. - handler.removeCallbacks(adLoadTimeoutRunnable); - } // The ad count may increase on successive loads of ads in the same ad pod, for example, due to // separate requests for ad tags with multiple ads within the ad pod completing after an earlier @@ -1129,6 +1126,11 @@ import java.util.Objects; } private void handleAdLoadTimeout() { + // We started the timeout when we were first waiting for the current ad to load. Check if we are + // still waiting after the timeout before triggering the error event. + if (!isWaitingForCurrentAdToLoad()) { + return; + } // IMA got stuck and didn't load an ad in time, so skip the entire group. handleAdGroupLoadError(new IOException("Ad loading timed out")); maybeNotifyPendingAdLoadError(); diff --git a/libraries/exoplayer_ima/src/test/java/androidx/media3/exoplayer/ima/ImaAdsLoaderTest.java b/libraries/exoplayer_ima/src/test/java/androidx/media3/exoplayer/ima/ImaAdsLoaderTest.java index a121992446..a94c1a314b 100644 --- a/libraries/exoplayer_ima/src/test/java/androidx/media3/exoplayer/ima/ImaAdsLoaderTest.java +++ b/libraries/exoplayer_ima/src/test/java/androidx/media3/exoplayer/ima/ImaAdsLoaderTest.java @@ -27,6 +27,7 @@ import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.robolectric.Shadows.shadowOf; @@ -95,6 +96,7 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; import org.mockito.stubbing.Answer; +import org.robolectric.shadows.ShadowLooper; import org.robolectric.shadows.ShadowSystemClock; /** Tests for {@link ImaAdsLoader}. */ @@ -450,6 +452,40 @@ public final class ImaAdsLoaderTest { .withContentDurationUs(CONTENT_PERIOD_DURATION_US)); } + @Test + public void playback_withAdErrorWhileWaitingForPreload_doesNotReportAdLoadErrorAfterTimeout() { + // Simulate an ad group at 0 seconds and another at 10 seconds. + when(mockAdsManager.getAdCuePoints()).thenReturn(ImmutableList.of(0f, 10.0f)); + AdEvent mockMidrollFetchErrorAdEvent = mock(AdEvent.class); + when(mockMidrollFetchErrorAdEvent.getType()).thenReturn(AdEventType.AD_BREAK_FETCH_ERROR); + when(mockMidrollFetchErrorAdEvent.getAdData()) + .thenReturn(ImmutableMap.of("adBreakTime", "0.0")); + AdEvent mockContentResumeAdEvent = mock(AdEvent.class); + when(mockContentResumeAdEvent.getType()).thenReturn(AdEventType.CONTENT_RESUME_REQUESTED); + AdsLoader.EventListener mockEventListener = + spy(new TestAdsLoaderListener(/* periodIndex= */ 0)); + // Advance playback to the start of the ad and simulate buffering. + imaAdsLoader.start( + adsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, mockEventListener); + videoAdPlayer.loadAd(TEST_AD_MEDIA_INFO, mockAdPodInfo); + fakePlayer.setState(Player.STATE_BUFFERING, /* playWhenReady= */ true); + fakePlayer.setPlayingAdPosition( + /* periodIndex= */ 0, + /* adGroupIndex= */ 0, + /* adIndexInAdGroup= */ 0, + /* positionMs= */ 0, + /* contentPositionMs= */ 0); + + // Simulate an ad load error and request to resume content from the Ads SDK. + adEventListener.onAdEvent(mockMidrollFetchErrorAdEvent); + adEventListener.onAdEvent(mockContentResumeAdEvent); + // Wait beyond the timeout to see if any error events from the timeout arrive after this point. + ShadowSystemClock.advanceBy(Duration.ofSeconds(10)); + ShadowLooper.idleMainLooper(); + + verify(mockEventListener, never()).onAdLoadError(any(), any()); + } + @Test public void playback_withAdNotPreloadingAfterTimeout_hasErrorAdGroup() { // Simulate an ad at 2 seconds. @@ -607,7 +643,7 @@ public final class ImaAdsLoaderTest { /* contentPositionMs= */ 0); shadowOf(Looper.getMainLooper()).runToEndOfTasks(); - verify(mockVideoAdPlayerCallback) + verify(mockVideoAdPlayerCallback, atLeastOnce()) .onAdProgress( TEST_AD_MEDIA_INFO, new VideoProgressUpdate(newPlayerPositionMs, Util.usToMs(TEST_AD_DURATION_US))); @@ -1438,7 +1474,7 @@ public final class ImaAdsLoaderTest { } /** Ad loader event listener that forwards ad playback state to a fake player. */ - private final class TestAdsLoaderListener implements AdsLoader.EventListener { + private class TestAdsLoaderListener implements AdsLoader.EventListener { private final int periodIndex;