path: root/remove-dependency-on-ffmpeg-internals-for-start-time.patch

From f9535bd6d61d7e0b2cb452e6976a914d4ee62a2b Mon Sep 17 00:00:00 2001
From: Dale Curtis <dalecurtis@chromium.org>
Date: Thu, 17 May 2018 23:44:41 +0000
Subject: [PATCH] Remove dependency on ffmpeg internals for start time
 calculations.

Some theora clips still have issues, but using first_dts where
we know it's the same as the first pts resolves the lingering
issues. It also looks like we don't need to use pts_buffer now.

This does the following small fixes:
- MEDIA_LOG(DEBUG) -> MEDIA_LOG(ERROR) for negative ts error.
- Enables previous disabled test in more limited state.
- Adds kNoFFmpegTimestamp so static_cast<int64_t>(AV_NOPTS_VALUE)
is no longer necessary everywhere. A followup patch set will use
this is more places.
- Removes pts_buffer and packet_buffer inspection.

BUG=731766
TEST=all tests pass.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I5aadf67a3b5ea2d2a8dd19bbddd7b107208094c5
Reviewed-on: https://chromium-review.googlesource.com/1064538
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559737}
---
 media/ffmpeg/ffmpeg_common.h             |  5 +-
 media/filters/ffmpeg_demuxer.cc          | 88 +++++++-----------------
 media/filters/ffmpeg_demuxer_unittest.cc | 24 +++----
 3 files changed, 37 insertions(+), 80 deletions(-)

diff --git a/media/ffmpeg/ffmpeg_common.h b/media/ffmpeg/ffmpeg_common.h
index ec33068fb84f..f641d6bcf92e 100644
--- a/media/ffmpeg/ffmpeg_common.h
+++ b/media/ffmpeg/ffmpeg_common.h
@@ -28,9 +28,6 @@ extern "C" {
 MSVC_PUSH_DISABLE_WARNING(4244);
 #include <libavcodec/avcodec.h>
 #include <libavformat/avformat.h>
-#if !BUILDFLAG(USE_SYSTEM_FFMPEG)
-#include <libavformat/internal.h>
-#endif  // !BUILDFLAG(USE_SYSTEM_FFMPEG)
 #include <libavformat/avio.h>
 #include <libavutil/avutil.h>
 #include <libavutil/imgutils.h>
@@ -42,6 +39,8 @@ MSVC_POP_WARNING();
 
 namespace media {
 
+constexpr int64_t kNoFFmpegTimestamp = static_cast<int64_t>(AV_NOPTS_VALUE);
+
 class AudioDecoderConfig;
 class EncryptionScheme;
 class VideoDecoderConfig;
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc
index 49ca01c4dc0b..7402ce16ab5e 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -85,29 +85,26 @@ static base::TimeDelta FramesToTimeDelta(int frames, double sample_rate) {
       frames * base::Time::kMicrosecondsPerSecond / sample_rate);
 }
 
-static base::TimeDelta ExtractStartTime(AVStream* stream,
-                                        base::TimeDelta start_time_estimate) {
-  DCHECK(start_time_estimate != kNoTimestamp);
-  if (stream->start_time == static_cast<int64_t>(AV_NOPTS_VALUE)) {
-    return start_time_estimate == kInfiniteDuration ? base::TimeDelta()
-                                                    : start_time_estimate;
+static base::TimeDelta ExtractStartTime(AVStream* stream) {
+  // The default start time is zero.
+  base::TimeDelta start_time;
+
+  // First try to use  the |start_time| value as is.
+  if (stream->start_time != kNoFFmpegTimestamp)
+    start_time = ConvertFromTimeBase(stream->time_base, stream->start_time);
+
+  // Next try to use the first DTS value, for codecs where we know PTS == DTS
+  // (excludes all H26x codecs). The start time must be returned in PTS.
+  if (stream->first_dts != kNoFFmpegTimestamp &&
+      stream->codecpar->codec_id != AV_CODEC_ID_HEVC &&
+      stream->codecpar->codec_id != AV_CODEC_ID_H264 &&
+      stream->codecpar->codec_id != AV_CODEC_ID_MPEG4) {
+    const base::TimeDelta first_pts =
+        ConvertFromTimeBase(stream->time_base, stream->first_dts);
+    if (first_pts < start_time)
+      start_time = first_pts;
   }
 
-  // First try the lower of the estimate and the |start_time| value.
-  base::TimeDelta start_time =
-      std::min(ConvertFromTimeBase(stream->time_base, stream->start_time),
-               start_time_estimate);
-
-  // Next see if the first buffered pts value is usable.
-  if (stream->pts_buffer[0] != static_cast<int64_t>(AV_NOPTS_VALUE)) {
-    const base::TimeDelta buffered_pts =
-        ConvertFromTimeBase(stream->time_base, stream->pts_buffer[0]);
-    if (buffered_pts < start_time)
-      start_time = buffered_pts;
-  }
-
-  // NOTE: Do not use AVStream->first_dts since |start_time| should be a
-  // presentation timestamp.
   return start_time;
 }
 
@@ -513,7 +510,7 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
     buffer->set_duration(kNoTimestamp);
   }
 
-  // Note: If pts is AV_NOPTS_VALUE, stream_timestamp will be kNoTimestamp.
+  // Note: If pts is kNoFFmpegTimestamp, stream_timestamp will be kNoTimestamp.
   const base::TimeDelta stream_timestamp =
       ConvertStreamTimestamp(stream_->time_base, packet->pts);
 
@@ -556,8 +553,8 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
   // code paths below; otherwise they should be treated as a parse error.
   if ((!fixup_chained_ogg_ || last_packet_timestamp_ == kNoTimestamp) &&
       buffer->timestamp() < base::TimeDelta()) {
-    MEDIA_LOG(DEBUG, media_log_)
-        << "FFmpegDemuxer: unfixable negative timestamp";
+    MEDIA_LOG(ERROR, media_log_)
+        << "FFmpegDemuxer: unfixable negative timestamp.";
     demuxer_->NotifyDemuxerError(DEMUXER_ERROR_COULD_NOT_PARSE);
     return;
   }
@@ -863,7 +860,7 @@ std::string FFmpegDemuxerStream::GetMetadata(const char* key) const {
 base::TimeDelta FFmpegDemuxerStream::ConvertStreamTimestamp(
     const AVRational& time_base,
     int64_t timestamp) {
-  if (timestamp == static_cast<int64_t>(AV_NOPTS_VALUE))
+  if (timestamp == kNoFFmpegTimestamp)
     return kNoTimestamp;
 
   return ConvertFromTimeBase(time_base, timestamp);
@@ -1256,42 +1253,6 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
   AVFormatContext* format_context = glue_->format_context();
   streams_.resize(format_context->nb_streams);
 
-  // Estimate the start time for each stream by looking through the packets
-  // buffered during avformat_find_stream_info().  These values will be
-  // considered later when determining the actual stream start time.
-  //
-  // These packets haven't been completely processed yet, so only look through
-  // these values if the AVFormatContext has a valid start time.
-  //
-  // If no estimate is found, the stream entry will be kInfiniteDuration.
-  std::vector<base::TimeDelta> start_time_estimates(format_context->nb_streams,
-                                                    kInfiniteDuration);
-#if !BUILDFLAG(USE_SYSTEM_FFMPEG)
-  const AVFormatInternal* internal = format_context->internal;
-  if (internal && internal->packet_buffer &&
-      format_context->start_time != static_cast<int64_t>(AV_NOPTS_VALUE)) {
-    struct AVPacketList* packet_buffer = internal->packet_buffer;
-    while (packet_buffer != internal->packet_buffer_end) {
-      DCHECK_LT(static_cast<size_t>(packet_buffer->pkt.stream_index),
-                start_time_estimates.size());
-      const AVStream* stream =
-          format_context->streams[packet_buffer->pkt.stream_index];
-      if (packet_buffer->pkt.pts != static_cast<int64_t>(AV_NOPTS_VALUE)) {
-        const base::TimeDelta packet_pts =
-            ConvertFromTimeBase(stream->time_base, packet_buffer->pkt.pts);
-        // We ignore kNoTimestamp here since -int64_t::min() is possible; see
-        // https://crbug.com/700501. Technically this is a valid value, but in
-        // practice shouldn't occur, so just ignore it when estimating.
-        if (packet_pts != kNoTimestamp && packet_pts != kInfiniteDuration &&
-            packet_pts < start_time_estimates[stream->index]) {
-          start_time_estimates[stream->index] = packet_pts;
-        }
-      }
-      packet_buffer = packet_buffer->next;
-    }
-  }
-#endif  // !BUILDFLAG(USE_SYSTEM_FFMPEG)
-
   std::unique_ptr<MediaTracks> media_tracks(new MediaTracks());
 
   DCHECK(track_id_to_demux_stream_map_.empty());
@@ -1440,8 +1401,7 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
 
     max_duration = std::max(max_duration, streams_[i]->duration());
 
-    base::TimeDelta start_time =
-        ExtractStartTime(stream, start_time_estimates[i]);
+    base::TimeDelta start_time = ExtractStartTime(stream);
 
     // Note: This value is used for seeking, so we must take the true value and
     // not the one possibly clamped to zero below.
@@ -1479,7 +1439,7 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
   if (text_enabled_)
     AddTextStreams();
 
-  if (format_context->duration != static_cast<int64_t>(AV_NOPTS_VALUE)) {
+  if (format_context->duration != kNoFFmpegTimestamp) {
     // If there is a duration value in the container use that to find the
     // maximum between it and the duration from A/V streams.
     const AVRational av_time_base = {1, AV_TIME_BASE};
diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc
index 71dbed07b376..b09816a3ba3a 100644
--- a/media/filters/ffmpeg_demuxer_unittest.cc
+++ b/media/filters/ffmpeg_demuxer_unittest.cc
@@ -696,12 +696,9 @@ TEST_F(FFmpegDemuxerTest, Read_InvalidNegativeTimestamp) {
   ReadUntilEndOfStream(GetStream(DemuxerStream::AUDIO));
 }
 
-// TODO(dalecurtis): Test is disabled since FFmpeg does not currently guarantee
-// the order of demuxed packets in OGG containers.  Re-enable and fix key frame
-// expectations once we decide to either workaround it or attempt a fix
-// upstream.  See http://crbug.com/387996.
-TEST_F(FFmpegDemuxerTest,
-       DISABLED_Read_AudioNegativeStartTimeAndOggDiscard_Bear) {
+// Android has no Theora support, so these tests doesn't work.
+#if !defined(OS_ANDROID)
+TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOggDiscard_Bear) {
   // Many ogg files have negative starting timestamps, so ensure demuxing and
   // seeking work correctly with a negative start time.
   CreateDemuxer("bear.ogv");
@@ -711,8 +708,12 @@ TEST_F(FFmpegDemuxerTest,
   DemuxerStream* video = GetStream(DemuxerStream::VIDEO);
   DemuxerStream* audio = GetStream(DemuxerStream::AUDIO);
 
-  // Run the test twice with a seek in between.
-  for (int i = 0; i < 2; ++i) {
+  // Run the test once (should be twice..., see note) with a seek in between.
+  //
+  // TODO(dalecurtis): We only run the test once since FFmpeg does not currently
+  // guarantee the order of demuxed packets in OGG containers. See
+  // http://crbug.com/387996.
+  for (int i = 0; i < 1; ++i) {
     audio->Read(
         NewReadCBWithCheckedDiscard(FROM_HERE, 40, 0, kInfiniteDuration, true));
     base::RunLoop().Run();
@@ -731,10 +732,10 @@ TEST_F(FFmpegDemuxerTest,
     video->Read(NewReadCB(FROM_HERE, 5751, 0, true));
     base::RunLoop().Run();
 
-    video->Read(NewReadCB(FROM_HERE, 846, 33367, true));
+    video->Read(NewReadCB(FROM_HERE, 846, 33367, false));
     base::RunLoop().Run();
 
-    video->Read(NewReadCB(FROM_HERE, 1255, 66733, true));
+    video->Read(NewReadCB(FROM_HERE, 1255, 66733, false));
     base::RunLoop().Run();
 
     // Seek back to the beginning and repeat the test.
@@ -747,9 +748,6 @@ TEST_F(FFmpegDemuxerTest,
 // Same test above, but using sync2.ogv which has video stream muxed before the
 // audio stream, so seeking based only on start time will fail since ffmpeg is
 // essentially just seeking based on file position.
-//
-// Android has no Theora support, so this test doesn't work.
-#if !defined(OS_ANDROID)
 TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOggDiscard_Sync) {
   // Many ogg files have negative starting timestamps, so ensure demuxing and
   // seeking work correctly with a negative start time.
-- 
2.17.0