From 9392dff22560736fd9169370e9a64ba1d49663cd Mon Sep 17 00:00:00 2001 From: andrewlewis Date: Mon, 3 Aug 2020 10:51:52 +0100 Subject: [PATCH] Call VideoAdPlayerCallback.onLoaded This callback was not notified before, which could theoretically lead to ad loading timing out. In practice it doesn't currently happen because the timeout appears to start when the ad cue point is reached, not when loadAd is called. We notify onLoaded when the ad media period is prepared (for HTML5 the recommendation is to notify on the HTMLMediaElement 'canplay' event, which this roughly corresponds to). PiperOrigin-RevId: 324568407 --- RELEASENOTES.md | 1 + .../exoplayer2/ext/ima/ImaAdsLoader.java | 23 ++++++++++++++++--- .../exoplayer2/source/MaskingMediaPeriod.java | 18 ++++++++++----- .../exoplayer2/source/ads/AdsLoader.java | 9 ++++++++ .../exoplayer2/source/ads/AdsMediaSource.java | 16 +++++++++---- .../android/exoplayer2/ExoPlayerTest.java | 3 +++ 6 files changed, 57 insertions(+), 13 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 310d013d43..669bae2013 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -260,6 +260,7 @@ `AdsLoader.AdViewProvider`. * Add `ImaAdsLoader.Builder.setCompanionAdSlots` so it's possible to set companion ad slots without accessing the `AdDisplayContainer`. + * Add missing notification of `VideoAdPlayerCallback.onLoaded`. * Demo app: * Retain previous position in list of samples. * Replace the `extensions` variant with `decoderExtensions` and make the 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 index 2b4137e147..834b7e546c 100644 --- 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 @@ -68,6 +68,8 @@ import com.google.android.exoplayer2.upstream.DataSpec; import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.MimeTypes; import com.google.android.exoplayer2.util.Util; +import com.google.common.collect.BiMap; +import com.google.common.collect.HashBiMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import java.io.IOException; @@ -78,7 +80,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -415,7 +416,7 @@ public final class ImaAdsLoader private final ComponentListener componentListener; private final List adCallbacks; private final Runnable updateAdProgressRunnable; - private final Map adInfoByAdMediaInfo; + private final BiMap adInfoByAdMediaInfo; private @MonotonicNonNull AdDisplayContainer adDisplayContainer; private @MonotonicNonNull AdsLoader adsLoader; @@ -563,7 +564,7 @@ public final class ImaAdsLoader componentListener = new ComponentListener(); adCallbacks = new ArrayList<>(/* initialCapacity= */ 1); updateAdProgressRunnable = this::updateAdProgress; - adInfoByAdMediaInfo = new HashMap<>(); + adInfoByAdMediaInfo = HashBiMap.create(); supportedMimeTypes = Collections.emptyList(); lastContentProgress = VideoProgressUpdate.VIDEO_TIME_NOT_READY; lastAdProgress = VideoProgressUpdate.VIDEO_TIME_NOT_READY; @@ -751,6 +752,22 @@ public final class ImaAdsLoader updateAdPlaybackState(); } + @Override + public void handlePrepareComplete(int adGroupIndex, int adIndexInAdGroup) { + AdInfo adInfo = new AdInfo(adGroupIndex, adIndexInAdGroup); + if (DEBUG) { + Log.d(TAG, "Prepared ad " + adInfo); + } + @Nullable AdMediaInfo adMediaInfo = adInfoByAdMediaInfo.inverse().get(adInfo); + if (adMediaInfo != null) { + for (int i = 0; i < adCallbacks.size(); i++) { + adCallbacks.get(i).onLoaded(adMediaInfo); + } + } else { + Log.w(TAG, "Unexpected prepared ad " + adInfo); + } + } + @Override public void handlePrepareError(int adGroupIndex, int adIndexInAdGroup, IOException exception) { if (player == null) { diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/MaskingMediaPeriod.java b/library/core/src/main/java/com/google/android/exoplayer2/source/MaskingMediaPeriod.java index 142527af7d..9514241035 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/MaskingMediaPeriod.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/MaskingMediaPeriod.java @@ -34,8 +34,11 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; */ public final class MaskingMediaPeriod implements MediaPeriod, MediaPeriod.Callback { - /** Listener for preparation errors. */ - public interface PrepareErrorListener { + /** Listener for preparation events. */ + public interface PrepareListener { + + /** Called when preparing the media period completes. */ + void onPrepareComplete(MediaPeriodId mediaPeriodId); /** * Called the first time an error occurs while refreshing source info or preparing the period. @@ -53,7 +56,7 @@ public final class MaskingMediaPeriod implements MediaPeriod, MediaPeriod.Callba @Nullable private MediaPeriod mediaPeriod; @Nullable private Callback callback; private long preparePositionUs; - @Nullable private PrepareErrorListener listener; + @Nullable private PrepareListener listener; private boolean notifiedPrepareError; private long preparePositionOverrideUs; @@ -75,13 +78,13 @@ public final class MaskingMediaPeriod implements MediaPeriod, MediaPeriod.Callba } /** - * Sets a listener for preparation errors. + * Sets a listener for preparation events. * - * @param listener An listener to be notified of media period preparation errors. If a listener is + * @param listener An listener to be notified of media period preparation events. If a listener is * set, {@link #maybeThrowPrepareError()} will not throw but will instead pass the first * preparation error (if any) to the listener. */ - public void setPrepareErrorListener(PrepareErrorListener listener) { + public void setPrepareListener(PrepareListener listener) { this.listener = listener; } @@ -231,6 +234,9 @@ public final class MaskingMediaPeriod implements MediaPeriod, MediaPeriod.Callba @Override public void onPrepared(MediaPeriod mediaPeriod) { castNonNull(callback).onPrepared(this); + if (listener != null) { + listener.onPrepareComplete(id); + } } private long getPreparePositionWithOverride(long preparePositionUs) { diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsLoader.java b/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsLoader.java index df50a31d74..093ca1d5c4 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsLoader.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsLoader.java @@ -210,6 +210,15 @@ public interface AdsLoader { */ void stop(); + /** + * Notifies the ads loader that preparation of an ad media period is complete. Called on the main + * thread by {@link AdsMediaSource}. + * + * @param adGroupIndex The index of the ad group. + * @param adIndexInAdGroup The index of the ad in the ad group. + */ + void handlePrepareComplete(int adGroupIndex, int adIndexInAdGroup); + /** * Notifies the ads loader that the player was not able to prepare media for a given ad. * Implementations should update the ad playback state as the specified ad has failed to load. diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsMediaSource.java b/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsMediaSource.java index d4cb455628..62c3e2ed17 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsMediaSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsMediaSource.java @@ -377,16 +377,24 @@ public final class AdsMediaSource extends CompositeMediaSource { } } - private final class AdPrepareErrorListener implements MaskingMediaPeriod.PrepareErrorListener { + private final class AdPrepareListener implements MaskingMediaPeriod.PrepareListener { private final Uri adUri; - public AdPrepareErrorListener(Uri adUri) { + public AdPrepareListener(Uri adUri) { this.adUri = adUri; } @Override - public void onPrepareError(MediaPeriodId mediaPeriodId, final IOException exception) { + public void onPrepareComplete(MediaPeriodId mediaPeriodId) { + mainHandler.post( + () -> + adsLoader.handlePrepareComplete( + mediaPeriodId.adGroupIndex, mediaPeriodId.adIndexInAdGroup)); + } + + @Override + public void onPrepareError(MediaPeriodId mediaPeriodId, IOException exception) { createEventDispatcher(mediaPeriodId) .loadError( new LoadEventInfo( @@ -419,7 +427,7 @@ public final class AdsMediaSource extends CompositeMediaSource { Uri adUri, MediaPeriodId id, Allocator allocator, long startPositionUs) { MaskingMediaPeriod maskingMediaPeriod = new MaskingMediaPeriod(adMediaSource, id, allocator, startPositionUs); - maskingMediaPeriod.setPrepareErrorListener(new AdPrepareErrorListener(adUri)); + maskingMediaPeriod.setPrepareListener(new AdPrepareListener(adUri)); activeMediaPeriods.add(maskingMediaPeriod); if (timeline != null) { Object periodUid = timeline.getUidOfPeriod(/* periodIndex= */ 0); diff --git a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java index 215b808cbf..33d379ef47 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java @@ -8362,6 +8362,9 @@ public final class ExoPlayerTest { @Override public void stop() {} + @Override + public void handlePrepareComplete(int adGroupIndex, int adIndexInAdGroup) {} + @Override public void handlePrepareError(int adGroupIndex, int adIndexInAdGroup, IOException exception) {} }