Avoid flooding main thread by caching state on media thread.

media/base/pipeline_impl.cc

Index: media/base/pipeline_impl.cc
diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index c5cb4841dc039970d0f8bb3cb8d1c587bcf99b69..1eada2e7a44f282d28fcccd8ed3b109bdfdc5d33 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -51,10 +51,46 @@ class PipelineImpl::RendererWrapper : public DemuxerHost,
   void Resume(std::unique_ptr<Renderer> renderer, base::TimeDelta time);
   void SetPlaybackRate(double playback_rate);
   void SetVolume(float volume);
-  base::TimeDelta GetMediaTime();
+  base::TimeDelta GetMediaTime() const;
+  Ranges<base::TimeDelta> GetBufferedTimeRanges() const;
+  bool DidLoadingProgress();
+  PipelineStatistics GetStatistics() const;
   void SetCdm(CdmContext* cdm_context, const CdmAttachedCB& cdm_attached_cb);
 
  private:
+  // Contains state shared between main and media thread.
+  // Main thread can only read. Media thread can both - read and write.
+  // So it is not necessary to lock when reading from the media thread.
+  // This struct should only contain state that is not immediately needed by
+  // PipelineClient and can be cached on the media thread until queried.
+  // Alternatively we could cache it on the main thread by posting the
+  // notification to the main thread. But some of the state change notifications
+  // (OnStatisticsUpdate and OnBufferedTimeRangesChanged) arrive much more
+  // frequently than needed. Posting all those notifications to the main thread
+  // causes performance issues: crbug.com/619975.
+  struct SharedState {
+    SharedState()

[DaleCurtis, 2016/06/22 15:21:15]

Just use = initialization.

[alokp, 2016/06/22 16:27:54]

Done.

+        : did_loading_progress(false), suspend_timestamp(kNoTimestamp()) {}
+
+    // TODO(scherkus): Enforce that Renderer is only called on a single thread,
+    // even for accessing media time http://crbug.com/370634
+    std::unique_ptr<Renderer> renderer;
+
+    // True when OnBufferedTimeRangesChanged() has been called more recently
+    // than DidLoadingProgress().
+    bool did_loading_progress;
+
+    // Amount of available buffered data as reported by Demuxer.
+    Ranges<base::TimeDelta> buffered_time_ranges;
+
+    // Accumulated statistics reported by the renderer.
+    PipelineStatistics statistics;
+
+    // The media timestamp to return while the pipeline is suspended.
+    // Otherwise set to kNoTimestamp().
+    base::TimeDelta suspend_timestamp;
+  };
+
   // DemuxerHost implementaion.
   void OnBufferedTimeRangesChanged(const Ranges<base::TimeDelta>& ranges) final;
   void SetDuration(base::TimeDelta duration) final;
@@ -88,6 +124,7 @@ class PipelineImpl::RendererWrapper : public DemuxerHost,
   void RemoveTextStreamTask(DemuxerStream* text_stream);
   void InitializeDemuxer(const PipelineStatusCB& done_cb);
   void InitializeRenderer(const PipelineStatusCB& done_cb);
+  void DestroyRenderer();
   void ReportMetadata();
 
   base::WeakPtr<PipelineImpl> weak_pipeline_;
@@ -96,16 +133,16 @@ class PipelineImpl::RendererWrapper : public DemuxerHost,
   const scoped_refptr<MediaLog> media_log_;
 
   Demuxer* demuxer_;
-  std::unique_ptr<Renderer> renderer_;
   std::unique_ptr<TextRenderer> text_renderer_;
   double playback_rate_;
   float volume_;
   CdmContext* cdm_context_;
 
-  // Lock used to serialize members - |renderer_| and |suspend_timestamp_| -
-  // accessed in GetMediaTime(), which is the only member function called on
-  // the main thread.
-  mutable base::Lock media_time_lock_;
+  // Lock used to serialize |shared_state_|.
+  mutable base::Lock shared_state_lock_;
+
+  // State shared between main and media thread.
+  SharedState shared_state_;
 
   // Current state of the pipeline.
   State state_;
@@ -120,10 +157,6 @@ class PipelineImpl::RendererWrapper : public DemuxerHost,
   // completed.
   base::TimeDelta start_timestamp_;
 
-  // The media timestamp to return while the pipeline is suspended.
-  // Otherwise set to kNoTimestamp().
-  base::TimeDelta suspend_timestamp_;
-
   // Whether we've received the audio/video/text ended events.
   bool renderer_ended_;
   bool text_renderer_ended_;
@@ -144,7 +177,6 @@ PipelineImpl::PipelineImpl(
       client_(nullptr),
       playback_rate_(kDefaultPlaybackRate),
       volume_(kDefaultVolume),
-      did_loading_progress_(false),
       weak_factory_(this) {
   DVLOG(2) << __FUNCTION__;
   renderer_wrapper_.reset(new RendererWrapper(weak_factory_.GetWeakPtr(),
@@ -158,13 +190,6 @@ PipelineImpl::~PipelineImpl() {
   DCHECK(seek_cb_.is_null());
   DCHECK(suspend_cb_.is_null());
 
-  // Invalidate self weak pointers effectively canceling all pending
-  // notifications in the message queue.
-  // Do this before posting the DeleteSoon task to the media thread to
-  // guarantee that the WeakPtr<PipelineImpl> passed to the RendererWrapper
-  // is not invalidated on the media thread.
-  weak_factory_.InvalidateWeakPtrs();
-
   // RendererWrapper is deleted on the media thread.
   media_task_runner_->DeleteSoon(FROM_HERE, renderer_wrapper_.release());
 }
@@ -210,10 +235,14 @@ void PipelineImpl::Stop() {
     return;
   }
 
-  if (statistics_.video_frames_decoded > 0) {
-    UMA_HISTOGRAM_COUNTS("Media.DroppedFrameCount",
-                         statistics_.video_frames_dropped);
-  }
+  // Once the pipeline is stopped, nothing is reported back to the client.
+  // Reset all callbacks and client handle.
+  // Invalidate self weak pointers effectively canceling all pending
+  // notifications in the message queue.
+  weak_factory_.InvalidateWeakPtrs();

[DaleCurtis, 2016/06/22 15:21:15]

Should be okay, we call Stop() and destruction on the same thread IIRC. Why did
you move this? I think this should go after the else clause (where it is
currently) otherwise the posted task will be triggered before.

[alokp, 2016/06/22 16:27:54]

It does not matter because we use WaitableEvent here. I find it cleaner to
shutdown all notifications from the media thread before posting the stop task to
the media thread (note that stop_cb is not posted back to the main thread; it
gets Run on the media thread).

I did not understand your "otherwise the posted task will be triggered before"
comment. Which posted task did you mean?

[DaleCurtis, 2016/06/22 20:40:39]

I just meant that you're now invalidating WeakPtrs to tasks which may not have
completed yet. You're also resetting these values before stop completes. This
seems like a risky change.

[alokp, 2016/06/22 20:52:03]

And those tasks would not complete anyways. We start waiting here after posting
the stop task during which time the main thread will be suspended and none of
the tasks posted on the main thread will run. After stop task completes, we
reset the WeakPtr and callbacks. So it is functionally equivalent (except for
unittests where we do not wait).

That said this change is not needed. I will revert.

+  seek_cb_.Reset();
+  suspend_cb_.Reset();
+  client_ = nullptr;
 
   if (media_task_runner_->BelongsToCurrentThread()) {
     // This path is executed by unittests that share media and main threads.
@@ -245,12 +274,6 @@ void PipelineImpl::Stop() {
                    base::Unretained(renderer_wrapper_.get()), stop_cb)));
     waiter.Wait();
   }
-
-  // Once the pipeline is stopped, nothing is reported back to the client.
-  // Reset all callbacks and client handle.
-  seek_cb_.Reset();
-  suspend_cb_.Reset();
-  client_ = nullptr;
 }
 
 void PipelineImpl::Seek(base::TimeDelta time, const PipelineStatusCB& seek_cb) {
@@ -351,7 +374,7 @@ base::TimeDelta PipelineImpl::GetMediaTime() const {
 
 Ranges<base::TimeDelta> PipelineImpl::GetBufferedTimeRanges() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return buffered_time_ranges_;
+  return renderer_wrapper_->GetBufferedTimeRanges();
 }
 
 base::TimeDelta PipelineImpl::GetMediaDuration() const {
@@ -361,14 +384,12 @@ base::TimeDelta PipelineImpl::GetMediaDuration() const {
 
 bool PipelineImpl::DidLoadingProgress() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  bool ret = did_loading_progress_;
-  did_loading_progress_ = false;
-  return ret;
+  return renderer_wrapper_->DidLoadingProgress();
 }
 
 PipelineStatistics PipelineImpl::GetStatistics() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return statistics_;
+  return renderer_wrapper_->GetStatistics();
 }
 
 void PipelineImpl::SetCdm(CdmContext* cdm_context,
@@ -501,11 +522,16 @@ void PipelineImpl::RendererWrapper::OnEnded() {
 
 void PipelineImpl::RendererWrapper::OnStatisticsUpdate(
     const PipelineStatistics& stats) {
+  DVLOG(3) << __FUNCTION__;
   DCHECK(media_task_runner_->BelongsToCurrentThread());
 
-  main_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&PipelineImpl::OnStatisticsUpdate, weak_pipeline_, stats));
+  base::AutoLock auto_lock(shared_state_lock_);
+  shared_state_.statistics.audio_bytes_decoded += stats.audio_bytes_decoded;
+  shared_state_.statistics.video_bytes_decoded += stats.video_bytes_decoded;
+  shared_state_.statistics.video_frames_decoded += stats.video_frames_decoded;
+  shared_state_.statistics.video_frames_dropped += stats.video_frames_dropped;
+  shared_state_.statistics.audio_memory_usage += stats.audio_memory_usage;
+  shared_state_.statistics.video_memory_usage += stats.video_memory_usage;
 }
 
 void PipelineImpl::RendererWrapper::OnBufferingStateChange(
@@ -597,10 +623,10 @@ void PipelineImpl::RendererWrapper::StateTransitionTask(PipelineStatus status) {
 
     case kPlaying:
       DCHECK(start_timestamp_ >= base::TimeDelta());
-      renderer_->StartPlayingFrom(start_timestamp_);
+      shared_state_.renderer->StartPlayingFrom(start_timestamp_);
       {
-        base::AutoLock auto_lock(media_time_lock_);
-        suspend_timestamp_ = kNoTimestamp();
+        base::AutoLock auto_lock(shared_state_lock_);
+        shared_state_.suspend_timestamp = kNoTimestamp();
       }
 
       if (text_renderer_)
@@ -610,15 +636,20 @@ void PipelineImpl::RendererWrapper::StateTransitionTask(PipelineStatus status) {
           FROM_HERE, base::Bind(&PipelineImpl::OnSeekDone, weak_pipeline_,
                                 start_timestamp_));
 
-      renderer_->SetPlaybackRate(playback_rate_);
-      renderer_->SetVolume(volume_);
+      shared_state_.renderer->SetPlaybackRate(playback_rate_);
+      shared_state_.renderer->SetVolume(volume_);
       return;
 
     case kSuspended:
-      renderer_.reset();
+      DestroyRenderer();
+      {
+        base::AutoLock auto_lock(shared_state_lock_);
+        shared_state_.statistics.audio_memory_usage = 0;
+        shared_state_.statistics.video_memory_usage = 0;
+      }
       main_task_runner_->PostTask(
           FROM_HERE, base::Bind(&PipelineImpl::OnSuspendDone, weak_pipeline_,
-                                suspend_timestamp_));
+                                shared_state_.suspend_timestamp));
       return;
 
     case kStopping:
@@ -651,9 +682,9 @@ void PipelineImpl::RendererWrapper::DoSeek(base::TimeDelta seek_timestamp,
   }
 
   // Flush.
-  DCHECK(renderer_);
-  bound_fns.Push(
-      base::Bind(&Renderer::Flush, base::Unretained(renderer_.get())));
+  DCHECK(shared_state_.renderer);
+  bound_fns.Push(base::Bind(&Renderer::Flush,
+                            base::Unretained(shared_state_.renderer.get())));
 
   if (text_renderer_) {
     bound_fns.Push(base::Bind(&TextRenderer::Flush,
@@ -673,14 +704,7 @@ void PipelineImpl::RendererWrapper::DoStop(const base::Closure& done_cb) {
   DCHECK_EQ(state_, kStopping);
   DCHECK(!pending_callbacks_.get());
 
-  // TODO(scherkus): Enforce that Renderer is only called on a single thread,
-  // even for accessing media time http://crbug.com/370634
-  std::unique_ptr<Renderer> renderer;
-  {
-    base::AutoLock auto_lock(media_time_lock_);
-    renderer.swap(renderer_);
-  }
-  renderer.reset();
+  DestroyRenderer();
   text_renderer_.reset();
 
   if (demuxer_) {
@@ -703,9 +727,9 @@ void PipelineImpl::RendererWrapper::OnBufferedTimeRangesChanged(
     const Ranges<base::TimeDelta>& ranges) {
   // TODO(alokp): Add thread DCHECK after ensuring that all Demuxer
   // implementations call DemuxerHost on the media thread.
-  main_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&PipelineImpl::OnBufferedTimeRangesChange,
-                            weak_pipeline_, ranges));
+  base::AutoLock auto_lock(shared_state_lock_);
+  shared_state_.did_loading_progress = true;
+  shared_state_.buffered_time_ranges = ranges;
 }
 
 void PipelineImpl::RendererWrapper::Start(
@@ -717,14 +741,14 @@ void PipelineImpl::RendererWrapper::Start(
                               << state_;
 
   DCHECK(!demuxer_);
-  DCHECK(!renderer_);
+  DCHECK(!shared_state_.renderer);
   DCHECK(!text_renderer_);
   DCHECK(!renderer_ended_);
   DCHECK(!text_renderer_ended_);
   demuxer_ = demuxer;
   {
-    base::AutoLock auto_lock(media_time_lock_);
-    renderer_ = std::move(renderer);
+    base::AutoLock auto_lock(shared_state_lock_);
+    shared_state_.renderer = std::move(renderer);
   }
   text_renderer_ = std::move(text_renderer);
   if (text_renderer_) {
@@ -741,6 +765,11 @@ void PipelineImpl::RendererWrapper::Stop(const base::Closure& stop_cb) {
 
   SetState(kStopping);
 
+  if (shared_state_.statistics.video_frames_decoded > 0) {
+    UMA_HISTOGRAM_COUNTS("Media.DroppedFrameCount",
+                         shared_state_.statistics.video_frames_dropped);
+  }
+
   // If we stop during starting/seeking/suspending/resuming we don't want to
   // leave outstanding callbacks around. The callbacks also do not get run if
   // the pipeline is stopped before it had a chance to complete outstanding
@@ -779,7 +808,7 @@ void PipelineImpl::RendererWrapper::SetPlaybackRate(double playback_rate) {
 
   playback_rate_ = playback_rate;
   if (state_ == kPlaying)
-    renderer_->SetPlaybackRate(playback_rate_);
+    shared_state_.renderer->SetPlaybackRate(playback_rate_);
 }
 
 void PipelineImpl::RendererWrapper::SetVolume(float volume) {
@@ -787,7 +816,7 @@ void PipelineImpl::RendererWrapper::SetVolume(float volume) {
 
   volume_ = volume;
   if (state_ == kPlaying)
-    renderer_->SetVolume(volume_);
+    shared_state_.renderer->SetVolume(volume_);
 }
 
 void PipelineImpl::RendererWrapper::Seek(base::TimeDelta time) {
@@ -823,18 +852,18 @@ void PipelineImpl::RendererWrapper::Suspend() {
     OnPipelineError(PIPELINE_ERROR_INVALID_STATE);
     return;
   }
-  DCHECK(renderer_);
+  DCHECK(shared_state_.renderer);
   DCHECK(!pending_callbacks_.get());
 
   SetState(kSuspending);
 
   // Freeze playback and record the media time before flushing. (Flushing clears
   // the value.)
-  renderer_->SetPlaybackRate(0.0);
+  shared_state_.renderer->SetPlaybackRate(0.0);
   {
-    base::AutoLock auto_lock(media_time_lock_);
-    suspend_timestamp_ = renderer_->GetMediaTime();
-    DCHECK(suspend_timestamp_ != kNoTimestamp());
+    base::AutoLock auto_lock(shared_state_lock_);
+    shared_state_.suspend_timestamp = shared_state_.renderer->GetMediaTime();
+    DCHECK(shared_state_.suspend_timestamp != kNoTimestamp());
   }
 
   // Queue the asynchronous actions required to stop playback. (Matches setup in
@@ -847,7 +876,8 @@ void PipelineImpl::RendererWrapper::Suspend() {
                         base::Unretained(text_renderer_.get())));
   }
 
-  fns.Push(base::Bind(&Renderer::Flush, base::Unretained(renderer_.get())));
+  fns.Push(base::Bind(&Renderer::Flush,
+                      base::Unretained(shared_state_.renderer.get())));
 
   if (text_renderer_) {
     fns.Push(base::Bind(&TextRenderer::Flush,
@@ -869,14 +899,14 @@ void PipelineImpl::RendererWrapper::Resume(std::unique_ptr<Renderer> renderer,
     OnPipelineError(PIPELINE_ERROR_INVALID_STATE);
     return;
   }
-  DCHECK(!renderer_);
+  DCHECK(!shared_state_.renderer);
   DCHECK(!pending_callbacks_.get());
 
   SetState(kResuming);
 
   {
-    base::AutoLock auto_lock(media_time_lock_);
-    renderer_ = std::move(renderer);
+    base::AutoLock auto_lock(shared_state_lock_);
+    shared_state_.renderer = std::move(renderer);
   }
 
   // Set up for a seek. (Matches setup in SeekTask().)
@@ -900,20 +930,44 @@ void PipelineImpl::RendererWrapper::Resume(std::unique_ptr<Renderer> renderer,
       fns, base::Bind(&RendererWrapper::StateTransitionTask, weak_this_));
 }
 
+Ranges<base::TimeDelta> PipelineImpl::RendererWrapper::GetBufferedTimeRanges()
+    const {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  base::AutoLock auto_lock(shared_state_lock_);
+  return shared_state_.buffered_time_ranges;
+}
+
+bool PipelineImpl::RendererWrapper::DidLoadingProgress() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  base::AutoLock auto_lock(shared_state_lock_);
+  bool did_progress = shared_state_.did_loading_progress;
+  shared_state_.did_loading_progress = false;
+  return did_progress;
+}
+
+PipelineStatistics PipelineImpl::RendererWrapper::GetStatistics() const {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  base::AutoLock auto_lock(shared_state_lock_);
+  return shared_state_.statistics;
+}
+
 void PipelineImpl::RendererWrapper::SetCdm(
     CdmContext* cdm_context,
     const CdmAttachedCB& cdm_attached_cb) {
   DCHECK(media_task_runner_->BelongsToCurrentThread());
 
-  if (!renderer_) {
+  if (!shared_state_.renderer) {
     cdm_context_ = cdm_context;
     cdm_attached_cb.Run(true);
     return;
   }
 
-  renderer_->SetCdm(cdm_context,
-                    base::Bind(&RendererWrapper::OnCdmAttached, weak_this_,
-                               cdm_attached_cb, cdm_context));
+  shared_state_.renderer->SetCdm(
+      cdm_context, base::Bind(&RendererWrapper::OnCdmAttached, weak_this_,
+                              cdm_attached_cb, cdm_context));
 }
 
 void PipelineImpl::RendererWrapper::OnCdmAttached(
@@ -943,7 +997,7 @@ void PipelineImpl::RendererWrapper::OnTextRendererEnded() {
 void PipelineImpl::RendererWrapper::RunEndedCallbackIfNeeded() {
   DCHECK(media_task_runner_->BelongsToCurrentThread());
 
-  if (renderer_ && !renderer_ended_)
+  if (shared_state_.renderer && !renderer_ended_)
     return;
 
   if (text_renderer_ && text_renderer_->HasTracks() && !text_renderer_ended_)
@@ -991,9 +1045,20 @@ void PipelineImpl::RendererWrapper::InitializeRenderer(
   }
 
   if (cdm_context_)
-    renderer_->SetCdm(cdm_context_, base::Bind(&IgnoreCdmAttached));
+    shared_state_.renderer->SetCdm(cdm_context_,
+                                   base::Bind(&IgnoreCdmAttached));
+
+  shared_state_.renderer->Initialize(demuxer_, this, done_cb);
+}
+
+void PipelineImpl::RendererWrapper::DestroyRenderer() {
+  DCHECK(media_task_runner_->BelongsToCurrentThread());
 
-  renderer_->Initialize(demuxer_, this, done_cb);
+  std::unique_ptr<Renderer> renderer;
+  {
+    base::AutoLock auto_lock(shared_state_lock_);

[DaleCurtis, 2016/06/22 15:21:15]

Weird scoping isn't necessary anymore. You can just use lock + renderer.reset().

[alokp, 2016/06/22 16:27:54]

If Renderer destructor is expensive, wouldn't we hold the lock for a long time
unnecessarily?

+    renderer.swap(shared_state_.renderer);
+  }
 }
 
 void PipelineImpl::RendererWrapper::ReportMetadata() {
@@ -1019,9 +1084,7 @@ void PipelineImpl::OnError(PipelineStatus error) {
   DVLOG(2) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_NE(PIPELINE_OK, error) << "PIPELINE_OK isn't an error!";
-
-  if (!IsRunning())
-    return;
+  DCHECK(IsRunning());
 
   // If the error happens during starting/seeking/suspending/resuming,
   // report the error via the completion callback for those tasks.
@@ -1042,129 +1105,94 @@ void PipelineImpl::OnError(PipelineStatus error) {
 void PipelineImpl::OnEnded() {
   DVLOG(2) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
-  if (IsRunning()) {
-    DCHECK(client_);
-    client_->OnEnded();
-  }
+  DCHECK(client_);
+  client_->OnEnded();
 }
 
 void PipelineImpl::OnMetadata(PipelineMetadata metadata) {
   DVLOG(2) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
-  if (IsRunning()) {
-    DCHECK(client_);
-    client_->OnMetadata(metadata);
-  }
+  DCHECK(client_);
+  client_->OnMetadata(metadata);
 }
 
 void PipelineImpl::OnBufferingStateChange(BufferingState state) {
   DVLOG(2) << __FUNCTION__ << "(" << state << ")";
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
-  if (IsRunning()) {
-    DCHECK(client_);
-    client_->OnBufferingStateChange(state);
-  }
+  DCHECK(client_);
+  client_->OnBufferingStateChange(state);
 }
 
 void PipelineImpl::OnDurationChange(base::TimeDelta duration) {
   DVLOG(2) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
   duration_ = duration;
 
-  if (IsRunning()) {
-    DCHECK(client_);
-    client_->OnDurationChange();
-  }
+  DCHECK(client_);
+  client_->OnDurationChange();
 }
 
 void PipelineImpl::OnAddTextTrack(const TextTrackConfig& config,
                                   const AddTextTrackDoneCB& done_cb) {
   DVLOG(2) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
-  if (IsRunning()) {
-    DCHECK(client_);
-    client_->OnAddTextTrack(config, done_cb);
-  }
+  DCHECK(client_);
+  client_->OnAddTextTrack(config, done_cb);
 }
 
 void PipelineImpl::OnWaitingForDecryptionKey() {
   DVLOG(2) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
-  if (IsRunning()) {
-    DCHECK(client_);
-    client_->OnWaitingForDecryptionKey();
-  }
+  DCHECK(client_);
+  client_->OnWaitingForDecryptionKey();
 }
 
 void PipelineImpl::OnVideoNaturalSizeChange(const gfx::Size& size) {
   DVLOG(2) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
-  if (IsRunning()) {
-    DCHECK(client_);
-    client_->OnVideoNaturalSizeChange(size);
-  }
+  DCHECK(client_);
+  client_->OnVideoNaturalSizeChange(size);
 }
 
 void PipelineImpl::OnVideoOpacityChange(bool opaque) {
   DVLOG(2) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
-  if (IsRunning()) {
-    DCHECK(client_);
-    client_->OnVideoOpacityChange(opaque);
-  }
-}
-
-void PipelineImpl::OnBufferedTimeRangesChange(
-    const Ranges<base::TimeDelta>& ranges) {
-  DVLOG(3) << __FUNCTION__;
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  buffered_time_ranges_ = ranges;
-  did_loading_progress_ = true;
-}
-
-void PipelineImpl::OnStatisticsUpdate(const PipelineStatistics& stats) {
-  DVLOG(3) << __FUNCTION__;
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  statistics_.audio_bytes_decoded += stats.audio_bytes_decoded;
-  statistics_.video_bytes_decoded += stats.video_bytes_decoded;
-  statistics_.video_frames_decoded += stats.video_frames_decoded;
-  statistics_.video_frames_dropped += stats.video_frames_dropped;
-  statistics_.audio_memory_usage += stats.audio_memory_usage;
-  statistics_.video_memory_usage += stats.video_memory_usage;
+  DCHECK(client_);
+  client_->OnVideoOpacityChange(opaque);
 }
 
 void PipelineImpl::OnSeekDone(base::TimeDelta start_time) {
   DVLOG(3) << __FUNCTION__ << "(" << start_time.InMicroseconds() << ")";
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
-  if (IsRunning()) {
-    DCHECK(!seek_cb_.is_null());
-    base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
-  }
+  DCHECK(!seek_cb_.is_null());
+  base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
 }
 
 void PipelineImpl::OnSuspendDone(base::TimeDelta suspend_time) {
   DVLOG(3) << __FUNCTION__ << "(" << suspend_time.InMicroseconds() << ")";
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(IsRunning());
 
-  // Reset audio-video memory usage since renderer has been destroyed.
-  statistics_.audio_memory_usage = 0;
-  statistics_.video_memory_usage = 0;
-
-  if (IsRunning()) {
-    DCHECK(!suspend_cb_.is_null());
-    base::ResetAndReturn(&suspend_cb_).Run(PIPELINE_OK);
-  }
+  DCHECK(!suspend_cb_.is_null());
+  base::ResetAndReturn(&suspend_cb_).Run(PIPELINE_OK);
 }
 
 PipelineImpl::RendererWrapper::RendererWrapper(
@@ -1181,7 +1209,6 @@ PipelineImpl::RendererWrapper::RendererWrapper(
       cdm_context_(nullptr),
       state_(kCreated),
       status_(PIPELINE_OK),
-      suspend_timestamp_(kNoTimestamp()),
       renderer_ended_(false),
       text_renderer_ended_(false),
       weak_factory_(this) {
@@ -1194,16 +1221,14 @@ PipelineImpl::RendererWrapper::~RendererWrapper() {
   DCHECK(state_ == kCreated || state_ == kStopped);
 }
 
-base::TimeDelta PipelineImpl::RendererWrapper::GetMediaTime() {
-  // This is the only member function that gets called on the main thread.
-  // TODO(alokp): Enforce that Renderer is only called on a single thread,
-  // even for accessing media time http://crbug.com/370634.
+base::TimeDelta PipelineImpl::RendererWrapper::GetMediaTime() const {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
-  base::AutoLock auto_lock(media_time_lock_);
-  if (suspend_timestamp_ != kNoTimestamp())
-    return suspend_timestamp_;
-  return renderer_ ? renderer_->GetMediaTime() : base::TimeDelta();
+  base::AutoLock auto_lock(shared_state_lock_);
+  if (shared_state_.suspend_timestamp != kNoTimestamp())
+    return shared_state_.suspend_timestamp;
+  return shared_state_.renderer ? shared_state_.renderer->GetMediaTime()
+                                : base::TimeDelta();
 }
 
 }  // namespace media