From 656c2172ddf6feedeb0bf7b012329ad8342b6fa9 Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 27 Jul 2018 09:25:27 -0700 Subject: [PATCH] Fix event reporting for merging and looping media sources. The looping media source doesn't convert the media period id to the externally visible media period id. And the merging media source reports media period creations multiple times which will break listeners assuming a media period with a specific id will only be created once. Also amend the doc for MediaSource.createPeriod to reflect that media periods created in parallel do not actually have the same id. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=206327241 --- .../exoplayer2/source/LoopingMediaSource.java | 30 +++++++++++++-- .../exoplayer2/source/MediaSource.java | 7 ++-- .../exoplayer2/source/MergingMediaSource.java | 6 +++ .../source/LoopingMediaSourceTest.java | 38 +++++++++++++++++-- .../source/MergingMediaSourceTest.java | 21 ++++++++++ .../testutil/MediaSourceTestRunner.java | 18 ++++++--- 6 files changed, 104 insertions(+), 16 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/LoopingMediaSource.java b/library/core/src/main/java/com/google/android/exoplayer2/source/LoopingMediaSource.java index d370a33302..e83138cd33 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/LoopingMediaSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/LoopingMediaSource.java @@ -24,6 +24,8 @@ import com.google.android.exoplayer2.source.ShuffleOrder.UnshuffledShuffleOrder; import com.google.android.exoplayer2.upstream.Allocator; import com.google.android.exoplayer2.upstream.TransferListener; import com.google.android.exoplayer2.util.Assertions; +import java.util.HashMap; +import java.util.Map; /** * Loops a {@link MediaSource} a specified number of times. @@ -35,6 +37,8 @@ public final class LoopingMediaSource extends CompositeMediaSource { private final MediaSource childSource; private final int loopCount; + private final Map childMediaPeriodIdToMediaPeriodId; + private final Map mediaPeriodToChildMediaPeriodId; private int childPeriodCount; @@ -58,6 +62,8 @@ public final class LoopingMediaSource extends CompositeMediaSource { Assertions.checkArgument(loopCount > 0); this.childSource = childSource; this.loopCount = loopCount; + childMediaPeriodIdToMediaPeriodId = new HashMap<>(); + mediaPeriodToChildMediaPeriodId = new HashMap<>(); } @Override @@ -71,15 +77,23 @@ public final class LoopingMediaSource extends CompositeMediaSource { @Override public MediaPeriod createPeriod(MediaPeriodId id, Allocator allocator) { - return loopCount != Integer.MAX_VALUE - ? childSource.createPeriod(id.copyWithPeriodIndex(id.periodIndex % childPeriodCount), - allocator) - : childSource.createPeriod(id, allocator); + if (loopCount == Integer.MAX_VALUE) { + return childSource.createPeriod(id, allocator); + } + MediaPeriodId childMediaPeriodId = id.copyWithPeriodIndex(id.periodIndex % childPeriodCount); + childMediaPeriodIdToMediaPeriodId.put(childMediaPeriodId, id); + MediaPeriod mediaPeriod = childSource.createPeriod(childMediaPeriodId, allocator); + mediaPeriodToChildMediaPeriodId.put(mediaPeriod, childMediaPeriodId); + return mediaPeriod; } @Override public void releasePeriod(MediaPeriod mediaPeriod) { childSource.releasePeriod(mediaPeriod); + MediaPeriodId childMediaPeriodId = mediaPeriodToChildMediaPeriodId.remove(mediaPeriod); + if (childMediaPeriodId != null) { + childMediaPeriodIdToMediaPeriodId.remove(childMediaPeriodId); + } } @Override @@ -99,6 +113,14 @@ public final class LoopingMediaSource extends CompositeMediaSource { refreshSourceInfo(loopingTimeline, manifest); } + @Override + protected @Nullable MediaPeriodId getMediaPeriodIdForChildMediaPeriodId( + Void id, MediaPeriodId mediaPeriodId) { + return loopCount != Integer.MAX_VALUE + ? childMediaPeriodIdToMediaPeriodId.get(mediaPeriodId) + : mediaPeriodId; + } + private static final class LoopingTimeline extends AbstractConcatenatedTimeline { private final Timeline childTimeline; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/MediaSource.java b/library/core/src/main/java/com/google/android/exoplayer2/source/MediaSource.java index cba636b690..fb4c64ae6e 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/MediaSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/MediaSource.java @@ -267,10 +267,9 @@ public interface MediaSource { /** * Returns a new {@link MediaPeriod} identified by {@code periodId}. This method may be called - * multiple times with the same period identifier without an intervening call to - * {@link #releasePeriod(MediaPeriod)}. - *

- * Should not be called directly from application code. + * multiple times without an intervening call to {@link #releasePeriod(MediaPeriod)}. + * + *

Should not be called directly from application code. * * @param id The identifier of the period. * @param allocator An {@link Allocator} from which to obtain media buffer allocations. diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/MergingMediaSource.java b/library/core/src/main/java/com/google/android/exoplayer2/source/MergingMediaSource.java index b29eb60af4..d33cfb8abd 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/MergingMediaSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/MergingMediaSource.java @@ -163,6 +163,12 @@ public final class MergingMediaSource extends CompositeMediaSource { } } + @Override + protected @Nullable MediaPeriodId getMediaPeriodIdForChildMediaPeriodId( + Integer id, MediaPeriodId mediaPeriodId) { + return id == 0 ? mediaPeriodId : null; + } + private IllegalMergeException checkTimelineMerges(Timeline timeline) { if (periodCount == PERIOD_COUNT_UNSET) { periodCount = timeline.getPeriodCount(); diff --git a/library/core/src/test/java/com/google/android/exoplayer2/source/LoopingMediaSourceTest.java b/library/core/src/test/java/com/google/android/exoplayer2/source/LoopingMediaSourceTest.java index d639bc168a..9b7455ee37 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/source/LoopingMediaSourceTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/source/LoopingMediaSourceTest.java @@ -48,7 +48,7 @@ public class LoopingMediaSourceTest { } @Test - public void testSingleLoop() throws IOException { + public void testSingleLoopTimeline() throws IOException { Timeline timeline = getLoopingTimeline(multiWindowTimeline, 1); TimelineAsserts.assertWindowTags(timeline, 111, 222, 333); TimelineAsserts.assertPeriodCounts(timeline, 1, 1, 1); @@ -67,7 +67,7 @@ public class LoopingMediaSourceTest { } @Test - public void testMultiLoop() throws IOException { + public void testMultiLoopTimeline() throws IOException { Timeline timeline = getLoopingTimeline(multiWindowTimeline, 3); TimelineAsserts.assertWindowTags(timeline, 111, 222, 333, 111, 222, 333, 111, 222, 333); TimelineAsserts.assertPeriodCounts(timeline, 1, 1, 1, 1, 1, 1, 1, 1, 1); @@ -88,7 +88,7 @@ public class LoopingMediaSourceTest { } @Test - public void testInfiniteLoop() throws IOException { + public void testInfiniteLoopTimeline() throws IOException { Timeline timeline = getLoopingTimeline(multiWindowTimeline, Integer.MAX_VALUE); TimelineAsserts.assertWindowTags(timeline, 111, 222, 333); TimelineAsserts.assertPeriodCounts(timeline, 1, 1, 1); @@ -117,6 +117,21 @@ public class LoopingMediaSourceTest { TimelineAsserts.assertEmpty(timeline); } + @Test + public void testSingleLoopPeriodCreation() throws Exception { + testMediaPeriodCreation(multiWindowTimeline, /* loopCount= */ 1); + } + + @Test + public void testMultiLoopPeriodCreation() throws Exception { + testMediaPeriodCreation(multiWindowTimeline, /* loopCount= */ 3); + } + + @Test + public void testInfiniteLoopPeriodCreation() throws Exception { + testMediaPeriodCreation(multiWindowTimeline, /* loopCount= */ Integer.MAX_VALUE); + } + /** * Wraps the specified timeline in a {@link LoopingMediaSource} and returns the looping timeline. */ @@ -133,4 +148,21 @@ public class LoopingMediaSourceTest { testRunner.release(); } } + + /** + * Wraps the specified timeline in a {@link LoopingMediaSource} and asserts that all periods of + * the looping timeline can be created and prepared. + */ + private static void testMediaPeriodCreation(Timeline timeline, int loopCount) throws Exception { + FakeMediaSource fakeMediaSource = new FakeMediaSource(timeline, null); + LoopingMediaSource mediaSource = new LoopingMediaSource(fakeMediaSource, loopCount); + MediaSourceTestRunner testRunner = new MediaSourceTestRunner(mediaSource, null); + try { + testRunner.prepareSource(); + testRunner.assertPrepareAndReleaseAllPeriods(); + testRunner.releaseSource(); + } finally { + testRunner.release(); + } + } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/source/MergingMediaSourceTest.java b/library/core/src/test/java/com/google/android/exoplayer2/source/MergingMediaSourceTest.java index 839492f196..e74347d2f4 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/source/MergingMediaSourceTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/source/MergingMediaSourceTest.java @@ -65,6 +65,27 @@ public class MergingMediaSourceTest { } } + @Test + public void testMergingMediaSourcePeriodCreation() throws Exception { + FakeMediaSource[] mediaSources = new FakeMediaSource[2]; + for (int i = 0; i < mediaSources.length; i++) { + mediaSources[i] = + new FakeMediaSource(new FakeTimeline(/* windowCount= */ 2), /* manifest= */ null); + } + MergingMediaSource mediaSource = new MergingMediaSource(mediaSources); + MediaSourceTestRunner testRunner = new MediaSourceTestRunner(mediaSource, null); + try { + testRunner.prepareSource(); + testRunner.assertPrepareAndReleaseAllPeriods(); + for (FakeMediaSource element : mediaSources) { + assertThat(element.getCreatedMediaPeriods()).isNotEmpty(); + } + testRunner.releaseSource(); + } finally { + testRunner.release(); + } + } + /** * Wraps the specified timelines in a {@link MergingMediaSource}, prepares it and checks that it * forwards the first of the wrapped timelines. diff --git a/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/MediaSourceTestRunner.java b/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/MediaSourceTestRunner.java index 6995b67a7b..3fffdb2696 100644 --- a/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/MediaSourceTestRunner.java +++ b/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/MediaSourceTestRunner.java @@ -295,17 +295,25 @@ public class MediaSourceTestRunner { assertThat(lastCreatedMediaPeriod.getAndSet(/* newValue= */ null)).isEqualTo(mediaPeriodId); CountDownLatch preparedCondition = preparePeriod(mediaPeriod, 0); assertThat(preparedCondition.await(TIMEOUT_MS, TimeUnit.MILLISECONDS)).isTrue(); - // MediaSource is supposed to support multiple calls to createPeriod with the same id without an - // intervening call to releasePeriod. - MediaPeriod secondMediaPeriod = createPeriod(mediaPeriodId); - assertThat(lastCreatedMediaPeriod.getAndSet(/* newValue= */ null)).isEqualTo(mediaPeriodId); + // MediaSource is supposed to support multiple calls to createPeriod without an intervening call + // to releasePeriod. + MediaPeriodId secondMediaPeriodId = + new MediaPeriodId( + mediaPeriodId.periodIndex, + mediaPeriodId.adGroupIndex, + mediaPeriodId.adIndexInAdGroup, + mediaPeriodId.windowSequenceNumber + 1000); + MediaPeriod secondMediaPeriod = createPeriod(secondMediaPeriodId); + assertThat(lastCreatedMediaPeriod.getAndSet(/* newValue= */ null)) + .isEqualTo(secondMediaPeriodId); CountDownLatch secondPreparedCondition = preparePeriod(secondMediaPeriod, 0); assertThat(secondPreparedCondition.await(TIMEOUT_MS, TimeUnit.MILLISECONDS)).isTrue(); // Release the periods. releasePeriod(mediaPeriod); assertThat(lastReleasedMediaPeriod.getAndSet(/* newValue= */ null)).isEqualTo(mediaPeriodId); releasePeriod(secondMediaPeriod); - assertThat(lastReleasedMediaPeriod.getAndSet(/* newValue= */ null)).isEqualTo(mediaPeriodId); + assertThat(lastReleasedMediaPeriod.getAndSet(/* newValue= */ null)) + .isEqualTo(secondMediaPeriodId); } /**