Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(257)

Issue 557333002: Revert of Switch to using media::TimeSource inside media::RendererImpl. (Closed)

Created:
6 years, 3 months ago by earthdok
Modified:
6 years, 3 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Revert of Switch to using media::TimeSource inside media::RendererImpl. (patchset #3 id:120001 of https://codereview.chromium.org/534073002/) Reason for revert: ThreadSanitizer reports data races (see issue). BUG=412764 Original issue's description: > Switch to using media::TimeSource inside media::RendererImpl. > > This finally replaces the time-updating-callback inside of AudioRenderer > and VideoRenderer with a model where clients can query the time. As a > result, many other bits of cleanup are now possible: > - Renderers no longer need to be aware of the media duration > - TimeDeltaInterpolator can be replaced with WallClockTimeSource > - We can separate time values used for syncing video vs. informational > > BUG=370634 > > Committed: https://crrev.com/ece894570c5f32f7f404f4c8da9ff8b9f03b75cd > Cr-Commit-Position: refs/heads/master@{#294029} TBR=xhwang@chromium.org,scherkus@chromium.org NOTREECHECKS=true NOTRY=true BUG=370634

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+515 lines, -220 lines) Patch
M media/base/audio_renderer.h View 3 chunks +7 lines, -0 lines 0 comments Download
M media/base/mock_filters.h View 3 chunks +28 lines, -25 lines 0 comments Download
M media/base/pipeline.cc View 1 chunk +2 lines, -1 line 0 comments Download
M media/base/pipeline_unittest.cc View 3 chunks +4 lines, -4 lines 0 comments Download
M media/base/renderer.h View 1 chunk +2 lines, -1 line 0 comments Download
M media/base/time_source.h View 1 chunk +0 lines, -16 lines 0 comments Download
M media/base/video_renderer.h View 3 chunks +11 lines, -1 line 0 comments Download
M media/base/wall_clock_time_source.h View 1 chunk +0 lines, -1 line 0 comments Download
M media/base/wall_clock_time_source.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M media/filters/audio_renderer_impl.h View 3 chunks +3 lines, -10 lines 0 comments Download
M media/filters/audio_renderer_impl.cc View 8 chunks +30 lines, -23 lines 0 comments Download
M media/filters/audio_renderer_impl_unittest.cc View 5 chunks +39 lines, -18 lines 0 comments Download
M media/filters/pipeline_integration_test_base.cc View 2 chunks +5 lines, -2 lines 0 comments Download
M media/filters/renderer_impl.h View 9 chunks +45 lines, -9 lines 0 comments Download
M media/filters/renderer_impl.cc View 18 chunks +146 lines, -89 lines 0 comments Download
M media/filters/renderer_impl_unittest.cc View 7 chunks +108 lines, -13 lines 0 comments Download
M media/filters/video_renderer_impl.h View 2 chunks +5 lines, -1 line 0 comments Download
M media/filters/video_renderer_impl.cc View 4 chunks +38 lines, -1 line 0 comments Download
M media/filters/video_renderer_impl_unittest.cc View 6 chunks +42 lines, -1 line 0 comments Download

Messages

Total messages: 4 (1 generated)
earthdok
Created Revert of Switch to using media::TimeSource inside media::RendererImpl.
6 years, 3 months ago (2014-09-10 13:46:54 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/earthdok@chromium.org/557333002/1
6 years, 3 months ago (2014-09-10 13:48:17 UTC) #2
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 13:51:10 UTC) #4
Failed to apply patch for media/filters/renderer_impl.cc:
While running git apply --index -p1;
  error: patch failed: media/filters/renderer_impl.cc:12
  error: media/filters/renderer_impl.cc: patch does not apply

Patch:       media/filters/renderer_impl.cc
Index: media/filters/renderer_impl.cc
diff --git a/media/filters/renderer_impl.cc b/media/filters/renderer_impl.cc
index
f3cbc02f147694832a31e187a8211417c089de88..1e5cb18048d869a00340df0aded69d8aab232547
100644
--- a/media/filters/renderer_impl.cc
+++ b/media/filters/renderer_impl.cc
@@ -12,9 +12,9 @@
 #include "base/single_thread_task_runner.h"
 #include "media/base/audio_renderer.h"
 #include "media/base/demuxer.h"
+#include "media/base/time_delta_interpolator.h"
 #include "media/base/time_source.h"
 #include "media/base/video_renderer.h"
-#include "media/base/wall_clock_time_source.h"
 
 namespace media {
 
@@ -29,26 +29,25 @@
       audio_renderer_(audio_renderer.Pass()),
       video_renderer_(video_renderer.Pass()),
       time_source_(NULL),
-      time_ticking_(false),
       audio_buffering_state_(BUFFERING_HAVE_NOTHING),
       video_buffering_state_(BUFFERING_HAVE_NOTHING),
       audio_ended_(false),
       video_ended_(false),
       underflow_disabled_for_testing_(false),
-      clockless_video_playback_enabled_for_testing_(false),
+      interpolator_(new TimeDeltaInterpolator(&default_tick_clock_)),
+      interpolation_state_(INTERPOLATION_STOPPED),
       weak_factory_(this),
       weak_this_(weak_factory_.GetWeakPtr()) {
   DVLOG(1) << __FUNCTION__;
+  interpolator_->SetBounds(base::TimeDelta(), base::TimeDelta());
 }
 
 RendererImpl::~RendererImpl() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  // Tear down in opposite order of construction as |video_renderer_| can still
-  // need |time_source_| (which can be |audio_renderer_|) to be alive.
+  audio_renderer_.reset();
   video_renderer_.reset();
-  audio_renderer_.reset();
 
   FireAllPendingCallbacks();
 }
@@ -57,7 +56,8 @@
                               const StatisticsCB& statistics_cb,
                               const base::Closure& ended_cb,
                               const PipelineStatusCB& error_cb,
-                              const BufferingStateCB& buffering_state_cb) {
+                              const BufferingStateCB& buffering_state_cb,
+                              const TimeDeltaCB& get_duration_cb) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_UNINITIALIZED) << state_;
@@ -66,6 +66,7 @@
   DCHECK(!ended_cb.is_null());
   DCHECK(!error_cb.is_null());
   DCHECK(!buffering_state_cb.is_null());
+  DCHECK(!get_duration_cb.is_null());
   DCHECK(demuxer_->GetStream(DemuxerStream::AUDIO) ||
          demuxer_->GetStream(DemuxerStream::VIDEO));
 
@@ -73,6 +74,7 @@
   ended_cb_ = ended_cb;
   error_cb_ = error_cb;
   buffering_state_cb_ = buffering_state_cb;
+  get_duration_cb_ = get_duration_cb;
 
   init_cb_ = init_cb;
   state_ = STATE_INITIALIZING;
@@ -85,12 +87,13 @@
   DCHECK_EQ(state_, STATE_PLAYING) << state_;
   DCHECK(flush_cb_.is_null());
 
+  {
+    base::AutoLock auto_lock(interpolator_lock_);
+    PauseClockAndStopTicking_Locked();
+  }
+
   flush_cb_ = flush_cb;
   state_ = STATE_FLUSHING;
-
-  if (time_ticking_)
-    PausePlayback();
-
   FlushAudioRenderer();
 }
 
@@ -99,8 +102,13 @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_PLAYING) << state_;
 
-  time_source_->SetMediaTime(time);
-
+  {
+    base::AutoLock auto_lock(interpolator_lock_);
+    interpolator_->SetBounds(time, time);
+  }
+
+  if (time_source_)
+    time_source_->SetMediaTime(time);
   if (audio_renderer_)
     audio_renderer_->StartPlaying();
   if (video_renderer_)
@@ -115,7 +123,13 @@
   if (state_ != STATE_PLAYING)
     return;
 
-  time_source_->SetPlaybackRate(playback_rate);
+  {
+    base::AutoLock auto_lock(interpolator_lock_);
+    interpolator_->SetPlaybackRate(playback_rate);
+  }
+
+  if (time_source_)
+    time_source_->SetPlaybackRate(playback_rate);
 }
 
 void RendererImpl::SetVolume(float volume) {
@@ -129,7 +143,8 @@
 base::TimeDelta RendererImpl::GetMediaTime() {
   // No BelongsToCurrentThread() checking because this can be called from other
   // threads.
-  return time_source_->CurrentMediaTime();
+  base::AutoLock auto_lock(interpolator_lock_);
+  return interpolator_->GetInterpolatedTime();
 }
 
 bool RendererImpl::HasAudio() {
@@ -158,26 +173,18 @@
   underflow_disabled_for_testing_ = true;
 }
 
-void RendererImpl::EnableClocklessVideoPlaybackForTesting() {
+void RendererImpl::SetTimeDeltaInterpolatorForTesting(
+    TimeDeltaInterpolator* interpolator) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_UNINITIALIZED);
-  DCHECK(underflow_disabled_for_testing_)
-      << "Underflow must be disabled for clockless video playback";
-
-  clockless_video_playback_enabled_for_testing_ = true;
-}
-
-base::TimeDelta RendererImpl::GetMediaTimeForSyncingVideo() {
-  // No BelongsToCurrentThread() checking because this can be called from other
-  // threads.
-  //
-  // TODO(scherkus): Currently called from VideoRendererImpl's internal thread,
-  // which should go away at some point http://crbug.com/110814
-  if (clockless_video_playback_enabled_for_testing_)
-    return base::TimeDelta::Max();
-
-  return time_source_->CurrentMediaTimeForSyncingVideo();
+
+  interpolator_.reset(interpolator);
+}
+
+base::TimeDelta RendererImpl::GetMediaDuration() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  return get_duration_cb_.Run();
 }
 
 void RendererImpl::InitializeAudioRenderer() {
@@ -199,6 +206,7 @@
       demuxer_->GetStream(DemuxerStream::AUDIO),
       done_cb,
       base::Bind(&RendererImpl::OnUpdateStatistics, weak_this_),
+      base::Bind(&RendererImpl::OnAudioTimeUpdate, weak_this_),
       base::Bind(&RendererImpl::OnBufferingStateChanged, weak_this_,
                  &audio_buffering_state_),
       base::Bind(&RendererImpl::OnAudioRendererEnded, weak_this_),
@@ -216,6 +224,9 @@
     OnError(status);
     return;
   }
+
+  if (audio_renderer_)
+    time_source_ = audio_renderer_->GetTimeSource();
 
   InitializeVideoRenderer();
 }
@@ -240,13 +251,13 @@
       demuxer_->GetLiveness() == Demuxer::LIVENESS_LIVE,
       done_cb,
       base::Bind(&RendererImpl::OnUpdateStatistics, weak_this_),
-      base::Bind(&RendererImpl::OnBufferingStateChanged,
-                 weak_this_,
+      base::Bind(&RendererImpl::OnVideoTimeUpdate, weak_this_),
+      base::Bind(&RendererImpl::OnBufferingStateChanged, weak_this_,
                  &video_buffering_state_),
       base::Bind(&RendererImpl::OnVideoRendererEnded, weak_this_),
       base::Bind(&RendererImpl::OnError, weak_this_),
-      base::Bind(&RendererImpl::GetMediaTimeForSyncingVideo,
-                 base::Unretained(this)));
+      base::Bind(&RendererImpl::GetMediaTime, base::Unretained(this)),
+      base::Bind(&RendererImpl::GetMediaDuration, base::Unretained(this)));
 }
 
 void RendererImpl::OnVideoRendererInitializeDone(PipelineStatus status) {
@@ -262,15 +273,7 @@
     return;
   }
 
-  if (audio_renderer_) {
-    time_source_ = audio_renderer_->GetTimeSource();
-  } else {
-    wall_clock_time_source_.reset(new WallClockTimeSource());
-    time_source_ = wall_clock_time_source_.get();
-  }
-
   state_ = STATE_PLAYING;
-  DCHECK(time_source_);
   DCHECK(audio_renderer_ || video_renderer_);
   base::ResetAndReturn(&init_cb_).Run();
 }
@@ -338,6 +341,42 @@
   video_ended_ = false;
   state_ = STATE_PLAYING;
   base::ResetAndReturn(&flush_cb_).Run();
+}
+
+void RendererImpl::OnAudioTimeUpdate(base::TimeDelta time,
+                                     base::TimeDelta max_time) {
+  DVLOG(2) << __FUNCTION__ << "(" << time.InMilliseconds()
+           << ", " << max_time.InMilliseconds() << ")";
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK_LE(time.InMicroseconds(), max_time.InMicroseconds());
+
+  base::AutoLock auto_lock(interpolator_lock_);
+
+  if (interpolation_state_ == INTERPOLATION_WAITING_FOR_AUDIO_TIME_UPDATE &&
+      time < interpolator_->GetInterpolatedTime()) {
+    return;
+  }
+
+  if (state_ == STATE_FLUSHING)
+    return;
+
+  interpolator_->SetBounds(time, max_time);
+  StartClockIfWaitingForTimeUpdate_Locked();
+}
+
+void RendererImpl::OnVideoTimeUpdate(base::TimeDelta max_time) {
+  DVLOG(2) << __FUNCTION__ << "(" << max_time.InMilliseconds() << ")";
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  if (audio_renderer_)
+    return;
+
+  if (state_ == STATE_FLUSHING)
+    return;
+
+  base::AutoLock auto_lock(interpolator_lock_);
+  DCHECK_NE(interpolation_state_, INTERPOLATION_WAITING_FOR_AUDIO_TIME_UPDATE);
+  interpolator_->SetUpperBound(max_time);
 }
 
 void RendererImpl::OnUpdateStatistics(const PipelineStatistics& stats) {
@@ -357,7 +396,7 @@
 
   // Disable underflow by ignoring updates that renderers have ran out of data.
   if (state_ == STATE_PLAYING && underflow_disabled_for_testing_ &&
-      time_ticking_) {
+      interpolation_state_ != INTERPOLATION_STOPPED) {
     DVLOG(1) << "Update ignored because underflow is disabled for testing.";
     return;
   }
@@ -393,37 +432,63 @@
 void RendererImpl::PausePlayback() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(time_ticking_);
-  switch (state_) {
-    case STATE_PLAYING:
-      DCHECK(PlaybackHasEnded() || WaitingForEnoughData())
-          << "Playback should only pause due to ending or underflowing";
+  DCHECK_EQ(state_, STATE_PLAYING);
+  DCHECK(WaitingForEnoughData());
+
+  base::AutoLock auto_lock(interpolator_lock_);
+  PauseClockAndStopTicking_Locked();
+}
+
+void RendererImpl::StartPlayback() {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, STATE_PLAYING);
+  DCHECK_EQ(int…
(message too large)

Powered by Google App Engine
This is Rietveld 408576698