Ignore invalid MP3 packets from ffmpeg.

Per upstream, this is totally normal to happen and they won't accept
changes to fix this without undertaking a large refactoring of the
AVParser infrastructure:

http://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225800.html

This behavior may also occur with ADTS streams, but is rarer in practice
because ffmpeg's ADTS demuxer does more validation on the packets, so
when invalid data is received, av_read_frame() fails and playback ends.
Since we haven't had a rash of complaints about too-short ADTS streams,
don't bother doing this for ADTS streams at this time.

BUG=822341,794782
TEST=existing tests all still pass.

Cq-Include-Trybots: luci.chromium.try:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Iabcad145bb75f742e4ba7658ab95c377b811efa1
Reviewed-on: https://chromium-review.googlesource.com/965121
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544181}

media/filters/audio_decoder_unittest.cc

@@ -452,13 +452,6 @@ const DecodedBufferExpectations kSfxMp3Expectations[] = {
     {27188, 26122, "4.24,3.95,4.22,4.78,5.13,4.93,"},
 };
 
-// File has trailing garbage, but should still decode without error.
-const DecodedBufferExpectations kTrailingGarbageMp3Expectations[] = {
-    {0, 26122, "-0.57,-0.77,-0.39,0.27,0.74,0.65,"},
-    {26122, 26122, "-0.57,-0.77,-0.39,0.27,0.74,0.65,"},
-    {52244, 26122, "-0.57,-0.77,-0.39,0.27,0.74,0.65,"},
-};
-
 #if BUILDFLAG(USE_PROPRIETARY_CODECS)
 const DecodedBufferExpectations kSfxAdtsExpectations[] = {
     {0, 23219, "-1.90,-1.53,-0.15,1.28,1.23,-0.33,"},
@@ -513,8 +506,6 @@ const DecodedBufferExpectations kSfxOpusExpectations[] = {
 
 const TestParams kFFmpegTestParams[] = {
     {kCodecMP3, "sfx.mp3", kSfxMp3Expectations, 0, 44100, CHANNEL_LAYOUT_MONO},
-    {kCodecMP3, "trailing-garbage.mp3", kTrailingGarbageMp3Expectations, 0,
-     44100, CHANNEL_LAYOUT_MONO},
 #if BUILDFLAG(USE_PROPRIETARY_CODECS)
     {kCodecAAC, "sfx.adts", kSfxAdtsExpectations, 0, 44100,
      CHANNEL_LAYOUT_MONO},

media/filters/ffmpeg_demuxer.cc

@@ -41,6 +41,7 @@
 #include "media/filters/ffmpeg_bitstream_converter.h"
 #include "media/filters/ffmpeg_glue.h"
 #include "media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.h"
+#include "media/formats/mpeg/mpeg1_audio_stream_parser.h"
 #include "media/formats/webm/webm_crypto_helpers.h"
 #include "media/media_features.h"
 #include "third_party/ffmpeg/ffmpeg_features.h"
@@ -306,7 +307,8 @@ FFmpegDemuxerStream::FFmpegDemuxerStream(
       waiting_for_keyframe_(false),
       aborted_(false),
       fixup_negative_timestamps_(false),
-      fixup_chained_ogg_(false) {
+      fixup_chained_ogg_(false),
+      num_discarded_packet_warnings_(0) {
   DCHECK(demuxer_);
 
   bool is_encrypted = false;
@@ -387,10 +389,9 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
   }
 #endif
 
-  const bool is_audio = type() == AUDIO;
-
   scoped_refptr<DecoderBuffer> buffer;
 
+  const bool is_audio = type() == AUDIO;
   if (type() == DemuxerStream::TEXT) {
     int id_size = 0;
     uint8_t* id_data = av_packet_get_side_data(
@@ -424,6 +425,39 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
       }
     }
 
+    // FFmpeg may return garbage packets for MP3 stream containers, so we need
+    // to drop these to avoid decoder errors. The ffmpeg team maintains that
+    // this behavior isn't ideal, but have asked for a significant refactoring
+    // of the AVParser infrastructure to fix this, which is overkill for now.
+    // See http://crbug.com/794782.
+    //
+    // This behavior may also occur with ADTS streams, but is rarer in practice
+    // because ffmpeg's ADTS demuxer does more validation on the packets, so
+    // when invalid data is received, av_read_frame() fails and playback ends.
+    if (is_audio && demuxer_->container() == container_names::CONTAINER_MP3) {
+      DCHECK(!data_offset);  // Only set for containers supporting encryption...
+
+      // MP3 packets may be zero-padded according to ffmpeg, so trim until we
+      // have the packet; adjust |data_offset| too so this work isn't repeated.
+      uint8_t* packet_end = packet->data + packet->size;
+      uint8_t* header_start = packet->data;
+      while (header_start < packet_end && !*header_start) {
+        ++header_start;
+        ++data_offset;
+      }
+
+      if (packet_end - header_start < MPEG1AudioStreamParser::kHeaderSize ||
+          !MPEG1AudioStreamParser::ParseHeader(nullptr, header_start,
+                                               nullptr)) {
+        LIMITED_MEDIA_LOG(INFO, nullptr, num_discarded_packet_warnings_, 5)
+            << "Discarding invalid MP3 packet, ts: "
+            << ConvertStreamTimestamp(stream_->time_base, packet->pts)
+            << ", duration: "
+            << ConvertStreamTimestamp(stream_->time_base, packet->duration);
+        return;
+      }
+    }
+
     // If a packet is returned by FFmpeg's av_parser_parse2() the packet will
     // reference inner memory of FFmpeg.  As such we should transfer the packet
     // into memory we control.

media/filters/ffmpeg_demuxer.h

@@ -200,6 +200,8 @@ class MEDIA_EXPORT FFmpegDemuxerStream : public DemuxerStream {
   bool fixup_negative_timestamps_;
   bool fixup_chained_ogg_;
 
+  int num_discarded_packet_warnings_;
+
   DISALLOW_COPY_AND_ASSIGN(FFmpegDemuxerStream);
 };
 
@@ -258,6 +260,10 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
     return blocking_task_runner_;
   }
 
+  container_names::MediaContainerName container() const {
+    return glue_->container();
+  }
+
  private:
   // To allow tests access to privates.
   friend class FFmpegDemuxerTest;

media/formats/mpeg/mpeg1_audio_stream_parser.cc

@@ -124,33 +124,37 @@ bool MPEG1AudioStreamParser::ParseHeader(MediaLog* media_log,
            << " sample_rate_index 0x" << sample_rate_index
            << " channel_mode 0x" << channel_mode;
 
-  if (sync != 0x7ff ||
-      version == kVersionReserved ||
-      layer == kLayerReserved ||
+  if (sync != 0x7ff || version == kVersionReserved || layer == kLayerReserved ||
       bitrate_index == kBitrateFree || bitrate_index == kBitrateBad ||
       sample_rate_index == kSampleRateReserved) {
-    MEDIA_LOG(ERROR, media_log)
-        << "Invalid header data :" << std::hex << " sync 0x" << sync
-        << " version 0x" << version << " layer 0x" << layer
-        << " bitrate_index 0x" << bitrate_index << " sample_rate_index 0x"
-        << sample_rate_index << " channel_mode 0x" << channel_mode;
+    if (media_log) {
+      MEDIA_LOG(ERROR, media_log)
+          << "Invalid MP3 header data :" << std::hex << " sync 0x" << sync
+          << " version 0x" << version << " layer 0x" << layer
+          << " bitrate_index 0x" << bitrate_index << " sample_rate_index 0x"
+          << sample_rate_index << " channel_mode 0x" << channel_mode;
+    }
     return false;
   }
 
   if (layer == kLayer2 && !kIsAllowed[bitrate_index][channel_mode]) {
-    MEDIA_LOG(ERROR, media_log) << "Invalid (bitrate_index, channel_mode)"
-                                << " combination :" << std::hex
-                                << " bitrate_index " << bitrate_index
-                                << " channel_mode " << channel_mode;
+    if (media_log) {
+      MEDIA_LOG(ERROR, media_log)
+          << "Invalid MP3 (bitrate_index, channel_mode)"
+          << " combination :" << std::hex << " bitrate_index " << bitrate_index
+          << " channel_mode " << channel_mode;
+    }
     return false;
   }
 
   int bitrate = kBitrateMap[bitrate_index][kVersionLayerMap[version][layer]];
 
   if (bitrate == 0) {
-    MEDIA_LOG(ERROR, media_log) << "Invalid bitrate :" << std::hex
-                                << " version " << version << " layer " << layer
-                                << " bitrate_index " << bitrate_index;
+    if (media_log) {
+      MEDIA_LOG(ERROR, media_log)
+          << "Invalid MP3 bitrate :" << std::hex << " version " << version
+          << " layer " << layer << " bitrate_index " << bitrate_index;
+    }
     return false;
   }
 
@@ -158,12 +162,13 @@ bool MPEG1AudioStreamParser::ParseHeader(MediaLog* media_log,
 
   int frame_sample_rate = kSampleRateMap[sample_rate_index][version];
   if (frame_sample_rate == 0) {
-    MEDIA_LOG(ERROR, media_log) << "Invalid sample rate :" << std::hex
-                                << " version " << version
-                                << " sample_rate_index " << sample_rate_index;
+    if (media_log) {
+      MEDIA_LOG(ERROR, media_log)
+          << "Invalid MP3 sample rate :" << std::hex << " version " << version
+          << " sample_rate_index " << sample_rate_index;
+    }
     return false;
   }
-  header->sample_rate = frame_sample_rate;
 
   // http://teslabs.com/openplayer/docs/docs/specs/mp3_structure2.pdf
   // Table 2.1.5
@@ -187,6 +192,11 @@ bool MPEG1AudioStreamParser::ParseHeader(MediaLog* media_log,
     default:
       return false;
   }
+
+  if (!header)
+    return true;
+
+  header->sample_rate = frame_sample_rate;
   header->sample_count = samples_per_frame;
 
   // http://teslabs.com/openplayer/docs/docs/specs/mp3_structure2.pdf

media/test/pipeline_integration_test.cc

@@ -2654,6 +2654,12 @@ TEST_F(PipelineIntegrationTest, BasicPlaybackChainedOgg) {
   ASSERT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
 }
 
+TEST_F(PipelineIntegrationTest, TrailingGarbage) {
+  ASSERT_EQ(PIPELINE_OK, Start("trailing-garbage.mp3"));
+  Play();
+  ASSERT_TRUE(WaitUntilOnEnded());
+}
+
 // Ensures audio-video playback with missing or negative timestamps fails
 // instead of crashing.  See http://crbug.com/396864.
 TEST_F(PipelineIntegrationTest, BasicPlaybackChainedOggVideo) {