Adding Abort() to DataSource to wake up hanging Read()

This change adds an Abort() method to BufferedDataSource, which will
wake up a blocking read if one exists. When WebMediaPlayerImpl being
destroyed, it now tells BufferedDataSource to abort before it tells
the pipeline to stop, so the pipeline will not hang while waiting
for a never-ending Read() to return.

BUG=54465
TEST=test_shell_tests

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@63548 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 168 insertions, 37 deletions

diff --git a/webkit/glue/media/buffered_data_source.cc b/webkit/glue/media/buffered_data_source.cc
index 3456e3a..a4e3901 100644
--- a/webkit/glue/media/buffered_data_source.cc
+++ b/webkit/glue/media/buffered_data_source.cc
@@ -15,6 +15,7 @@
 #include "net/http/http_response_headers.h"
 #include "webkit/glue/media/buffered_data_source.h"
 #include "webkit/glue/webkit_glue.h"
+#include "webkit/glue/webmediaplayer_impl.h"
 
 namespace {
 
@@ -542,7 +543,8 @@ bool BufferedDataSource::IsMediaFormatSupported(
 // BufferedDataSource, protected
 BufferedDataSource::BufferedDataSource(
     MessageLoop* render_loop,
-    webkit_glue::MediaResourceLoaderBridgeFactory* bridge_factory)
+    webkit_glue::MediaResourceLoaderBridgeFactory* bridge_factory,
+    webkit_glue::WebMediaPlayerImpl::Proxy* proxy)
     : total_bytes_(kPositionNotSpecified),
       loaded_(false),
       streaming_(false),
@@ -562,6 +564,8 @@ BufferedDataSource::BufferedDataSource(
       stopped_on_render_loop_(false),
       media_is_paused_(true),
       using_range_request_(true) {
+  if (proxy)
+    proxy->SetDataSource(this);
 }
 
 BufferedDataSource::~BufferedDataSource() {
@@ -623,6 +627,7 @@ void BufferedDataSource::Stop(media::FilterCallback* callback) {
     callback->Run();
     delete callback;
   }
+
   render_loop_->PostTask(FROM_HERE,
       NewRunnableMethod(this, &BufferedDataSource::CleanupTask));
 }
@@ -655,6 +660,20 @@ bool BufferedDataSource::IsStreaming() {
   return streaming_;
 }
 
+void BufferedDataSource::Abort() {
+  DCHECK(MessageLoop::current() == render_loop_);
+
+  // If we are told to abort, immediately return from any pending read
+  // with an error.
+  if (read_callback_.get()) {
+    {
+      AutoLock auto_lock(lock_);
+      DoneRead_Locked(net::ERR_FAILED);
+    }
+    CleanupTask();
+  }
+}
+
 /////////////////////////////////////////////////////////////////////////////
 // BufferedDataSource, render thread tasks
 void BufferedDataSource::InitializeTask() {
@@ -720,7 +739,10 @@ void BufferedDataSource::ReadTask(
 
 void BufferedDataSource::CleanupTask() {
   DCHECK(MessageLoop::current() == render_loop_);
-  DCHECK(!stopped_on_render_loop_);
+
+  // If we have already stopped, do nothing.
+  if (stopped_on_render_loop_)
+    return;
 
   // Stop the watch dog.
   watch_dog_timer_.Stop();
diff --git a/webkit/glue/media/buffered_data_source.h b/webkit/glue/media/buffered_data_source.h
index e7b2e47..6b740c9 100644
--- a/webkit/glue/media/buffered_data_source.h
+++ b/webkit/glue/media/buffered_data_source.h
@@ -21,6 +21,8 @@
 #include "net/base/completion_callback.h"
 #include "net/base/file_stream.h"
 #include "webkit/glue/media/media_resource_loader_bridge_factory.h"
+#include "webkit/glue/media/web_data_source.h"
+#include "webkit/glue/webmediaplayer_impl.h"
 
 namespace webkit_glue {
 /////////////////////////////////////////////////////////////////////////////
@@ -212,18 +214,20 @@ class BufferedResourceLoader :
   DISALLOW_COPY_AND_ASSIGN(BufferedResourceLoader);
 };
 
-class BufferedDataSource : public media::DataSource {
+class BufferedDataSource : public WebDataSource {
  public:
   // Methods called from pipeline thread
   // Static methods for creating this class.
   static media::FilterFactory* CreateFactory(
       MessageLoop* message_loop,
-      webkit_glue::MediaResourceLoaderBridgeFactory* bridge_factory) {
-    return new media::FilterFactoryImpl2<
+      webkit_glue::MediaResourceLoaderBridgeFactory* bridge_factory,
+      webkit_glue::WebMediaPlayerImpl::Proxy* proxy) {
+    return new media::FilterFactoryImpl3<
         BufferedDataSource,
         MessageLoop*,
-        webkit_glue::MediaResourceLoaderBridgeFactory*>(
-        message_loop, bridge_factory);
+        webkit_glue::MediaResourceLoaderBridgeFactory*,
+        webkit_glue::WebMediaPlayerImpl::Proxy*>(
+        message_loop, bridge_factory, proxy);
   }
 
   // media::FilterFactoryImpl2 implementation.
@@ -248,10 +252,14 @@ class BufferedDataSource : public media::DataSource {
     return media_format_;
   }
 
+  // webkit_glue::WebDataSource implementation.
+  virtual void Abort();
+
  protected:
   BufferedDataSource(
       MessageLoop* render_loop,
-      webkit_glue::MediaResourceLoaderBridgeFactory* bridge_factory);
+      webkit_glue::MediaResourceLoaderBridgeFactory* bridge_factory,
+      webkit_glue::WebMediaPlayerImpl::Proxy* proxy);
   virtual ~BufferedDataSource();
 
   // A factory method to create a BufferedResourceLoader based on the read
@@ -266,10 +274,11 @@ class BufferedDataSource : public media::DataSource {
   virtual base::TimeDelta GetTimeoutMilliseconds();
 
  private:
-  friend class media::FilterFactoryImpl2<
+  friend class media::FilterFactoryImpl3<
       BufferedDataSource,
       MessageLoop*,
-      webkit_glue::MediaResourceLoaderBridgeFactory*>;
+      webkit_glue::MediaResourceLoaderBridgeFactory*,
+      webkit_glue::WebMediaPlayerImpl::Proxy*>;
 
   // Posted to perform initialization on render thread and start resource
   // loading.
diff --git a/webkit/glue/media/buffered_data_source_unittest.cc b/webkit/glue/media/buffered_data_source_unittest.cc
index f032595..b78f759 100644
--- a/webkit/glue/media/buffered_data_source_unittest.cc
+++ b/webkit/glue/media/buffered_data_source_unittest.cc
@@ -561,7 +561,7 @@ class MockBufferedDataSource : public BufferedDataSource {
  protected:
   MockBufferedDataSource(
       MessageLoop* message_loop, MediaResourceLoaderBridgeFactory* factory)
-      : BufferedDataSource(message_loop, factory) {
+      : BufferedDataSource(message_loop, factory, NULL) {
   }
 
  private:
@@ -757,6 +757,23 @@ class BufferedDataSourceTest : public testing::Test {
               memcmp(buffer_, data_ + static_cast<int>(position), read_size));
   }
 
+  void ReadDataSourceHang(int64 position, int size) {
+    EXPECT_TRUE(loader_);
+
+    // Expect a call to read, but the call never returns.
+    EXPECT_CALL(*loader_, Read(position, size, NotNull(), NotNull()));
+    data_source_->Read(
+        position, size, buffer_,
+        NewCallback(this, &BufferedDataSourceTest::ReadCallback));
+    message_loop_->RunAllPending();
+
+    // Now expect the read to return after aborting the data source.
+    EXPECT_CALL(*this, ReadCallback(_));
+    EXPECT_CALL(*loader_, Stop());
+    data_source_->Abort();
+    message_loop_->RunAllPending();
+  }
+
   void ReadDataSourceMiss(int64 position, int size) {
     EXPECT_TRUE(loader_);
 
@@ -946,6 +963,11 @@ TEST_F(BufferedDataSourceTest, ReadCacheMiss) {
   StopDataSource();
 }
 
+TEST_F(BufferedDataSourceTest, ReadHang) {
+  InitializeDataSource(kHttpUrl, net::OK, true, 25, LOADING);
+  ReadDataSourceHang(10, 10);
+}
+
 TEST_F(BufferedDataSourceTest, ReadFailed) {
   InitializeDataSource(kHttpUrl, net::OK, true, 1024, LOADING);
   ReadDataSourceHit(10, 10, 10);
diff --git a/webkit/glue/media/web_data_source.cc b/webkit/glue/media/web_data_source.cc
new file mode 100644
index 0000000..a46d594
--- /dev/null
+++ b/webkit/glue/media/web_data_source.cc
@@ -0,0 +1,17 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/base/filters.h"
+#include "webkit/glue/media/web_data_source.h"
+
+namespace webkit_glue {
+
+WebDataSource::WebDataSource()
+    : media::DataSource() {
+}
+
+WebDataSource::~WebDataSource() {
+}
+
+}  // namespace webkit_glue
diff --git a/webkit/glue/media/web_data_source.h b/webkit/glue/media/web_data_source.h
new file mode 100644
index 0000000..2bbfd1c
--- /dev/null
+++ b/webkit/glue/media/web_data_source.h
@@ -0,0 +1,31 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef WEBKIT_GLUE_MEDIA_WEB_DATA_SOURCE_H_
+#define WEBKIT_GLUE_MEDIA_WEB_DATA_SOURCE_H_
+
+#include "media/base/filters.h"
+
+namespace webkit_glue {
+
+// An interface that allows WebMediaPlayerImpl::Proxy to communicate with the
+// DataSource in the pipeline.
+class WebDataSource : public media::DataSource {
+ public:
+  WebDataSource();
+  virtual ~WebDataSource();
+
+  // This method is used to unblock any read calls that would cause the
+  // media pipeline to stall.
+  //
+  // Method called on the render thread.
+  virtual void Abort() = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(WebDataSource);
+};
+
+}  // namespace webkit_glue
+
+#endif  // WEBKIT_GLUE_MEDIA_WEB_DATA_SOURCE_H_
diff --git a/webkit/glue/webkit_glue.gypi b/webkit/glue/webkit_glue.gypi
index 8249fb5..01731d3 100644
--- a/webkit/glue/webkit_glue.gypi
+++ b/webkit/glue/webkit_glue.gypi
@@ -175,6 +175,8 @@
         'media/simple_data_source.h',
         'media/video_renderer_impl.cc',
         'media/video_renderer_impl.h',
+        'media/web_data_source.cc',
+        'media/web_data_source.h',
         'media/web_video_renderer.h',
         'plugins/carbon_plugin_window_tracker_mac.h',
         'plugins/carbon_plugin_window_tracker_mac.cc',
diff --git a/webkit/glue/webmediaplayer_impl.cc b/webkit/glue/webmediaplayer_impl.cc
index e4aca5a..205be78 100644
--- a/webkit/glue/webmediaplayer_impl.cc
+++ b/webkit/glue/webmediaplayer_impl.cc
@@ -21,6 +21,9 @@
 #include "third_party/WebKit/WebKit/chromium/public/WebSize.h"
 #include "third_party/WebKit/WebKit/chromium/public/WebURL.h"
 #include "third_party/WebKit/WebKit/chromium/public/WebVideoFrame.h"
+#include "webkit/glue/media/buffered_data_source.h"
+#include "webkit/glue/media/media_resource_loader_bridge_factory.h"
+#include "webkit/glue/media/simple_data_source.h"
 #include "webkit/glue/media/video_renderer_impl.h"
 #include "webkit/glue/media/web_video_renderer.h"
 #include "webkit/glue/webvideoframe_impl.h"
@@ -85,6 +88,10 @@ void WebMediaPlayerImpl::Proxy::Repaint() {
   }
 }
 
+void WebMediaPlayerImpl::Proxy::SetDataSource(WebDataSource* data_source) {
+  data_source_ = data_source;
+}
+
 void WebMediaPlayerImpl::Proxy::SetVideoRenderer(
     WebVideoRenderer* video_renderer) {
   video_renderer_ = video_renderer;
@@ -105,10 +112,18 @@ void WebMediaPlayerImpl::Proxy::SetSize(const gfx::Rect& rect) {
   }
 }
 
+void WebMediaPlayerImpl::Proxy::AbortDataSource() {
+  DCHECK(MessageLoop::current() == render_loop_);
+  if (data_source_) {
+    data_source_->Abort();
+  }
+}
+
 void WebMediaPlayerImpl::Proxy::Detach() {
   DCHECK(MessageLoop::current() == render_loop_);
   webmediaplayer_ = NULL;
   video_renderer_ = NULL;
+  data_source_ = NULL;
 }
 
 void WebMediaPlayerImpl::Proxy::PipelineInitializationCallback() {
@@ -198,10 +213,12 @@ void WebMediaPlayerImpl::Proxy::PutCurrentFrame(
 /////////////////////////////////////////////////////////////////////////////
 // WebMediaPlayerImpl implementation
 
-WebMediaPlayerImpl::WebMediaPlayerImpl(WebKit::WebMediaPlayerClient* client,
-                                       media::FilterFactoryCollection* factory,
-                                       WebVideoRendererFactoryFactory*
-                                           video_renderer_factory)
+WebMediaPlayerImpl::WebMediaPlayerImpl(
+    WebKit::WebMediaPlayerClient* client,
+    media::FilterFactoryCollection* factory,
+    MediaResourceLoaderBridgeFactory* bridge_factory,
+    bool use_simple_data_source,
+    WebVideoRendererFactoryFactory* video_renderer_factory)
     : network_state_(WebKit::WebMediaPlayer::Empty),
       ready_state_(WebKit::WebMediaPlayer::HaveNothing),
       main_loop_(NULL),
@@ -241,6 +258,23 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(WebKit::WebMediaPlayerClient* client,
   pipeline_->SetNetworkEventCallback(NewCallback(proxy_.get(),
       &WebMediaPlayerImpl::Proxy::NetworkEventCallback));
 
+  // A simple data source that keeps all data in memory.
+  media::FilterFactory* simple_data_source_factory =
+      webkit_glue::SimpleDataSource::CreateFactory(MessageLoop::current(),
+                                                   bridge_factory);
+  // A sophisticated data source that does memory caching.
+  media::FilterFactory* buffered_data_source_factory =
+      webkit_glue::BufferedDataSource::CreateFactory(MessageLoop::current(),
+                                                     bridge_factory,
+                                                     proxy_);
+  if (use_simple_data_source) {
+    factory->AddFactory(simple_data_source_factory);
+    factory->AddFactory(buffered_data_source_factory);
+  } else {
+    factory->AddFactory(buffered_data_source_factory);
+    factory->AddFactory(simple_data_source_factory);
+  }
+
   // Add in the default filter factories.
   filter_factory_->AddFactory(media::FFmpegDemuxer::CreateFilterFactory());
   filter_factory_->AddFactory(media::FFmpegAudioDecoder::CreateFactory());
@@ -727,6 +761,11 @@ void WebMediaPlayerImpl::SetReadyState(
 void WebMediaPlayerImpl::Destroy() {
   DCHECK(MessageLoop::current() == main_loop_);
 
+  // Tell the data source to abort any pending reads so that the pipeline is
+  // not blocked when issuing stop commands to the other filters.
+  if (proxy_)
+    proxy_->AbortDataSource();
+
   // Make sure to kill the pipeline so there's no more media threads running.
   // Note: stopping the pipeline might block for a long time.
   pipeline_->Stop(NewCallback(this,
diff --git a/webkit/glue/webmediaplayer_impl.h b/webkit/glue/webmediaplayer_impl.h
index eefe1a9..30a8dcb 100644
--- a/webkit/glue/webmediaplayer_impl.h
+++ b/webkit/glue/webmediaplayer_impl.h
@@ -74,6 +74,8 @@ class FilterFactoryCollection;
 
 namespace webkit_glue {
 
+class MediaResourceLoaderBridgeFactory;
+class WebDataSource;
 class WebVideoRenderer;
 class WebVideoRendererFactoryFactory;
 
@@ -96,6 +98,7 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
     // Public methods called from the video renderer.
     void Repaint();
     void SetVideoRenderer(WebVideoRenderer* video_renderer);
+    void SetDataSource(WebDataSource* data_source);
 
     // Public methods called from WebMediaPlayerImpl.
     void Paint(skia::PlatformCanvas* canvas, const gfx::Rect& dest_rect);
@@ -103,6 +106,7 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
     void Detach();
     void GetCurrentFrame(scoped_refptr<media::VideoFrame>* frame_out);
     void PutCurrentFrame(scoped_refptr<media::VideoFrame> frame);
+    void AbortDataSource();
 
     // Public methods called from the pipeline via callback issued by
     // WebMediaPlayerImpl.
@@ -141,6 +145,7 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
     // The render message loop where WebKit lives.
     MessageLoop* render_loop_;
     WebMediaPlayerImpl* webmediaplayer_;
+    scoped_refptr<WebDataSource> data_source_;
     scoped_refptr<WebVideoRenderer> video_renderer_;
 
     Lock lock_;
@@ -172,6 +177,8 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
   // a subclass of WebVideoRenderer.  Is deleted by WebMediaPlayerImpl.
   WebMediaPlayerImpl(WebKit::WebMediaPlayerClient* client,
                      media::FilterFactoryCollection* factory,
+                     MediaResourceLoaderBridgeFactory* bridge_factory,
+                     bool use_simple_data_source,
                      WebVideoRendererFactoryFactory* video_renderer_factory);
   virtual ~WebMediaPlayerImpl();
 
diff --git a/webkit/support/webkit_support.cc b/webkit/support/webkit_support.cc
index 7844d98c7..c199cc3 100644
--- a/webkit/support/webkit_support.cc
+++ b/webkit/support/webkit_support.cc
@@ -281,18 +281,9 @@ WebKit::WebMediaPlayer* CreateMediaPlayer(WebFrame* frame,
           base::GetCurrentProcId(),
           appcache_host ? appcache_host->host_id() : appcache::kNoHostId,
           0);
-  // A simple data source that keeps all data in memory.
-  media::FilterFactory* simple_data_source_factory =
-      webkit_glue::SimpleDataSource::CreateFactory(MessageLoop::current(),
-                                                   bridge_factory);
-  // A sophisticated data source that does memory caching.
-  media::FilterFactory* buffered_data_source_factory =
-      webkit_glue::BufferedDataSource::CreateFactory(MessageLoop::current(),
-                                                     bridge_factory);
-  factory->AddFactory(buffered_data_source_factory);
-  factory->AddFactory(simple_data_source_factory);
+
   return new webkit_glue::WebMediaPlayerImpl(
-      client, factory,
+      client, factory, bridge_factory, false,
       new webkit_glue::VideoRendererImpl::FactoryFactory(false));
 }
 
diff --git a/webkit/tools/test_shell/test_webview_delegate.cc b/webkit/tools/test_shell/test_webview_delegate.cc
index 5727cab..0fb610f 100644
--- a/webkit/tools/test_shell/test_webview_delegate.cc
+++ b/webkit/tools/test_shell/test_webview_delegate.cc
@@ -732,18 +732,9 @@ WebMediaPlayer* TestWebViewDelegate::createMediaPlayer(
           base::GetCurrentProcId(),
           appcache_host ? appcache_host->host_id() : appcache::kNoHostId,
           0);
-  // A simple data source that keeps all data in memory.
-  media::FilterFactory* simple_data_source_factory =
-      webkit_glue::SimpleDataSource::CreateFactory(MessageLoop::current(),
-                                                   bridge_factory);
-  // A sophisticated data source that does memory caching.
-  media::FilterFactory* buffered_data_source_factory =
-      webkit_glue::BufferedDataSource::CreateFactory(MessageLoop::current(),
-                                                     bridge_factory);
-  factory->AddFactory(buffered_data_source_factory);
-  factory->AddFactory(simple_data_source_factory);
+
   return new webkit_glue::WebMediaPlayerImpl(
-      client, factory,
+      client, factory, bridge_factory, false,
       new webkit_glue::VideoRendererImpl::FactoryFactory(false));
 }