From 50f9f35353b608a7f383533c0f1d7210bdf17570 Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 19 Jul 2024 04:25:01 -0700 Subject: [PATCH] Guard timeline access in ImaSSAIMS against empty timelines All methods check if the player is currently handling the ad source by calling isCurrentAdPlaying(). This method was missing a check for empty timelines that throws an exception when trying to access a non-existent period. Also add this check to two methods that assume the current item is the ads source, but didn't check it yet. PiperOrigin-RevId: 653963557 --- RELEASENOTES.md | 3 ++ libraries/exoplayer_ima/build.gradle | 1 + .../ImaServerSideAdInsertionMediaSource.java | 19 +++++----- ...aServerSideAdInsertionMediaSourceTest.java | 35 +++++++++++++++++++ 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index d601b8f3c4..0285b7f5b4 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -61,6 +61,9 @@ * Effect: * Muxers: * IMA extension: + * Fix bug where clearing the playlist may cause an + `ArrayIndexOutOfBoundsException` in + `ImaServerSideAdInsertionMediaSource`. * Session: * Add `MediaButtonReceiver.shouldStartForegroundService(Intent)` to allow apps to suppress a play command coming in for playback resumption by diff --git a/libraries/exoplayer_ima/build.gradle b/libraries/exoplayer_ima/build.gradle index 16a7de38bf..70b39cd2b3 100644 --- a/libraries/exoplayer_ima/build.gradle +++ b/libraries/exoplayer_ima/build.gradle @@ -39,6 +39,7 @@ dependencies { androidTestImplementation 'androidx.test:runner:' + androidxTestRunnerVersion androidTestCompileOnly 'org.checkerframework:checker-qual:' + checkerframeworkVersion testImplementation project(modulePrefix + 'test-utils') + testImplementation project(modulePrefix + 'test-utils-robolectric') testImplementation 'org.robolectric:robolectric:' + robolectricVersion } diff --git a/libraries/exoplayer_ima/src/main/java/androidx/media3/exoplayer/ima/ImaServerSideAdInsertionMediaSource.java b/libraries/exoplayer_ima/src/main/java/androidx/media3/exoplayer/ima/ImaServerSideAdInsertionMediaSource.java index e6a2b16330..f823a8e78e 100644 --- a/libraries/exoplayer_ima/src/main/java/androidx/media3/exoplayer/ima/ImaServerSideAdInsertionMediaSource.java +++ b/libraries/exoplayer_ima/src/main/java/androidx/media3/exoplayer/ima/ImaServerSideAdInsertionMediaSource.java @@ -1021,7 +1021,7 @@ public final class ImaServerSideAdInsertionMediaSource extends CompositeMediaSou @Override public void onMetadata(Metadata metadata) { - if (!isCurrentAdPlaying(player, getMediaItem(), adsId)) { + if (!isCurrentlyPlayingMediaPeriodFromThisSource(player, getMediaItem(), adsId)) { return; } for (int i = 0; i < metadata.length(); i++) { @@ -1041,14 +1041,15 @@ public final class ImaServerSideAdInsertionMediaSource extends CompositeMediaSou @Override public void onPlaybackStateChanged(@Player.State int state) { - if (state == Player.STATE_ENDED && isCurrentAdPlaying(player, getMediaItem(), adsId)) { + if (state == Player.STATE_ENDED + && isCurrentlyPlayingMediaPeriodFromThisSource(player, getMediaItem(), adsId)) { streamPlayer.onContentCompleted(); } } @Override public void onVolumeChanged(float volume) { - if (!isCurrentAdPlaying(player, getMediaItem(), adsId)) { + if (!isCurrentlyPlayingMediaPeriodFromThisSource(player, getMediaItem(), adsId)) { return; } int volumePct = (int) Math.floor(volume * 100); @@ -1312,7 +1313,7 @@ public final class ImaServerSideAdInsertionMediaSource extends CompositeMediaSou @Override public VideoProgressUpdate getContentProgress() { - if (!isCurrentAdPlaying(player, mediaItem, adsId)) { + if (!isCurrentlyPlayingMediaPeriodFromThisSource(player, mediaItem, adsId)) { return VideoProgressUpdate.VIDEO_TIME_NOT_READY; } else if (adPlaybackStates.isEmpty()) { return new VideoProgressUpdate(/* currentTimeMs= */ 0, /* durationMs= */ C.TIME_UNSET); @@ -1428,9 +1429,9 @@ public final class ImaServerSideAdInsertionMediaSource extends CompositeMediaSou } } - private static boolean isCurrentAdPlaying( + private static boolean isCurrentlyPlayingMediaPeriodFromThisSource( Player player, MediaItem mediaItem, @Nullable Object adsId) { - if (player.getPlaybackState() == Player.STATE_IDLE) { + if (player.getPlaybackState() == Player.STATE_IDLE || player.getMediaItemCount() == 0) { return false; } Timeline.Period period = new Timeline.Period(); @@ -1510,7 +1511,8 @@ public final class ImaServerSideAdInsertionMediaSource extends CompositeMediaSou private class SinglePeriodLiveAdEventListener implements AdEventListener { @Override public void onAdEvent(AdEvent event) { - if (!Objects.equals(event.getType(), LOADED)) { + if (!Objects.equals(event.getType(), LOADED) + || !isCurrentlyPlayingMediaPeriodFromThisSource(player, getMediaItem(), adsId)) { return; } AdPlaybackState newAdPlaybackState = adPlaybackState; @@ -1541,7 +1543,8 @@ public final class ImaServerSideAdInsertionMediaSource extends CompositeMediaSou private class MultiPeriodLiveAdEventListener implements AdEventListener { @Override public void onAdEvent(AdEvent event) { - if (!Objects.equals(event.getType(), LOADED)) { + if (!Objects.equals(event.getType(), LOADED) + || !isCurrentlyPlayingMediaPeriodFromThisSource(player, getMediaItem(), adsId)) { return; } AdPodInfo adPodInfo = event.getAd().getAdPodInfo(); diff --git a/libraries/exoplayer_ima/src/test/java/androidx/media3/exoplayer/ima/ImaServerSideAdInsertionMediaSourceTest.java b/libraries/exoplayer_ima/src/test/java/androidx/media3/exoplayer/ima/ImaServerSideAdInsertionMediaSourceTest.java index 331db5d0fb..ba8ce2c39f 100644 --- a/libraries/exoplayer_ima/src/test/java/androidx/media3/exoplayer/ima/ImaServerSideAdInsertionMediaSourceTest.java +++ b/libraries/exoplayer_ima/src/test/java/androidx/media3/exoplayer/ima/ImaServerSideAdInsertionMediaSourceTest.java @@ -15,12 +15,21 @@ */ package androidx.media3.exoplayer.ima; +import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.run; import static com.google.common.truth.Truth.assertThat; +import android.content.Context; +import android.widget.LinearLayout; import androidx.media3.common.AdPlaybackState; import androidx.media3.common.C; +import androidx.media3.common.MediaItem; +import androidx.media3.exoplayer.ExoPlayer; import androidx.media3.exoplayer.ima.ImaServerSideAdInsertionMediaSource.AdsLoader.State; +import androidx.media3.exoplayer.source.DefaultMediaSourceFactory; +import androidx.media3.exoplayer.source.MediaSource; import androidx.media3.exoplayer.source.ads.ServerSideAdInsertionUtil; +import androidx.media3.test.utils.TestExoPlayerBuilder; +import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.common.collect.ImmutableMap; import org.junit.Test; @@ -70,4 +79,30 @@ public class ImaServerSideAdInsertionMediaSourceTest { assertThat(State.fromBundle(state.toBundle())).isEqualTo(state); } + + @Test + public void clearPlaylist_withAdsSource_handlesCleanupWithoutThrowing() throws Exception { + Context context = ApplicationProvider.getApplicationContext(); + ExoPlayer player = new TestExoPlayerBuilder(context).build(); + ImaServerSideAdInsertionMediaSource.AdsLoader adsLoader = + new ImaServerSideAdInsertionMediaSource.AdsLoader.Builder( + context, /* adViewProvider= */ () -> new LinearLayout(context)) + .build(); + adsLoader.setPlayer(player); + MediaSource mediaSource = + new ImaServerSideAdInsertionMediaSource.Factory( + adsLoader, new DefaultMediaSourceFactory(context)) + .createMediaSource( + MediaItem.fromUri("ssai://dai.google.com/?assetKey=ABC&format=0&adsId=2")); + player.setMediaSource(mediaSource); + player.prepare(); + run(player).untilPendingCommandsAreFullyHandled(); + + // Clearing the playlist will cause internal state of the ads source to be invalid and + // potentially accessing empty timelines. See b/354026260. The test simply ensures that clearing + // the playlist will not throw any exceptions. + player.clearMediaItems(); + run(player).untilPendingCommandsAreFullyHandled(); + player.release(); + } }