Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions RELEASENOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
side when `Presentation` is created with a single side length.
* Muxers:
* IMA extension:
* Fix issue where content preparation error for content after an ad would
be wrongly reported as an ad playback error
([#2656](https://github.com/androidx/media/issues/2656)).
* Session:
* Add `CommandButton.executeAction` so that controllers can more easily
trigger the intended action. Also allow to specify parameters for some
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,21 @@ public void activate(Player player) {
/** Deactivates playback. */
public void deactivate() {
Player player = checkNotNull(this.player);
if (!AdPlaybackState.NONE.equals(adPlaybackState) && imaPausedContent) {
// Post deactivation behind any already queued Player.Listener events to ensure that
// any pending events are processed before the listener is removed and the ads manager paused.
handler.post(() -> deactivateInternal(player));
}

/**
* Deactivates playback internally, after the Listener.onEvents() cycle completes so the complete
* state change picture is clear. For example, if an error caused the deactivation, the error
* callback can be handled first.
*/
private void deactivateInternal(Player player) {
if (!adPlaybackState.equals(AdPlaybackState.NONE)
&& imaPausedContent
&& player.getPlayerError() == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevemayhew Could you explain why this additional check is needed again? What would go wrong with marking the adsmanager as paused and storing the ad resume position in case of an error in the player?

// Only need to pause and store resume position if not in error state.
if (adsManager != null) {
adsManager.pause();
}
Expand All @@ -402,9 +416,7 @@ public void deactivate() {
lastVolumePercent = getPlayerVolumePercent();
lastAdProgress = getAdVideoProgressUpdate();
lastContentProgress = getContentVideoProgressUpdate();

// Post release of listener so that we can report any already pending errors via onPlayerError.
handler.post(() -> player.removeListener(this));
player.removeListener(this);
this.player = null;
}

Expand Down Expand Up @@ -539,7 +551,7 @@ public void onPlayWhenReadyChanged(

@Override
public void onPlayerError(PlaybackException error) {
if (imaAdState != IMA_AD_STATE_NONE) {
if (imaAdState != IMA_AD_STATE_NONE && checkNotNull(player).isPlayingAd()) {
AdMediaInfo adMediaInfo = checkNotNull(imaAdMediaInfo);
for (int i = 0; i < adCallbacks.size(); i++) {
adCallbacks.get(i).onError(adMediaInfo);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1481,6 +1481,41 @@ public void buildWithEnableContinuousPlayback_setsAdsRequestProperty() {
verify(mockAdsRequest).setContinuousPlayback(true);
}

@Test
public void contentErrorDuringAdPlayback_doesNotMarkAdAsFailed() throws IOException {
// Load and play a preroll ad.
imaAdsLoader.start(
adsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, adsLoaderListener);
adEventListener.onAdEvent(getAdEvent(AdEventType.LOADED, mockPrerollSingleAd));
videoAdPlayer.loadAd(TEST_AD_MEDIA_INFO, mockAdPodInfo);
adEventListener.onAdEvent(getAdEvent(AdEventType.CONTENT_PAUSE_REQUESTED, mockPrerollSingleAd));
videoAdPlayer.playAd(TEST_AD_MEDIA_INFO);
fakePlayer.setPlayingAdPosition(
/* periodIndex= */ 0,
/* adGroupIndex= */ 0,
/* adIndexInAdGroup= */ 0,
/* positionMs= */ 0,
/* contentPositionMs= */ 0);
fakePlayer.setState(Player.STATE_READY, /* playWhenReady= */ true);
adEventListener.onAdEvent(getAdEvent(AdEventType.STARTED, mockPrerollSingleAd));

// Simulate a content preparation error while the ad is playing.
// The player is not playing an ad from its perspective, so isPlayingAd() is false.
fakePlayer.setPlayingContentPosition(/* periodIndex= */ 0, /* positionMs= */ 0);
ExoPlaybackException error =
ExoPlaybackException.createForSource(
new IOException("Content preparation error"),
PlaybackException.ERROR_CODE_IO_UNSPECIFIED);
fakePlayer.setPlayerError(error);
shadowOf(Looper.getMainLooper()).runToEndOfTasks();

// Verify that the ad is not marked as failed.
AdPlaybackState adPlaybackState = getAdPlaybackState(0);
assertThat(adPlaybackState.getAdGroup(0).states[0])
.isNotEqualTo(AdPlaybackState.AD_STATE_ERROR);
verify(mockVideoAdPlayerCallback, never()).onError(any());
}

private void setupMocks() {
ArgumentCaptor<Object> userRequestContextCaptor = ArgumentCaptor.forClass(Object.class);
doNothing().when(mockAdsRequest).setUserRequestContext(userRequestContextCaptor.capture());
Expand Down