From 698835d3e64c96bb5e7a29ab66d2ecbee375fc63 Mon Sep 17 00:00:00 2001
From: "hclam@chromium.org"
 <hclam@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Fri, 27 Mar 2009 22:41:55 +0000
Subject: Fixed a lot threading issues during tear down of <video> Fixed a lot
 of dead locks during tead down of <video> due to DataSourceImpl. Most of the
 issues come from that during a tab close RenderThread is destroyed and new
 tasks posted on it will not executed, but DataSourceImpl is waiting for those
 tasks to finish to complete stopping. Another dead lock comes from that when
 RenderThread is destroyed the owner loop of it (a IO Message Loop) is being
 destroyed too and DataSourceImpl shouldn't post tasks to that message loop
 when stopping.

Review URL: http://codereview.chromium.org/42675

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@12720 0039d316-1c4b-4281-b951-d872f2087c98
---
 chrome/renderer/media/audio_renderer_impl.cc    | 20 +++++--
 chrome/renderer/media/audio_renderer_impl.h     |  8 ++-
 chrome/renderer/media/data_source_impl.cc       | 74 +++++++++++--------------
 chrome/renderer/media/data_source_impl.h        | 26 +++++----
 chrome/renderer/webmediaplayer_delegate_impl.cc | 46 ++++++++-------
 chrome/renderer/webmediaplayer_delegate_impl.h  |  4 ++
 6 files changed, 94 insertions(+), 84 deletions(-)

(limited to 'chrome/renderer')

diff --git a/chrome/renderer/media/audio_renderer_impl.cc b/chrome/renderer/media/audio_renderer_impl.cc
index 9677b32..a830b40 100644
--- a/chrome/renderer/media/audio_renderer_impl.cc
+++ b/chrome/renderer/media/audio_renderer_impl.cc
@@ -57,7 +57,8 @@ bool AudioRendererImpl::OnInitialize(const media::MediaFormat& media_format) {
 void AudioRendererImpl::OnStop() {
   if (!resource_release_event_.IsSignaled()) {
     render_loop_->PostTask(FROM_HERE,
-      NewRunnableMethod(this, &AudioRendererImpl::ReleaseRendererResources));
+      NewRunnableMethod(this,
+                        &AudioRendererImpl::ReleaseResources, false));
     resource_release_event_.Wait();
   }
 }
@@ -90,6 +91,7 @@ void AudioRendererImpl::SetVolume(float volume) {
 void AudioRendererImpl::OnCreated(base::SharedMemoryHandle handle,
                                   size_t length) {
   shared_memory_.reset(new base::SharedMemory(handle, false));
+  shared_memory_->Map(length);
   shared_memory_size_ = length;
   // TODO(hclam): is there any better place to do this?
   OnStartAudioStream();
@@ -97,6 +99,9 @@ void AudioRendererImpl::OnCreated(base::SharedMemoryHandle handle,
 
 void AudioRendererImpl::OnRequestPacket() {
   packet_requested_ = true;
+  // Post a task to render thread and try to grab a packet for sending back.
+  render_loop_->PostTask(FROM_HERE,
+      NewRunnableMethod(this, &AudioRendererImpl::OnNotifyAudioPacketReady));
 }
 
 void AudioRendererImpl::OnStateChanged(AudioOutputStream::State state,
@@ -120,8 +125,9 @@ void AudioRendererImpl::OnVolume(double left, double right) {
   // pipeline.
 }
 
-void AudioRendererImpl::ReleaseRendererResources() {
-  OnCloseAudioStream();
+void AudioRendererImpl::ReleaseResources(bool is_render_thread_dying) {
+  if (!is_render_thread_dying)
+    OnCloseAudioStream();
   resource_release_event_.Signal();
 }
 
@@ -151,8 +157,10 @@ void AudioRendererImpl::OnNotifyAudioPacketReady() {
     // Fill into the shared memory.
     size_t filled = FillBuffer(static_cast<uint8*>(shared_memory_->memory()),
                                shared_memory_size_);
-    packet_requested_ = false;
-    // Then tell browser process we are done filling into the buffer.
-    delegate_->view()->NotifyAudioPacketReady(stream_id_, filled);
+    if (filled > 0) {
+      packet_requested_ = false;
+      // Then tell browser process we are done filling into the buffer.
+      delegate_->view()->NotifyAudioPacketReady(stream_id_, filled);
+    }
   }
 }
diff --git a/chrome/renderer/media/audio_renderer_impl.h b/chrome/renderer/media/audio_renderer_impl.h
index d062bb0..8834f4ec 100644
--- a/chrome/renderer/media/audio_renderer_impl.h
+++ b/chrome/renderer/media/audio_renderer_impl.h
@@ -63,7 +63,7 @@
 // |     in the browser process, called along with a SharedMemoryHandle.
 // |-- OnVolume()
 // |     Called from RenderView about the volume of the audio output stream.
-// \-- ReleaseRendererResource()
+// \-- ReleaseResource()
 //       Release resources that live inside render thread.
 //
 // Pipeline thread
@@ -84,7 +84,7 @@
 //
 // Audio decoder thread (If there's one.)
 // \-- OnAssignment()
-//       A raw PCM audio packet buffer is recevied here, this method is called
+//       A raw PCM audio packet buffer is received here, this method is called
 //       from pipeline thread if audio decoder thread does not exist.
 
 #ifndef CHROME_RENDERER_MEDIA_AUDIO_RENDERER_IMPL_H_
@@ -118,7 +118,9 @@ class AudioRendererImpl : public media::AudioRendererBase {
   void OnVolume(double left, double right);
 
   // Release resources that lives in renderer thread, i.e. audio output streams.
-  void ReleaseRendererResources();
+  // |render_thread_is_dying| tells us if render thread is being destroyed,
+  // if true it's not safe to access any object that lives inside render thread.
+  void ReleaseResources(bool render_thread_is_dying);
 
   // Methods called on pipeline thread ----------------------------------------
   // media::MediaFilter implementation.
diff --git a/chrome/renderer/media/data_source_impl.cc b/chrome/renderer/media/data_source_impl.cc
index 51efb6f..93a4f5c 100644
--- a/chrome/renderer/media/data_source_impl.cc
+++ b/chrome/renderer/media/data_source_impl.cc
@@ -22,6 +22,7 @@ DataSourceImpl::DataSourceImpl(WebMediaPlayerDelegateImpl* delegate)
       downloaded_bytes_(0),
       total_bytes_(0),
       total_bytes_known_(false),
+      download_completed_(false),
       resource_loader_bridge_(NULL),
       resource_release_event_(true, false),
       read_event_(false, false),
@@ -31,7 +32,6 @@ DataSourceImpl::DataSourceImpl(WebMediaPlayerDelegateImpl* delegate)
       last_read_size_(0),
       position_(0),
       io_loop_(delegate->view()->GetMessageLoopForIO()),
-      close_event_(false, false),
       seek_event_(false, false) {
   // Register ourselves with WebMediaPlayerDelgateImpl.
   delegate_->SetDataSource(this);
@@ -45,28 +45,20 @@ void DataSourceImpl::Stop() {
     return;
   stopped_ = true;
 
-  // 1. Cancel the resource request.
+  // Wakes up demuxer waiting on |read_event_| in Read().
+  read_event_.Signal();
+  // Wakes up demuxer waiting on |seek_event_| in SetPosition().
+  seek_event_.Signal();
+  // Wakes up demuxer waiting on |download_event_| in Read() or SetPosition().
+  download_event_.Signal();
+
   if (!resource_release_event_.IsSignaled()) {
     render_loop_->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &DataSourceImpl::ReleaseRendererResources));
+        NewRunnableMethod(this, &DataSourceImpl::ReleaseResources, false));
+    // We wait until the resource is released to make sure we won't receive
+    // any call from render thread when we wake up.
     resource_release_event_.Wait();
   }
-
-  // 2. Signal download_event_.
-  download_event_.Signal();
-
-  // Post a close file stream task to IO message loop, it will signal the read
-  // event.
-  // TODO(hclam): make sure it's safe to do this during destruction of
-  // RenderThread.
-  io_loop_->PostTask(
-      FROM_HERE, NewRunnableMethod(this, &DataSourceImpl::OnCloseFileStream));
-
-  // Wait for close to finish for FileStream.
-  close_event_.Wait();
-
-  // Make sure that when we wake up the stream is closed and destroyed.
-  DCHECK(stream_.get() == NULL);
 }
 
 bool DataSourceImpl::Initialize(const std::string& url) {
@@ -84,7 +76,7 @@ size_t DataSourceImpl::Read(uint8* data, size_t size) {
   while (!stopped_) {
     {
       AutoLock auto_lock(lock_);
-      if (position_ + size <= downloaded_bytes_)
+      if (download_completed_ || position_ + size <= downloaded_bytes_)
         break;
     }
     download_event_.Wait();
@@ -94,7 +86,7 @@ size_t DataSourceImpl::Read(uint8* data, size_t size) {
   if (!stopped_) {
     if (logging::DEBUG_MODE) {
       AutoLock auto_lock(lock_);
-      DCHECK(position_ + size <= downloaded_bytes_);
+      DCHECK(download_completed_ || position_ + size <= downloaded_bytes_);
     }
     // Post a task to IO message loop to perform the actual reading.
     io_loop_->PostTask(FROM_HERE,
@@ -115,7 +107,7 @@ bool DataSourceImpl::SetPosition(int64 position) {
   while (!stopped_) {
     {
       AutoLock auto_lock(lock_);
-      if (position < downloaded_bytes_)
+      if (download_completed_ || position < downloaded_bytes_)
         break;
     }
     download_event_.Wait();
@@ -123,7 +115,7 @@ bool DataSourceImpl::SetPosition(int64 position) {
   if (!stopped_) {
     if (logging::DEBUG_MODE) {
       AutoLock auto_lock_(lock_);
-      DCHECK(position < downloaded_bytes_);
+      DCHECK(download_completed_ || position < downloaded_bytes_);
     }
     // Perform the seek operation on IO message loop.
     io_loop_->PostTask(FROM_HERE,
@@ -171,19 +163,6 @@ void DataSourceImpl::OnReadFileStream(uint8* data, size_t size) {
   }
 }
 
-void DataSourceImpl::OnCloseFileStream() {
-  if (stream_.get()) {
-    stream_->Close();
-    stream_.reset();
-  }
-  // Wakes up demuxer waiting on read_event_ in Read().
-  read_event_.Signal();
-  // Wakes up demuxer waiting on seek_event_ in SetPosition().
-  seek_event_.Signal();
-  // Wakes up pipeline thread blocked in Stop() by close_event_.
-  close_event_.Signal();
-}
-
 void DataSourceImpl::OnSeekFileStream(net::Whence whence, int64 position) {
   if (!stopped_ && stream_.get()) {
     int64 new_position = stream_->Seek(whence, position);
@@ -198,12 +177,14 @@ void DataSourceImpl::OnSeekFileStream(net::Whence whence, int64 position) {
 }
 
 void DataSourceImpl::OnDidFileStreamRead(int size) {
-  {
+  if (size < 0) {
+    host_->Error(media::PIPELINE_ERROR_READ);
+  } else {
     AutoLock auto_lock(lock_);
-    last_read_size_ = size;
     // TODO(hclam): size may be an error code, handle that.
     position_ += size;
   }
+  last_read_size_ = size;
   read_event_.Signal();
 }
 
@@ -230,9 +211,13 @@ void DataSourceImpl::OnInitialize(std::string uri) {
   resource_loader_bridge_->Start(this);
 }
 
-void DataSourceImpl::ReleaseRendererResources() {
+void DataSourceImpl::ReleaseResources(bool render_thread_is_dying) {
   DCHECK(MessageLoop::current() == render_loop_);
-  if (resource_loader_bridge_) {
+  if (render_thread_is_dying) {
+    // If render thread is dying we can't call cancel on this pointer, we will
+    // just let this object leak.
+    resource_loader_bridge_ = NULL;
+  } else if (resource_loader_bridge_) {
     resource_loader_bridge_->Cancel();
     resource_loader_bridge_ = NULL;
   }
@@ -245,6 +230,8 @@ void DataSourceImpl::OnDownloadProgress(uint64 position, uint64 size) {
     downloaded_bytes_ = position;
     if (!total_bytes_known_) {
       if (size == kuint64max) {
+        // If we receive an invalid value for size, we keep on updating the
+        // total number of bytes.
         total_bytes_ = position;
       } else {
         total_bytes_ = size;
@@ -297,10 +284,13 @@ void DataSourceImpl::OnReceivedData(const char* data, int len) {
 
 void DataSourceImpl::OnCompletedRequest(const URLRequestStatus& status,
                                         const std::string& security_info) {
+  {
+    AutoLock auto_lock(lock_);
+    total_bytes_known_ = true;
+    download_completed_ = true;
+  }
   if (status.status() != URLRequestStatus::SUCCESS) {
     host_->Error(media::PIPELINE_ERROR_NETWORK);
-  } else {
-    // TODO(hclam): notify end of stream to pipeline.
   }
 }
 
diff --git a/chrome/renderer/media/data_source_impl.h b/chrome/renderer/media/data_source_impl.h
index 6f8d688..d5326cb 100644
--- a/chrome/renderer/media/data_source_impl.h
+++ b/chrome/renderer/media/data_source_impl.h
@@ -41,7 +41,7 @@
 //   |-- OnReceivedData()
 //   |-- OnCompletedRequest()
 //   |-- GetURLForDebugging()
-//   \-- ReleaseRendererResources();
+//   \-- ReleaseResources()
 //
 // Pipeline thread
 //   +-- Initialize()
@@ -64,8 +64,6 @@
 //   |     Callback for construction of net::FileStream in an IO message loop.
 //   |-- OnReadFileStream()
 //   |     Actual read operation on FileStream performs here.
-//   |-- OnCloseFileStream()
-//   |     Closing of FileSteram happens in this method.
 //   |-- OnSeekFileStream()
 //   |     Actual seek operation happens here.
 //   \-- OnDidFileStreamRead()
@@ -116,13 +114,18 @@ class DataSourceImpl : public media::DataSource,
 
   // Release all resources associated with RenderView, particularly
   // ResourceLoaderBridge created by ResourceDispatcher. This method should only
-  // be executed on render thread, we have this method standalone and public
-  // here because WebMediaPlayerDelegateImpl will be calling this method when
-  // render thread is being destroyed, and in that case we can't post tasks to
-  // render thread from pipeline thread anymore so we have to release resources
-  // manually from WebMediaPlayerDelegateImpl and we will not do the same thing
-  // in Stop().
-  void ReleaseRendererResources();
+  // be executed on render thread.
+  // There are three cases for this method to be called:
+  // 1. Posted as a task from DataSourceImpl::Stop() caused by an error in the
+  //    pipeline.
+  // 2. WebMediaPlayerDelegateImpl is being destroyed and all resources
+  //    associated with the pipeline should go away. In this case we can call
+  //    to objects that live inside render thread to cleanup,
+  //    e.g. ResourceDispatcher.
+  // 3. RenderThread is being destroyed, in this case we can't access any
+  //    object that lives inside render thread and should let the resources
+  //    leak.
+  void ReleaseResources(bool is_render_thread_dying);
 
   // Methods called from pipeline thread --------------------------------------
   virtual bool Initialize(const std::string& url);
@@ -154,7 +157,6 @@ class DataSourceImpl : public media::DataSource,
   // Handlers for file reading.
   void OnCreateFileStream(base::PlatformFile file);
   void OnReadFileStream(uint8* data, size_t size);
-  void OnCloseFileStream();
   void OnSeekFileStream(net::Whence whence, int64 position);
   void OnDidFileStreamRead(int size);
 
@@ -181,6 +183,7 @@ class DataSourceImpl : public media::DataSource,
   int64 downloaded_bytes_;
   int64 total_bytes_;
   bool total_bytes_known_;
+  bool download_completed_;
 
   // Members related to resource loading with RenderView.
   webkit_glue::ResourceLoaderBridge* resource_loader_bridge_;
@@ -195,7 +198,6 @@ class DataSourceImpl : public media::DataSource,
   MessageLoop* io_loop_;
 
   // Events for other operations on stream_.
-  base::WaitableEvent close_event_;
   base::WaitableEvent seek_event_;
 
   DISALLOW_COPY_AND_ASSIGN(DataSourceImpl);
diff --git a/chrome/renderer/webmediaplayer_delegate_impl.cc b/chrome/renderer/webmediaplayer_delegate_impl.cc
index 5f16cf3..b7d087d 100644
--- a/chrome/renderer/webmediaplayer_delegate_impl.cc
+++ b/chrome/renderer/webmediaplayer_delegate_impl.cc
@@ -68,7 +68,7 @@ WebMediaPlayerDelegateImpl::WebMediaPlayerDelegateImpl(RenderView* view)
 WebMediaPlayerDelegateImpl::~WebMediaPlayerDelegateImpl() {
   // Stop the pipeline in the first place so we won't receive any more method
   // calls from it.
-  pipeline_.Stop();
+  StopPipeline(false);
 
   // Cancel all tasks posted on the main_loop_.
   CancelAllTasks();
@@ -131,7 +131,7 @@ void WebMediaPlayerDelegateImpl::Stop() {
   DCHECK(main_loop_ && MessageLoop::current() == main_loop_);
 
   // We can fire Stop() multiple times.
-  pipeline_.Stop();
+  StopPipeline(false);
 }
 
 void WebMediaPlayerDelegateImpl::Seek(float time) {
@@ -283,31 +283,15 @@ void WebMediaPlayerDelegateImpl::Paint(skia::PlatformCanvas *canvas,
 }
 
 void WebMediaPlayerDelegateImpl::WillDestroyCurrentMessageLoop() {
-  // Instruct the renderers and data source to release all Renderer related
-  // resources during destruction of render thread, because they won't have any
-  // chance to release these resources on render thread by posting tasks on it.
-  if (audio_renderer_) {
-    audio_renderer_->ReleaseRendererResources();
-    audio_renderer_ = NULL;
-  }
-
-  if (data_source_) {
-    data_source_->ReleaseRendererResources();
-    data_source_ = NULL;
-  }
-
-  // Stop the pipeline when the render thread is being destroyed so we won't be
-  // posting any more messages onto it. And we just let this object and
-  // associated WebMediaPlayer to leak.
-  pipeline_.Stop();
+  StopPipeline(true);
 }
 
 void WebMediaPlayerDelegateImpl::DidInitializePipeline(bool successful) {
   if (successful) {
     // Since we have initialized the pipeline, we should be able to play it.
     // And we skip LOADED_METADATA state and starting with LOADED_FIRST_FRAME.
-    ready_state_ = webkit_glue::WebMediaPlayer::CAN_PLAY;
-    network_state_ = webkit_glue::WebMediaPlayer::LOADED_FIRST_FRAME;
+    ready_state_ = webkit_glue::WebMediaPlayer::CAN_PLAY_THROUGH;
+    network_state_ = webkit_glue::WebMediaPlayer::LOADED;
   } else {
     // TODO(hclam): should use pipeline_.GetError() to determine the state
     // properly and reports error using MediaError.
@@ -374,3 +358,23 @@ void WebMediaPlayerDelegateImpl::PostTask(int index,
 void WebMediaPlayerDelegateImpl::PostRepaintTask() {
   PostTask(kRepaintTaskIndex, &webkit_glue::WebMediaPlayer::Repaint);
 }
+
+void WebMediaPlayerDelegateImpl::StopPipeline(bool render_thread_is_dying) {
+  // Instruct the renderers and data source to release all Renderer related
+  // resources during destruction of render thread, because they won't have any
+  // chance to release these resources on render thread by posting tasks on it.
+  if (audio_renderer_) {
+    audio_renderer_->ReleaseResources(render_thread_is_dying);
+    audio_renderer_ = NULL;
+  }
+
+  if (data_source_) {
+    data_source_->ReleaseResources(render_thread_is_dying);
+    data_source_ = NULL;
+  }
+
+  // Stop the pipeline when the render thread is being destroyed so we won't be
+  // posting any more messages onto it. And we just let this object and
+  // associated WebMediaPlayer to leak.
+  pipeline_.Stop();
+}
diff --git a/chrome/renderer/webmediaplayer_delegate_impl.h b/chrome/renderer/webmediaplayer_delegate_impl.h
index b557393..1f0c726 100644
--- a/chrome/renderer/webmediaplayer_delegate_impl.h
+++ b/chrome/renderer/webmediaplayer_delegate_impl.h
@@ -160,6 +160,10 @@ class WebMediaPlayerDelegateImpl : public webkit_glue::WebMediaPlayerDelegate,
   // Cancel all tasks currently lives in |main_loop_|.
   void CancelAllTasks();
 
+  // Calls to renderers and data source to release all resources that live in
+  // render thread and stop the pipeline.
+  void StopPipeline(bool render_thread_is_dying);
+
   // Indexes for tasks.
   enum {
     kRepaintTaskIndex = 0,
-- 
cgit v1.1