Ensures errors are correctly attributed to ad or content #2655
+54
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note a branch with extensive logging and a partial solution for User Facing Issue #4 is in my branch from this change: https://github.com/stevemayhew/media/tree/p-fixes-ad-error-handling-logging |
1 task
|
Thanks for providing the fix, only now found the time to have a closer look. The fix looks correct to me, I'm going to merge it internally. |
This change is part of a fix to the issue "Player Errors during Ad playback not attributed correctly" The change introduced in commit 7cf2fd9 was a partial fix to allow ad playback errors to be reported. It delayed removing the listener when an error released the AdsMediaSource. What remained was the larger issue: When an error occurs **during content preparation while an ad is still playing**, ExoPlayer cannot correctly attribute the error to the content source. Instead, the error bubbles up through `ExoPlayerImplInternal.stopInternal()` and causes the **entire CompositeMediaSource (AdsMediaSource + content)** to be torn down. This results in the `ImaAdsLoader` being stopped prematurely — even when the ad is not at fault — and breaks proper ad lifecycle handling. **User-Facing Issues** 1. **The ad is incorrectly marked as failed** A content preparation error is misattributed to the currently playing ad, resulting in an invalid `AD_STATE_ERROR`. 2. **The ad is never marked as completed** Even though `ALL_ADS_COMPLETED` is dispatched by IMA, the player is torn down before ExoPlayer updates `AdPlaybackState` to reflect completion. 3. **A single failing ad in a pod is not skipped** If one ad in a pod fails (e.g., due to a 404 or DRM error), the player stops entirely instead of skipping to the next ad in the group. 4. **A failing ad does not fall back to content** When an ad fails, the player should skip to content. Instead, the entire playback pipeline is torn down via `stopInternal()`. This change fixes the first two User-Facing issues. When combined with error recovery logic it also fixes the 4th issue. The 3rd issue may require some work on the IMA SDK side, the mid-pod failure aborts all ad playback with this fix, rather than skipping just the failing ad.
tonihei
force-pushed
the
p-fixes-ad-error-handling
branch
from
d54529b to
f946714
Compare
tonihei
reviewed
Oct 13, 2025
| private void deactivateInternal(Player player) { | ||
| if (!adPlaybackState.equals(AdPlaybackState.NONE) | ||
| && imaPausedContent | ||
| && player.getPlayerError() == null) { |
There was a problem hiding this comment.
@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?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change is part of a fix to the issue "Player Errors during Ad playback not attributed correctly"
The change introduced in commit 7cf2fd9 was a partial fix to allow ad playback errors to be reported. It delayed removing the listener when an error released the AdsMediaSource.
What remained was the larger issue: When an error occurs during content preparation while an ad is still playing, ExoPlayer cannot correctly attribute the error to the content source. Instead, the error bubbles up through
ExoPlayerImplInternal.stopInternal()and causes the entire CompositeMediaSource (AdsMediaSource + content) to be torn down.This results in the
ImaAdsLoaderbeing stopped prematurely — even when the ad is not at fault — and breaks proper ad lifecycle handling.User-Facing Issues
AD_STATE_ERROR.ALL_ADS_COMPLETEDis dispatched by IMA, the player is torn down before ExoPlayer updatesAdPlaybackStateto reflect completion.stopInternal().This change fixes the first two User-Facing issues. When combined with error recovery logic it also fixes the 4th issue. The 3rd issue may require some work on the IMA SDK side, the mid-pod failure aborts all ad playback with this fix, rather than skipping just the failing ad.