Remove some ad playback state change requirements

Ads can appear due to asynchronous ad tag requests completing after earlier ads in a pod have loaded, so remove the requirement that the ad count can't change. The MediaPeriodQueue should handling discarding buffered content if an ad appears before already buffered content, so I think this case is actually handled correctly by the core player already. Also remove the requirement that an ad URI can't change. This is a defensive measure for now, but it's likely that a later fix in the IMA SDK for an issue where loadAd is not called after preloading then seeking before a preloaded ad plays will result in loadAd being called more than once, and I think it's possible that the second call to loadAd may have a different URI. Because the ad URI should only change after an intermediate seek to another MediaPeriod, there shouldn't be any problems with buffered data not getting discarded. Issue: #7477 PiperOrigin-RevId: 316871371
google · Jun 17, 2020 · fc76dbf · fc76dbf
1 parent 5fd287b
commit fc76dbf
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 20 deletions.
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -13,6 +13,8 @@
     *   Fix a bug that caused playback to be stuck buffering on resuming from
         the background after all ads had played to the end
         ([#7508](https://github.com/google/ExoPlayer/issues/7508)).
+    *   Fix a bug where the number of ads in an ad group couldn't change
+        ([#7477](https://github.com/google/ExoPlayer/issues/7477)).
 
 ### 2.11.5 (2020-06-05) ###
 

diff --git a/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoader.java b/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoader.java
@@ -1476,6 +1476,7 @@ public void loadAd(AdMediaInfo adMediaInfo, AdPodInfo adPodInfo) {
           // Drop events after release.
           return;
         }
+
         int adGroupIndex = getAdGroupIndexForAdPod(adPodInfo);
         int adIndexInAdGroup = adPodInfo.getAdPosition() - 1;
         AdInfo adInfo = new AdInfo(adGroupIndex, adIndexInAdGroup);
@@ -1485,21 +1486,23 @@ public void loadAd(AdMediaInfo adMediaInfo, AdPodInfo adPodInfo) {
           // will timeout after its media load timeout.
           return;
         }
+
+        // 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 ad has loaded. See also https://github.com/google/ExoPlayer/issues/7477.
         AdPlaybackState.AdGroup adGroup = adPlaybackState.adGroups[adInfo.adGroupIndex];
-        if (adGroup.count == C.LENGTH_UNSET) {
-          adPlaybackState =
-              adPlaybackState.withAdCount(
-                  adInfo.adGroupIndex, Math.max(adPodInfo.getTotalAds(), adGroup.states.length));
-          adGroup = adPlaybackState.adGroups[adInfo.adGroupIndex];
-        }
+        adPlaybackState =
+            adPlaybackState.withAdCount(
+                adInfo.adGroupIndex, Math.max(adPodInfo.getTotalAds(), adGroup.states.length));
+        adGroup = adPlaybackState.adGroups[adInfo.adGroupIndex];
         for (int i = 0; i < adIndexInAdGroup; i++) {
           // Any preceding ads that haven't loaded are not going to load.
           if (adGroup.states[i] == AdPlaybackState.AD_STATE_UNAVAILABLE) {
             adPlaybackState =
-                adPlaybackState.withAdLoadError(
-                    /* adGroupIndex= */ adGroupIndex, /* adIndexInAdGroup= */ i);
+                adPlaybackState.withAdLoadError(adGroupIndex, /* adIndexInAdGroup= */ i);
           }
         }
+
         Uri adUri = Uri.parse(adMediaInfo.getUrl());
         adPlaybackState =
             adPlaybackState.withAdUri(adInfo.adGroupIndex, adInfo.adIndexInAdGroup, adUri);

diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdPlaybackState.java b/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdPlaybackState.java
@@ -124,13 +124,9 @@ public int hashCode() {
       return result;
     }
 
-    /**
-     * Returns a new instance with the ad count set to {@code count}. This method may only be called
-     * if this instance's ad count has not yet been specified.
-     */
+    /** Returns a new instance with the ad count set to {@code count}. */
     @CheckResult
     public AdGroup withAdCount(int count) {
-      Assertions.checkArgument(this.count == C.LENGTH_UNSET && states.length <= count);
       @AdState int[] states = copyStatesWithSpaceForAdCount(this.states, count);
       long[] durationsUs = copyDurationsUsWithSpaceForAdCount(this.durationsUs, count);
       @NullableType Uri[] uris = Arrays.copyOf(this.uris, count);
@@ -139,17 +135,11 @@ public AdGroup withAdCount(int count) {
 
     /**
      * Returns a new instance with the specified {@code uri} set for the specified ad, and the ad
-     * marked as {@link #AD_STATE_AVAILABLE}. The specified ad must currently be in {@link
-     * #AD_STATE_UNAVAILABLE}, which is the default state.
-     *
-     * <p>This instance's ad count may be unknown, in which case {@code index} must be less than the
-     * ad count specified later. Otherwise, {@code index} must be less than the current ad count.
+     * marked as {@link #AD_STATE_AVAILABLE}.
      */
     @CheckResult
     public AdGroup withAdUri(Uri uri, int index) {
-      Assertions.checkArgument(count == C.LENGTH_UNSET || index < count);
       @AdState int[] states = copyStatesWithSpaceForAdCount(this.states, index + 1);
-      Assertions.checkArgument(states[index] == AD_STATE_UNAVAILABLE);
       long[] durationsUs =
           this.durationsUs.length == states.length
               ? this.durationsUs