Run Demuxer::GetMemoryUsage() on media thread

media/blink/webmediaplayer_impl.cc

Index: media/blink/webmediaplayer_impl.cc
diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc
index 2e148192b01a65d44b370ecd4f2a195e5d28ad68..c0eab2d68b999010587173db91ade9287dbb5181 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -17,6 +17,7 @@
 #include "base/metrics/histogram.h"
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/task_runner_util.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/blink/web_layer_impl.h"
@@ -894,7 +895,7 @@ void WebMediaPlayerImpl::OnPipelineBufferingStateChanged(
 
   // Once we have enough, start reporting the total memory usage. We'll also
   // report once playback starts.
-  ReportMemoryUsage();
+  StartMemoryUsageReport();
 }
 
 void WebMediaPlayerImpl::OnDemuxerOpened() {
@@ -1103,9 +1104,9 @@ void WebMediaPlayerImpl::NotifyPlaybackStarted() {
   if (delegate_)
     delegate_->DidPlay(this);
   if (!memory_usage_reporting_timer_.IsRunning()) {
-    memory_usage_reporting_timer_.Start(FROM_HERE,
-                                        base::TimeDelta::FromSeconds(2), this,
-                                        &WebMediaPlayerImpl::ReportMemoryUsage);
+    memory_usage_reporting_timer_.Start(

[DaleCurtis, 2015/12/01 20:14:20]

Should we run the timer on the media thread instead? Right now it's
RenderThread::PostDelayedTask()->MediaThread::PostTask()->RenderThread::PostTask()
versus MediaThread::PostDelayedTask()->RenderThread::PostTask().

[wdzierzanowski, 2015/12/02 09:27:54]

Interesting. I didn't think of that.

While we could indeed save one thread hop per each of the repeating memory
reports, it seems this would have to come at the cost of increased code
complexity:

  - The timer would have to be destroyed on the media thread as well.

  - We would no longer be able to use the handy PostTaskAndReplyWithResult()
utility. Instead, we would have to handle the sequence of calls on different
threads manually: The timer would call a WebMediaPlayerImpl function on the
media thread, which would post a task to the render thread.

  - With more pieces of WebMediaPlayerImpl state accessed on the media thread
there would come more instances of base::Unretained() uses requiring analysis of
threading issues. Some cases would become particularly hard, like checking for
existence of |demuxer_| on the media thread in a non-racy way given that it is
created on the renderer thread.

So I'm afraid in trying to save one thread hop in a piece of work that happens
once every 2 seconds we would make the implementation more difficult to follow
and error-prone.

+        FROM_HERE, base::TimeDelta::FromSeconds(2), this,
+        &WebMediaPlayerImpl::StartMemoryUsageReport);
   }
 }
 
@@ -1113,22 +1114,40 @@ void WebMediaPlayerImpl::NotifyPlaybackPaused() {
   if (delegate_)
     delegate_->DidPause(this);
   memory_usage_reporting_timer_.Stop();
-  ReportMemoryUsage();
+  StartMemoryUsageReport();
 }
 
-void WebMediaPlayerImpl::ReportMemoryUsage() {
+void WebMediaPlayerImpl::StartMemoryUsageReport() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  // About base::Unretained() usage below: We destroy |demuxer_| on the main
+  // thread.  Before that, however, ~WebMediaPlayerImpl() posts a task to the
+  // media thread and waits for it to finish.  Hence, the GetMemoryUsage() task
+  // posted here must finish earlier.
+
+  if (demuxer_) {
+    base::PostTaskAndReplyWithResult(
+        media_task_runner_.get(), FROM_HERE,
+        base::Bind(&Demuxer::GetMemoryUsage, base::Unretained(demuxer_.get())),
+        base::Bind(&WebMediaPlayerImpl::FinishMemoryUsageReport, AsWeakPtr()));
+  } else {
+    FinishMemoryUsageReport(0);
+  }
+}
+
+void WebMediaPlayerImpl::FinishMemoryUsageReport(int64_t demuxer_memory_usage) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   const PipelineStatistics stats = pipeline_.GetStatistics();
   const int64_t current_memory_usage =
       stats.audio_memory_usage + stats.video_memory_usage +
       (data_source_ ? data_source_->GetMemoryUsage() : 0) +
-      (demuxer_ ? demuxer_->GetMemoryUsage() : 0);
+      demuxer_memory_usage;
 
   DVLOG(2) << "Memory Usage -- Audio: " << stats.audio_memory_usage
            << ", Video: " << stats.video_memory_usage << ", DataSource: "
            << (data_source_ ? data_source_->GetMemoryUsage() : 0)
-           << ", Demuxer: " << (demuxer_ ? demuxer_->GetMemoryUsage() : 0);
+           << ", Demuxer: " << demuxer_memory_usage;
 
   const int64_t delta = current_memory_usage - last_reported_memory_usage_;
   last_reported_memory_usage_ = current_memory_usage;