From c64d9fd6da90497547daf3deea536af2007784a5 Mon Sep 17 00:00:00 2001 From: tonihei Date: Mon, 17 Jul 2023 12:17:17 +0100 Subject: [PATCH] Fix race condition in clipped sample streams The streams return end-of-input if they read no samples, but know that they are fully buffered to at least the clipped end time. This helps to detect the end of stream even if there are no new buffers after the end of the clip (e.g. for sparse metadata tracks). The race condition occurs because the buffered position is evaluated after reading the sample. So between reading "no sample" and checking the buffered position, the source may have loaded arbitrary amounts of data. This may lead to a situation where the source has not read all samples, reads NOTHING_READ (because the queue is empty) and then immediately returns end-of-stream (because the buffered position jumped forward), causing all remaining samples in the stream to be skipped. This can fixed by moving the buffered position check to before reading the sample, so that it never exceeds the buffered position at the time of reading "no sample". #minor-release PiperOrigin-RevId: 548646464 --- .../exoplayer/source/ClippingMediaPeriod.java | 3 +- .../ads/ServerSideAdInsertionMediaSource.java | 3 +- .../source/ClippingMediaPeriodTest.java | 146 ++++++++++++++++++ .../ServerSideAdInsertionMediaSourceTest.java | 143 +++++++++++++++++ 4 files changed, 293 insertions(+), 2 deletions(-) create mode 100644 libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/ClippingMediaPeriodTest.java diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/ClippingMediaPeriod.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/ClippingMediaPeriod.java index e4dfd16a3f..8acbc4044e 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/ClippingMediaPeriod.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/ClippingMediaPeriod.java @@ -327,6 +327,7 @@ public final class ClippingMediaPeriod implements MediaPeriod, MediaPeriod.Callb buffer.setFlags(C.BUFFER_FLAG_END_OF_STREAM); return C.RESULT_BUFFER_READ; } + long bufferedPositionUs = getBufferedPositionUs(); @ReadDataResult int result = childStream.readData(formatHolder, buffer, readFlags); if (result == C.RESULT_FORMAT_READ) { Format format = Assertions.checkNotNull(formatHolder.format); @@ -346,7 +347,7 @@ public final class ClippingMediaPeriod implements MediaPeriod, MediaPeriod.Callb if (endUs != C.TIME_END_OF_SOURCE && ((result == C.RESULT_BUFFER_READ && buffer.timeUs >= endUs) || (result == C.RESULT_NOTHING_READ - && getBufferedPositionUs() == C.TIME_END_OF_SOURCE + && bufferedPositionUs == C.TIME_END_OF_SOURCE && !buffer.waitingForKeys))) { buffer.clear(); buffer.setFlags(C.BUFFER_FLAG_END_OF_STREAM); diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/ads/ServerSideAdInsertionMediaSource.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/ads/ServerSideAdInsertionMediaSource.java index 81203c5cba..98db8c05d8 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/ads/ServerSideAdInsertionMediaSource.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/source/ads/ServerSideAdInsertionMediaSource.java @@ -879,6 +879,7 @@ public final class ServerSideAdInsertionMediaSource extends BaseMediaSource @SampleStream.ReadFlags int readFlags) { @SampleStream.ReadFlags int peekingFlags = readFlags | SampleStream.FLAG_PEEK | SampleStream.FLAG_OMIT_SAMPLE_DATA; + long bufferedPositionUs = getBufferedPositionUs(mediaPeriod); @SampleStream.ReadDataResult int result = castNonNull(sampleStreams[streamIndex]).readData(formatHolder, buffer, peekingFlags); @@ -886,7 +887,7 @@ public final class ServerSideAdInsertionMediaSource extends BaseMediaSource getMediaPeriodPositionUsWithEndOfSourceHandling(mediaPeriod, buffer.timeUs); if ((result == C.RESULT_BUFFER_READ && adjustedTimeUs == C.TIME_END_OF_SOURCE) || (result == C.RESULT_NOTHING_READ - && getBufferedPositionUs(mediaPeriod) == C.TIME_END_OF_SOURCE + && bufferedPositionUs == C.TIME_END_OF_SOURCE && !buffer.waitingForKeys)) { maybeNotifyDownstreamFormatChanged(mediaPeriod, streamIndex); buffer.clear(); diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/ClippingMediaPeriodTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/ClippingMediaPeriodTest.java new file mode 100644 index 0000000000..cc9804be59 --- /dev/null +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/ClippingMediaPeriodTest.java @@ -0,0 +1,146 @@ +/* + * Copyright 2023 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 androidx.media3.exoplayer.source; + +import static androidx.media3.test.utils.FakeSampleStream.FakeSampleStreamItem.END_OF_STREAM_ITEM; +import static androidx.media3.test.utils.FakeSampleStream.FakeSampleStreamItem.oneByteSample; +import static com.google.common.truth.Truth.assertThat; + +import androidx.annotation.Nullable; +import androidx.media3.common.C; +import androidx.media3.common.Format; +import androidx.media3.common.TrackGroup; +import androidx.media3.decoder.DecoderInputBuffer; +import androidx.media3.exoplayer.FormatHolder; +import androidx.media3.exoplayer.drm.DrmSessionEventListener; +import androidx.media3.exoplayer.drm.DrmSessionManager; +import androidx.media3.exoplayer.trackselection.ExoTrackSelection; +import androidx.media3.exoplayer.trackselection.FixedTrackSelection; +import androidx.media3.exoplayer.upstream.Allocator; +import androidx.media3.exoplayer.upstream.DefaultAllocator; +import androidx.media3.test.utils.FakeMediaPeriod; +import androidx.media3.test.utils.FakeSampleStream; +import androidx.media3.test.utils.robolectric.RobolectricUtil; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.common.collect.ImmutableList; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** Unit tests for {@link ClippingMediaPeriod}. */ +@RunWith(AndroidJUnit4.class) +public class ClippingMediaPeriodTest { + + @Test + public void fastLoadingStreamAfterFirstRead_canBeReadFully() throws Exception { + TrackGroup trackGroup = new TrackGroup(new Format.Builder().build()); + // Set up MediaPeriod with no samples and only add samples after the first SampleStream read. + FakeMediaPeriod mediaPeriod = + new FakeMediaPeriod( + new TrackGroupArray(trackGroup), + new DefaultAllocator(/* trimOnReset= */ true, /* individualAllocationSize= */ 1024), + /* trackDataFactory= */ (format, mediaPeriodId) -> ImmutableList.of(), + new MediaSourceEventListener.EventDispatcher() + .withParameters( + /* windowIndex= */ 0, + new MediaSource.MediaPeriodId(/* periodUid= */ new Object())), + DrmSessionManager.DRM_UNSUPPORTED, + new DrmSessionEventListener.EventDispatcher(), + /* deferOnPrepared= */ false) { + @Override + protected FakeSampleStream createSampleStream( + Allocator allocator, + @Nullable MediaSourceEventListener.EventDispatcher mediaSourceEventDispatcher, + DrmSessionManager drmSessionManager, + DrmSessionEventListener.EventDispatcher drmEventDispatcher, + Format initialFormat, + List fakeSampleStreamItems) { + return new FakeSampleStream( + allocator, + mediaSourceEventDispatcher, + drmSessionManager, + drmEventDispatcher, + initialFormat, + /* fakeSampleStreamItems= */ ImmutableList.of()) { + private boolean addedSamples = false; + + @Override + public int readData( + FormatHolder formatHolder, DecoderInputBuffer buffer, @ReadFlags int readFlags) { + int result = super.readData(formatHolder, buffer, readFlags); + if (!addedSamples) { + append( + ImmutableList.of( + oneByteSample(/* timeUs= */ 0, C.BUFFER_FLAG_KEY_FRAME), + oneByteSample(/* timeUs= */ 200, C.BUFFER_FLAG_KEY_FRAME), + oneByteSample(/* timeUs= */ 400, C.BUFFER_FLAG_KEY_FRAME), + oneByteSample(/* timeUs= */ 600, C.BUFFER_FLAG_KEY_FRAME), + oneByteSample(/* timeUs= */ 800, C.BUFFER_FLAG_KEY_FRAME), + END_OF_STREAM_ITEM)); + writeData(/* startPositionUs= */ 0); + addedSamples = true; + } + return result; + } + }; + } + }; + ClippingMediaPeriod clippingMediaPeriod = + new ClippingMediaPeriod( + mediaPeriod, + /* enableInitialDiscontinuity= */ true, + /* startUs= */ 0, + /* endUs= */ 500); + AtomicBoolean periodPrepared = new AtomicBoolean(); + clippingMediaPeriod.prepare( + new MediaPeriod.Callback() { + @Override + public void onPrepared(MediaPeriod mediaPeriod) { + periodPrepared.set(true); + } + + @Override + public void onContinueLoadingRequested(MediaPeriod source) { + clippingMediaPeriod.continueLoading(/* positionUs= */ 0); + } + }, + /* positionUs= */ 0); + RobolectricUtil.runMainLooperUntil(periodPrepared::get); + SampleStream[] sampleStreams = new SampleStream[1]; + clippingMediaPeriod.selectTracks( + new ExoTrackSelection[] {new FixedTrackSelection(trackGroup, /* track= */ 0)}, + /* mayRetainStreamFlags= */ new boolean[] {false}, + sampleStreams, + /* streamResetFlags= */ new boolean[] {true}, + /* positionUs= */ 0); + FormatHolder formatHolder = new FormatHolder(); + DecoderInputBuffer buffer = + new DecoderInputBuffer(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_NORMAL); + ArrayList readSamples = new ArrayList<>(); + + int result; + do { + result = sampleStreams[0].readData(formatHolder, buffer, /* readFlags= */ 0); + if (result == C.RESULT_BUFFER_READ && !buffer.isEndOfStream()) { + readSamples.add(buffer.timeUs); + } + } while (result != C.RESULT_BUFFER_READ || !buffer.isEndOfStream()); + + assertThat(readSamples).containsExactly(0L, 200L, 400L).inOrder(); + } +} diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/ads/ServerSideAdInsertionMediaSourceTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/ads/ServerSideAdInsertionMediaSourceTest.java index 2141ee3f08..a1cad8a2a9 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/ads/ServerSideAdInsertionMediaSourceTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/source/ads/ServerSideAdInsertionMediaSourceTest.java @@ -18,6 +18,8 @@ package androidx.media3.exoplayer.source.ads; import static androidx.media3.common.C.DATA_TYPE_MEDIA; import static androidx.media3.common.util.Assertions.checkNotNull; import static androidx.media3.exoplayer.source.ads.ServerSideAdInsertionUtil.addAdGroupToAdPlaybackState; +import static androidx.media3.test.utils.FakeSampleStream.FakeSampleStreamItem.END_OF_STREAM_ITEM; +import static androidx.media3.test.utils.FakeSampleStream.FakeSampleStreamItem.oneByteSample; import static androidx.media3.test.utils.robolectric.RobolectricUtil.runMainLooperUntil; import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.playUntilPosition; import static androidx.media3.test.utils.robolectric.TestPlayerRunHelper.runUntilIsLoading; @@ -45,29 +47,46 @@ import androidx.media3.common.Format; import androidx.media3.common.MediaItem; import androidx.media3.common.Player; import androidx.media3.common.Timeline; +import androidx.media3.common.TrackGroup; import androidx.media3.common.util.Clock; import androidx.media3.common.util.Util; +import androidx.media3.datasource.TransferListener; +import androidx.media3.decoder.DecoderInputBuffer; import androidx.media3.exoplayer.ExoPlayer; +import androidx.media3.exoplayer.FormatHolder; import androidx.media3.exoplayer.analytics.AnalyticsListener; import androidx.media3.exoplayer.analytics.PlayerId; +import androidx.media3.exoplayer.drm.DrmSessionEventListener; +import androidx.media3.exoplayer.drm.DrmSessionManager; import androidx.media3.exoplayer.source.DefaultMediaSourceFactory; import androidx.media3.exoplayer.source.MediaLoadData; import androidx.media3.exoplayer.source.MediaPeriod; import androidx.media3.exoplayer.source.MediaSource; import androidx.media3.exoplayer.source.MediaSourceEventListener; +import androidx.media3.exoplayer.source.SampleStream; import androidx.media3.exoplayer.source.SinglePeriodTimeline; +import androidx.media3.exoplayer.source.TrackGroupArray; +import androidx.media3.exoplayer.trackselection.ExoTrackSelection; +import androidx.media3.exoplayer.trackselection.FixedTrackSelection; +import androidx.media3.exoplayer.upstream.Allocator; import androidx.media3.exoplayer.upstream.DefaultAllocator; import androidx.media3.test.utils.CapturingRenderersFactory; import androidx.media3.test.utils.DumpFileAsserts; import androidx.media3.test.utils.FakeClock; +import androidx.media3.test.utils.FakeMediaPeriod; import androidx.media3.test.utils.FakeMediaSource; +import androidx.media3.test.utils.FakeSampleStream; import androidx.media3.test.utils.FakeTimeline; import androidx.media3.test.utils.robolectric.PlaybackOutput; +import androidx.media3.test.utils.robolectric.RobolectricUtil; import androidx.media3.test.utils.robolectric.ShadowMediaCodecConfig; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import org.junit.Assert; import org.junit.Rule; @@ -689,4 +708,128 @@ public final class ServerSideAdInsertionMediaSourceTest { // Assert playback progression was smooth (=no unexpected delays that cause audio to underrun) verify(listener, never()).onAudioUnderrun(any(), anyInt(), anyLong(), anyLong()); } + + @Test + public void serverSideAdInsertionSampleStream_withFastLoadingSourceAfterFirstRead_canBeReadFully() + throws Exception { + TrackGroup trackGroup = new TrackGroup(new Format.Builder().build()); + // Set up MediaPeriod with no samples and only add samples after the first SampleStream read. + FakeMediaPeriod mediaPeriod = + new FakeMediaPeriod( + new TrackGroupArray(trackGroup), + new DefaultAllocator(/* trimOnReset= */ true, /* individualAllocationSize= */ 1024), + /* trackDataFactory= */ (format, mediaPeriodId) -> ImmutableList.of(), + new MediaSourceEventListener.EventDispatcher() + .withParameters( + /* windowIndex= */ 0, + new MediaSource.MediaPeriodId(/* periodUid= */ new Object())), + DrmSessionManager.DRM_UNSUPPORTED, + new DrmSessionEventListener.EventDispatcher(), + /* deferOnPrepared= */ false) { + @Override + protected FakeSampleStream createSampleStream( + Allocator allocator, + @Nullable MediaSourceEventListener.EventDispatcher mediaSourceEventDispatcher, + DrmSessionManager drmSessionManager, + DrmSessionEventListener.EventDispatcher drmEventDispatcher, + Format initialFormat, + List fakeSampleStreamItems) { + return new FakeSampleStream( + allocator, + mediaSourceEventDispatcher, + drmSessionManager, + drmEventDispatcher, + initialFormat, + /* fakeSampleStreamItems= */ ImmutableList.of()) { + private boolean addedSamples = false; + + @Override + public int readData( + FormatHolder formatHolder, DecoderInputBuffer buffer, @ReadFlags int readFlags) { + int result = super.readData(formatHolder, buffer, readFlags); + if (!addedSamples) { + append( + ImmutableList.of( + oneByteSample(/* timeUs= */ 0, C.BUFFER_FLAG_KEY_FRAME), + oneByteSample(/* timeUs= */ 200, C.BUFFER_FLAG_KEY_FRAME), + oneByteSample(/* timeUs= */ 400, C.BUFFER_FLAG_KEY_FRAME), + oneByteSample(/* timeUs= */ 600, C.BUFFER_FLAG_KEY_FRAME), + oneByteSample(/* timeUs= */ 800, C.BUFFER_FLAG_KEY_FRAME), + END_OF_STREAM_ITEM)); + writeData(/* startPositionUs= */ 0); + addedSamples = true; + } + return result; + } + }; + } + }; + FakeMediaSource mediaSource = + new FakeMediaSource() { + @Override + protected MediaPeriod createMediaPeriod( + MediaPeriodId id, + TrackGroupArray trackGroupArray, + Allocator allocator, + MediaSourceEventListener.EventDispatcher mediaSourceEventDispatcher, + DrmSessionManager drmSessionManager, + DrmSessionEventListener.EventDispatcher drmEventDispatcher, + @Nullable TransferListener transferListener) { + return mediaPeriod; + } + }; + ServerSideAdInsertionMediaSource serverSideAdInsertionMediaSource = + new ServerSideAdInsertionMediaSource(mediaSource, /* adPlaybackStateUpdater= */ null); + Timeline timeline = new FakeTimeline(); + Object periodUid = timeline.getUidOfPeriod(/* periodIndex= */ 0); + serverSideAdInsertionMediaSource.setAdPlaybackStates( + ImmutableMap.of(periodUid, new AdPlaybackState(/* adsId= */ new Object())), timeline); + AtomicBoolean sourcePrepared = new AtomicBoolean(); + serverSideAdInsertionMediaSource.prepareSource( + (source, newTimeline) -> sourcePrepared.set(true), + /* mediaTransferListener= */ null, + PlayerId.UNSET); + RobolectricUtil.runMainLooperUntil(sourcePrepared::get); + MediaPeriod serverSideAdInsertionMediaPeriod = + serverSideAdInsertionMediaSource.createPeriod( + new MediaSource.MediaPeriodId(periodUid), + /* allocator= */ null, + /* startPositionUs= */ 0); + AtomicBoolean periodPrepared = new AtomicBoolean(); + serverSideAdInsertionMediaPeriod.prepare( + new MediaPeriod.Callback() { + @Override + public void onPrepared(MediaPeriod mediaPeriod) { + periodPrepared.set(true); + } + + @Override + public void onContinueLoadingRequested(MediaPeriod source) { + serverSideAdInsertionMediaPeriod.continueLoading(/* positionUs= */ 0); + } + }, + /* positionUs= */ 0); + RobolectricUtil.runMainLooperUntil(periodPrepared::get); + SampleStream[] sampleStreams = new SampleStream[1]; + serverSideAdInsertionMediaPeriod.selectTracks( + new ExoTrackSelection[] {new FixedTrackSelection(trackGroup, /* track= */ 0)}, + /* mayRetainStreamFlags= */ new boolean[] {false}, + sampleStreams, + /* streamResetFlags= */ new boolean[] {true}, + /* positionUs= */ 0); + FormatHolder formatHolder = new FormatHolder(); + DecoderInputBuffer buffer = + new DecoderInputBuffer(DecoderInputBuffer.BUFFER_REPLACEMENT_MODE_NORMAL); + ArrayList readSamples = new ArrayList<>(); + + int result; + do { + result = sampleStreams[0].readData(formatHolder, buffer, /* readFlags= */ 0); + if (result == C.RESULT_BUFFER_READ && !buffer.isEndOfStream()) { + readSamples.add(buffer.timeUs); + } + } while (result != C.RESULT_BUFFER_READ || !buffer.isEndOfStream()); + + assertThat(readSamples).containsExactly(0L, 200L, 400L, 600L, 800L).inOrder(); + } }