Skip to content

SDK / exoplayer

Go to a project
Commit 0633778c by andrewlewis Committed by Christos Tsilopoulos
Options

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
parent 9982e1f1
Inline Side-by-side
Showing with 103 additions and 40 deletions
extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdTagLoader.java
...@@ -132,6 +132,7 @@ import java.util.Map; ...@@ -132,6 +132,7 @@ import java.util.Map;
private final Timeline.Period period; private final Timeline.Period period;
private final Handler handler; private final Handler handler;
private final ComponentListener componentListener; private final ComponentListener componentListener;
private final List<EventListener> eventListeners;
private final List<VideoAdPlayer.VideoAdPlayerCallback> adCallbacks; private final List<VideoAdPlayer.VideoAdPlayerCallback> adCallbacks;
private final Runnable updateAdProgressRunnable; private final Runnable updateAdProgressRunnable;
private final BiMap<AdMediaInfo, AdInfo> adInfoByAdMediaInfo; private final BiMap<AdMediaInfo, AdInfo> adInfoByAdMediaInfo;
...@@ -139,7 +140,6 @@ import java.util.Map; ...@@ -139,7 +140,6 @@ import java.util.Map;
private final AdsLoader adsLoader; private final AdsLoader adsLoader;
@Nullable private Object pendingAdRequestContext; @Nullable private Object pendingAdRequestContext;
@Nullable private EventListener eventListener;
@Nullable private Player player; @Nullable private Player player;
private VideoProgressUpdate lastContentProgress; private VideoProgressUpdate lastContentProgress;
private VideoProgressUpdate lastAdProgress; private VideoProgressUpdate lastAdProgress;
...@@ -235,6 +235,7 @@ import java.util.Map; ...@@ -235,6 +235,7 @@ import java.util.Map;
period = new Timeline.Period(); period = new Timeline.Period();
handler = Util.createHandler(getImaLooper(), /* callback= */ null); handler = Util.createHandler(getImaLooper(), /* callback= */ null);
componentListener = new ComponentListener(); componentListener = new ComponentListener();
eventListeners = new ArrayList<>();
adCallbacks = new ArrayList<>(/* initialCapacity= */ 1); adCallbacks = new ArrayList<>(/* initialCapacity= */ 1);
if (configuration.applicationVideoAdPlayerCallback != null) { if (configuration.applicationVideoAdPlayerCallback != null) {
adCallbacks.add(configuration.applicationVideoAdPlayerCallback); adCallbacks.add(configuration.applicationVideoAdPlayerCallback);
...@@ -284,8 +285,16 @@ import java.util.Map; ...@@ -284,8 +285,16 @@ import java.util.Map;
* Starts passing events from this instance (including any pending ad playback state) and * Starts passing events from this instance (including any pending ad playback state) and
* registers obstructions. * registers obstructions.
*/ */
public void start(AdViewProvider adViewProvider, EventListener eventListener) { public void addListenerWithAdView(EventListener eventListener, AdViewProvider adViewProvider) {
this.eventListener = eventListener; 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; lastVolumePercent = 0;
lastAdProgress = VideoProgressUpdate.VIDEO_TIME_NOT_READY; lastAdProgress = VideoProgressUpdate.VIDEO_TIME_NOT_READY;
lastContentProgress = VideoProgressUpdate.VIDEO_TIME_NOT_READY; lastContentProgress = VideoProgressUpdate.VIDEO_TIME_NOT_READY;
...@@ -350,9 +359,11 @@ import java.util.Map; ...@@ -350,9 +359,11 @@ import java.util.Map;
} }
/** Stops passing of events from this instance and unregisters obstructions. */ /** Stops passing of events from this instance and unregisters obstructions. */
public void stop() { public void removeListener(EventListener eventListener) {
eventListener = null; eventListeners.remove(eventListener);
adDisplayContainer.unregisterAllFriendlyObstructions(); if (eventListeners.isEmpty()) {
adDisplayContainer.unregisterAllFriendlyObstructions();
}
} }
/** Releases all resources used by the ad tag loader. */ /** Releases all resources used by the ad tag loader. */
...@@ -713,13 +724,13 @@ import java.util.Map; ...@@ -713,13 +724,13 @@ import java.util.Map;
pauseContentInternal(); pauseContentInternal();
break; break;
case TAPPED: case TAPPED:
if (eventListener != null) { for (int i = 0; i < eventListeners.size(); i++) {
eventListener.onAdTapped(); eventListeners.get(i).onAdTapped();
} }
break; break;
case CLICKED: case CLICKED:
if (eventListener != null) { for (int i = 0; i < eventListeners.size(); i++) {
eventListener.onAdClicked(); eventListeners.get(i).onAdClicked();
} }
break; break;
case CONTENT_RESUME_REQUESTED: case CONTENT_RESUME_REQUESTED:
...@@ -1115,15 +1126,16 @@ import java.util.Map; ...@@ -1115,15 +1126,16 @@ import java.util.Map;
} }
private void updateAdPlaybackState() { private void updateAdPlaybackState() {
// Ignore updates while detached. When a player is attached it will receive the latest state. for (int i = 0; i < eventListeners.size(); i++) {
if (eventListener != null) { eventListeners.get(i).onAdPlaybackState(adPlaybackState);
eventListener.onAdPlaybackState(adPlaybackState);
} }
} }
private void maybeNotifyPendingAdLoadError() { private void maybeNotifyPendingAdLoadError() {
if (pendingAdLoadError != null && eventListener != null) { if (pendingAdLoadError != null) {
eventListener.onAdLoadError(pendingAdLoadError, adTagDataSpec); for (int i = 0; i < eventListeners.size(); i++) {
eventListeners.get(i).onAdLoadError(pendingAdLoadError, adTagDataSpec);
}
pendingAdLoadError = null; pendingAdLoadError = null;
} }
} }
...@@ -1136,9 +1148,12 @@ import java.util.Map; ...@@ -1136,9 +1148,12 @@ import java.util.Map;
adPlaybackState = adPlaybackState.withSkippedAdGroup(i); adPlaybackState = adPlaybackState.withSkippedAdGroup(i);
} }
updateAdPlaybackState(); updateAdPlaybackState();
if (eventListener != null) { for (int i = 0; i < eventListeners.size(); i++) {
eventListener.onAdLoadError( eventListeners
AdLoadException.createForUnexpected(new RuntimeException(message, cause)), adTagDataSpec); .get(i)
.onAdLoadError(
AdLoadException.createForUnexpected(new RuntimeException(message, cause)),
adTagDataSpec);
} }
} }
......
extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoader.java
...@@ -530,16 +530,16 @@ public final class ImaAdsLoader implements Player.EventListener, AdsLoader { ...@@ -530,16 +530,16 @@ public final class ImaAdsLoader implements Player.EventListener, AdsLoader {
adTagLoader = adTagLoaderByAdsId.get(adsId); adTagLoader = adTagLoaderByAdsId.get(adsId);
} }
adTagLoaderByAdsMediaSource.put(adsMediaSource, checkNotNull(adTagLoader)); adTagLoaderByAdsMediaSource.put(adsMediaSource, checkNotNull(adTagLoader));
checkNotNull(adTagLoader).start(adViewProvider, eventListener); adTagLoader.addListenerWithAdView(eventListener, adViewProvider);
maybeUpdateCurrentAdTagLoader(); maybeUpdateCurrentAdTagLoader();
} }
@Override @Override
public void stop(AdsMediaSource adsMediaSource) { public void stop(AdsMediaSource adsMediaSource, EventListener eventListener) {
@Nullable AdTagLoader removedAdTagLoader = adTagLoaderByAdsMediaSource.remove(adsMediaSource); @Nullable AdTagLoader removedAdTagLoader = adTagLoaderByAdsMediaSource.remove(adsMediaSource);
maybeUpdateCurrentAdTagLoader(); maybeUpdateCurrentAdTagLoader();
if (removedAdTagLoader != null) { if (removedAdTagLoader != null) {
removedAdTagLoader.stop(); removedAdTagLoader.removeListener(eventListener);
} }
if (player != null && adTagLoaderByAdsMediaSource.isEmpty()) { if (player != null && adTagLoaderByAdsMediaSource.isEmpty()) {
......
extensions/ima/src/test/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoaderTest.java
...@@ -952,7 +952,7 @@ public final class ImaAdsLoaderTest { ...@@ -952,7 +952,7 @@ public final class ImaAdsLoaderTest {
imaAdsLoader.start( imaAdsLoader.start(
adsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, adsLoaderListener); adsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, adsLoaderListener);
imaAdsLoader.requestAds(TEST_DATA_SPEC, TEST_ADS_ID, adViewGroup); imaAdsLoader.requestAds(TEST_DATA_SPEC, TEST_ADS_ID, adViewGroup);
imaAdsLoader.stop(adsMediaSource); imaAdsLoader.stop(adsMediaSource, adsLoaderListener);
InOrder inOrder = inOrder(mockAdDisplayContainer); InOrder inOrder = inOrder(mockAdDisplayContainer);
inOrder.verify(mockAdDisplayContainer).registerFriendlyObstruction(mockFriendlyObstruction); inOrder.verify(mockAdDisplayContainer).registerFriendlyObstruction(mockFriendlyObstruction);
...@@ -1115,8 +1115,8 @@ public final class ImaAdsLoaderTest { ...@@ -1115,8 +1115,8 @@ public final class ImaAdsLoaderTest {
secondAdsMediaSource, TEST_DATA_SPEC, secondAdsId, adViewProvider, secondAdsLoaderListener); secondAdsMediaSource, TEST_DATA_SPEC, secondAdsId, adViewProvider, secondAdsLoaderListener);
// Simulate backgrounding/resuming. // Simulate backgrounding/resuming.
imaAdsLoader.stop(adsMediaSource); imaAdsLoader.stop(adsMediaSource, adsLoaderListener);
imaAdsLoader.stop(secondAdsMediaSource); imaAdsLoader.stop(secondAdsMediaSource, secondAdsLoaderListener);
imaAdsLoader.start( imaAdsLoader.start(
adsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, adsLoaderListener); adsMediaSource, TEST_DATA_SPEC, TEST_ADS_ID, adViewProvider, adsLoaderListener);
imaAdsLoader.start( imaAdsLoader.start(
...@@ -1138,6 +1138,52 @@ public final class ImaAdsLoaderTest { ...@@ -1138,6 +1138,52 @@ public final class ImaAdsLoaderTest {
} }
@Test @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() { public void buildWithDefaultEnableContinuousPlayback_doesNotSetAdsRequestProperty() {
imaAdsLoader = imaAdsLoader =
new ImaAdsLoader.Builder(getApplicationContext()) new ImaAdsLoader.Builder(getApplicationContext())
......
library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsLoader.java
...@@ -40,11 +40,11 @@ import java.util.List; ...@@ -40,11 +40,11 @@ import java.util.List;
* *
* <p>{@link #start(AdsMediaSource, DataSpec, Object, AdViewProvider, EventListener)} will be called * <p>{@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 * 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 * player enters the background, {@link #stop(AdsMediaSource, EventListener)} will be called.
* maintain any ad playback state in preparation for a later call to {@link #start(AdsMediaSource, * Loaders should maintain any ad playback state in preparation for a later call to {@link
* DataSpec, Object, AdViewProvider, EventListener)}. If an ad is playing when the player is * #start(AdsMediaSource, DataSpec, Object, AdViewProvider, EventListener)}. If an ad is playing
* detached, update the ad playback state with the current playback position using {@link * when the player is detached, update the ad playback state with the current playback position
* AdPlaybackState#withAdResumePositionUs(long)}. * using {@link AdPlaybackState#withAdResumePositionUs(long)}.
* *
* <p>If {@link EventListener#onAdPlaybackState(AdPlaybackState)} has been called, the * <p>If {@link EventListener#onAdPlaybackState(AdPlaybackState)} has been called, the
* implementation of {@link #start(AdsMediaSource, DataSpec, Object, AdViewProvider, EventListener)} * implementation of {@link #start(AdsMediaSource, DataSpec, Object, AdViewProvider, EventListener)}
...@@ -220,8 +220,9 @@ public interface AdsLoader { ...@@ -220,8 +220,9 @@ public interface AdsLoader {
* thread by {@link AdsMediaSource}. * thread by {@link AdsMediaSource}.
* *
* @param adsMediaSource The ads media source requesting to stop loading/playing ads. * @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 * Notifies the ads loader that preparation of an ad media period is complete. Called on the main
......
library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsMediaSource.java
...@@ -252,12 +252,13 @@ public final class AdsMediaSource extends CompositeMediaSource<MediaPeriodId> { ...@@ -252,12 +252,13 @@ public final class AdsMediaSource extends CompositeMediaSource<MediaPeriodId> {
@Override @Override
protected void releaseSourceInternal() { protected void releaseSourceInternal() {
super.releaseSourceInternal(); super.releaseSourceInternal();
checkNotNull(componentListener).release(); ComponentListener componentListener = checkNotNull(this.componentListener);
componentListener = null; this.componentListener = null;
componentListener.stop();
contentTimeline = null; contentTimeline = null;
adPlaybackState = null; adPlaybackState = null;
adMediaSourceHolders = new AdMediaSourceHolder[0][]; adMediaSourceHolders = new AdMediaSourceHolder[0][];
mainHandler.post(() -> adsLoader.stop(/* adsMediaSource= */ this)); mainHandler.post(() -> adsLoader.stop(/* adsMediaSource= */ this, componentListener));
} }
@Override @Override
...@@ -355,7 +356,7 @@ public final class AdsMediaSource extends CompositeMediaSource<MediaPeriodId> { ...@@ -355,7 +356,7 @@ public final class AdsMediaSource extends CompositeMediaSource<MediaPeriodId> {
private final Handler playerHandler; 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 * Creates new listener which forwards ad playback states on the creating thread and all other
...@@ -365,20 +366,20 @@ public final class AdsMediaSource extends CompositeMediaSource<MediaPeriodId> { ...@@ -365,20 +366,20 @@ public final class AdsMediaSource extends CompositeMediaSource<MediaPeriodId> {
playerHandler = Util.createHandlerForCurrentLooper(); playerHandler = Util.createHandlerForCurrentLooper();
} }
/** Releases the component listener. */ /** Stops event delivery from this instance. */
public void release() { public void stop() {
released = true; stopped = true;
playerHandler.removeCallbacksAndMessages(null); playerHandler.removeCallbacksAndMessages(null);
} }
@Override @Override
public void onAdPlaybackState(final AdPlaybackState adPlaybackState) { public void onAdPlaybackState(final AdPlaybackState adPlaybackState) {
if (released) { if (stopped) {
return; return;
} }
playerHandler.post( playerHandler.post(
() -> { () -> {
if (released) { if (stopped) {
return; return;
} }
AdsMediaSource.this.onAdPlaybackState(adPlaybackState); AdsMediaSource.this.onAdPlaybackState(adPlaybackState);
...@@ -387,7 +388,7 @@ public final class AdsMediaSource extends CompositeMediaSource<MediaPeriodId> { ...@@ -387,7 +388,7 @@ public final class AdsMediaSource extends CompositeMediaSource<MediaPeriodId> {
@Override @Override
public void onAdLoadError(final AdLoadException error, DataSpec dataSpec) { public void onAdLoadError(final AdLoadException error, DataSpec dataSpec) {
if (released) { if (stopped) {
return; return;
} }
createEventDispatcher(/* mediaPeriodId= */ null) createEventDispatcher(/* mediaPeriodId= */ null)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment