Fix seeking when the start time is non-zero.

media/filters/ffmpeg_demuxer.cc

Index: media/filters/ffmpeg_demuxer.cc
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc
index f5b4fddad3b97f03040d8f5f209f940f9d8d3d4c..e1e5f5cedd34fdfa0dabc829a38f3f59825b0ae2 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -55,6 +55,24 @@ static base::TimeDelta FramesToTimeDelta(int frames, double sample_rate) {
       frames * base::Time::kMicrosecondsPerSecond / sample_rate);
 }
 
+static base::TimeDelta ExtractStartTime(AVStream* stream) {
+  if (stream->start_time == static_cast<int64_t>(AV_NOPTS_VALUE))
+    return kNoTimestamp();
+
+  // First try to use the |start_time| value directly.
+  const base::TimeDelta start_time =
+      ConvertFromTimeBase(stream->time_base, stream->start_time);
+
+  // Then compare against the first timestamp to see if adjustment is required.
+  if (stream->first_dts == static_cast<int64_t>(AV_NOPTS_VALUE))
+    return start_time;
+
+  const base::TimeDelta first_dts =
+      ConvertFromTimeBase(stream->time_base, stream->first_dts);
+
+  return first_dts < start_time ? first_dts : start_time;
+}
+
 //
 // FFmpegDemuxerStream
 //
@@ -67,7 +85,8 @@ FFmpegDemuxerStream::FFmpegDemuxerStream(
       type_(UNKNOWN),
       end_of_stream_(false),
       last_packet_timestamp_(kNoTimestamp()),
-      bitstream_converter_enabled_(false) {
+      bitstream_converter_enabled_(false),
+      discard_negative_timestamps_(false) {
   DCHECK(demuxer_);
 
   bool is_encrypted = false;
@@ -226,10 +245,19 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
       buffer->set_decrypt_config(decrypt_config.Pass());
   }
 
-  buffer->set_timestamp(ConvertStreamTimestamp(
-      stream_->time_base, packet->pts));
-  buffer->set_duration(ConvertStreamTimestamp(
-      stream_->time_base, packet->duration));
+  const base::TimeDelta stream_timestamp =
+      ConvertStreamTimestamp(stream_->time_base, packet->pts);
+
+  buffer->set_timestamp(stream_timestamp - demuxer_->start_time());

[acolwell GONE FROM CHROMIUM, 2014/06/10 23:20:38]

Does this hold up with chained Ogg? Does FFmpeg guarantee that timestamps will
always increase? I'm just wondering if it protects us from having to worry about
timestamps later in the stream that end up being < start_time()

[DaleCurtis, 2014/06/11 16:59:07]

Depends on how short the first link in the chain is I guess. I've never seen the
chained ogg go backwards by more than few seconds.  I suspect it's possible
today and my patch doesn't do anything to fix it.  It's possible if we add the
DCHECK(timestamp > 0) to the decoder we'll now fail hard instead of soft there
though.

[acolwell GONE FROM CHROMIUM, 2014/06/11 17:40:24]

I guess my concern is that if FFmpeg allows timestamps to go backwards, how can
we reliably seek? If you use something like oggz-chop to split an ogg file into
2 pieces(A & B) and then concatenate them so they are in B,A order, what does
FFmpeg do? 

I realize that this may be broken in the existing code as well. I'm just trying
to understand what will happen.

[DaleCurtis, 2014/06/12 00:21:56]

I chopped the file as you described it seems timestamps are not rewritten, you
get the zero timestamp for the first part at the end of the second part which
generates a non-monotonic timestamp warning with a huge number.  FFmpeg then
returns a packet without a timestamp which errors out playback.  The behavior is
unchanged by my patch.

+  buffer->set_duration(
+      ConvertStreamTimestamp(stream_->time_base, packet->duration));
+
+  // If enabled, mark packets with negative timestamps for post-decode discard.
+  if (discard_negative_timestamps_ && stream_timestamp < base::TimeDelta()) {
+    buffer->set_discard_padding(
+        std::make_pair(kInfiniteDuration(), base::TimeDelta()));
+  }
+
   if (buffer->timestamp() != kNoTimestamp() &&
       last_packet_timestamp_ != kNoTimestamp() &&
       last_packet_timestamp_ < buffer->timestamp()) {
@@ -268,10 +296,6 @@ void FFmpegDemuxerStream::Stop() {
   end_of_stream_ = true;
 }
 
-base::TimeDelta FFmpegDemuxerStream::duration() {
-  return duration_;
-}
-
 DemuxerStream::Type FFmpegDemuxerStream::type() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   return type_;
@@ -415,11 +439,12 @@ FFmpegDemuxer::FFmpegDemuxer(
       data_source_(data_source),
       media_log_(media_log),
       bitrate_(0),
-      start_time_(kNoTimestamp()),
+      start_time_(kInfiniteDuration()),

[acolwell GONE FROM CHROMIUM, 2014/06/10 23:20:38]

This seems a little out of place since we typically use kNoTimestamp() to signal
"not set". It seems like your use of kInfiniteDurat() should be constrained to
OnFindStreamInfoDone() so it is closer to the code that actually uses it for
special purposes.

[DaleCurtis, 2014/06/12 00:21:55]

Done.

       liveness_(LIVENESS_UNKNOWN),
       text_enabled_(false),
       duration_known_(false),
       need_key_cb_(need_key_cb),
+      stream_index_for_seeking_(0),
       weak_factory_(this) {
   DCHECK(task_runner_.get());
   DCHECK(data_source_);
@@ -445,21 +470,22 @@ void FFmpegDemuxer::Seek(base::TimeDelta time, const PipelineStatusCB& cb) {
   // otherwise we can end up waiting for a pre-seek read to complete even though
   // we know we're going to drop it on the floor.
 
+  const AVStream* seeking_stream =
+      glue_->format_context()->streams[stream_index_for_seeking_];
+
   // Always seek to a timestamp less than or equal to the desired timestamp.
-  int flags = AVSEEK_FLAG_BACKWARD;
+  const int flags = AVSEEK_FLAG_BACKWARD;

[acolwell GONE FROM CHROMIUM, 2014/06/10 23:20:38]

Why not simply inline this below?

[DaleCurtis, 2014/06/12 00:21:55]

Done.

 
-  // Passing -1 as our stream index lets FFmpeg pick a default stream.  FFmpeg
-  // will attempt to use the lowest-index video stream, if present, followed by
-  // the lowest-index audio stream.
   pending_seek_ = true;
   base::PostTaskAndReplyWithResult(
       blocking_thread_.message_loop_proxy().get(),
       FROM_HERE,
-      base::Bind(&av_seek_frame,
-                 glue_->format_context(),
-                 -1,
-                 time.InMicroseconds(),
-                 flags),
+      base::Bind(
+          &av_seek_frame,
+          glue_->format_context(),
+          stream_index_for_seeking_,
+          ConvertToTimeBase(seeking_stream->time_base, time + start_time()),
+          flags),
       base::Bind(
           &FFmpegDemuxer::OnSeekFrameDone, weak_factory_.GetWeakPtr(), cb));
 }
@@ -508,11 +534,6 @@ FFmpegDemuxerStream* FFmpegDemuxer::GetFFmpegStream(
   return NULL;
 }
 
-base::TimeDelta FFmpegDemuxer::GetStartTime() const {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  return start_time_;
-}
-
 base::Time FFmpegDemuxer::GetTimelineOffset() const {
   return timeline_offset_;
 }
@@ -633,6 +654,7 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
     AVStream* stream = format_context->streams[i];
     AVCodecContext* codec_context = stream->codec;
     AVMediaType codec_type = codec_context->codec_type;
+    bool discard_negative_timestamps = false;
 
     if (codec_type == AVMEDIA_TYPE_AUDIO) {
       if (audio_stream)
@@ -647,6 +669,20 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
       if (!audio_config.IsValidConfig())
         continue;
       audio_stream = stream;
+
+      const base::TimeDelta audio_start_time = ExtractStartTime(stream);
+      if (audio_start_time != kNoTimestamp() &&
+          audio_start_time < start_time_) {
+        stream_index_for_seeking_ = i;
+        start_time_ = audio_start_time;
+      }
+
+      // Enable post-decode frame dropping for packets with negative timestamps
+      // as outlined in section A.2 in the Ogg Vorbis spec:
+      // http://xiph.org/vorbis/doc/Vorbis_I_spec.html
+      discard_negative_timestamps =
+          audio_config.codec() == kCodecVorbis &&
+          strcmp(glue_->format_context()->iformat->name, "ogg") == 0;
     } else if (codec_type == AVMEDIA_TYPE_VIDEO) {
       if (video_stream)
         continue;
@@ -661,6 +697,15 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
       if (!video_config.IsValidConfig())
         continue;
       video_stream = stream;
+
+      // Find the video start time.  Note the <= comparison causes the video
+      // stream to be preferred for seeking when start times match.
+      const base::TimeDelta video_start_time = ExtractStartTime(stream);
+      if (video_start_time != kNoTimestamp() &&
+          video_start_time <= start_time_) {
+        stream_index_for_seeking_ = i;
+        start_time_ = video_start_time;
+      }
     } else if (codec_type == AVMEDIA_TYPE_SUBTITLE) {
       if (codec_context->codec_id != AV_CODEC_ID_WEBVTT || !text_enabled_) {
         continue;
@@ -671,13 +716,8 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
 
     streams_[i] = new FFmpegDemuxerStream(this, stream);
     max_duration = std::max(max_duration, streams_[i]->duration());
-
-    if (stream->first_dts != static_cast<int64_t>(AV_NOPTS_VALUE)) {
-      const base::TimeDelta first_dts = ConvertFromTimeBase(
-          stream->time_base, stream->first_dts);
-      if (start_time_ == kNoTimestamp() || first_dts < start_time_)
-        start_time_ = first_dts;
-    }
+    if (discard_negative_timestamps)
+      streams_[i]->enable_negative_timestamp_discard();

[acolwell GONE FROM CHROMIUM, 2014/06/10 23:20:38]

nit: Just pass this via the constructor 2 lines up since this value isn't
expected to change during the life of the stream.

[DaleCurtis, 2014/06/12 00:21:55]

Done.

   }
 
   if (!audio_stream && !video_stream) {
@@ -700,10 +740,14 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
     max_duration = kInfiniteDuration();
   }
 
-  // Some demuxers, like WAV, do not put timestamps on their frames. We
-  // assume the the start time is 0.
-  if (start_time_ == kNoTimestamp())
+  // If no start time could be determined, default to zero and prefer the video
+  // stream over the audio stream for seeking.  E.g., The WAV demuxer does not
+  // put timestamps on its frames.
+  if (start_time_ == kInfiniteDuration()) {
     start_time_ = base::TimeDelta();

[acolwell GONE FROM CHROMIUM, 2014/06/10 23:20:38]

If we don't know at this point, shouldn't we just try to defer this decision
until we are able to get the first packet from all of the streams? If we don't
it seems like we could accidentally trigger bad behavior if the first packets
happen to be negative or really large.

[DaleCurtis, 2014/06/11 16:59:07]

Maybe.  Couldn't a seek happen before the first frame is read?  I don't think
it's worth adding this functionality.  What I've done here will fall back to
what we do today.

[acolwell GONE FROM CHROMIUM, 2014/06/11 17:40:24]

A seek can't happen until we transition to HAVE_METADATA and that can't happen
until the demuxer calls its init callback and all the renderers are initialized.
It should be pretty straightforward to delay calling the init callback until we
have received the first buffer for each stream. Either that or we should block
anything with a timestamp < start_time_ from being sent downstream so that other
components don't have to deal with it.

[DaleCurtis, 2014/06/12 00:21:55]

This is not easy to do since the preroll decision is handled by pipeline the 
demuxer and renderers have completed initialization.  We could trigger Preroll
until we've received all stream start times, but that may delay the rest of
pipeline initialization since those would be network loads.

I've instead just added a DCHECK() for this to the demuxer.

+    stream_index_for_seeking_ =
+        video_stream ? video_stream->index : audio_stream->index;
+  }
 
   // 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
@@ -712,6 +756,11 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
 
   timeline_offset_ = ExtractTimelineOffset(format_context);
 
+  // Since we're shifting the externally visible start time to zero, we need to
+  // adjust the timeline offset to compensate.
+  if (!timeline_offset_.is_null())
+    timeline_offset_ += start_time_;
+
   if (max_duration == kInfiniteDuration() && !timeline_offset_.is_null()) {
     liveness_ = LIVENESS_LIVE;
   } else if (max_duration != kInfiniteDuration()) {
@@ -783,7 +832,6 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,
     media_log_->SetBooleanProperty("found_video_stream", false);
   }
 
-
   media_log_->SetTimeProperty("max_duration", max_duration);
   media_log_->SetTimeProperty("start_time", start_time_);
   media_log_->SetIntegerProperty("bitrate", bitrate_);