Gracefully handle unexpected non-MP3 trailing data

The `bear-cbr-no-seek-table-trailing-garbage.mp3` test file was generated by appending 150kB of `0xDEADBEEF` onto the end of `bear-cbr-variable-frame-size-no-seek-table.mp3`. Issue: #1563 #cherrypick PiperOrigin-RevId: 670131828
androidx · Sep 2, 2024 · 551cbad · 551cbad
1 parent 854566d
commit 551cbad
Show file tree

Hide file tree

Showing 10 changed files with 1,442 additions and 4 deletions.
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -51,6 +51,12 @@
     *   Allow `Mp4Extractor` and `FragmentedMp4Extractor` to identify H264
         samples that are not used as reference by subsequent samples.
     *   Add option to enable index-based seeking in `AmrExtractor`.
+    *   Treat MP3 files with more than 128kB between valid frames as truncated
+        (instead of invalid). This means files with non-MP3 data at the end,
+        with no other metadata to indicate the length of the MP3 bytes, now stop
+        playback at the end of the MP3 data instead of failing with
+        `ParserException: Searched too many bytes.{contentIsMalformed=true,
+        dataType=1}` ([#1563](https://github.com/androidx/media/issues/1563)).
 *   DataSource:
     *   Update `HttpEngineDataSource` to allow use starting at version S
         extension 7 instead of API level 34

diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/mp3/ConstantBitrateSeeker.java b/libraries/extractor/src/main/java/androidx/media3/extractor/mp3/ConstantBitrateSeeker.java
@@ -24,7 +24,10 @@
  */
 /* package */ final class ConstantBitrateSeeker extends ConstantBitrateSeekMap implements Seeker {
 
+  private final long firstFramePosition;
   private final int bitrate;
+  private final int frameSize;
+  private final boolean allowSeeksIfLengthUnknown;
   private final long dataEndPosition;
 
   /**
@@ -60,7 +63,10 @@ public ConstantBitrateSeeker(
       int frameSize,
       boolean allowSeeksIfLengthUnknown) {
     super(inputLength, firstFramePosition, bitrate, frameSize, allowSeeksIfLengthUnknown);
+    this.firstFramePosition = firstFramePosition;
     this.bitrate = bitrate;
+    this.frameSize = frameSize;
+    this.allowSeeksIfLengthUnknown = allowSeeksIfLengthUnknown;
     dataEndPosition = inputLength != C.LENGTH_UNSET ? inputLength : C.INDEX_UNSET;
   }
 
@@ -78,4 +84,13 @@ public long getDataEndPosition() {
   public int getAverageBitrate() {
     return bitrate;
   }
+
+  public ConstantBitrateSeeker copyWithNewDataEndPosition(long dataEndPosition) {
+    return new ConstantBitrateSeeker(
+        /* inputLength= */ dataEndPosition,
+        firstFramePosition,
+        bitrate,
+        frameSize,
+        allowSeeksIfLengthUnknown);
+  }
 }
diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/mp3/Mp3Extractor.java b/libraries/extractor/src/main/java/androidx/media3/extractor/mp3/Mp3Extractor.java
@@ -15,6 +15,7 @@
  */
 package androidx.media3.extractor.mp3;
 
+import static androidx.media3.common.util.Assertions.checkNotNull;
 import static java.lang.annotation.ElementType.TYPE_USE;
 import static java.lang.annotation.RetentionPolicy.SOURCE;
 
@@ -23,7 +24,6 @@
 import androidx.media3.common.C;
 import androidx.media3.common.Format;
 import androidx.media3.common.Metadata;
-import androidx.media3.common.ParserException;
 import androidx.media3.common.PlaybackException;
 import androidx.media3.common.Player;
 import androidx.media3.common.util.Assertions;
@@ -177,6 +177,7 @@ public final class Mp3Extractor implements Extractor {
   private long basisTimeUs;
   private long samplesRead;
   private long firstSamplePosition;
+  private long endPositionOfLastSampleRead;
   private int sampleBytesRemaining;
 
   private @MonotonicNonNull Seeker seeker;
@@ -213,6 +214,7 @@ public Mp3Extractor(@Flags int flags, long forcedFirstSampleTimestampUs) {
     id3Peeker = new Id3Peeker();
     skippingTrackOutput = new DiscardingTrackOutput();
     currentTrackOutput = skippingTrackOutput;
+    endPositionOfLastSampleRead = C.INDEX_UNSET;
   }
 
   // Extractor implementation.
@@ -335,13 +337,14 @@ private int readSample(ExtractorInput extractorInput) throws IOException {
         }
       }
       sampleBytesRemaining = synchronizedHeader.frameSize;
+      endPositionOfLastSampleRead = extractorInput.getPosition() + synchronizedHeader.frameSize;
       if (seeker instanceof IndexSeeker) {
         IndexSeeker indexSeeker = (IndexSeeker) seeker;
         // Add seek point corresponding to the next frame instead of the current one to be able to
         // start writing to the realTrackOutput on time when a seek is in progress.
         indexSeeker.maybeAddSeekPoint(
             computeTimeUs(samplesRead + synchronizedHeader.samplesPerFrame),
-            extractorInput.getPosition() + synchronizedHeader.frameSize);
+            endPositionOfLastSampleRead);
         if (isSeekInProgress && indexSeeker.isTimeUsInIndex(seekTimeUs)) {
           isSeekInProgress = false;
           currentTrackOutput = realTrackOutput;
@@ -395,6 +398,7 @@ private boolean synchronize(ExtractorInput input, boolean sniffing) throws IOExc
           // We reached the end of the stream but found at least one valid frame.
           break;
         }
+        maybeUpdateCbrDurationToLastSample();
         throw new EOFException();
       }
       scratch.setPosition(0);
@@ -406,8 +410,8 @@ private boolean synchronize(ExtractorInput input, boolean sniffing) throws IOExc
         // The header doesn't match the candidate header or is invalid. Try the next byte offset.
         if (searchedBytes++ == searchLimitBytes) {
           if (!sniffing) {
-            throw ParserException.createForMalformedContainer(
-                "Searched too many bytes.", /* cause= */ null);
+            maybeUpdateCbrDurationToLastSample();
+            throw new EOFException();
           }
           return false;
         }
@@ -636,6 +640,22 @@ private Seeker getConstantBitrateSeeker(
         /* allowSeeksIfLengthUnknown= */ false);
   }
 
+  /**
+   * If {@link #seeker} is a seekable {@link ConstantBitrateSeeker}, this updates it to end at the
+   * last sample we read (because we've failed to find a subsequent synchronization word so we
+   * assume the MP3 data has ended).
+   */
+  private void maybeUpdateCbrDurationToLastSample() {
+    if (seeker instanceof ConstantBitrateSeeker
+        && seeker.isSeekable()
+        && endPositionOfLastSampleRead != C.INDEX_UNSET
+        && endPositionOfLastSampleRead != seeker.getDataEndPosition()) {
+      seeker =
+          ((ConstantBitrateSeeker) seeker).copyWithNewDataEndPosition(endPositionOfLastSampleRead);
+      checkNotNull(extractorOutput).seekMap(seeker);
+    }
+  }
+
   @EnsuresNonNull({"extractorOutput", "realTrackOutput"})
   private void assertInitialized() {
     Assertions.checkStateNotNull(realTrackOutput);

diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/mp3/Mp3ExtractorTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/mp3/Mp3ExtractorTest.java
@@ -15,6 +15,8 @@
  */
 package androidx.media3.extractor.mp3;
 
+import static org.junit.Assume.assumeFalse;
+
 import androidx.media3.test.utils.ExtractorAsserts;
 import androidx.media3.test.utils.ExtractorAsserts.AssertionConfig;
 import com.google.common.collect.ImmutableList;
@@ -96,6 +98,18 @@ public void mp3SampleWithIndexSeeker() throws Exception {
         simulationConfig);
   }
 
+  // https://github.com/androidx/media/issues/1563
+  @Test
+  public void mp3CbrSampleWithNoSeekTableAndTrailingGarbage() throws Exception {
+    assumeFalse(
+        "Skipping I/O error testing with unknown length due to b/362727473",
+        simulationConfig.simulateIOErrors && simulationConfig.simulateUnknownLength);
+    ExtractorAsserts.assertBehavior(
+        Mp3Extractor::new,
+        "media/mp3/bear-cbr-no-seek-table-trailing-garbage.mp3",
+        simulationConfig);
+  }
+
   @Test
   public void trimmedMp3Sample() throws Exception {
     ExtractorAsserts.assertBehavior(