diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 36fd99ef42..ff9eea4477 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -70,6 +70,7 @@ ([#7207](https://github.com/google/ExoPlayer/issues/7207)). * Add `SilenceMediaSource.Factory` to support tags ([PR #7245](https://github.com/google/ExoPlayer/pull/7245)). + * Fix `AdsMediaSource` child `MediaSource`s not being released. * Text: * Parse `` and `` tags in WebVTT subtitles (rendering is coming later). 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 d1b5e84fb4..b7b87c36c8 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 @@ -45,10 +45,9 @@ import java.lang.annotation.RetentionPolicy; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.List; -import java.util.Map; import org.checkerframework.checker.nullness.compatqual.NullableType; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; /** * A {@link MediaSource} that inserts ads linearly with a provided content media source. This source @@ -129,15 +128,13 @@ public final class AdsMediaSource extends CompositeMediaSource { private final AdsLoader adsLoader; private final AdsLoader.AdViewProvider adViewProvider; private final Handler mainHandler; - private final Map> maskingMediaPeriodByAdMediaSource; private final Timeline.Period period; // Accessed on the player thread. @Nullable private ComponentListener componentListener; @Nullable private Timeline contentTimeline; @Nullable private AdPlaybackState adPlaybackState; - private @NullableType MediaSource[][] adGroupMediaSources; - private @NullableType Timeline[][] adGroupTimelines; + private @NullableType AdMediaSourceHolder[][] adMediaSourceHolders; /** * Constructs a new source that inserts ads linearly with the content specified by {@code @@ -179,10 +176,8 @@ public final class AdsMediaSource extends CompositeMediaSource { this.adsLoader = adsLoader; this.adViewProvider = adViewProvider; mainHandler = new Handler(Looper.getMainLooper()); - maskingMediaPeriodByAdMediaSource = new HashMap<>(); period = new Timeline.Period(); - adGroupMediaSources = new MediaSource[0][]; - adGroupTimelines = new Timeline[0][]; + adMediaSourceHolders = new AdMediaSourceHolder[0][]; adsLoader.setSupportedContentTypes(adMediaSourceFactory.getSupportedTypes()); } @@ -209,36 +204,21 @@ public final class AdsMediaSource extends CompositeMediaSource { int adIndexInAdGroup = id.adIndexInAdGroup; Uri adUri = Assertions.checkNotNull(adPlaybackState.adGroups[adGroupIndex].uris[adIndexInAdGroup]); - if (adGroupMediaSources[adGroupIndex].length <= adIndexInAdGroup) { + if (adMediaSourceHolders[adGroupIndex].length <= adIndexInAdGroup) { int adCount = adIndexInAdGroup + 1; - adGroupMediaSources[adGroupIndex] = - Arrays.copyOf(adGroupMediaSources[adGroupIndex], adCount); - adGroupTimelines[adGroupIndex] = Arrays.copyOf(adGroupTimelines[adGroupIndex], adCount); + adMediaSourceHolders[adGroupIndex] = + Arrays.copyOf(adMediaSourceHolders[adGroupIndex], adCount); } - MediaSource mediaSource = adGroupMediaSources[adGroupIndex][adIndexInAdGroup]; - if (mediaSource == null) { - mediaSource = adMediaSourceFactory.createMediaSource(adUri); - adGroupMediaSources[adGroupIndex][adIndexInAdGroup] = mediaSource; - maskingMediaPeriodByAdMediaSource.put(mediaSource, new ArrayList<>()); - prepareChildSource(id, mediaSource); + @Nullable + AdMediaSourceHolder adMediaSourceHolder = + adMediaSourceHolders[adGroupIndex][adIndexInAdGroup]; + if (adMediaSourceHolder == null) { + MediaSource adMediaSource = adMediaSourceFactory.createMediaSource(adUri); + adMediaSourceHolder = new AdMediaSourceHolder(adMediaSource); + adMediaSourceHolders[adGroupIndex][adIndexInAdGroup] = adMediaSourceHolder; + prepareChildSource(id, adMediaSource); } - MaskingMediaPeriod maskingMediaPeriod = - new MaskingMediaPeriod(mediaSource, id, allocator, startPositionUs); - maskingMediaPeriod.setPrepareErrorListener( - new AdPrepareErrorListener(adUri, adGroupIndex, adIndexInAdGroup)); - List mediaPeriods = maskingMediaPeriodByAdMediaSource.get(mediaSource); - if (mediaPeriods == null) { - Object periodUid = - Assertions.checkNotNull(adGroupTimelines[adGroupIndex][adIndexInAdGroup]) - .getUidOfPeriod(/* periodIndex= */ 0); - MediaPeriodId adSourceMediaPeriodId = new MediaPeriodId(periodUid, id.windowSequenceNumber); - maskingMediaPeriod.createPeriod(adSourceMediaPeriodId); - } else { - // Keep track of the masking media period so it can be populated with the real media period - // when the source's info becomes available. - mediaPeriods.add(maskingMediaPeriod); - } - return maskingMediaPeriod; + return adMediaSourceHolder.createMediaPeriod(adUri, id, allocator, startPositionUs); } else { MaskingMediaPeriod mediaPeriod = new MaskingMediaPeriod(contentMediaSource, id, allocator, startPositionUs); @@ -250,12 +230,18 @@ public final class AdsMediaSource extends CompositeMediaSource { @Override public void releasePeriod(MediaPeriod mediaPeriod) { MaskingMediaPeriod maskingMediaPeriod = (MaskingMediaPeriod) mediaPeriod; - List mediaPeriods = - maskingMediaPeriodByAdMediaSource.get(maskingMediaPeriod.mediaSource); - if (mediaPeriods != null) { - mediaPeriods.remove(maskingMediaPeriod); + MediaPeriodId id = maskingMediaPeriod.id; + if (id.isAd()) { + AdMediaSourceHolder adMediaSourceHolder = + Assertions.checkNotNull(adMediaSourceHolders[id.adGroupIndex][id.adIndexInAdGroup]); + adMediaSourceHolder.releaseMediaPeriod(maskingMediaPeriod); + if (adMediaSourceHolder.isInactive()) { + releaseChildSource(id); + adMediaSourceHolders[id.adGroupIndex][id.adIndexInAdGroup] = null; + } + } else { + maskingMediaPeriod.releasePeriod(); } - maskingMediaPeriod.releasePeriod(); } @Override @@ -263,11 +249,9 @@ public final class AdsMediaSource extends CompositeMediaSource { super.releaseSourceInternal(); Assertions.checkNotNull(componentListener).release(); componentListener = null; - maskingMediaPeriodByAdMediaSource.clear(); contentTimeline = null; adPlaybackState = null; - adGroupMediaSources = new MediaSource[0][]; - adGroupTimelines = new Timeline[0][]; + adMediaSourceHolders = new AdMediaSourceHolder[0][]; mainHandler.post(adsLoader::stop); } @@ -277,10 +261,13 @@ public final class AdsMediaSource extends CompositeMediaSource { if (mediaPeriodId.isAd()) { int adGroupIndex = mediaPeriodId.adGroupIndex; int adIndexInAdGroup = mediaPeriodId.adIndexInAdGroup; - onAdSourceInfoRefreshed(mediaSource, adGroupIndex, adIndexInAdGroup, timeline); + Assertions.checkNotNull(adMediaSourceHolders[adGroupIndex][adIndexInAdGroup]) + .handleSourceInfoRefresh(timeline); } else { - onContentSourceInfoRefreshed(timeline); + Assertions.checkArgument(timeline.getPeriodCount() == 1); + contentTimeline = timeline; } + maybeUpdateSourceInfo(); } @Override @@ -295,42 +282,17 @@ public final class AdsMediaSource extends CompositeMediaSource { private void onAdPlaybackState(AdPlaybackState adPlaybackState) { if (this.adPlaybackState == null) { - adGroupMediaSources = new MediaSource[adPlaybackState.adGroupCount][]; - Arrays.fill(adGroupMediaSources, new MediaSource[0]); - adGroupTimelines = new Timeline[adPlaybackState.adGroupCount][]; - Arrays.fill(adGroupTimelines, new Timeline[0]); + adMediaSourceHolders = new AdMediaSourceHolder[adPlaybackState.adGroupCount][]; + Arrays.fill(adMediaSourceHolders, new AdMediaSourceHolder[0]); } this.adPlaybackState = adPlaybackState; maybeUpdateSourceInfo(); } - private void onContentSourceInfoRefreshed(Timeline timeline) { - Assertions.checkArgument(timeline.getPeriodCount() == 1); - contentTimeline = timeline; - maybeUpdateSourceInfo(); - } - - private void onAdSourceInfoRefreshed(MediaSource mediaSource, int adGroupIndex, - int adIndexInAdGroup, Timeline timeline) { - Assertions.checkArgument(timeline.getPeriodCount() == 1); - adGroupTimelines[adGroupIndex][adIndexInAdGroup] = timeline; - List mediaPeriods = maskingMediaPeriodByAdMediaSource.remove(mediaSource); - if (mediaPeriods != null) { - Object periodUid = timeline.getUidOfPeriod(/* periodIndex= */ 0); - for (int i = 0; i < mediaPeriods.size(); i++) { - MaskingMediaPeriod mediaPeriod = mediaPeriods.get(i); - MediaPeriodId adSourceMediaPeriodId = - new MediaPeriodId(periodUid, mediaPeriod.id.windowSequenceNumber); - mediaPeriod.createPeriod(adSourceMediaPeriodId); - } - } - maybeUpdateSourceInfo(); - } - private void maybeUpdateSourceInfo() { - Timeline contentTimeline = this.contentTimeline; + @Nullable Timeline contentTimeline = this.contentTimeline; if (adPlaybackState != null && contentTimeline != null) { - adPlaybackState = adPlaybackState.withAdDurationsUs(getAdDurations(adGroupTimelines, period)); + adPlaybackState = adPlaybackState.withAdDurationsUs(getAdDurationsUs()); Timeline timeline = adPlaybackState.adGroupCount == 0 ? contentTimeline @@ -339,19 +301,16 @@ public final class AdsMediaSource extends CompositeMediaSource { } } - private static long[][] getAdDurations( - @NullableType Timeline[][] adTimelines, Timeline.Period period) { - long[][] adDurations = new long[adTimelines.length][]; - for (int i = 0; i < adTimelines.length; i++) { - adDurations[i] = new long[adTimelines[i].length]; - for (int j = 0; j < adTimelines[i].length; j++) { - adDurations[i][j] = - adTimelines[i][j] == null - ? C.TIME_UNSET - : adTimelines[i][j].getPeriod(/* periodIndex= */ 0, period).getDurationUs(); + private long[][] getAdDurationsUs() { + long[][] adDurationsUs = new long[adMediaSourceHolders.length][]; + for (int i = 0; i < adMediaSourceHolders.length; i++) { + adDurationsUs[i] = new long[adMediaSourceHolders[i].length]; + for (int j = 0; j < adMediaSourceHolders[i].length; j++) { + @Nullable AdMediaSourceHolder holder = adMediaSourceHolders[i][j]; + adDurationsUs[i][j] = holder == null ? C.TIME_UNSET : holder.getDurationUs(); } } - return adDurations; + return adDurationsUs; } /** Listener for component events. All methods are called on the main thread. */ @@ -437,4 +396,61 @@ public final class AdsMediaSource extends CompositeMediaSource { () -> adsLoader.handlePrepareError(adGroupIndex, adIndexInAdGroup, exception)); } } + + private final class AdMediaSourceHolder { + + private final MediaSource adMediaSource; + private final List activeMediaPeriods; + + @MonotonicNonNull private Timeline timeline; + + public AdMediaSourceHolder(MediaSource adMediaSource) { + this.adMediaSource = adMediaSource; + activeMediaPeriods = new ArrayList<>(); + } + + public MediaPeriod createMediaPeriod( + Uri adUri, MediaPeriodId id, Allocator allocator, long startPositionUs) { + MaskingMediaPeriod maskingMediaPeriod = + new MaskingMediaPeriod(adMediaSource, id, allocator, startPositionUs); + maskingMediaPeriod.setPrepareErrorListener( + new AdPrepareErrorListener(adUri, id.adGroupIndex, id.adIndexInAdGroup)); + activeMediaPeriods.add(maskingMediaPeriod); + if (timeline != null) { + Object periodUid = timeline.getUidOfPeriod(/* periodIndex= */ 0); + MediaPeriodId adSourceMediaPeriodId = new MediaPeriodId(periodUid, id.windowSequenceNumber); + maskingMediaPeriod.createPeriod(adSourceMediaPeriodId); + } + return maskingMediaPeriod; + } + + public void handleSourceInfoRefresh(Timeline timeline) { + Assertions.checkArgument(timeline.getPeriodCount() == 1); + if (this.timeline == null) { + Object periodUid = timeline.getUidOfPeriod(/* periodIndex= */ 0); + for (int i = 0; i < activeMediaPeriods.size(); i++) { + MaskingMediaPeriod mediaPeriod = activeMediaPeriods.get(i); + MediaPeriodId adSourceMediaPeriodId = + new MediaPeriodId(periodUid, mediaPeriod.id.windowSequenceNumber); + mediaPeriod.createPeriod(adSourceMediaPeriodId); + } + } + this.timeline = timeline; + } + + public long getDurationUs() { + return timeline == null + ? C.TIME_UNSET + : timeline.getPeriod(/* periodIndex= */ 0, period).getDurationUs(); + } + + public void releaseMediaPeriod(MaskingMediaPeriod maskingMediaPeriod) { + activeMediaPeriods.remove(maskingMediaPeriod); + maskingMediaPeriod.releasePeriod(); + } + + public boolean isInactive() { + return activeMediaPeriods.isEmpty(); + } + } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/source/ads/AdsMediaSourceTest.java b/library/core/src/test/java/com/google/android/exoplayer2/source/ads/AdsMediaSourceTest.java new file mode 100644 index 0000000000..255d1298b7 --- /dev/null +++ b/library/core/src/test/java/com/google/android/exoplayer2/source/ads/AdsMediaSourceTest.java @@ -0,0 +1,216 @@ +/* + * Copyright 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.android.exoplayer2.source.ads; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.robolectric.Shadows.shadowOf; +import static org.robolectric.annotation.LooperMode.Mode.PAUSED; + +import android.net.Uri; +import android.os.Looper; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.android.exoplayer2.C; +import com.google.android.exoplayer2.Timeline; +import com.google.android.exoplayer2.source.MediaPeriod; +import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId; +import com.google.android.exoplayer2.source.MediaSource.MediaSourceCaller; +import com.google.android.exoplayer2.source.MediaSourceFactory; +import com.google.android.exoplayer2.source.SinglePeriodTimeline; +import com.google.android.exoplayer2.source.ads.AdsLoader.AdViewProvider; +import com.google.android.exoplayer2.source.ads.AdsLoader.EventListener; +import com.google.android.exoplayer2.testutil.FakeMediaSource; +import com.google.android.exoplayer2.upstream.Allocator; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; +import org.robolectric.annotation.LooperMode; + +/** Unit tests for {@link AdsMediaSource}. */ +@RunWith(AndroidJUnit4.class) +@LooperMode(PAUSED) +public final class AdsMediaSourceTest { + + private static final long PREROLL_AD_DURATION_US = 10 * C.MICROS_PER_SECOND; + private static final Timeline PREROLL_AD_TIMELINE = + new SinglePeriodTimeline( + PREROLL_AD_DURATION_US, + /* isSeekable= */ true, + /* isDynamic= */ false, + /* isLive= */ false); + private static final Object PREROLL_AD_PERIOD_UID = + PREROLL_AD_TIMELINE.getUidOfPeriod(/* periodIndex= */ 0); + + private static final long CONTENT_DURATION_US = 30 * C.MICROS_PER_SECOND; + private static final Timeline CONTENT_TIMELINE = + new SinglePeriodTimeline( + CONTENT_DURATION_US, /* isSeekable= */ true, /* isDynamic= */ false, /* isLive= */ false); + private static final Object CONTENT_PERIOD_UID = + CONTENT_TIMELINE.getUidOfPeriod(/* periodIndex= */ 0); + + private static final AdPlaybackState AD_PLAYBACK_STATE = + new AdPlaybackState(/* adGroupTimesUs...= */ 0) + .withContentDurationUs(CONTENT_DURATION_US) + .withAdCount(/* adGroupIndex= */ 0, /* adCount= */ 1) + .withAdUri(/* adGroupIndex= */ 0, /* adIndexInAdGroup= */ 0, Uri.EMPTY) + .withPlayedAd(/* adGroupIndex= */ 0, /* adIndexInAdGroup= */ 0) + .withAdResumePositionUs(/* adResumePositionUs= */ 0); + + @Rule public final MockitoRule mockito = MockitoJUnit.rule(); + + private FakeMediaSource contentMediaSource; + private FakeMediaSource prerollAdMediaSource; + @Mock private MediaSourceCaller mockMediaSourceCaller; + private AdsMediaSource adsMediaSource; + + @Before + public void setUp() { + // Set up content and ad media sources, passing a null timeline so tests can simulate setting it + // later. + contentMediaSource = new FakeMediaSource(/* timeline= */ null); + prerollAdMediaSource = new FakeMediaSource(/* timeline= */ null); + MediaSourceFactory adMediaSourceFactory = mock(MediaSourceFactory.class); + when(adMediaSourceFactory.createMediaSource(any(Uri.class))).thenReturn(prerollAdMediaSource); + + // Prepare the AdsMediaSource and capture its ads loader listener. + AdsLoader mockAdsLoader = mock(AdsLoader.class); + AdViewProvider mockAdViewProvider = mock(AdViewProvider.class); + ArgumentCaptor eventListenerArgumentCaptor = + ArgumentCaptor.forClass(AdsLoader.EventListener.class); + adsMediaSource = + new AdsMediaSource( + contentMediaSource, adMediaSourceFactory, mockAdsLoader, mockAdViewProvider); + adsMediaSource.prepareSource(mockMediaSourceCaller, /* mediaTransferListener= */ null); + shadowOf(Looper.getMainLooper()).idle(); + verify(mockAdsLoader).start(eventListenerArgumentCaptor.capture(), eq(mockAdViewProvider)); + + // Simulate loading a preroll ad. + AdsLoader.EventListener adsLoaderEventListener = eventListenerArgumentCaptor.getValue(); + adsLoaderEventListener.onAdPlaybackState(AD_PLAYBACK_STATE); + shadowOf(Looper.getMainLooper()).idle(); + } + + @Test + public void createPeriod_preparesChildAdMediaSourceAndRefreshesSourceInfo() { + contentMediaSource.setNewSourceInfo(CONTENT_TIMELINE); + adsMediaSource.createPeriod( + new MediaPeriodId( + CONTENT_PERIOD_UID, + /* adGroupIndex= */ 0, + /* adIndexInAdGroup= */ 0, + /* windowSequenceNumber= */ 0), + mock(Allocator.class), + /* startPositionUs= */ 0); + shadowOf(Looper.getMainLooper()).idle(); + + assertThat(prerollAdMediaSource.isPrepared()).isTrue(); + verify(mockMediaSourceCaller) + .onSourceInfoRefreshed( + adsMediaSource, new SinglePeriodAdTimeline(CONTENT_TIMELINE, AD_PLAYBACK_STATE)); + } + + @Test + public void createPeriod_preparesChildAdMediaSourceAndRefreshesSourceInfoWithAdMediaSourceInfo() { + contentMediaSource.setNewSourceInfo(CONTENT_TIMELINE); + adsMediaSource.createPeriod( + new MediaPeriodId( + CONTENT_PERIOD_UID, + /* adGroupIndex= */ 0, + /* adIndexInAdGroup= */ 0, + /* windowSequenceNumber= */ 0), + mock(Allocator.class), + /* startPositionUs= */ 0); + prerollAdMediaSource.setNewSourceInfo(PREROLL_AD_TIMELINE); + shadowOf(Looper.getMainLooper()).idle(); + + verify(mockMediaSourceCaller) + .onSourceInfoRefreshed( + adsMediaSource, + new SinglePeriodAdTimeline( + CONTENT_TIMELINE, + AD_PLAYBACK_STATE.withAdDurationsUs(new long[][] {{PREROLL_AD_DURATION_US}}))); + } + + @Test + public void createPeriod_createsChildPrerollAdMediaPeriod() { + contentMediaSource.setNewSourceInfo(CONTENT_TIMELINE); + adsMediaSource.createPeriod( + new MediaPeriodId( + CONTENT_PERIOD_UID, + /* adGroupIndex= */ 0, + /* adIndexInAdGroup= */ 0, + /* windowSequenceNumber= */ 0), + mock(Allocator.class), + /* startPositionUs= */ 0); + prerollAdMediaSource.setNewSourceInfo(PREROLL_AD_TIMELINE); + shadowOf(Looper.getMainLooper()).idle(); + + prerollAdMediaSource.assertMediaPeriodCreated( + new MediaPeriodId(PREROLL_AD_PERIOD_UID, /* windowSequenceNumber= */ 0)); + } + + @Test + public void createPeriod_createsChildContentMediaPeriod() { + contentMediaSource.setNewSourceInfo(CONTENT_TIMELINE); + shadowOf(Looper.getMainLooper()).idle(); + adsMediaSource.createPeriod( + new MediaPeriodId(CONTENT_PERIOD_UID, /* windowSequenceNumber= */ 0), + mock(Allocator.class), + /* startPositionUs= */ 0); + + contentMediaSource.assertMediaPeriodCreated( + new MediaPeriodId(CONTENT_PERIOD_UID, /* windowSequenceNumber= */ 0)); + } + + @Test + public void releasePeriod_releasesChildMediaPeriodsAndSources() { + contentMediaSource.setNewSourceInfo(CONTENT_TIMELINE); + MediaPeriod prerollAdMediaPeriod = + adsMediaSource.createPeriod( + new MediaPeriodId( + CONTENT_PERIOD_UID, + /* adGroupIndex= */ 0, + /* adIndexInAdGroup= */ 0, + /* windowSequenceNumber= */ 0), + mock(Allocator.class), + /* startPositionUs= */ 0); + prerollAdMediaSource.setNewSourceInfo(PREROLL_AD_TIMELINE); + shadowOf(Looper.getMainLooper()).idle(); + MediaPeriod contentMediaPeriod = + adsMediaSource.createPeriod( + new MediaPeriodId(CONTENT_PERIOD_UID, /* windowSequenceNumber= */ 0), + mock(Allocator.class), + /* startPositionUs= */ 0); + adsMediaSource.releasePeriod(prerollAdMediaPeriod); + + prerollAdMediaSource.assertReleased(); + + adsMediaSource.releasePeriod(contentMediaPeriod); + adsMediaSource.releaseSource(mockMediaSourceCaller); + shadowOf(Looper.getMainLooper()).idle(); + prerollAdMediaSource.assertReleased(); + contentMediaSource.assertReleased(); + } +}