From aec5ff8be1ab0d252beeba506599201f96a465f2 Mon Sep 17 00:00:00 2001 From: andrewlewis Date: Mon, 22 Jun 2020 09:04:34 +0100 Subject: [PATCH] Fix incorrect rounding of ad cue points We currently get float ad cue points from IMA, but store these as longs in microseconds. The cast from double to long would take the floor of the value, which could lead to stored ad cue points being off-by-one. Use Math.round to avoid this. ImaAdsLoader also has code to map a double AdPodInfo position (which should match a cue point) onto the corresponding ad group index by searching the long ad cue points. Match the calculation used where we map float cue points, including narrowing the position to a float first to avoid regressions if IMA SDK behavior changes to represent positions in more than float precision later, and also remove the requirement that the ad positions match exactly as a defensive measure. PiperOrigin-RevId: 317607017 --- RELEASENOTES.md | 1 + demos/main/src/main/assets/media.exolist.json | 5 ++ .../ext/ima/AdPlaybackStateFactory.java | 2 +- .../exoplayer2/ext/ima/ImaAdsLoader.java | 12 ++++- .../exoplayer2/ext/ima/ImaAdsLoaderTest.java | 49 +++++++++++++++++++ 5 files changed, 66 insertions(+), 3 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 96370e7e5b..aaf25b1cd5 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -228,6 +228,7 @@ * Work around unexpected `pauseAd`/`stopAd` for ads that have preloaded on seeking to another position ([#7492](https://github.com/google/ExoPlayer/issues/7492)). + * Fix incorrect rounding of ad cue points. ### 2.11.5 (2020-06-05) ### diff --git a/demos/main/src/main/assets/media.exolist.json b/demos/main/src/main/assets/media.exolist.json index db07652312..a1ea669f0e 100644 --- a/demos/main/src/main/assets/media.exolist.json +++ b/demos/main/src/main/assets/media.exolist.json @@ -487,6 +487,11 @@ "name": "VMAP full, empty, full midrolls", "uri": "https://storage.googleapis.com/exoplayer-test-media-1/mkv/android-screens-lavf-56.36.100-aac-avc-main-1280x720.mkv", "ad_tag_uri": "https://vastsynthesizer.appspot.com/empty-midroll-2" + }, + { + "name": "VMAP midroll at 1765 s", + "uri": "https://storage.googleapis.com/exoplayer-test-media-1/mp4/frame-counter-one-hour.mp4", + "ad_tag_uri": "https://vastsynthesizer.appspot.com/midroll-large" } ] }, diff --git a/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdPlaybackStateFactory.java b/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdPlaybackStateFactory.java index 3c1b6954aa..a97307a419 100644 --- a/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdPlaybackStateFactory.java +++ b/extensions/ima/src/main/java/com/google/android/exoplayer2/ext/ima/AdPlaybackStateFactory.java @@ -46,7 +46,7 @@ import java.util.List; if (cuePoint == -1.0) { adGroupTimesUs[count - 1] = C.TIME_END_OF_SOURCE; } else { - adGroupTimesUs[adGroupIndex++] = (long) (C.MICROS_PER_SECOND * cuePoint); + adGroupTimesUs[adGroupIndex++] = Math.round(C.MICROS_PER_SECOND * cuePoint); } } // Cue points may be out of order, so sort them. 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 a8748219ef..c055fb60d2 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 @@ -343,6 +343,8 @@ public final class ImaAdsLoader implements Player.EventListener, AdsLoader { * milliseconds. */ private static final long THRESHOLD_AD_PRELOAD_MS = 4000; + /** The threshold below which ad cue points are treated as matching, in microseconds. */ + private static final long THRESHOLD_AD_MATCH_US = 1000; private static final int TIMEOUT_UNSET = -1; private static final int BITRATE_UNSET = -1; @@ -1261,9 +1263,15 @@ public final class ImaAdsLoader implements Player.EventListener, AdsLoader { } // adPodInfo.podIndex may be 0-based or 1-based, so for now look up the cue point instead. - long adGroupTimeUs = (long) (((float) adPodInfo.getTimeOffset()) * C.MICROS_PER_SECOND); + // We receive cue points from IMA SDK as floats. This code replicates the same calculation used + // to populate adGroupTimesUs (having truncated input back to float, to avoid failures if the + // behavior of the IMA SDK changes to provide greater precision in AdPodInfo). + long adPodTimeUs = + Math.round((double) ((float) adPodInfo.getTimeOffset()) * C.MICROS_PER_SECOND); for (int adGroupIndex = 0; adGroupIndex < adPlaybackState.adGroupCount; adGroupIndex++) { - if (adPlaybackState.adGroupTimesUs[adGroupIndex] == adGroupTimeUs) { + long adGroupTimeUs = adPlaybackState.adGroupTimesUs[adGroupIndex]; + if (adGroupTimeUs != C.TIME_END_OF_SOURCE + && Math.abs(adGroupTimeUs - adPodTimeUs) < THRESHOLD_AD_MATCH_US) { return adGroupIndex; } } 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 c3202a26be..5a59690b44 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 @@ -641,6 +641,55 @@ public final class ImaAdsLoaderTest { inOrder.verify(mockAdDisplayContainer).unregisterAllVideoControlsOverlays(); } + @Test + public void loadAd_withLargeAdCuePoint_updatesAdPlaybackStateWithLoadedAd() { + float midrollTimeSecs = 1_765f; + ImmutableList cuePoints = ImmutableList.of(midrollTimeSecs); + setupPlayback(CONTENT_TIMELINE, cuePoints); + imaAdsLoader.start(adsLoaderListener, adViewProvider); + videoAdPlayer.loadAd( + TEST_AD_MEDIA_INFO, + new AdPodInfo() { + @Override + public int getTotalAds() { + return 1; + } + + @Override + public int getAdPosition() { + return 1; + } + + @Override + public boolean isBumper() { + return false; + } + + @Override + public double getMaxDuration() { + return 0; + } + + @Override + public int getPodIndex() { + return 0; + } + + @Override + public double getTimeOffset() { + return midrollTimeSecs; + } + }); + + assertThat(adsLoaderListener.adPlaybackState) + .isEqualTo( + AdPlaybackStateFactory.fromCuePoints(cuePoints) + .withContentDurationUs(CONTENT_PERIOD_DURATION_US) + .withAdCount(/* adGroupIndex= */ 0, /* adCount= */ 1) + .withAdUri(/* adGroupIndex= */ 0, /* adIndexInAdGroup= */ 0, TEST_URI) + .withAdDurationsUs(new long[][] {{TEST_AD_DURATION_US}})); + } + private void setupPlayback(Timeline contentTimeline, List cuePoints) { setupPlayback( contentTimeline,