Commit dfb28d8e authored by falken@chromium.org's avatar falken@chromium.org

Revert of Remove media::VideoRenderer::SetPlaybackRate()....

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
parent ba28fc47
...@@ -130,6 +130,7 @@ class MockVideoRenderer : public VideoRenderer { ...@@ -130,6 +130,7 @@ class MockVideoRenderer : public VideoRenderer {
MOCK_METHOD1(Flush, void(const base::Closure& callback)); MOCK_METHOD1(Flush, void(const base::Closure& callback));
MOCK_METHOD1(StartPlayingFrom, void(base::TimeDelta timestamp)); MOCK_METHOD1(StartPlayingFrom, void(base::TimeDelta timestamp));
MOCK_METHOD1(Stop, void(const base::Closure& callback)); MOCK_METHOD1(Stop, void(const base::Closure& callback));
MOCK_METHOD1(SetPlaybackRate, void(float playback_rate));
private: private:
DISALLOW_COPY_AND_ASSIGN(MockVideoRenderer); DISALLOW_COPY_AND_ASSIGN(MockVideoRenderer);
......
...@@ -649,6 +649,8 @@ void Pipeline::PlaybackRateChangedTask(float playback_rate) { ...@@ -649,6 +649,8 @@ void Pipeline::PlaybackRateChangedTask(float playback_rate) {
if (audio_renderer_) if (audio_renderer_)
audio_renderer_->SetPlaybackRate(playback_rate_); audio_renderer_->SetPlaybackRate(playback_rate_);
if (video_renderer_)
video_renderer_->SetPlaybackRate(playback_rate_);
} }
void Pipeline::VolumeChangedTask(float volume) { void Pipeline::VolumeChangedTask(float volume) {
......
...@@ -210,6 +210,7 @@ class PipelineTest : public ::testing::Test { ...@@ -210,6 +210,7 @@ class PipelineTest : public ::testing::Test {
} }
if (video_stream_) { if (video_stream_) {
EXPECT_CALL(*video_renderer_, SetPlaybackRate(0.0f));
EXPECT_CALL(*video_renderer_, StartPlayingFrom(base::TimeDelta())) EXPECT_CALL(*video_renderer_, StartPlayingFrom(base::TimeDelta()))
.WillOnce(SetBufferingState(&video_buffering_state_cb_, .WillOnce(SetBufferingState(&video_buffering_state_cb_,
BUFFERING_HAVE_ENOUGH)); BUFFERING_HAVE_ENOUGH));
...@@ -286,6 +287,7 @@ class PipelineTest : public ::testing::Test { ...@@ -286,6 +287,7 @@ class PipelineTest : public ::testing::Test {
EXPECT_CALL(*video_renderer_, StartPlayingFrom(seek_time)) EXPECT_CALL(*video_renderer_, StartPlayingFrom(seek_time))
.WillOnce(SetBufferingState(&video_buffering_state_cb_, .WillOnce(SetBufferingState(&video_buffering_state_cb_,
BUFFERING_HAVE_ENOUGH)); BUFFERING_HAVE_ENOUGH));
EXPECT_CALL(*video_renderer_, SetPlaybackRate(_));
} }
// We expect a successful seek callback followed by a buffering update. // We expect a successful seek callback followed by a buffering update.
...@@ -633,6 +635,7 @@ TEST_F(PipelineTest, AudioStreamShorterThanVideo) { ...@@ -633,6 +635,7 @@ TEST_F(PipelineTest, AudioStreamShorterThanVideo) {
EXPECT_EQ(0, pipeline_->GetMediaTime().ToInternalValue()); EXPECT_EQ(0, pipeline_->GetMediaTime().ToInternalValue());
float playback_rate = 1.0f; float playback_rate = 1.0f;
EXPECT_CALL(*video_renderer_, SetPlaybackRate(playback_rate));
EXPECT_CALL(*audio_renderer_, SetPlaybackRate(playback_rate)); EXPECT_CALL(*audio_renderer_, SetPlaybackRate(playback_rate));
pipeline_->SetPlaybackRate(playback_rate); pipeline_->SetPlaybackRate(playback_rate);
message_loop_.RunUntilIdle(); message_loop_.RunUntilIdle();
...@@ -1001,6 +1004,7 @@ class PipelineTeardownTest : public PipelineTest { ...@@ -1001,6 +1004,7 @@ class PipelineTeardownTest : public PipelineTest {
BUFFERING_HAVE_ENOUGH)); BUFFERING_HAVE_ENOUGH));
EXPECT_CALL(*audio_renderer_, SetPlaybackRate(0.0f)); 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_, SetVolume(1.0f));
EXPECT_CALL(*audio_renderer_, StartRendering()); EXPECT_CALL(*audio_renderer_, StartRendering());
......
...@@ -75,6 +75,9 @@ class MEDIA_EXPORT VideoRenderer { ...@@ -75,6 +75,9 @@ class MEDIA_EXPORT VideoRenderer {
// when complete. // when complete.
virtual void Stop(const base::Closure& callback) = 0; virtual void Stop(const base::Closure& callback) = 0;
// Updates the current playback rate.
virtual void SetPlaybackRate(float playback_rate) = 0;
private: private:
DISALLOW_COPY_AND_ASSIGN(VideoRenderer); DISALLOW_COPY_AND_ASSIGN(VideoRenderer);
}; };
......
...@@ -34,6 +34,7 @@ VideoRendererImpl::VideoRendererImpl( ...@@ -34,6 +34,7 @@ VideoRendererImpl::VideoRendererImpl(
thread_(), thread_(),
pending_read_(false), pending_read_(false),
drop_frames_(drop_frames), drop_frames_(drop_frames),
playback_rate_(0),
buffering_state_(BUFFERING_HAVE_NOTHING), buffering_state_(BUFFERING_HAVE_NOTHING),
paint_cb_(paint_cb), paint_cb_(paint_cb),
last_timestamp_(kNoTimestamp()), last_timestamp_(kNoTimestamp()),
...@@ -105,6 +106,12 @@ void VideoRendererImpl::Stop(const base::Closure& callback) { ...@@ -105,6 +106,12 @@ void VideoRendererImpl::Stop(const base::Closure& callback) {
video_frame_stream_.Stop(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) { void VideoRendererImpl::StartPlayingFrom(base::TimeDelta timestamp) {
DCHECK(task_runner_->BelongsToCurrentThread()); DCHECK(task_runner_->BelongsToCurrentThread());
base::AutoLock auto_lock(lock_); base::AutoLock auto_lock(lock_);
...@@ -214,7 +221,8 @@ void VideoRendererImpl::ThreadMain() { ...@@ -214,7 +221,8 @@ void VideoRendererImpl::ThreadMain() {
return; return;
// Remain idle as long as we're not playing. // 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); UpdateStatsAndWait_Locked(kIdleTimeDelta);
continue; continue;
} }
...@@ -230,10 +238,16 @@ void VideoRendererImpl::ThreadMain() { ...@@ -230,10 +238,16 @@ void VideoRendererImpl::ThreadMain() {
continue; continue;
} }
base::TimeDelta now = get_time_cb_.Run(); base::TimeDelta remaining_time =
base::TimeDelta target_timestamp = ready_frames_.front()->timestamp(); CalculateSleepDuration(ready_frames_.front(), playback_rate_);
base::TimeDelta earliest_paint_timestamp;
base::TimeDelta latest_paint_timestamp; // 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 // 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 // frame, using the delta between this frame and the previous frame as the
...@@ -246,24 +260,15 @@ void VideoRendererImpl::ThreadMain() { ...@@ -246,24 +260,15 @@ void VideoRendererImpl::ThreadMain() {
// //
// TODO(scherkus): This can be vastly improved. Use a histogram to measure // TODO(scherkus): This can be vastly improved. Use a histogram to measure
// the accuracy of our frame timing code. http://crbug.com/149829 // the accuracy of our frame timing code. http://crbug.com/149829
if (last_timestamp_ == kNoTimestamp()) { if (drop_frames_ && last_timestamp_ != kNoTimestamp()) {
earliest_paint_timestamp = target_timestamp; base::TimeDelta now = get_time_cb_.Run();
latest_paint_timestamp = base::TimeDelta::Max(); base::TimeDelta deadline = ready_frames_.front()->timestamp() +
} else { (ready_frames_.front()->timestamp() - last_timestamp_) / 2;
base::TimeDelta duration = target_timestamp - last_timestamp_;
earliest_paint_timestamp = target_timestamp - duration / 2; if (now > deadline) {
latest_paint_timestamp = target_timestamp + duration / 2; DropNextReadyFrame_Locked();
} continue;
}
// 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;
} }
// Congratulations! You've made it past the video frame timing gauntlet. // Congratulations! You've made it past the video frame timing gauntlet.
...@@ -472,6 +477,19 @@ void VideoRendererImpl::OnVideoFrameStreamResetDone() { ...@@ -472,6 +477,19 @@ void VideoRendererImpl::OnVideoFrameStreamResetDone() {
base::ResetAndReturn(&flush_cb_).Run(); base::ResetAndReturn(&flush_cb_).Run();
} }
base::TimeDelta VideoRendererImpl::CalculateSleepDuration(
const scoped_refptr<VideoFrame>& 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<int64>(sleep.InMicroseconds() / playback_rate));
}
void VideoRendererImpl::DoStopOrError_Locked() { void VideoRendererImpl::DoStopOrError_Locked() {
lock_.AssertAcquired(); lock_.AssertAcquired();
last_timestamp_ = kNoTimestamp(); last_timestamp_ = kNoTimestamp();
......
...@@ -69,6 +69,7 @@ class MEDIA_EXPORT VideoRendererImpl ...@@ -69,6 +69,7 @@ class MEDIA_EXPORT VideoRendererImpl
virtual void Flush(const base::Closure& callback) OVERRIDE; virtual void Flush(const base::Closure& callback) OVERRIDE;
virtual void StartPlayingFrom(base::TimeDelta timestamp) OVERRIDE; virtual void StartPlayingFrom(base::TimeDelta timestamp) OVERRIDE;
virtual void Stop(const base::Closure& callback) OVERRIDE; virtual void Stop(const base::Closure& callback) OVERRIDE;
virtual void SetPlaybackRate(float playback_rate) OVERRIDE;
// PlatformThread::Delegate implementation. // PlatformThread::Delegate implementation.
virtual void ThreadMain() OVERRIDE; virtual void ThreadMain() OVERRIDE;
...@@ -94,6 +95,14 @@ class MEDIA_EXPORT VideoRendererImpl ...@@ -94,6 +95,14 @@ class MEDIA_EXPORT VideoRendererImpl
// Called when VideoFrameStream::Reset() completes. // Called when VideoFrameStream::Reset() completes.
void OnVideoFrameStreamResetDone(); 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<VideoFrame>& next_frame,
float playback_rate);
// Helper function that flushes the buffers when a Stop() or error occurs. // Helper function that flushes the buffers when a Stop() or error occurs.
void DoStopOrError_Locked(); void DoStopOrError_Locked();
...@@ -177,6 +186,8 @@ class MEDIA_EXPORT VideoRendererImpl ...@@ -177,6 +186,8 @@ class MEDIA_EXPORT VideoRendererImpl
bool drop_frames_; bool drop_frames_;
float playback_rate_;
BufferingState buffering_state_; BufferingState buffering_state_;
// Playback operation callbacks. // Playback operation callbacks.
......
...@@ -27,6 +27,7 @@ ...@@ -27,6 +27,7 @@
using ::testing::_; using ::testing::_;
using ::testing::AnyNumber; using ::testing::AnyNumber;
using ::testing::AtLeast; using ::testing::AtLeast;
using ::testing::InSequence;
using ::testing::Invoke; using ::testing::Invoke;
using ::testing::NiceMock; using ::testing::NiceMock;
using ::testing::NotNull; using ::testing::NotNull;
...@@ -94,6 +95,11 @@ class VideoRendererImplTest : public ::testing::Test { ...@@ -94,6 +95,11 @@ class VideoRendererImplTest : public ::testing::Test {
EXPECT_CALL(*decoder_, Reset(_)) EXPECT_CALL(*decoder_, Reset(_))
.WillRepeatedly(Invoke(this, &VideoRendererImplTest::FlushRequested)); .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. // Initialize, we shouldn't have any reads.
InitializeRenderer(PIPELINE_OK, low_delay); InitializeRenderer(PIPELINE_OK, low_delay);
} }
...@@ -543,6 +549,7 @@ TEST_F(VideoRendererImplTest, StopDuringOutstandingRead) { ...@@ -543,6 +549,7 @@ TEST_F(VideoRendererImplTest, StopDuringOutstandingRead) {
} }
TEST_F(VideoRendererImplTest, VideoDecoder_InitFailure) { TEST_F(VideoRendererImplTest, VideoDecoder_InitFailure) {
InSequence s;
InitializeRenderer(DECODER_ERROR_NOT_SUPPORTED, false); InitializeRenderer(DECODER_ERROR_NOT_SUPPORTED, false);
Stop(); Stop();
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment