From 9bc89ae98908d446a128f28c9f3bb653fdd3b940 Mon Sep 17 00:00:00 2001 From: rohks Date: Tue, 17 Sep 2024 05:24:10 -0700 Subject: [PATCH] Ensure track indices are only added when samples are committed Previously, track IDs were added to `trackIndicesPerSampleInQueuedOrder` even when the sample was not committed. This caused issues where attempts to read samples from the `SampleQueue` returned `C.RESULT_READ_NOTHING`, which led to an exception being thrown due to the assumption that samples were available to read. This fix updates the logic to track sample commits by comparing the write index before and after calling `SampleQueue.sampleMetadata`. Track indices are only added if the sample was committed, ensuring accurate sample handling and avoiding exceptions. PiperOrigin-RevId: 675526115 --- .../exoplayer/MediaExtractorCompat.java | 26 +++++------- .../exoplayer/MediaExtractorCompatTest.java | 40 +++++++++++++++++++ 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/MediaExtractorCompat.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/MediaExtractorCompat.java index efeb57853c..8211f740dd 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/MediaExtractorCompat.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/MediaExtractorCompat.java @@ -17,6 +17,7 @@ package androidx.media3.exoplayer; import static androidx.annotation.VisibleForTesting.NONE; import static androidx.media3.common.util.Assertions.checkNotNull; +import static androidx.media3.common.util.Assertions.checkState; import static androidx.media3.exoplayer.source.SampleStream.FLAG_OMIT_SAMPLE_DATA; import static androidx.media3.exoplayer.source.SampleStream.FLAG_PEEK; import static androidx.media3.exoplayer.source.SampleStream.FLAG_REQUIRE_FORMAT; @@ -609,17 +610,8 @@ public final class MediaExtractorCompat { formatHolder, decoderInputBuffer, readFlags, /* loadingFinished= */ false); } formatHolder.clear(); - // Additional logging is added to debug b/241321832. - if (result != C.RESULT_BUFFER_READ) { - // This method should only be called when there is a sample available for reading. - throw new IllegalStateException( - Util.formatInvariant( - "Sample read result: %s\n" - + "Track sample: %s\n" - + "TrackIndicesPerSampleInQueuedOrder: %s\n" - + "Tracks added: %s\n", - result, trackOfSample, trackIndicesPerSampleInQueuedOrder, tracks)); - } + // This method must only be called when there is a sample available for reading. + checkState(result == C.RESULT_BUFFER_READ); } /** @@ -909,12 +901,16 @@ public final class MediaExtractorCompat { // Disable BUFFER_FLAG_LAST_SAMPLE to prevent the sample queue from ignoring // FLAG_REQUIRE_FORMAT. See b/191518632. flags &= ~C.BUFFER_FLAG_LAST_SAMPLE; - if (compatibilityTrackIndex != C.INDEX_UNSET) { - trackIndicesPerSampleInQueuedOrder.addLast(compatibilityTrackIndex); - } Assertions.checkState(mainTrackIndex != C.INDEX_UNSET); - trackIndicesPerSampleInQueuedOrder.addLast(mainTrackIndex); + int writeIndexBeforeCommitting = this.getWriteIndex(); super.sampleMetadata(timeUs, flags, size, offset, cryptoData); + // Add the track index if the sample was committed + if (this.getWriteIndex() == writeIndexBeforeCommitting + 1) { + if (compatibilityTrackIndex != C.INDEX_UNSET) { + trackIndicesPerSampleInQueuedOrder.addLast(compatibilityTrackIndex); + } + trackIndicesPerSampleInQueuedOrder.addLast(mainTrackIndex); + } } @Override diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/MediaExtractorCompatTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/MediaExtractorCompatTest.java index 7fd55d1890..f810b1d26c 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/MediaExtractorCompatTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/MediaExtractorCompatTest.java @@ -576,6 +576,46 @@ public class MediaExtractorCompatTest { assertThat(mediaExtractorCompat.getTrackCount()).isEqualTo(2); } + @Test + public void readNonSyncSample_whenSyncSampleIsExpected_noSampleIsQueued() throws IOException { + TrackOutput[] outputs = new TrackOutput[1]; + byte[] sampleData = new byte[] {(byte) 1, (byte) 2, (byte) 3}; + fakeExtractor.addReadAction( + (input, seekPosition) -> { + outputs[0] = extractorOutput.track(/* id= */ 0, C.TRACK_TYPE_VIDEO); + extractorOutput.endTracks(); + outputs[0].format( + new Format.Builder() + .setSampleMimeType(MimeTypes.VIDEO_H264) + .setCodecs("avc.123") + .build()); + return Extractor.RESULT_CONTINUE; + }); + // Add a non-sync sample. This sample should be ignored as a sync sample is expected + // at the start of the video. + fakeExtractor.addReadAction( + (input, seekPosition) -> { + outputSampleData(outputs[0], sampleData); + outputs[0].sampleMetadata( + /* timeUs= */ 7, + /* flags= */ 0, + /* size= */ 3, + /* offset= */ 0, + /* cryptoData= */ null); + return Extractor.RESULT_CONTINUE; + }); + mediaExtractorCompat.setDataSource(PLACEHOLDER_URI, /* offset= */ 0); + mediaExtractorCompat.selectTrack(0); + + // Assert that when a keyframe is expected, no sample is queued if a non-keyframe sample is + // read. + assertThat(mediaExtractorCompat.getSampleTrackIndex()).isEqualTo(-1); + assertThat(mediaExtractorCompat.getSampleTime()).isEqualTo(-1); + assertThat(mediaExtractorCompat.getSampleSize()).isEqualTo(-1); + assertThat(mediaExtractorCompat.readSampleData(ByteBuffer.allocate(0), /* offset= */ 0)) + .isEqualTo(-1); + } + // Internal methods. private void assertReadSample(int trackIndex, long timeUs, int size, byte... sampleData) {