Remove synchronous blocking from PipelineImpl::Stop() - take 2.

We should be able to avoid this with careful use of a trampoline
through the media thread after PipelineImpl::Stop() completes.

This is an updated version of the last CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1336659

The difference is we now also preserve the RendererFactorySelector
until after renderer shutdown. This passes all the failing tests
I could find locally.

BUG=521176, 905506
TEST=passes cq.
R=sandersd

Change-Id: I86c4814a91ee6c41f20f6bb8a05859ac44171d7e
Reviewed-on: https://chromium-review.googlesource.com/c/1351791Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611305}

media/base/data_source.h

@@ -33,7 +33,9 @@ class MEDIA_EXPORT DataSource {
                     const DataSource::ReadCB& read_cb) = 0;
 
   // Stops the DataSource. Once this is called all future Read() calls will
-  // return an error.
+  // return an error. This is a synchronous call and may be called from any
+  // thread. Once called, the DataSource may no longer be used and should be
+  // destructed shortly thereafter.
   virtual void Stop() = 0;
 
   // Similar to Stop(), but only aborts current reads and not future reads.

media/base/pipeline_impl.cc

@@ -56,7 +56,7 @@ class PipelineImpl::RendererWrapper : public DemuxerHost,
              Demuxer* demuxer,
              std::unique_ptr<Renderer> renderer,
              base::WeakPtr<PipelineImpl> weak_pipeline);
-  void Stop(const base::Closure& stop_cb);
+  void Stop();
   void Seek(base::TimeDelta time);
   void Suspend();
   void Resume(std::unique_ptr<Renderer> renderer, base::TimeDelta time);
@@ -270,7 +270,7 @@ void PipelineImpl::RendererWrapper::Start(
                                         weak_this_, base::TimeDelta()));
 }
 
-void PipelineImpl::RendererWrapper::Stop(const base::Closure& stop_cb) {
+void PipelineImpl::RendererWrapper::Stop() {
   DCHECK(media_task_runner_->BelongsToCurrentThread());
   DCHECK(state_ != kStopping && state_ != kStopped);
 
@@ -296,17 +296,9 @@ void PipelineImpl::RendererWrapper::Stop(const base::Closure& stop_cb) {
 
   SetState(kStopped);
 
-  // Reset the status. Otherwise, if we encountered an error, new erros will
+  // Reset the status. Otherwise, if we encountered an error, new errors will
   // never be propagated. See https://crbug.com/812465.
   status_ = PIPELINE_OK;
-
-  // Post the stop callback to enqueue it after the tasks that may have been
-  // posted by Demuxer and Renderer during stopping. Note that in theory the
-  // tasks posted by Demuxer/Renderer may post even more tasks that will get
-  // enqueued after |stop_cb|. This may be problematic because Demuxer may
-  // get destroyed as soon as |stop_cb| is run. In practice this is not a
-  // problem, but ideally Demuxer should be destroyed on the media thread.
-  media_task_runner_->PostTask(FROM_HERE, stop_cb);
 }
 
 void PipelineImpl::RendererWrapper::Seek(base::TimeDelta time) {
@@ -1053,29 +1045,13 @@ void PipelineImpl::Stop() {
 
   if (media_task_runner_->BelongsToCurrentThread()) {
     // This path is executed by unittests that share media and main threads.
-    renderer_wrapper_->Stop(base::DoNothing());
+    renderer_wrapper_->Stop();
   } else {
     // This path is executed by production code where the two task runners -
     // main and media - live on different threads.
-    //
-    // TODO(alokp): We should not have to wait for the RendererWrapper::Stop.
-    // RendererWrapper holds a raw reference to Demuxer, which in turn holds a
-    // raw reference to DataSource. Both Demuxer and DataSource need to live
-    // until RendererWrapper is stopped. If RendererWrapper owned Demuxer and
-    // Demuxer owned DataSource, we could simply let RendererWrapper get lazily
-    // destroyed on the media thread.
-    base::WaitableEvent waiter(base::WaitableEvent::ResetPolicy::AUTOMATIC,
-                               base::WaitableEvent::InitialState::NOT_SIGNALED);
-    base::Closure stop_cb =
-        base::Bind(&base::WaitableEvent::Signal, base::Unretained(&waiter));
-    // If posting the task fails or the posted task fails to run,
-    // we will wait here forever. So add a CHECK to make sure we do not run
-    // into those situations.
-    CHECK(media_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&RendererWrapper::Stop,
-                   base::Unretained(renderer_wrapper_.get()), stop_cb)));
-    waiter.Wait();
+    media_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(&RendererWrapper::Stop,
+                                  base::Unretained(renderer_wrapper_.get())));
   }
 
   // Once the pipeline is stopped, nothing is reported back to the client.

media/blink/multibuffer_data_source.cc

@@ -160,6 +160,7 @@ bool MultibufferDataSource::media_has_played() const {
 }
 
 bool MultibufferDataSource::assume_fully_buffered() {
+  DCHECK(url_data());
   return !url_data()->url().SchemeIsHTTPOrHTTPS();
 }
 
@@ -322,6 +323,13 @@ void MultibufferDataSource::Stop() {
   {
     base::AutoLock auto_lock(lock_);
     StopInternal_Locked();
+
+    // Cleanup resources immediately if we're already on the right thread.
+    if (render_task_runner_->BelongsToCurrentThread()) {
+      reader_.reset();
+      url_data_and_loading_state_.SetUrlData(nullptr);
+      return;
+    }
   }
 
   render_task_runner_->PostTask(FROM_HERE,
@@ -634,25 +642,21 @@ void MultibufferDataSource::ProgressCallback(int64_t begin, int64_t end) {
   DVLOG(1) << __func__ << "(" << begin << ", " << end << ")";
   DCHECK(render_task_runner_->BelongsToCurrentThread());
 
-  if (assume_fully_buffered())
-    return;
-
   base::AutoLock auto_lock(lock_);
+  if (stop_signal_received_)
+    return;
 
-  if (end > begin) {
-    // TODO(scherkus): we shouldn't have to lock to signal host(), see
-    // http://crbug.com/113712 for details.
-    if (stop_signal_received_)
-      return;
+  if (assume_fully_buffered())
+    return;
 
+  if (end > begin)
     host_->AddBufferedByteRange(begin, end);
-  }
 
-  if (buffer_size_update_counter_ > 0) {
+  if (buffer_size_update_counter_ > 0)
     buffer_size_update_counter_--;
-  } else {
+  else
     UpdateBufferSizes();
-  }
+
   UpdateLoadingState_Locked(false);
 }
 

media/blink/webmediaplayer_impl.cc

@@ -210,6 +210,65 @@ bool IsLocalFile(const GURL& url) {
 }
 #endif
 
+// Handles destruction of media::Renderer dependent components after the
+// renderer has been destructed on the media thread.
+void DestructionHelper(
+    scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> vfc_task_runner,
+    std::unique_ptr<Demuxer> demuxer,
+    std::unique_ptr<DataSource> data_source,
+    std::unique_ptr<VideoFrameCompositor> compositor,
+    std::unique_ptr<CdmContextRef> cdm_context_1,
+    std::unique_ptr<CdmContextRef> cdm_context_2,
+    std::unique_ptr<MediaLog> media_log,
+    std::unique_ptr<RendererFactorySelector> renderer_factory_selector,
+    bool is_chunk_demuxer) {
+  // Since the media::Renderer is gone we can now destroy the compositor and
+  // renderer factory selector.
+  vfc_task_runner->DeleteSoon(FROM_HERE, std::move(compositor));
+  main_task_runner->DeleteSoon(FROM_HERE, std::move(renderer_factory_selector));
+
+  // ChunkDemuxer can be deleted on any thread, but other demuxers are bound to
+  // the main thread and must be deleted there now that the renderer is gone.
+  if (!is_chunk_demuxer) {
+    main_task_runner->DeleteSoon(FROM_HERE, std::move(demuxer));
+    main_task_runner->DeleteSoon(FROM_HERE, std::move(data_source));
+    main_task_runner->DeleteSoon(FROM_HERE, std::move(cdm_context_1));
+    main_task_runner->DeleteSoon(FROM_HERE, std::move(cdm_context_2));
+    main_task_runner->DeleteSoon(FROM_HERE, std::move(media_log));
+    return;
+  }
+
+  // ChunkDemuxer's streams may contain much buffered, compressed media that
+  // may need to be paged back in during destruction.  Paging delay may exceed
+  // the renderer hang monitor's threshold on at least Windows while also
+  // blocking other work on the renderer main thread, so we do the actual
+  // destruction in the background without blocking WMPI destruction or
+  // |task_runner|.  On advice of task_scheduler OWNERS, MayBlock() is not
+  // used because virtual memory overhead is not considered blocking I/O; and
+  // CONTINUE_ON_SHUTDOWN is used to allow process termination to not block on
+  // completing the task.
+  base::PostTaskWithTraits(
+      FROM_HERE,
+      {base::TaskPriority::BEST_EFFORT,
+       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+      base::BindOnce(
+          [](scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+             std::unique_ptr<Demuxer> demuxer_to_destroy,
+             std::unique_ptr<CdmContextRef> cdm_context_1,
+             std::unique_ptr<CdmContextRef> cdm_context_2,
+             std::unique_ptr<MediaLog> media_log) {
+            SCOPED_UMA_HISTOGRAM_TIMER("Media.MSE.DemuxerDestructionTime");
+            demuxer_to_destroy.reset();
+            main_task_runner->DeleteSoon(FROM_HERE, std::move(cdm_context_1));
+            main_task_runner->DeleteSoon(FROM_HERE, std::move(cdm_context_2));
+            main_task_runner->DeleteSoon(FROM_HERE, std::move(media_log));
+          },
+          std::move(main_task_runner), std::move(demuxer),
+          std::move(cdm_context_1), std::move(cdm_context_2),
+          std::move(media_log)));
+}
+
 }  // namespace
 
 class BufferedDataSourceHostImpl;
@@ -360,6 +419,11 @@ WebMediaPlayerImpl::~WebMediaPlayerImpl() {
   watch_time_reporter_.reset();
 
   // The underlying Pipeline must be stopped before it is destroyed.
+  //
+  // Note: This destruction happens synchronously on the media thread and
+  // |demuxer_|, |data_source_|, |compositor_|, and |media_log_| must outlive
+  // this process. They will be destructed by the DestructionHelper below
+  // after trampolining through the media thread.
   pipeline_controller_.Stop();
 
   if (last_reported_memory_usage_)
@@ -371,48 +435,25 @@ WebMediaPlayerImpl::~WebMediaPlayerImpl() {
   client_->MediaRemotingStopped(
       blink::WebLocalizedString::kMediaRemotingStopNoText);
 
-  if (!surface_layer_for_video_enabled_ && video_layer_) {
+  if (!surface_layer_for_video_enabled_ && video_layer_)
     video_layer_->StopUsingProvider();
-  }
-
-  vfc_task_runner_->DeleteSoon(FROM_HERE, std::move(compositor_));
-
-  if (chunk_demuxer_) {
-    // Continue destruction of |chunk_demuxer_| on the |media_task_runner_| to
-    // avoid racing other pending tasks on |chunk_demuxer_| on that runner while
-    // not further blocking |main_task_runner_| to perform the destruction.
-    media_task_runner_->PostTask(
-        FROM_HERE, base::BindOnce(&WebMediaPlayerImpl::DemuxerDestructionHelper,
-                                  media_task_runner_, std::move(demuxer_)));
-  }
 
   media_log_->AddEvent(
       media_log_->CreateEvent(MediaLogEvent::WEBMEDIAPLAYER_DESTROYED));
-}
 
-// static
-void WebMediaPlayerImpl::DemuxerDestructionHelper(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    std::unique_ptr<Demuxer> demuxer) {
-  DCHECK(task_runner->BelongsToCurrentThread());
-  // ChunkDemuxer's streams may contain much buffered, compressed media that may
-  // need to be paged back in during destruction.  Paging delay may exceed the
-  // renderer hang monitor's threshold on at least Windows while also blocking
-  // other work on the renderer main thread, so we do the actual destruction in
-  // the background without blocking WMPI destruction or |task_runner|.  On
-  // advice of task_scheduler OWNERS, MayBlock() is not used because virtual
-  // memory overhead is not considered blocking I/O; and CONTINUE_ON_SHUTDOWN is
-  // used to allow process termination to not block on completing the task.
-  base::PostTaskWithTraits(
+  if (data_source_)
+    data_source_->Stop();
+
+  // Handle destruction of things that need to be destructed after the pipeline
+  // completes stopping on the media thread.
+  media_task_runner_->PostTask(
       FROM_HERE,
-      {base::TaskPriority::BEST_EFFORT,
-       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
-      base::BindOnce(
-          [](std::unique_ptr<Demuxer> demuxer_to_destroy) {
-            SCOPED_UMA_HISTOGRAM_TIMER("Media.MSE.DemuxerDestructionTime");
-            demuxer_to_destroy.reset();
-          },
-          std::move(demuxer)));
+      base::BindOnce(&DestructionHelper, std::move(main_task_runner_),
+                     std::move(vfc_task_runner_), std::move(demuxer_),
+                     std::move(data_source_), std::move(compositor_),
+                     std::move(cdm_context_ref_),
+                     std::move(pending_cdm_context_ref_), std::move(media_log_),
+                     std::move(renderer_factory_selector_), !!chunk_demuxer_));
 }
 
 WebMediaPlayer::LoadTiming WebMediaPlayerImpl::Load(

media/blink/webmediaplayer_impl.h

@@ -109,15 +109,6 @@ class MEDIA_BLINK_EXPORT WebMediaPlayerImpl
       std::unique_ptr<WebMediaPlayerParams> params);
   ~WebMediaPlayerImpl() override;
 
-  // Destroys |demuxer| and records a UMA for the time taken to destroy it.
-  // |task_runner| is the expected runner on which this method is called, and is
-  // used as a parameter to ensure a scheduled task bound to this method is run
-  // (to prevent uncontrolled |demuxer| destruction if |task_runner| has no
-  // other references before such task is executed.)
-  static void DemuxerDestructionHelper(
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-      std::unique_ptr<Demuxer> demuxer);
-
   // WebSurfaceLayerBridgeObserver implementation.
   void OnWebLayerUpdated() override;
   void RegisterContentsLayer(cc::Layer* layer) override;

media/blink/webmediaplayer_impl_unittest.cc

@@ -424,7 +424,7 @@ class WebMediaPlayerImplTest : public testing::Test {
     // destructed since WMPI may reference them during destruction.
     wmpi_.reset();
 
-    base::RunLoop().RunUntilIdle();
+    CycleThreads();
 
     web_view_->MainFrameWidget()->Close();
   }
@@ -669,13 +669,8 @@ class WebMediaPlayerImplTest : public testing::Test {
     // Ensure any tasks waiting to be posted to the media thread are posted.
     base::RunLoop().RunUntilIdle();
 
-    // Cycle media thread.
-    {
-      base::RunLoop loop;
-      media_thread_.task_runner()->PostTaskAndReply(
-          FROM_HERE, base::DoNothing(), loop.QuitClosure());
-      loop.Run();
-    }
+    // Flush all media tasks.
+    media_thread_.FlushForTesting();
 
     // Cycle anything that was posted back from the media thread.
     base::RunLoop().RunUntilIdle();