Skip to content

Commit

Permalink
Fix handling of repeated ads identifiers
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@andrewlewis @christosts
andrewlewis authored and christosts committed Dec 17, 2020
1 parent 9982e1f commit 0633778
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 40 deletions.
51 changes: 33 additions & 18 deletions extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdTagLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,14 @@
private final Timeline.Period period;
private final Handler handler;
private final ComponentListener componentListener;
private final List<EventListener> eventListeners;
private final List<VideoAdPlayer.VideoAdPlayerCallback> adCallbacks;
private final Runnable updateAdProgressRunnable;
private final BiMap<AdMediaInfo, AdInfo> adInfoByAdMediaInfo;
private final AdDisplayContainer adDisplayContainer;
private final AdsLoader adsLoader;

@Nullable private Object pendingAdRequestContext;
@Nullable private EventListener eventListener;
@Nullable private Player player;
private VideoProgressUpdate lastContentProgress;
private VideoProgressUpdate lastAdProgress;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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);
}
}

Expand Down
6 changes: 3 additions & 3 deletions extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
52 changes: 49 additions & 3 deletions extensions/ima/src/test/java/com/google/android/exoplayer2/ext/ima/ImaAdsLoaderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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(
Expand All @@ -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 =
Expand Down
13 changes: 7 additions & 6 deletions library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@
*
* <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
* 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)}.
*
* <p>If {@link EventListener#onAdPlaybackState(AdPlaybackState)} has been called, the
* implementation of {@link #start(AdsMediaSource, DataSpec, Object, AdViewProvider, EventListener)}
Expand Down Expand Up @@ -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
Expand Down
21 changes: 11 additions & 10 deletions library/core/src/main/java/com/google/android/exoplayer2/source/ads/AdsMediaSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -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)
Expand Down

0 comments on commit 0633778

Please sign in to comment.