Stop conflating negative ts and chained ogg workarounds.

Now that opus is supported in containers outside of ogg, we shouldn't
allow negative timestamp fixups to imply the need for chained ogg
support. I.e. we shouldn't apply chained ogg workarounds to mp4
files or any other container that opus is allowed in now.

This has been broken for a while, but h264+bframes and opus is not
common, so the issue hasn't arisen before. The fix is simply to
separate these workaround flags and restrict the chained ogg one
to only content which we expect to support.

BUG=795392
TEST=new unittest.

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I3a7465110fd1f1defe4b41f1f471e5050d8c469a
Reviewed-on: https://chromium-review.googlesource.com/830831
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524566}

media/filters/ffmpeg_demuxer.cc

@@ -306,7 +306,8 @@ FFmpegDemuxerStream::FFmpegDemuxerStream(
       is_enabled_(true),
       waiting_for_keyframe_(false),
       aborted_(false),
-      fixup_negative_timestamps_(false) {
+      fixup_negative_timestamps_(false),
+      fixup_chained_ogg_(false) {
   DCHECK(demuxer_);
 
   bool is_encrypted = false;
@@ -515,7 +516,7 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
 
   // Only allow negative timestamps past if we know they'll be fixed up by the
   // code paths below; otherwise they should be treated as a parse error.
-  if ((!fixup_negative_timestamps_ || last_packet_timestamp_ == kNoTimestamp) &&
+  if ((!fixup_chained_ogg_ || last_packet_timestamp_ == kNoTimestamp) &&
       buffer->timestamp() < base::TimeDelta()) {
     MEDIA_LOG(DEBUG, media_log_)
         << "FFmpegDemuxer: unfixable negative timestamp";
@@ -558,8 +559,10 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
     // Fixing chained ogg is non-trivial, so for now just reuse the last good
     // timestamp.  The decoder will rewrite the timestamps to be sample accurate
     // later.  See http://crbug.com/396864.
-    if (fixup_negative_timestamps_ &&
-        buffer->timestamp() < last_packet_timestamp_) {
+    //
+    // Note: This will not work with codecs that have out of order frames like
+    // H.264 with b-frames, but luckily you can't put those in ogg files...
+    if (fixup_chained_ogg_ && buffer->timestamp() < last_packet_timestamp_) {
       buffer->set_timestamp(last_packet_timestamp_ +
                             (last_packet_duration_ != kNoTimestamp
                                  ? last_packet_duration_
@@ -1278,6 +1281,8 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
   int detected_text_track_count = 0;
   int supported_audio_track_count = 0;
   int supported_video_track_count = 0;
+  bool has_opus_or_vorbis_audio = false;
+  bool needs_negative_timestamp_fixup = false;
   for (size_t i = 0; i < format_context->nb_streams; ++i) {
     AVStream* stream = format_context->streams[i];
     const AVCodecParameters* codec_parameters = stream->codecpar;
@@ -1411,12 +1416,29 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
 
     max_duration = std::max(max_duration, streams_[i]->duration());
 
-    const base::TimeDelta start_time =
+    base::TimeDelta start_time =
         ExtractStartTime(stream, start_time_estimates[i]);
-    streams_[i]->set_start_time(start_time);
-    if (start_time < start_time_) {
+
+    // Note: This value is used for seeking, so we must take the true value and
+    // not the one possibly clamped to zero below.
+    if (start_time < start_time_)
       start_time_ = start_time;
+
+    const bool is_opus_or_vorbis =
+        codec_id == AV_CODEC_ID_OPUS || codec_id == AV_CODEC_ID_VORBIS;
+    if (!has_opus_or_vorbis_audio)
+      has_opus_or_vorbis_audio = is_opus_or_vorbis;
+
+    if (codec_type == AVMEDIA_TYPE_AUDIO && start_time < base::TimeDelta() &&
+        is_opus_or_vorbis) {
+      needs_negative_timestamp_fixup = true;
+
+      // Fixup the seeking information to avoid selecting the audio stream
+      // simply because it has a lower starting time.
+      start_time = base::TimeDelta();
     }
+
+    streams_[i]->set_start_time(start_time);
   }
 
   RecordDetectedTrackTypeStats(detected_audio_track_count,
@@ -1445,10 +1467,16 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
     max_duration = kInfiniteDuration;
   }
 
+  // Chained ogg is only allowed on single track audio only opus/vorbis media.
+  const bool needs_chained_ogg_fixup =
+      glue_->container() == container_names::CONTAINER_OGG &&
+      supported_audio_track_count == 1 && !supported_video_track_count &&
+      has_opus_or_vorbis_audio;
+
   // FFmpeg represents audio data marked as before the beginning of stream as
   // having negative timestamps.  This data must be discarded after it has been
   // decoded, not before since it is used to warmup the decoder.  There are
-  // currently two known cases for this: vorbis in ogg and opus in ogg and webm.
+  // currently two known cases for this: vorbis in ogg and opus.
   //
   // For API clarity, it was decided that the rest of the media pipeline should
   // not be exposed to negative timestamps.  Which means we need to rebase these
@@ -1461,33 +1489,20 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
   // FFmpeg's use of negative timestamps for opus pre-skip is nonstandard, but
   // for more information on pre-skip see section 4.2 of the Ogg Opus spec:
   // https://tools.ietf.org/html/draft-ietf-codec-oggopus-08#section-4.2
-  for (const auto& stream : streams_) {
-    if (!stream || stream->type() != DemuxerStream::AUDIO)
-      continue;
-    const AVStream* audio_stream = stream->av_stream();
-    DCHECK(audio_stream);
-    if (audio_stream->codecpar->codec_id == AV_CODEC_ID_OPUS ||
-        (glue_->container() == container_names::CONTAINER_OGG &&
-         audio_stream->codecpar->codec_id == AV_CODEC_ID_VORBIS)) {
-      for (size_t i = 0; i < streams_.size(); ++i) {
-        if (!streams_[i])
-          continue;
-        streams_[i]->enable_negative_timestamp_fixups();
-
-        // Fixup the seeking information to avoid selecting the audio stream
-        // simply because it has a lower starting time.
-        if (streams_[i]->av_stream() == audio_stream &&
-            streams_[i]->start_time() < base::TimeDelta()) {
-          streams_[i]->set_start_time(base::TimeDelta());
-        }
-      }
+  if (needs_negative_timestamp_fixup || needs_chained_ogg_fixup) {
+    for (auto& stream : streams_) {
+      if (!stream)
+        continue;
+      if (needs_negative_timestamp_fixup)
+        stream->enable_negative_timestamp_fixups();
+      if (needs_chained_ogg_fixup)
+        stream->enable_chained_ogg_fixups();
     }
   }
 
   // If no start time could be determined, default to zero.
-  if (start_time_ == kInfiniteDuration) {
+  if (start_time_ == kInfiniteDuration)
     start_time_ = base::TimeDelta();
-  }
 
   // MPEG-4 B-frames cause grief for a simple container like AVI. Enable PTS
   // generation so we always get timestamps, see http://crbug.com/169570

media/filters/ffmpeg_demuxer.h

@@ -105,6 +105,7 @@ class MEDIA_EXPORT FFmpegDemuxerStream : public DemuxerStream {
   void enable_negative_timestamp_fixups() {
     fixup_negative_timestamps_ = true;
   }
+  void enable_chained_ogg_fixups() { fixup_chained_ogg_ = true; }
 
   // DemuxerStream implementation.
   Type type() const override;
@@ -197,6 +198,7 @@ class MEDIA_EXPORT FFmpegDemuxerStream : public DemuxerStream {
 
   std::string encryption_key_id_;
   bool fixup_negative_timestamps_;
+  bool fixup_chained_ogg_;
 
   DISALLOW_COPY_AND_ASSIGN(FFmpegDemuxerStream);
 };

media/filters/ffmpeg_demuxer_unittest.cc

@@ -800,7 +800,7 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOggDiscard_Sync) {
               demuxer_->start_time());
 
     // Though the internal start time may be below zero, the exposed media time
-    // must always be greater than zero.
+    // must always be >= zero.
     EXPECT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
 
     video->Read(NewReadCB(FROM_HERE, 9997, 0, true));
@@ -847,7 +847,7 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOpusDiscard_Sync) {
     }
 
     // Though the internal start time may be below zero, the exposed media time
-    // must always be greater than zero.
+    // must always be >= zero.
     EXPECT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
 
     video->Read(NewReadCB(FROM_HERE, 16009, 0, true));
@@ -866,6 +866,59 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOpusDiscard_Sync) {
   }
 }
 
+#if BUILDFLAG(USE_PROPRIETARY_CODECS)
+// Similar to the test above, but using an opus clip plus h264 b-frames to
+// ensure we don't apply chained ogg workarounds to other content.
+TEST_F(FFmpegDemuxerTest,
+       Read_AudioNegativeStartTimeAndOpusDiscardH264Mp4_Sync) {
+  CreateDemuxer("tos-h264-opus.mp4");
+  InitializeDemuxer();
+
+  // Attempt a read from the video stream and run the message loop until done.
+  DemuxerStream* video = GetStream(DemuxerStream::VIDEO);
+  DemuxerStream* audio = GetStream(DemuxerStream::AUDIO);
+  EXPECT_EQ(audio->audio_decoder_config().codec_delay(), 312);
+
+  // Packet size to timestamp (in microseconds) mapping for the first N packets
+  // which should be fully discarded.
+  static const int kTestExpectations[][2] = {
+      {234, 20000}, {228, 40000}, {340, 60000}};
+
+  // Run the test twice with a seek in between.
+  for (int i = 0; i < 2; ++i) {
+    audio->Read(NewReadCBWithCheckedDiscard(
+        FROM_HERE, 408, 0, base::TimeDelta::FromMicroseconds(6500), true));
+    base::RunLoop().Run();
+
+    for (size_t j = 0; j < arraysize(kTestExpectations); ++j) {
+      audio->Read(NewReadCB(FROM_HERE, kTestExpectations[j][0],
+                            kTestExpectations[j][1], true));
+      base::RunLoop().Run();
+    }
+
+    // Though the internal start time may be below zero, the exposed media time
+    // must always be >= zero.
+    EXPECT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
+
+    video->Read(NewReadCB(FROM_HERE, 185105, 0, true));
+    base::RunLoop().Run();
+
+    video->Read(NewReadCB(FROM_HERE, 35941, 125000, false));
+    base::RunLoop().Run();
+
+    // If things aren't working correctly, this expectation will fail because
+    // the chained ogg workaround breaks out of order timestamps.
+    video->Read(NewReadCB(FROM_HERE, 8129, 84000, false));
+    base::RunLoop().Run();
+
+    // Seek back to the beginning and repeat the test.
+    WaitableMessageLoopEvent event;
+    demuxer_->Seek(base::TimeDelta(), event.GetPipelineStatusCB());
+    event.RunAndWaitForStatus(PIPELINE_OK);
+  }
+}
+#endif
+
 // Similar to the test above, but using sfx-opus.ogg, which has a much smaller
 // amount of discard padding and no |start_time| set on the AVStream.
 TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOpusSfxDiscard_Sync) {
@@ -885,7 +938,7 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOpusSfxDiscard_Sync) {
     base::RunLoop().Run();
 
     // Though the internal start time may be below zero, the exposed media time
-    // must always be greater than zero.
+    // must always be >= zero.
     EXPECT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
 
     // Seek back to the beginning and repeat the test.

media/test/pipeline_integration_test.cc

@@ -2632,13 +2632,11 @@ TEST_F(PipelineIntegrationTest, BasicPlaybackChainedOgg) {
   ASSERT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
 }
 
-// Ensures audio-video playback with missing or negative timestamps fails softly
+// Ensures audio-video playback with missing or negative timestamps fails
 // instead of crashing.  See http://crbug.com/396864.
 TEST_F(PipelineIntegrationTest, BasicPlaybackChainedOggVideo) {
-  ASSERT_EQ(PIPELINE_OK, Start("double-bear.ogv", kUnreliableDuration));
-  Play();
-  EXPECT_EQ(PIPELINE_ERROR_DECODE, WaitUntilEndedOrError());
-  ASSERT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
+  ASSERT_EQ(DEMUXER_ERROR_COULD_NOT_PARSE,
+            Start("double-bear.ogv", kUnreliableDuration));
 }
 
 // Tests that we signal ended even when audio runs longer than video track.