From d54529b7b7eff67f553103632f6e67826f1bfe8a Mon Sep 17 00:00:00 2001
From: Steve Mayhew <mayhew.steve@gmail.com>
Date: Fri, 25 Jul 2025 09:53:47 -0700
Subject: [PATCH] Ensures errors are correctly attributed to ad or content
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../media3/exoplayer/ima/AdTagLoader.java     | 24 +++++++++++++++----
 1 file changed, 19 insertions(+), 5 deletions(-)

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 e03b2293fce..c6a61fd05e8 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
@@ -391,7 +391,23 @@ public void activate(Player player) {
   /** Deactivates playback. */
   public void deactivate() {
     Player player = checkNotNull(this.player);
-    if (!AdPlaybackState.NONE.equals(adPlaybackState) && imaPausedContent) {
+    // Post release of listener behind any already queued Player.Listener events to ensure that
+    // any pending events are processed before the player is deferred.
+    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, need to
+   * determine whether the error was for an ad or content.
+   */
+  private void deactivateInternal(Player player) {
+    if (!AdPlaybackState.NONE.equals(adPlaybackState)
+        && imaPausedContent
+        && player.getPlayerError() == null) {
       if (adsManager != null) {
         adsManager.pause();
       }
@@ -402,9 +418,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;
   }
 
@@ -539,7 +553,7 @@ public void onPlayWhenReadyChanged(
 
   @Override
   public void onPlayerError(PlaybackException error) {
-    if (imaAdState != IMA_AD_STATE_NONE) {
+    if (imaAdState != IMA_AD_STATE_NONE && player.isPlayingAd()) {
       AdMediaInfo adMediaInfo = checkNotNull(imaAdMediaInfo);
       for (int i = 0; i < adCallbacks.size(); i++) {
         adCallbacks.get(i).onError(adMediaInfo);