From 0633778c700bbff2d03172b14635c61730b03232 Mon Sep 17 00:00:00 2001 From: andrewlewis Date: Tue, 15 Dec 2020 10:29:44 +0000 Subject: [PATCH] Fix handling of repeated ads identifiers Previously the `AdTagLoader` only had one listener which meant that updates that should affect all periods with matching identifiers in the timeline only affected the last-attached one. Fix this by having `AdTagLoader` track all its listeners. Issue: #3750 PiperOrigin-RevId: 347571323 --- .../exoplayer2/ext/ima/AdTagLoader.java | 51 +++++++++++------- .../exoplayer2/ext/ima/ImaAdsLoader.java | 6 +-- .../exoplayer2/ext/ima/ImaAdsLoaderTest.java | 52 +++++++++++++++++-- .../exoplayer2/source/ads/AdsLoader.java | 13 ++--- .../exoplayer2/source/ads/AdsMediaSource.java | 21 ++++---- 5 files changed, 103 insertions(+), 40 deletions(-) diff --git a/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdTagLoader.java b/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdTagLoader.java index 6e4b4beaf5c..b5fd13e6f9e 100644 --- a/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdTagLoader.java +++ b/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdTagLoader.java @@ -132,6 +132,7 @@ private final Timeline.Period period; private final Handler handler; private final ComponentListener componentListener; + private final List eventListeners; private final List adCallbacks; private final Runnable updateAdProgressRunnable; private final BiMap adInfoByAdMediaInfo; @@ -139,7 +140,6 @@ private final AdsLoader adsLoader; @Nullable private Object pendingAdRequestContext; - @Nullable private EventListener eventListener; @Nullable private Player player; private VideoProgressUpdate lastContentProgress; private VideoProgressUpdate lastAdProgress; @@ -235,6 +235,7 @@ public AdTagLoader( period = new Timeline.Period(); handler = Util.createHandler(getImaLooper(), /* callback= */ null); componentListener = new ComponentListener(); + eventListeners = new ArrayList<>(); adCallbacks = new ArrayList<>(/* initialCapacity= */ 1); if (configuration.applicationVideoAdPlayerCallback != null) { adCallbacks.add(configuration.applicationVideoAdPlayerCallback); @@ -284,8 +285,16 @@ public void skipAd() { * Starts passing events from this instance (including any pending ad playback state) and * registers obstructions. */ - public void start(AdViewProvider adViewProvider, EventListener eventListener) { - this.eventListener = eventListener; + public void addListenerWithAdView(EventListener eventListener, AdViewProvider adViewProvider) { + boolean isStarted = !eventListeners.isEmpty(); + eventListeners.add(eventListener); + if (isStarted) { + if (!AdPlaybackState.NONE.equals(adPlaybackState)) { + // Pass the existing ad playback state to the new listener. + eventListener.onAdPlaybackState(adPlaybackState); + } + return; + } lastVolumePercent = 0; lastAdProgress = VideoProgressUpdate.VIDEO_TIME_NOT_READY; lastContentProgress = VideoProgressUpdate.VIDEO_TIME_NOT_READY; @@ -350,9 +359,11 @@ public void deactivate() { } /** Stops passing of events from this instance and unregisters obstructions. */ - public void stop() { - eventListener = null; - adDisplayContainer.unregisterAllFriendlyObstructions(); + public void removeListener(EventListener eventListener) { + eventListeners.remove(eventListener); + if (eventListeners.isEmpty()) { + adDisplayContainer.unregisterAllFriendlyObstructions(); + } } /** Releases all resources used by the ad tag loader. */ @@ -713,13 +724,13 @@ private void handleAdEvent(AdEvent adEvent) { pauseContentInternal(); break; case TAPPED: - if (eventListener != null) { - eventListener.onAdTapped(); + for (int i = 0; i < eventListeners.size(); i++) { + eventListeners.get(i).onAdTapped(); } break; case CLICKED: - if (eventListener != null) { - eventListener.onAdClicked(); + for (int i = 0; i < eventListeners.size(); i++) { + eventListeners.get(i).onAdClicked(); } break; case CONTENT_RESUME_REQUESTED: @@ -1115,15 +1126,16 @@ private void sendContentComplete() { } private void updateAdPlaybackState() { - // Ignore updates while detached. When a player is attached it will receive the latest state. - if (eventListener != null) { - eventListener.onAdPlaybackState(adPlaybackState); + for (int i = 0; i < eventListeners.size(); i++) { + eventListeners.get(i).onAdPlaybackState(adPlaybackState); } } private void maybeNotifyPendingAdLoadError() { - if (pendingAdLoadError != null && eventListener != null) { - eventListener.onAdLoadError(pendingAdLoadError, adTagDataSpec); + if (pendingAdLoadError != null) { + for (int i = 0; i < eventListeners.size(); i++) { + eventListeners.get(i).onAdLoadError(pendingAdLoadError, adTagDataSpec); + } pendingAdLoadError = null; } } @@ -1136,9 +1148,12 @@ private void maybeNotifyInternalError(String name, Exception cause) { adPlaybackState = adPlaybackState.withSkippedAdGroup(i); } updateAdPlaybackState(); - if (eventListener != null) { - eventListener.onAdLoadError( - AdLoadException.createForUnexpected(new RuntimeException(message, cause)), adTagDataSpec); + for (int i = 0; i < eventListeners.size(); i++) { + eventListeners + .get(i) + .onAdLoadError( + AdLoadException.createForUnexpected(new RuntimeException(message, cause)), + adTagDataSpec); } } 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 fa249eac283..5eae985fcd4 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 @@ -530,16 +530,16 @@ public void start( adTagLoader = adTagLoaderByAdsId.get(adsId); } adTagLoaderByAdsMediaSource.put(adsMediaSource, checkNotNull(adTagLoader)); - checkNotNull(adTagLoader).start(adViewProvider, eventListener); + adTagLoader.addListenerWithAdView(eventListener, adViewProvider); maybeUpdateCurrentAdTagLoader(); } @Override - public void stop(AdsMediaSource adsMediaSource) { + public void stop(AdsMediaSource adsMediaSource, EventListener eventListener) { @Nullable AdTagLoader removedAdTagLoader = adTagLoaderByAdsMediaSource.remove(adsMediaSource); maybeUpdateCurrentAdTagLoader(); if (removedAdTagLoader != null) { - removedAdTagLoader.stop(); + removedAdTagLoader.removeListener(eventListener); } if (player != null && adTagLoaderByAdsMediaSource.isEmpty()) { diff --git a/extensions/ima/src/test/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoaderTest.java b/extensions/ima/src/test/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoaderTest.java index 64d2d5cf74d..f7f5ef43fd4 100644 --- a/extensions/ima/src/test/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoaderTest.java +++ b/extensions/ima/src/test/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoaderTest.java @@ -952,7 +952,7 @@ public void stop_unregistersAllVideoControlOverlays() { imaAdsLoader.start( adsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, adsLoaderListener); imaAdsLoader.requestAds(TEST_DATA_SPEC, TEST_ADS_ID, adViewGroup); - imaAdsLoader.stop(adsMediaSource); + imaAdsLoader.stop(adsMediaSource, adsLoaderListener); InOrder inOrder = inOrder(mockAdDisplayContainer); inOrder.verify(mockAdDisplayContainer).registerFriendlyObstruction(mockFriendlyObstruction); @@ -1115,8 +1115,8 @@ public void playbackWithTwoAdsMediaSources_preloadsSecondAdTagWithBackgroundResu secondAdsMediaSource, TEST_DATA_SPEC, secondAdsId, adViewProvider, secondAdsLoaderListener); // Simulate backgrounding/resuming. - imaAdsLoader.stop(adsMediaSource); - imaAdsLoader.stop(secondAdsMediaSource); + imaAdsLoader.stop(adsMediaSource, adsLoaderListener); + imaAdsLoader.stop(secondAdsMediaSource, secondAdsLoaderListener); imaAdsLoader.start( adsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, adsLoaderListener); imaAdsLoader.start( @@ -1137,6 +1137,52 @@ public void playbackWithTwoAdsMediaSources_preloadsSecondAdTagWithBackgroundResu .isEqualTo(new AdPlaybackState(secondAdsId, /* adGroupTimesUs...= */ 0)); } + @Test + public void playbackWithTwoAdsMediaSourcesAndMatchingAdsIds_hasMatchingAdPlaybackState() { + AdsMediaSource secondAdsMediaSource = + new AdsMediaSource( + new FakeMediaSource(CONTENT_TIMELINE), + TEST_DATA_SPEC, + TEST_ADS_ID, + new DefaultMediaSourceFactory((Context) getApplicationContext()), + imaAdsLoader, + adViewProvider); + timelineWindowDefinitions = + new TimelineWindowDefinition[] { + getInitialTimelineWindowDefinition(TEST_ADS_ID), + getInitialTimelineWindowDefinition(TEST_ADS_ID) + }; + TestAdsLoaderListener secondAdsLoaderListener = new TestAdsLoaderListener(/* periodIndex= */ 1); + + // Load and play the preroll ad then content. + imaAdsLoader.start( + adsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, adsLoaderListener); + adEventListener.onAdEvent(getAdEvent(AdEventType.LOADED, mockPrerollSingleAd)); + videoAdPlayer.loadAd(TEST_AD_MEDIA_INFO, mockAdPodInfo); + imaAdsLoader.start( + secondAdsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, secondAdsLoaderListener); + 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)); + adEventListener.onAdEvent(getAdEvent(AdEventType.FIRST_QUARTILE, mockPrerollSingleAd)); + adEventListener.onAdEvent(getAdEvent(AdEventType.MIDPOINT, mockPrerollSingleAd)); + adEventListener.onAdEvent(getAdEvent(AdEventType.THIRD_QUARTILE, mockPrerollSingleAd)); + fakePlayer.setPlayingContentPosition(/* periodIndex= */ 0, /* positionMs= */ 0); + videoAdPlayer.stopAd(TEST_AD_MEDIA_INFO); + adEventListener.onAdEvent(getAdEvent(AdEventType.CONTENT_RESUME_REQUESTED, /* ad= */ null)); + + // Verify that the ad playback states for the two periods match. + assertThat(getAdPlaybackState(/* periodIndex= */ 0)) + .isEqualTo(getAdPlaybackState(/* periodIndex= */ 1)); + } + @Test public void buildWithDefaultEnableContinuousPlayback_doesNotSetAdsRequestProperty() { imaAdsLoader = 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 b982eaae581..a6e450f8e9f 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 @@ -40,11 +40,11 @@ * *

{@link #start(AdsMediaSource, DataSpec, Object, AdViewProvider, EventListener)} will be called * when an ads media source first initializes, at which point the loader can request ads. If the - * player enters the background, {@link #stop(AdsMediaSource)} will be called. Loaders should - * maintain any ad playback state in preparation for a later call to {@link #start(AdsMediaSource, - * DataSpec, Object, AdViewProvider, EventListener)}. If an ad is playing when the player is - * detached, update the ad playback state with the current playback position using {@link - * AdPlaybackState#withAdResumePositionUs(long)}. + * player enters the background, {@link #stop(AdsMediaSource, EventListener)} will be called. + * Loaders should maintain any ad playback state in preparation for a later call to {@link + * #start(AdsMediaSource, DataSpec, Object, AdViewProvider, EventListener)}. If an ad is playing + * when the player is detached, update the ad playback state with the current playback position + * using {@link AdPlaybackState#withAdResumePositionUs(long)}. * *

If {@link EventListener#onAdPlaybackState(AdPlaybackState)} has been called, the * implementation of {@link #start(AdsMediaSource, DataSpec, Object, AdViewProvider, EventListener)} @@ -220,8 +220,9 @@ void start( * thread by {@link AdsMediaSource}. * * @param adsMediaSource The ads media source requesting to stop loading/playing ads. + * @param eventListener The ads media source's listener for ads loader events. */ - void stop(AdsMediaSource adsMediaSource); + void stop(AdsMediaSource adsMediaSource, EventListener eventListener); /** * Notifies the ads loader that preparation of an ad media period is complete. Called on the main 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 c3d0311ef98..4f2617e8687 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 @@ -252,12 +252,13 @@ public void releasePeriod(MediaPeriod mediaPeriod) { @Override protected void releaseSourceInternal() { super.releaseSourceInternal(); - checkNotNull(componentListener).release(); - componentListener = null; + ComponentListener componentListener = checkNotNull(this.componentListener); + this.componentListener = null; + componentListener.stop(); contentTimeline = null; adPlaybackState = null; adMediaSourceHolders = new AdMediaSourceHolder[0][]; - mainHandler.post(() -> adsLoader.stop(/* adsMediaSource= */ this)); + mainHandler.post(() -> adsLoader.stop(/* adsMediaSource= */ this, componentListener)); } @Override @@ -355,7 +356,7 @@ private final class ComponentListener implements AdsLoader.EventListener { private final Handler playerHandler; - private volatile boolean released; + private volatile boolean stopped; /** * Creates new listener which forwards ad playback states on the creating thread and all other @@ -365,20 +366,20 @@ public ComponentListener() { playerHandler = Util.createHandlerForCurrentLooper(); } - /** Releases the component listener. */ - public void release() { - released = true; + /** Stops event delivery from this instance. */ + public void stop() { + stopped = true; playerHandler.removeCallbacksAndMessages(null); } @Override public void onAdPlaybackState(final AdPlaybackState adPlaybackState) { - if (released) { + if (stopped) { return; } playerHandler.post( () -> { - if (released) { + if (stopped) { return; } AdsMediaSource.this.onAdPlaybackState(adPlaybackState); @@ -387,7 +388,7 @@ public void onAdPlaybackState(final AdPlaybackState adPlaybackState) { @Override public void onAdLoadError(final AdLoadException error, DataSpec dataSpec) { - if (released) { + if (stopped) { return; } createEventDispatcher(/* mediaPeriodId= */ null)