From b9ec078cf3b5355a1a19c55a75ebb6ab101b4483 Mon Sep 17 00:00:00 2001 From: dalecurtis Date: Tue, 16 Sep 2014 15:23:21 -0700 Subject: [PATCH] Initialize media timeline correctly for positive start times. The previous fix for positive start times http://crrev.com/93c37f1b is incomplete. It fixed seeking, but did not fix rendering. As such, the media timeline will start playback from 0 and advance in real time to any positive start time... even if the actual start is hours away. When testing I used a test clip with a short positive start time so did not notice the issue. In attempting to write a layout test, I retested with a larger start time which revealed the deficiency in my first solution. This change reverts the removal of Demuxer::StartTime() with the added restriction that it must always be positive. Pipeline uses this time like it did before, during initialization and as a minimum bound for seeking. BUG=413292 TEST=four new tests! layout test forthcoming. Review URL: https://codereview.chromium.org/575643002 Cr-Commit-Position: refs/heads/master@{#295159} --- media/base/demuxer.h | 3 +++ media/base/mock_filters.h | 1 + media/base/pipeline.cc | 9 +++++++-- media/base/pipeline_unittest.cc | 21 ++++++++++++++++++++- media/filters/chunk_demuxer.cc | 6 +++++- media/filters/chunk_demuxer.h | 1 + media/filters/ffmpeg_demuxer.cc | 6 +++++- media/filters/ffmpeg_demuxer.h | 6 ++++-- media/filters/ffmpeg_demuxer_unittest.cc | 9 ++++++--- media/filters/pipeline_integration_test.cc | 11 +++++++++++ 10 files changed, 63 insertions(+), 10 deletions(-) diff --git a/media/base/demuxer.h b/media/base/demuxer.h index 3968add1bf77..fb27bc9141b8 100644 --- a/media/base/demuxer.h +++ b/media/base/demuxer.h @@ -73,6 +73,9 @@ class MEDIA_EXPORT Demuxer : public DemuxerStreamProvider { // method (including Stop()) after a demuxer has stopped. virtual void Stop() = 0; + // Returns the starting time for the media file; it's always positive. + 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. diff --git a/media/base/mock_filters.h b/media/base/mock_filters.h index ccaaaf614234..8a425271e0a4 100644 --- a/media/base/mock_filters.h +++ b/media/base/mock_filters.h @@ -39,6 +39,7 @@ class MockDemuxer : public Demuxer { MOCK_METHOD0(Stop, void()); 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()); diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc index 086dd9651b16..ee742bf408b4 100644 --- a/media/base/pipeline.cc +++ b/media/base/pipeline.cc @@ -343,10 +343,12 @@ void Pipeline::StateTransitionTask(PipelineStatus status) { if (!is_initialized_) { is_initialized_ = true; ReportMetadata(); + start_timestamp_ = demuxer_->GetStartTime(); } base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK); + DCHECK(start_timestamp_ >= base::TimeDelta()); renderer_->StartPlayingFrom(start_timestamp_); if (text_renderer_) @@ -584,13 +586,16 @@ void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) { DCHECK(seek_cb_.is_null()); + const base::TimeDelta seek_timestamp = + std::max(time, demuxer_->GetStartTime()); + SetState(kSeeking); seek_cb_ = seek_cb; renderer_ended_ = false; text_renderer_ended_ = false; - start_timestamp_ = time; + start_timestamp_ = seek_timestamp; - DoSeek(time, + DoSeek(seek_timestamp, base::Bind(&Pipeline::OnStateTransition, weak_factory_.GetWeakPtr())); } diff --git a/media/base/pipeline_unittest.cc b/media/base/pipeline_unittest.cc index 8a4dea158236..7b29698c3b73 100644 --- a/media/base/pipeline_unittest.cc +++ b/media/base/pipeline_unittest.cc @@ -103,6 +103,8 @@ class PipelineTest : public ::testing::Test { EXPECT_CALL(*renderer_, GetMediaTime()) .WillRepeatedly(Return(base::TimeDelta())); + + EXPECT_CALL(*demuxer_, GetStartTime()).WillRepeatedly(Return(start_time_)); } virtual ~PipelineTest() { @@ -203,7 +205,7 @@ class PipelineTest : public ::testing::Test { EXPECT_CALL(callbacks_, OnMetadata(_)).WillOnce(SaveArg<0>(&metadata_)); EXPECT_CALL(*renderer_, SetPlaybackRate(0.0f)); EXPECT_CALL(*renderer_, SetVolume(1.0f)); - EXPECT_CALL(*renderer_, StartPlayingFrom(base::TimeDelta())) + EXPECT_CALL(*renderer_, StartPlayingFrom(start_time_)) .WillOnce(SetBufferingState(&buffering_state_cb_, BUFFERING_HAVE_ENOUGH)); EXPECT_CALL(callbacks_, OnBufferingStateChange(BUFFERING_HAVE_ENOUGH)); @@ -313,6 +315,7 @@ class PipelineTest : public ::testing::Test { base::Closure ended_cb_; VideoDecoderConfig video_decoder_config_; PipelineMetadata metadata_; + base::TimeDelta start_time_; private: DISALLOW_COPY_AND_ASSIGN(PipelineTest); @@ -742,6 +745,22 @@ TEST_F(PipelineTest, Underflow) { DoSeek(expected); } +TEST_F(PipelineTest, PositiveStartTime) { + start_time_ = base::TimeDelta::FromSeconds(1); + EXPECT_CALL(*demuxer_, GetStartTime()).WillRepeatedly(Return(start_time_)); + CreateAudioStream(); + MockDemuxerStreamVector streams; + streams.push_back(audio_stream()); + SetDemuxerExpectations(&streams); + SetRendererExpectations(); + StartPipelineAndExpect(PIPELINE_OK); + ExpectDemuxerStop(); + ExpectPipelineStopAndDestroyPipeline(); + pipeline_->Stop( + base::Bind(&CallbackHelper::OnStop, base::Unretained(&callbacks_))); + message_loop_.RunUntilIdle(); +} + class PipelineTeardownTest : public PipelineTest { public: enum TeardownState { diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc index 383aa84f26e2..505d9c1f69c6 100644 --- a/media/filters/chunk_demuxer.cc +++ b/media/filters/chunk_demuxer.cc @@ -1141,6 +1141,10 @@ DemuxerStream* ChunkDemuxer::GetStream(DemuxerStream::Type type) { return NULL; } +TimeDelta ChunkDemuxer::GetStartTime() const { + return TimeDelta(); +} + Demuxer::Liveness ChunkDemuxer::GetLiveness() const { return liveness_; } @@ -1642,7 +1646,7 @@ void ChunkDemuxer::OnSourceInitDone( return; } - SeekAllSources(base::TimeDelta()); + SeekAllSources(GetStartTime()); StartReturningData(); if (duration_ == kNoTimestamp()) diff --git a/media/filters/chunk_demuxer.h b/media/filters/chunk_demuxer.h index a48310895e56..4837dff49fc2 100644 --- a/media/filters/chunk_demuxer.h +++ b/media/filters/chunk_demuxer.h @@ -164,6 +164,7 @@ class MEDIA_EXPORT ChunkDemuxer : public Demuxer { virtual void Seek(base::TimeDelta time, const PipelineStatusCB& cb) OVERRIDE; virtual base::Time GetTimelineOffset() const OVERRIDE; virtual DemuxerStream* GetStream(DemuxerStream::Type type) OVERRIDE; + virtual base::TimeDelta GetStartTime() const OVERRIDE; virtual Liveness GetLiveness() const OVERRIDE; // Methods used by an external object to control this demuxer. diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc index 27414974c798..2cc89130b1c4 100644 --- a/media/filters/ffmpeg_demuxer.cc +++ b/media/filters/ffmpeg_demuxer.cc @@ -686,6 +686,10 @@ FFmpegDemuxerStream* FFmpegDemuxer::GetFFmpegStream( return NULL; } +base::TimeDelta FFmpegDemuxer::GetStartTime() const { + return std::max(start_time_, base::TimeDelta()); +} + Demuxer::Liveness FFmpegDemuxer::GetLiveness() const { DCHECK(task_runner_->BelongsToCurrentThread()); return liveness_; @@ -959,7 +963,7 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb, // 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()) + if (!timeline_offset_.is_null() && start_time_ < base::TimeDelta()) timeline_offset_ += start_time_; if (max_duration == kInfiniteDuration() && !timeline_offset_.is_null()) { diff --git a/media/filters/ffmpeg_demuxer.h b/media/filters/ffmpeg_demuxer.h index 9680b26310cc..d2855e0d9743 100644 --- a/media/filters/ffmpeg_demuxer.h +++ b/media/filters/ffmpeg_demuxer.h @@ -168,6 +168,7 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer { virtual void Seek(base::TimeDelta time, const PipelineStatusCB& cb) OVERRIDE; virtual base::Time GetTimelineOffset() const OVERRIDE; virtual DemuxerStream* GetStream(DemuxerStream::Type type) OVERRIDE; + virtual base::TimeDelta GetStartTime() const OVERRIDE; virtual Liveness GetLiveness() const OVERRIDE; // Calls |need_key_cb_| with the initialization data encountered in the file. @@ -179,8 +180,9 @@ 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. + // The lowest demuxed timestamp. If negative, DemuxerStreams 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: diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc index d20d1aaa5a5d..36e4824b82a5 100644 --- a/media/filters/ffmpeg_demuxer_unittest.cc +++ b/media/filters/ffmpeg_demuxer_unittest.cc @@ -447,9 +447,8 @@ TEST_F(FFmpegDemuxerTest, Read_VideoPositiveStartTime) { // 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()); + // Verify that the timeline offset has not been adjusted by the start time. + EXPECT_EQ(kTimelineOffsetMs, demuxer_->GetTimelineOffset().ToJavaTime()); // Seek back to the beginning and repeat the test. WaitableMessageLoopEvent event; @@ -549,6 +548,10 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOggDiscard_Sync) { EXPECT_EQ(base::TimeDelta::FromMicroseconds(-2902), demuxer_->start_time()); + // Though the internal start time may be below zero, the exposed media time + // must always be greater than zero. + EXPECT_EQ(base::TimeDelta(), demuxer_->GetStartTime()); + video->Read(NewReadCB(FROM_HERE, 9997, 0)); message_loop_.Run(); diff --git a/media/filters/pipeline_integration_test.cc b/media/filters/pipeline_integration_test.cc index 3d70558aeb1d..bce580d76fad 100644 --- a/media/filters/pipeline_integration_test.cc +++ b/media/filters/pipeline_integration_test.cc @@ -1563,6 +1563,7 @@ TEST_F(PipelineIntegrationTest, BasicPlaybackChainedOgg) { ASSERT_TRUE(Start(GetTestDataFilePath("double-sfx.ogg"), PIPELINE_OK)); Play(); ASSERT_TRUE(WaitUntilOnEnded()); + ASSERT_EQ(base::TimeDelta(), demuxer_->GetStartTime()); } // Ensures audio-video playback with missing or negative timestamps fails softly @@ -1571,6 +1572,7 @@ TEST_F(PipelineIntegrationTest, BasicPlaybackChainedOggVideo) { ASSERT_TRUE(Start(GetTestDataFilePath("double-bear.ogv"), PIPELINE_OK)); Play(); EXPECT_EQ(PIPELINE_ERROR_DECODE, WaitUntilEndedOrError()); + ASSERT_EQ(base::TimeDelta(), demuxer_->GetStartTime()); } // Tests that we signal ended even when audio runs longer than video track. @@ -1595,4 +1597,13 @@ TEST_F(PipelineIntegrationTest, BasicPlaybackAudioShorterThanVideo) { ASSERT_TRUE(WaitUntilOnEnded()); } +TEST_F(PipelineIntegrationTest, BasicPlaybackPositiveStartTime) { + ASSERT_TRUE( + Start(GetTestDataFilePath("nonzero-start-time.webm"), PIPELINE_OK)); + Play(); + ASSERT_TRUE(WaitUntilOnEnded()); + ASSERT_EQ(base::TimeDelta::FromMicroseconds(396000), + demuxer_->GetStartTime()); +} + } // namespace media -- 2.11.4.GIT