Fix seeking when the start time is non-zero.

The seek timestamp should always be adjusted by the start time,
though this manipulation should not be visible to external (web)
clients.

Since this is an FFmpeg only problem, I've removed the concept
of start time from the Demuxer interface in favor of local method
only for FFmpegDemuxer.  FFmpegDemuxerStream's will now use this
value to adjust timestamps such that external clients always see
a zero based timeline.

Doing so required moving some of our ogg vorbis discard code into
the FFmpegDemuxerStream, which actually makes it more accurate and
more narrowly scoped to ogg w/ vorbis instead of all vorbis.

These changes subtly change how we handle seeking.  Previously we
would let FFmpeg choose the stream to perform seeking within.  Now
we will use the video stream only if it contains the seek timestamp,
if none exists or does not contain the seek timestamp, we'll use
the stream with the lowest start time.

I've extended the tests around non-zero start times to verify the
new behavior.

An FFmpeg DEPS roll is required for the new tests to pass:
5c3de80 Pass remaining command-line arguments to ffmpeg's configure
de80875 Change the sigs file to use c-style comments.
cb19b2d Update ffmpeg's GN build to use new yasm assemble format.
9c12290 Revert "avformat/mp3dec: fix start time in light of initial skip samples"
1e661a6 avcodec/vorbisdec: Reset first_frame
7d88be4 avformat/oggparsevorbis: Dont attempt to calculate timestamps from gp=0

BUG=377295
TEST=New unittests.  Demo page from bug works.  Layout tests pass.

Review URL: https://codereview.chromium.org/335273002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278046 0039d316-1c4b-4281-b951-d872f2087c98

DEPS

@@ -211,7 +211,7 @@ deps = {
 
   "src/third_party/ffmpeg":
     Var("chromium_git") +
-    "/chromium/third_party/ffmpeg.git@9caa384561d53a85b4d86bf6ab7426c1362914cf",
+    "/chromium/third_party/ffmpeg.git@5c3de8094903dd2162232b8f1be916c46acdd8f5",
 
   "src/third_party/libjingle/source/talk":
     (Var("googlecode_url") % "webrtc") + "/trunk/talk@" +

media/base/demuxer.h

@@ -82,9 +82,6 @@ class MEDIA_EXPORT Demuxer {
   // to be DemuxerStream::TEXT), or NULL if that type of stream is not present.
   virtual DemuxerStream* GetStream(DemuxerStream::Type type) = 0;
 
-  // Returns the starting time for the media file.
-  virtual base::TimeDelta GetStartTime() const = 0;
-
   // Returns Time represented by presentation timestamp 0.
   // If the timstamps are not associated with a Time, then
   // a null Time is returned.

media/base/mock_filters.h

@@ -38,7 +38,6 @@ class MockDemuxer : public Demuxer {
   MOCK_METHOD1(Stop, void(const base::Closure& callback));
   MOCK_METHOD0(OnAudioRendererDisabled, void());
   MOCK_METHOD1(GetStream, DemuxerStream*(DemuxerStream::Type));
-  MOCK_CONST_METHOD0(GetStartTime, base::TimeDelta());
   MOCK_CONST_METHOD0(GetTimelineOffset, base::Time());
   MOCK_CONST_METHOD0(GetLiveness, Liveness());
 

media/base/pipeline.cc

@@ -386,7 +386,7 @@ void Pipeline::StateTransitionTask(PipelineStatus status) {
         base::AutoLock l(lock_);
         // We do not want to start the clock running. We only want to set the
         // base media time so our timestamp calculations will be correct.
-        clock_->SetTime(demuxer_->GetStartTime(), demuxer_->GetStartTime());
+        clock_->SetTime(base::TimeDelta(), base::TimeDelta());
       }
       if (!audio_renderer_ && !video_renderer_) {
         done_cb.Run(PIPELINE_ERROR_COULD_NOT_RENDER);
@@ -445,7 +445,7 @@ void Pipeline::DoInitialPreroll(const PipelineStatusCB& done_cb) {
   DCHECK(!pending_callbacks_.get());
   SerialRunner::Queue bound_fns;
 
-  base::TimeDelta seek_timestamp = demuxer_->GetStartTime();
+  const base::TimeDelta seek_timestamp = base::TimeDelta();
 
   // Preroll renderers.
   if (audio_renderer_) {
@@ -743,7 +743,6 @@ void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) {
   DCHECK(seek_cb_.is_null());
 
   SetState(kSeeking);
-  base::TimeDelta seek_timestamp = std::max(time, demuxer_->GetStartTime());
   seek_cb_ = seek_cb;
   audio_ended_ = false;
   video_ended_ = false;
@@ -753,9 +752,9 @@ void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) {
   {
     base::AutoLock auto_lock(lock_);
     PauseClockAndStopRendering_Locked();
-    clock_->SetTime(seek_timestamp, seek_timestamp);
+    clock_->SetTime(time, time);
   }
-  DoSeek(seek_timestamp, base::Bind(
+  DoSeek(time, base::Bind(
       &Pipeline::OnStateTransition, base::Unretained(this)));
 }
 

media/base/pipeline_unittest.cc

@@ -107,9 +107,6 @@ class PipelineTest : public ::testing::Test {
     EXPECT_CALL(*demuxer_, GetStream(_))
         .WillRepeatedly(Return(null_pointer));
 
-    EXPECT_CALL(*demuxer_, GetStartTime())
-        .WillRepeatedly(Return(base::TimeDelta()));
-
     EXPECT_CALL(*demuxer_, GetTimelineOffset())
         .WillRepeatedly(Return(base::Time()));
 
@@ -174,7 +171,7 @@ class PipelineTest : public ::testing::Test {
     EXPECT_CALL(*video_renderer_, SetPlaybackRate(0.0f));
 
     // Startup sequence.
-    EXPECT_CALL(*video_renderer_, Preroll(demuxer_->GetStartTime(), _))
+    EXPECT_CALL(*video_renderer_, Preroll(base::TimeDelta(), _))
         .WillOnce(RunCallback<1>(PIPELINE_OK));
     EXPECT_CALL(*video_renderer_, Play(_))
         .WillOnce(RunClosure<0>());
@@ -739,43 +736,6 @@ TEST_F(PipelineTest, NoMessageDuringTearDownFromError) {
   message_loop_.RunUntilIdle();
 }
 
-TEST_F(PipelineTest, StartTimeIsZero) {
-  CreateVideoStream();
-  MockDemuxerStreamVector streams;
-  streams.push_back(video_stream());
-
-  const base::TimeDelta kDuration = base::TimeDelta::FromSeconds(100);
-  InitializeDemuxer(&streams, kDuration);
-  InitializeVideoRenderer(video_stream());
-
-  InitializePipeline(PIPELINE_OK);
-  EXPECT_FALSE(metadata_.has_audio);
-  EXPECT_TRUE(metadata_.has_video);
-
-  EXPECT_EQ(base::TimeDelta(), pipeline_->GetMediaTime());
-}
-
-TEST_F(PipelineTest, StartTimeIsNonZero) {
-  const base::TimeDelta kStartTime = base::TimeDelta::FromSeconds(4);
-  const base::TimeDelta kDuration = base::TimeDelta::FromSeconds(100);
-
-  EXPECT_CALL(*demuxer_, GetStartTime())
-      .WillRepeatedly(Return(kStartTime));
-
-  CreateVideoStream();
-  MockDemuxerStreamVector streams;
-  streams.push_back(video_stream());
-
-  InitializeDemuxer(&streams, kDuration);
-  InitializeVideoRenderer(video_stream());
-
-  InitializePipeline(PIPELINE_OK);
-  EXPECT_FALSE(metadata_.has_audio);
-  EXPECT_TRUE(metadata_.has_video);
-
-  EXPECT_EQ(kStartTime, pipeline_->GetMediaTime());
-}
-
 static void RunTimeCB(const AudioRenderer::TimeCB& time_cb,
                        int time_in_ms,
                        int max_time_in_ms) {

media/filters/chunk_demuxer.cc

@@ -1102,10 +1102,6 @@ DemuxerStream* ChunkDemuxer::GetStream(DemuxerStream::Type type) {
   return NULL;
 }
 
-TimeDelta ChunkDemuxer::GetStartTime() const {
-  return TimeDelta();
-}
-
 base::Time ChunkDemuxer::GetTimelineOffset() const {
   return timeline_offset_;
 }
@@ -1607,7 +1603,7 @@ void ChunkDemuxer::OnSourceInitDone(
     return;
   }
 
-  SeekAllSources(GetStartTime());
+  SeekAllSources(base::TimeDelta());
   StartReturningData();
 
   if (duration_ == kNoTimestamp())

media/filters/chunk_demuxer.h

@@ -161,7 +161,6 @@ class MEDIA_EXPORT ChunkDemuxer : public Demuxer {
   virtual void Stop(const base::Closure& callback) OVERRIDE;
   virtual void Seek(base::TimeDelta time, const PipelineStatusCB&  cb) OVERRIDE;
   virtual DemuxerStream* GetStream(DemuxerStream::Type type) OVERRIDE;
-  virtual base::TimeDelta GetStartTime() const OVERRIDE;
   virtual base::Time GetTimelineOffset() const OVERRIDE;
   virtual Liveness GetLiveness() const OVERRIDE;
 

media/filters/ffmpeg_audio_decoder.cc

@@ -249,16 +249,6 @@ void FFmpegAudioDecoder::DecodeBuffer(
     return;
   }
 
-  if (!buffer->end_of_stream() && !discard_helper_->initialized() &&
-      codec_context_->codec_id == AV_CODEC_ID_VORBIS &&
-      buffer->timestamp() < base::TimeDelta()) {
-    // Dropping frames for negative timestamps as outlined in section A.2
-    // in the Vorbis spec. http://xiph.org/vorbis/doc/Vorbis_I_spec.html
-    const int discard_frames =
-        discard_helper_->TimeDeltaToFrames(-buffer->timestamp());
-    discard_helper_->Reset(discard_frames);
-  }
-
   if (!FFmpegDecode(buffer)) {
     state_ = kError;
     decode_cb.Run(kDecodeError);

media/filters/ffmpeg_demuxer.h

@@ -23,6 +23,7 @@
 #define MEDIA_FILTERS_FFMPEG_DEMUXER_H_
 
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/callback.h"
@@ -55,9 +56,14 @@ typedef scoped_ptr<AVPacket, ScopedPtrAVFreePacket> ScopedAVPacket;
 
 class FFmpegDemuxerStream : public DemuxerStream {
  public:
-  // Keeps a copy of |demuxer| and initializes itself using information
-  // inside |stream|.  Both parameters must outlive |this|.
-  FFmpegDemuxerStream(FFmpegDemuxer* demuxer, AVStream* stream);
+  // Keeps a copy of |demuxer| and initializes itself using information inside
+  // |stream|.  Both parameters must outlive |this|.
+  // |discard_negative_timestamps| tells the DemuxerStream that all packets with
+  // negative timestamps should be marked for post-decode discard.  All decoded
+  // data before time zero will be discarded.
+  FFmpegDemuxerStream(FFmpegDemuxer* demuxer,
+                      AVStream* stream,
+                      bool discard_negative_timestamps);
   virtual ~FFmpegDemuxerStream();
 
   // Enqueues the given AVPacket. It is invalid to queue a |packet| after
@@ -74,8 +80,7 @@ class FFmpegDemuxerStream : public DemuxerStream {
   // Empties the queues and ignores any additional calls to Read().
   void Stop();
 
-  // Returns the duration of this stream.
-  base::TimeDelta duration();
+  base::TimeDelta duration() const { return duration_; }
 
   // DemuxerStream implementation.
   virtual Type type() OVERRIDE;
@@ -136,6 +141,7 @@ class FFmpegDemuxerStream : public DemuxerStream {
   bool bitstream_converter_enabled_;
 
   std::string encryption_key_id_;
+  const bool discard_negative_timestamps_;
 
   DISALLOW_COPY_AND_ASSIGN(FFmpegDemuxerStream);
 };
@@ -155,7 +161,6 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
   virtual void Stop(const base::Closure& callback) OVERRIDE;
   virtual void Seek(base::TimeDelta time, const PipelineStatusCB& cb) OVERRIDE;
   virtual DemuxerStream* GetStream(DemuxerStream::Type type) OVERRIDE;
-  virtual base::TimeDelta GetStartTime() const OVERRIDE;
   virtual base::Time GetTimelineOffset() const OVERRIDE;
   virtual Liveness GetLiveness() const OVERRIDE;
 
@@ -168,6 +173,10 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
   void NotifyCapacityAvailable();
   void NotifyBufferingChanged();
 
+  // The lowest demuxed timestamp.  DemuxerStream's must use this to adjust
+  // packet timestamps such that external clients see a zero-based timeline.
+  base::TimeDelta start_time() const { return start_time_; }
+
  private:
   // To allow tests access to privates.
   friend class FFmpegDemuxerTest;
@@ -224,7 +233,7 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
   // results of pre-seek av_read_frame() operations.
   bool pending_seek_;
 
-  // |streams_| mirrors the AVStream array in |format_context_|. It contains
+  // |streams_| mirrors the AVStream array in AVFormatContext. It contains
   // FFmpegDemuxerStreams encapsluating AVStream objects at the same index.
   //
   // Since we only support a single audio and video stream, |streams_| will
@@ -245,11 +254,22 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
   // Derived bitrate after initialization has completed.
   int bitrate_;
 
-  // The first timestamp of the opened media file. This is used to set the
-  // starting clock value to match the timestamps in the media file. Default
-  // is 0.
+  // The first timestamp of the audio or video stream, whichever is lower.  This
+  // is used to adjust timestamps so that external consumers always see a zero
+  // based timeline.
   base::TimeDelta start_time_;
 
+  // The index and start time of the preferred streams for seeking.  Filled upon
+  // completion of OnFindStreamInfoDone().  Each info entry represents an index
+  // into |streams_| and the start time of that stream.
+  //
+  // Seek() will attempt to use |preferred_stream_for_seeking_| if the seek
+  // point occurs after its associated start time.  Otherwise it will use
+  // |fallback_stream_for_seeking_|.
+  typedef std::pair<int, base::TimeDelta> StreamSeekInfo;
+  StreamSeekInfo preferred_stream_for_seeking_;
+  StreamSeekInfo fallback_stream_for_seeking_;
+
   // The Time associated with timestamp 0. Set to a null
   // time if the file doesn't have an association to Time.
   base::Time timeline_offset_;

media/filters/ffmpeg_demuxer_unittest.cc

@@ -92,13 +92,19 @@ class FFmpegDemuxerTest : public testing::Test {
 
   MOCK_METHOD1(CheckPoint, void(int v));
 
-  void InitializeDemuxerText(bool enable_text) {
+  void InitializeDemuxerWithTimelineOffset(bool enable_text,
+                                           base::Time timeline_offset) {
     EXPECT_CALL(host_, SetDuration(_));
     WaitableMessageLoopEvent event;
     demuxer_->Initialize(&host_, event.GetPipelineStatusCB(), enable_text);
+    demuxer_->timeline_offset_ = timeline_offset;
     event.RunAndWaitForStatus(PIPELINE_OK);
   }
 
+  void InitializeDemuxerText(bool enable_text) {
+    InitializeDemuxerWithTimelineOffset(enable_text, base::Time());
+  }
+
   void InitializeDemuxer() {
     InitializeDemuxerText(false);
   }
@@ -109,7 +115,9 @@ class FFmpegDemuxerTest : public testing::Test {
   // |location| simply indicates where the call to this function was made.
   // This makes it easier to track down where test failures occur.
   void OnReadDone(const tracked_objects::Location& location,
-                  int size, int64 timestampInMicroseconds,
+                  int size,
+                  int64 timestamp_us,
+                  base::TimeDelta discard_front_padding,
                   DemuxerStream::Status status,
                   const scoped_refptr<DecoderBuffer>& buffer) {
     std::string location_str;
@@ -117,21 +125,39 @@ class FFmpegDemuxerTest : public testing::Test {
     location_str += "\n";
     SCOPED_TRACE(location_str);
     EXPECT_EQ(status, DemuxerStream::kOk);
-    OnReadDoneCalled(size, timestampInMicroseconds);
+    OnReadDoneCalled(size, timestamp_us);
     EXPECT_TRUE(buffer.get() != NULL);
     EXPECT_EQ(size, buffer->data_size());
-    EXPECT_EQ(base::TimeDelta::FromMicroseconds(timestampInMicroseconds),
-              buffer->timestamp());
-
+    EXPECT_EQ(timestamp_us, buffer->timestamp().InMicroseconds());
+    EXPECT_EQ(discard_front_padding, buffer->discard_padding().first);
     DCHECK_EQ(&message_loop_, base::MessageLoop::current());
     message_loop_.PostTask(FROM_HERE, base::MessageLoop::QuitWhenIdleClosure());
   }
 
   DemuxerStream::ReadCB NewReadCB(const tracked_objects::Location& location,
-                                  int size, int64 timestampInMicroseconds) {
-    EXPECT_CALL(*this, OnReadDoneCalled(size, timestampInMicroseconds));
-    return base::Bind(&FFmpegDemuxerTest::OnReadDone, base::Unretained(this),
-                      location, size, timestampInMicroseconds);
+                                  int size,
+                                  int64 timestamp_us) {
+    EXPECT_CALL(*this, OnReadDoneCalled(size, timestamp_us));
+    return base::Bind(&FFmpegDemuxerTest::OnReadDone,
+                      base::Unretained(this),
+                      location,
+                      size,
+                      timestamp_us,
+                      base::TimeDelta());
+  }
+
+  DemuxerStream::ReadCB NewReadCBWithCheckedDiscard(
+      const tracked_objects::Location& location,
+      int size,
+      int64 timestamp_us,
+      base::TimeDelta discard_front_padding) {
+    EXPECT_CALL(*this, OnReadDoneCalled(size, timestamp_us));
+    return base::Bind(&FFmpegDemuxerTest::OnReadDone,
+                      base::Unretained(this),
+                      location,
+                      size,
+                      timestamp_us,
+                      discard_front_padding);
   }
 
   // TODO(xhwang): This is a workaround of the issue that move-only parameters
@@ -386,25 +412,102 @@ TEST_F(FFmpegDemuxerTest, Read_Text) {
   message_loop_.Run();
 }
 
-TEST_F(FFmpegDemuxerTest, Read_VideoNonZeroStart) {
+TEST_F(FFmpegDemuxerTest, Read_VideoPositiveStartTime) {
+  const int64 kTimelineOffsetMs = 1352550896000LL;
+
   // Test the start time is the first timestamp of the video and audio stream.
   CreateDemuxer("nonzero-start-time.webm");
-  InitializeDemuxer();
+  InitializeDemuxerWithTimelineOffset(
+      false, base::Time::FromJsTime(kTimelineOffsetMs));
 
   // Attempt a read from the video stream and run the message loop until done.
   DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
   DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
 
-  // Check first buffer in video stream.
-  video->Read(NewReadCB(FROM_HERE, 5636, 400000));
-  message_loop_.Run();
+  const base::TimeDelta video_start_time =
+      base::TimeDelta::FromMicroseconds(400000);
+  const base::TimeDelta audio_start_time =
+      base::TimeDelta::FromMicroseconds(396000);
+
+  // Run the test twice with a seek in between.
+  for (int i = 0; i < 2; ++i) {
+    // Check first buffer in video stream.  It should have been adjusted such
+    // that it starts 400ms after the first audio buffer.
+    video->Read(
+        NewReadCB(FROM_HERE,
+                  5636,
+                  (video_start_time - audio_start_time).InMicroseconds()));
+    message_loop_.Run();
 
-  // Check first buffer in audio stream.
-  audio->Read(NewReadCB(FROM_HERE, 165, 396000));
-  message_loop_.Run();
+    // Since the audio buffer has a lower first timestamp, it should become
+    // zero.
+    audio->Read(NewReadCB(FROM_HERE, 165, 0));
+    message_loop_.Run();
+
+    // Verify that the start time is equal to the lowest timestamp (ie the
+    // audio).
+    EXPECT_EQ(audio_start_time, demuxer_->start_time());
+
+    // Verify that the timeline offset has been adjusted by the start time.
+    EXPECT_EQ(kTimelineOffsetMs + audio_start_time.InMilliseconds(),
+              demuxer_->GetTimelineOffset().ToJavaTime());
+
+    // Seek back to the beginning and repeat the test.
+    WaitableMessageLoopEvent event;
+    demuxer_->Seek(base::TimeDelta(), event.GetPipelineStatusCB());
+    event.RunAndWaitForStatus(PIPELINE_OK);
+  }
+}
 
-  // Verify that the start time is equal to the lowest timestamp (ie the audio).
-  EXPECT_EQ(demuxer_->GetStartTime().InMicroseconds(), 396000);
+TEST_F(FFmpegDemuxerTest, Read_AudioNoStartTime) {
+  // FFmpeg does not set timestamps when demuxing wave files.  Ensure that the
+  // demuxer sets a start time of zero in this case.
+  CreateDemuxer("sfx_s24le.wav");
+  InitializeDemuxer();
+
+  // Run the test twice with a seek in between.
+  for (int i = 0; i < 2; ++i) {
+    demuxer_->GetStream(DemuxerStream::AUDIO)
+        ->Read(NewReadCB(FROM_HERE, 4095, 0));
+    message_loop_.Run();
+    EXPECT_EQ(base::TimeDelta(), demuxer_->start_time());
+
+    // Seek back to the beginning and repeat the test.
+    WaitableMessageLoopEvent event;
+    demuxer_->Seek(base::TimeDelta(), event.GetPipelineStatusCB());
+    event.RunAndWaitForStatus(PIPELINE_OK);
+  }
+}
+
+TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOggDiscard) {
+  // Many ogg files have negative starting timestamps, so ensure demuxing and
+  // seeking work correctly with a negative start time.
+  CreateDemuxer("bear.ogv");
+  InitializeDemuxer();
+
+  // Run the test twice with a seek in between.
+  for (int i = 0; i < 2; ++i) {
+    demuxer_->GetStream(DemuxerStream::AUDIO)->Read(
+        NewReadCBWithCheckedDiscard(FROM_HERE, 40, 0, kInfiniteDuration()));
+    message_loop_.Run();
+    demuxer_->GetStream(DemuxerStream::AUDIO)->Read(
+        NewReadCBWithCheckedDiscard(FROM_HERE, 41, 2903, kInfiniteDuration()));
+    message_loop_.Run();
+    demuxer_->GetStream(DemuxerStream::AUDIO)->Read(NewReadCBWithCheckedDiscard(
+        FROM_HERE, 173, 5805, base::TimeDelta::FromMicroseconds(10159)));
+    message_loop_.Run();
+
+    demuxer_->GetStream(DemuxerStream::AUDIO)
+        ->Read(NewReadCB(FROM_HERE, 148, 18866));
+    message_loop_.Run();
+    EXPECT_EQ(base::TimeDelta::FromMicroseconds(-15964),
+              demuxer_->start_time());
+
+    // Seek back to the beginning and repeat the test.
+    WaitableMessageLoopEvent event;
+    demuxer_->Seek(base::TimeDelta(), event.GetPipelineStatusCB());
+    event.RunAndWaitForStatus(PIPELINE_OK);
+  }
 }
 
 TEST_F(FFmpegDemuxerTest, Read_EndOfStream) {