From dfb28d8eff00175d94eaf79c09e43090129adb86 Mon Sep 17 00:00:00 2001 From: "falken@chromium.org" Date: Mon, 14 Jul 2014 02:48:56 +0000 Subject: [PATCH] Revert of Remove media::VideoRenderer::SetPlaybackRate(). (https://codereview.chromium.org/384943002/) Reason for revert: Sorry to revert this change. It made the following layout tests flakily fail: media/track/track-cue-rendering-horizontal.html media/track/track-cue-rendering-vertical.html Reverting locally healed the tests. The image diffs show what appears to be a timestamp change (something like 0:18 to 0:19): https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux__dbg_/20779/layout-test-results/results.html Original issue's description: > Remove media::VideoRenderer::SetPlaybackRate(). > > Video renderers don't need the information as they are already making > audio/video synchronization decisions based on a media timeline that > already incorporates the current playback rate via the time callback. > > In particular, VideoRendererImpl only used playback rate to estimate > a better duration to sleep until the current frame was ready ... > except that in most cases we'd sleep for kIdleTimeDelta and wait > until the media timeline had progressed past the current frame's > timestamp. In other words, there's absolutely no reason to even try > to estimate the sleep duration based on the playback rate. > > BUG=370634 > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282797 TBR=xhwang@chromium.org,scherkus@chromium.org NOTREECHECKS=true NOTRY=true BUG=370634 Review URL: https://codereview.chromium.org/390733002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282891 0039d316-1c4b-4281-b951-d872f2087c98 --- media/base/mock_filters.h | 1 + media/base/pipeline.cc | 2 + media/base/pipeline_unittest.cc | 4 ++ media/base/video_renderer.h | 3 ++ media/filters/video_renderer_impl.cc | 64 +++++++++++++++++---------- media/filters/video_renderer_impl.h | 11 +++++ media/filters/video_renderer_impl_unittest.cc | 7 +++ 7 files changed, 69 insertions(+), 23 deletions(-) diff --git a/media/base/mock_filters.h b/media/base/mock_filters.h index 7afa62501678..c74190e8dc02 100644 --- a/media/base/mock_filters.h +++ b/media/base/mock_filters.h @@ -130,6 +130,7 @@ class MockVideoRenderer : public VideoRenderer { MOCK_METHOD1(Flush, void(const base::Closure& callback)); MOCK_METHOD1(StartPlayingFrom, void(base::TimeDelta timestamp)); MOCK_METHOD1(Stop, void(const base::Closure& callback)); + MOCK_METHOD1(SetPlaybackRate, void(float playback_rate)); private: DISALLOW_COPY_AND_ASSIGN(MockVideoRenderer); diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc index ca53be0ceb87..71c7248421d0 100644 --- a/media/base/pipeline.cc +++ b/media/base/pipeline.cc @@ -649,6 +649,8 @@ void Pipeline::PlaybackRateChangedTask(float playback_rate) { if (audio_renderer_) audio_renderer_->SetPlaybackRate(playback_rate_); + if (video_renderer_) + video_renderer_->SetPlaybackRate(playback_rate_); } void Pipeline::VolumeChangedTask(float volume) { diff --git a/media/base/pipeline_unittest.cc b/media/base/pipeline_unittest.cc index 2685b8eacde7..e259a827ef22 100644 --- a/media/base/pipeline_unittest.cc +++ b/media/base/pipeline_unittest.cc @@ -210,6 +210,7 @@ class PipelineTest : public ::testing::Test { } if (video_stream_) { + EXPECT_CALL(*video_renderer_, SetPlaybackRate(0.0f)); EXPECT_CALL(*video_renderer_, StartPlayingFrom(base::TimeDelta())) .WillOnce(SetBufferingState(&video_buffering_state_cb_, BUFFERING_HAVE_ENOUGH)); @@ -286,6 +287,7 @@ class PipelineTest : public ::testing::Test { EXPECT_CALL(*video_renderer_, StartPlayingFrom(seek_time)) .WillOnce(SetBufferingState(&video_buffering_state_cb_, BUFFERING_HAVE_ENOUGH)); + EXPECT_CALL(*video_renderer_, SetPlaybackRate(_)); } // We expect a successful seek callback followed by a buffering update. @@ -633,6 +635,7 @@ TEST_F(PipelineTest, AudioStreamShorterThanVideo) { EXPECT_EQ(0, pipeline_->GetMediaTime().ToInternalValue()); float playback_rate = 1.0f; + EXPECT_CALL(*video_renderer_, SetPlaybackRate(playback_rate)); EXPECT_CALL(*audio_renderer_, SetPlaybackRate(playback_rate)); pipeline_->SetPlaybackRate(playback_rate); message_loop_.RunUntilIdle(); @@ -1001,6 +1004,7 @@ class PipelineTeardownTest : public PipelineTest { BUFFERING_HAVE_ENOUGH)); EXPECT_CALL(*audio_renderer_, SetPlaybackRate(0.0f)); + EXPECT_CALL(*video_renderer_, SetPlaybackRate(0.0f)); EXPECT_CALL(*audio_renderer_, SetVolume(1.0f)); EXPECT_CALL(*audio_renderer_, StartRendering()); diff --git a/media/base/video_renderer.h b/media/base/video_renderer.h index 2e36c7f56d1a..e92eca8342b3 100644 --- a/media/base/video_renderer.h +++ b/media/base/video_renderer.h @@ -75,6 +75,9 @@ class MEDIA_EXPORT VideoRenderer { // when complete. virtual void Stop(const base::Closure& callback) = 0; + // Updates the current playback rate. + virtual void SetPlaybackRate(float playback_rate) = 0; + private: DISALLOW_COPY_AND_ASSIGN(VideoRenderer); }; diff --git a/media/filters/video_renderer_impl.cc b/media/filters/video_renderer_impl.cc index 7bf14d7a5d87..8735a0a96f2a 100644 --- a/media/filters/video_renderer_impl.cc +++ b/media/filters/video_renderer_impl.cc @@ -34,6 +34,7 @@ VideoRendererImpl::VideoRendererImpl( thread_(), pending_read_(false), drop_frames_(drop_frames), + playback_rate_(0), buffering_state_(BUFFERING_HAVE_NOTHING), paint_cb_(paint_cb), last_timestamp_(kNoTimestamp()), @@ -105,6 +106,12 @@ void VideoRendererImpl::Stop(const base::Closure& callback) { video_frame_stream_.Stop(callback); } +void VideoRendererImpl::SetPlaybackRate(float playback_rate) { + DCHECK(task_runner_->BelongsToCurrentThread()); + base::AutoLock auto_lock(lock_); + playback_rate_ = playback_rate; +} + void VideoRendererImpl::StartPlayingFrom(base::TimeDelta timestamp) { DCHECK(task_runner_->BelongsToCurrentThread()); base::AutoLock auto_lock(lock_); @@ -214,7 +221,8 @@ void VideoRendererImpl::ThreadMain() { return; // Remain idle as long as we're not playing. - if (state_ != kPlaying || buffering_state_ != BUFFERING_HAVE_ENOUGH) { + if (state_ != kPlaying || playback_rate_ == 0 || + buffering_state_ != BUFFERING_HAVE_ENOUGH) { UpdateStatsAndWait_Locked(kIdleTimeDelta); continue; } @@ -230,10 +238,16 @@ void VideoRendererImpl::ThreadMain() { continue; } - base::TimeDelta now = get_time_cb_.Run(); - base::TimeDelta target_timestamp = ready_frames_.front()->timestamp(); - base::TimeDelta earliest_paint_timestamp; - base::TimeDelta latest_paint_timestamp; + base::TimeDelta remaining_time = + CalculateSleepDuration(ready_frames_.front(), playback_rate_); + + // Sleep up to a maximum of our idle time until we're within the time to + // render the next frame. + if (remaining_time.InMicroseconds() > 0) { + remaining_time = std::min(remaining_time, kIdleTimeDelta); + UpdateStatsAndWait_Locked(remaining_time); + continue; + } // Deadline is defined as the midpoint between this frame and the next // frame, using the delta between this frame and the previous frame as the @@ -246,24 +260,15 @@ void VideoRendererImpl::ThreadMain() { // // TODO(scherkus): This can be vastly improved. Use a histogram to measure // the accuracy of our frame timing code. http://crbug.com/149829 - if (last_timestamp_ == kNoTimestamp()) { - earliest_paint_timestamp = target_timestamp; - latest_paint_timestamp = base::TimeDelta::Max(); - } else { - base::TimeDelta duration = target_timestamp - last_timestamp_; - earliest_paint_timestamp = target_timestamp - duration / 2; - latest_paint_timestamp = target_timestamp + duration / 2; - } - - // Remain idle until we've reached our target paint window. - if (now < earliest_paint_timestamp) { - UpdateStatsAndWait_Locked(kIdleTimeDelta); - continue; - } - - if (now > latest_paint_timestamp && drop_frames_) { - DropNextReadyFrame_Locked(); - continue; + if (drop_frames_ && last_timestamp_ != kNoTimestamp()) { + base::TimeDelta now = get_time_cb_.Run(); + base::TimeDelta deadline = ready_frames_.front()->timestamp() + + (ready_frames_.front()->timestamp() - last_timestamp_) / 2; + + if (now > deadline) { + DropNextReadyFrame_Locked(); + continue; + } } // Congratulations! You've made it past the video frame timing gauntlet. @@ -472,6 +477,19 @@ void VideoRendererImpl::OnVideoFrameStreamResetDone() { base::ResetAndReturn(&flush_cb_).Run(); } +base::TimeDelta VideoRendererImpl::CalculateSleepDuration( + const scoped_refptr& next_frame, + float playback_rate) { + // Determine the current and next presentation timestamps. + base::TimeDelta now = get_time_cb_.Run(); + base::TimeDelta next_pts = next_frame->timestamp(); + + // Scale our sleep based on the playback rate. + base::TimeDelta sleep = next_pts - now; + return base::TimeDelta::FromMicroseconds( + static_cast(sleep.InMicroseconds() / playback_rate)); +} + void VideoRendererImpl::DoStopOrError_Locked() { lock_.AssertAcquired(); last_timestamp_ = kNoTimestamp(); diff --git a/media/filters/video_renderer_impl.h b/media/filters/video_renderer_impl.h index 0f142be35205..bcba124b5200 100644 --- a/media/filters/video_renderer_impl.h +++ b/media/filters/video_renderer_impl.h @@ -69,6 +69,7 @@ class MEDIA_EXPORT VideoRendererImpl virtual void Flush(const base::Closure& callback) OVERRIDE; virtual void StartPlayingFrom(base::TimeDelta timestamp) OVERRIDE; virtual void Stop(const base::Closure& callback) OVERRIDE; + virtual void SetPlaybackRate(float playback_rate) OVERRIDE; // PlatformThread::Delegate implementation. virtual void ThreadMain() OVERRIDE; @@ -94,6 +95,14 @@ class MEDIA_EXPORT VideoRendererImpl // Called when VideoFrameStream::Reset() completes. void OnVideoFrameStreamResetDone(); + // Calculates the duration to sleep for based on |last_timestamp_|, + // the next frame timestamp (may be NULL), and the provided playback rate. + // + // We don't use |playback_rate_| to avoid locking. + base::TimeDelta CalculateSleepDuration( + const scoped_refptr& next_frame, + float playback_rate); + // Helper function that flushes the buffers when a Stop() or error occurs. void DoStopOrError_Locked(); @@ -177,6 +186,8 @@ class MEDIA_EXPORT VideoRendererImpl bool drop_frames_; + float playback_rate_; + BufferingState buffering_state_; // Playback operation callbacks. diff --git a/media/filters/video_renderer_impl_unittest.cc b/media/filters/video_renderer_impl_unittest.cc index efa45fb1ab52..c7eb556bc717 100644 --- a/media/filters/video_renderer_impl_unittest.cc +++ b/media/filters/video_renderer_impl_unittest.cc @@ -27,6 +27,7 @@ using ::testing::_; using ::testing::AnyNumber; using ::testing::AtLeast; +using ::testing::InSequence; using ::testing::Invoke; using ::testing::NiceMock; using ::testing::NotNull; @@ -94,6 +95,11 @@ class VideoRendererImplTest : public ::testing::Test { EXPECT_CALL(*decoder_, Reset(_)) .WillRepeatedly(Invoke(this, &VideoRendererImplTest::FlushRequested)); + InSequence s; + + // Set playback rate before anything else happens. + renderer_->SetPlaybackRate(1.0f); + // Initialize, we shouldn't have any reads. InitializeRenderer(PIPELINE_OK, low_delay); } @@ -543,6 +549,7 @@ TEST_F(VideoRendererImplTest, StopDuringOutstandingRead) { } TEST_F(VideoRendererImplTest, VideoDecoder_InitFailure) { + InSequence s; InitializeRenderer(DECODER_ERROR_NOT_SUPPORTED, false); Stop(); } -- 2.11.4.GIT