Report WMPI components memory usage to UMA

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 4cc6abe818db3059ca58ade50b6d58ec021ff159..bd3380cdab0f59dd81f538d1bb13b702395531be 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -18,6 +18,7 @@
 #include "base/debug/alias.h"
 #include "base/debug/crash_logging.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
@@ -1080,7 +1081,7 @@ void WebMediaPlayerImpl::OnPipelineSuspended() {
       data_source_->OnBufferingHaveEnough(true);
   }
 
-  ReportMemoryUsage();
+  ReportMemoryUsage(MaybeMemPressureLvl());
 
   if (pending_suspend_resume_cycle_) {
     pending_suspend_resume_cycle_ = false;
@@ -1092,6 +1093,17 @@ void WebMediaPlayerImpl::OnDemuxerOpened() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   client_->mediaSourceOpened(
       new WebMediaSourceImpl(chunk_demuxer_, media_log_));
+  // Using base::Unretained(this) is safe here, because MemoryPressureListener
+  // object is owned by this (WMPI::mem_pressure_listener_).

[watk, 2016/12/13 23:34:31]

"because the".

Or if you want to reword it: "base::Unretained is safe because |this| outlives
memory_pressure_listener_"

[servolk, 2016/12/14 00:07:23]

Done.

+  mem_pressure_listener_ =
+      base::WrapUnique(new base::MemoryPressureListener(base::Bind(

[watk, 2016/12/13 23:34:31]

Prefer MakeUnique(). (Unless there's a reason you can't)

[servolk, 2016/12/14 00:07:23]

Done.

+          &WebMediaPlayerImpl::OnMemoryPressure, base::Unretained(this))));
+}
+
+void WebMediaPlayerImpl::OnMemoryPressure(
+    base::MemoryPressureListener::MemoryPressureLevel lvl) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  ReportMemoryUsage(lvl);
 }
 
 void WebMediaPlayerImpl::OnError(PipelineStatus status) {
@@ -1199,7 +1211,7 @@ void WebMediaPlayerImpl::OnBufferingStateChange(BufferingState state) {
 
     // Once we have enough, start reporting the total memory usage. We'll also
     // report once playback starts.
-    ReportMemoryUsage();
+    ReportMemoryUsage(MaybeMemPressureLvl());
 
     // Report the amount of time it took to leave the underflow state. Don't
     // bother to report this for MSE playbacks since it's out of our control.
@@ -1764,12 +1776,12 @@ void WebMediaPlayerImpl::SetMemoryReportingState(
   }
 
   if (is_memory_reporting_enabled) {
-    memory_usage_reporting_timer_.Start(FROM_HERE,
-                                        base::TimeDelta::FromSeconds(2), this,
-                                        &WebMediaPlayerImpl::ReportMemoryUsage);
+    memory_usage_reporting_timer_.Start(
+        FROM_HERE, base::TimeDelta::FromSeconds(2), this,
+        &WebMediaPlayerImpl::ReportMemoryUsageOnTimer);
   } else {
     memory_usage_reporting_timer_.Stop();
-    ReportMemoryUsage();
+    ReportMemoryUsage(MaybeMemPressureLvl());
   }
 }
 
@@ -1910,7 +1922,12 @@ WebMediaPlayerImpl::UpdatePlayState_ComputePlayState(bool is_remote,
   return result;
 }
 
-void WebMediaPlayerImpl::ReportMemoryUsage() {
+void WebMediaPlayerImpl::ReportMemoryUsageOnTimer() {

[watk, 2016/12/13 23:34:31]

Can you bind the nullopt into the callback and delete this?

[servolk, 2016/12/14 00:07:23]

This is passed into timer.Start (see
https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0...).
For some reason the timer start API is using a function pointer and an object
pointer instead of something like base::Closure (see
https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=238).
I'm not sure why that is, but I'd prefer not to change base::Timer API in this
CL, so leaving it as it is for now.

[servolk, 2016/12/14 00:11:23]

Ah, wait, despite the weird Start method at
https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=238
there is also a friendlier (taking base::Closure) Timer::Start in the base class
(https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=110).
So maybe I will be able to get rid of this. I'll give it a try in the next
patchset.

[servolk, 2016/12/14 00:15:37]

Done.

+  ReportMemoryUsage(MaybeMemPressureLvl());
+}
+
+void WebMediaPlayerImpl::ReportMemoryUsage(
+    MaybeMemPressureLvl maybe_mem_pressure_lvl) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   // About base::Unretained() usage below: We destroy |demuxer_| on the main
@@ -1922,32 +1939,70 @@ void WebMediaPlayerImpl::ReportMemoryUsage() {
     base::PostTaskAndReplyWithResult(
         media_task_runner_.get(), FROM_HERE,
         base::Bind(&Demuxer::GetMemoryUsage, base::Unretained(demuxer_.get())),
-        base::Bind(&WebMediaPlayerImpl::FinishMemoryUsageReport, AsWeakPtr()));
+        base::Bind(&WebMediaPlayerImpl::FinishMemoryUsageReport, AsWeakPtr(),
+                   maybe_mem_pressure_lvl));
   } else {
-    FinishMemoryUsageReport(0);
+    FinishMemoryUsageReport(maybe_mem_pressure_lvl, 0);
   }
 }
 
-void WebMediaPlayerImpl::FinishMemoryUsageReport(int64_t demuxer_memory_usage) {
+std::string MaybeMemPressureToLogString(
+    const base::Optional<base::MemoryPressureListener::MemoryPressureLevel>&

[watk, 2016/12/13 23:34:31]

Use the type alias?

[servolk, 2016/12/14 00:07:23]

I could, but the type alias is currently private in WMPI. Do you think it's
worth making the type alias public just to use it here?

[watk, 2016/12/14 00:34:05]

Ah, didn't notice this was a non-member function. Leaving it sgtm

[servolk, 2016/12/14 02:34:33]

Acknowledged.

+        maybe_mem_pressure_lvl) {
+  if (maybe_mem_pressure_lvl) {
+    switch (*maybe_mem_pressure_lvl) {
+      case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE:
+        return "none";
+      case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE:
+        return "moderate";
+      case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL:
+        return "critical";
+    }
+  }
+  return "";
+}
+
+void WebMediaPlayerImpl::FinishMemoryUsageReport(
+    MaybeMemPressureLvl maybe_mem_pressure_lvl,
+    int64_t demuxer_memory_usage) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   const PipelineStatistics stats = pipeline_.GetStatistics();
+  const int64_t data_source_mem_usage =

[watk, 2016/12/13 23:34:31]

"memory_usage" for consistency with other variables.

[servolk, 2016/12/14 00:07:23]

Done.

+      (data_source_ ? data_source_->GetMemoryUsage() : 0);
   const int64_t current_memory_usage =
       stats.audio_memory_usage + stats.video_memory_usage +
-      (data_source_ ? data_source_->GetMemoryUsage() : 0) +
-      demuxer_memory_usage;
+      data_source_mem_usage + demuxer_memory_usage;
+
+  std::string mem_pressure_log =
+      MaybeMemPressureToLogString(maybe_mem_pressure_lvl);
+  if (mem_pressure_log != "")
+    mem_pressure_log = " (Mem pressure: " + mem_pressure_log + ")";

[watk, 2016/12/13 23:34:31]

Do this inside MaybeMemPressureToLogString and inline the call into the DVLOG
below?

[servolk, 2016/12/14 00:07:23]

This would make MaybeMemPressureToLogString less re-usable (usable only in this
context). Perhaps we'll want to convert MemoryPressure enum into string in
somewhere else, where we don't want to add this log comment to it.

 
   // Note, this isn't entirely accurate, there may be VideoFrames held by the
   // compositor or other resources that we're unaware of.
-
-  DVLOG(2) << "Memory Usage -- Audio: " << stats.audio_memory_usage
-           << ", Video: " << stats.video_memory_usage << ", DataSource: "
-           << (data_source_ ? data_source_->GetMemoryUsage() : 0)
+  DVLOG(2) << "Memory Usage " << mem_pressure_log
+           << " -- Audio: " << stats.audio_memory_usage
+           << ", Video: " << stats.video_memory_usage
+           << ", DataSource: " << data_source_mem_usage
            << ", Demuxer: " << demuxer_memory_usage;
 
   const int64_t delta = current_memory_usage - last_reported_memory_usage_;
   last_reported_memory_usage_ = current_memory_usage;
   adjust_allocated_memory_cb_.Run(delta);
+
+  if (maybe_mem_pressure_lvl &&

[DaleCurtis, 2016/12/12 23:49:48]

Hmm, why? Why not just always report the average memory usage? I think we should
use pressure to evict, but I don't think we should be slicing the data before
reporting based on it.

[servolk, 2016/12/12 23:56:12]

ReportMemoryUsage and FinishMemoryUsageReport are invoked every 2 seconds using
memory_usage_reporting_timer_, not sure if we want to report to UMA that often.
I'm pretty sure that we always want to report media memory usage when we are
under memory pressure. So the way this CL currently implements it is we'll only
report stats to UMA if this got invoked due to memory pressure being reported,
and we won't report stats to UMA for regular 2-second timer updates. I can
change it to always report to UMA, even for timer updates, I'm just not sure if
that a good idea, WDYT?

[DaleCurtis, 2016/12/13 00:04:07]

Reporting UMA is mostly just updating a static value IIRC; certainly every 2
seconds seems fine. I don't think it actually does much on a per sample basis,
though I defer to UMA folk.

+      *maybe_mem_pressure_lvl >=
+          base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE) {
+    UMA_HISTOGRAM_MEMORY_KB("Media.WebMediaPlayerImpl.Memory.AudioKb",
+                            stats.audio_memory_usage / 1024);
+    UMA_HISTOGRAM_MEMORY_KB("Media.WebMediaPlayerImpl.Memory.VideoKb",
+                            stats.video_memory_usage / 1024);
+    UMA_HISTOGRAM_MEMORY_KB("Media.WebMediaPlayerImpl.Memory.DataSourceKb",
+                            stats.video_memory_usage / 1024);
+    UMA_HISTOGRAM_MEMORY_KB("Media.WebMediaPlayerImpl.Memory.DemuxerKb",
+                            demuxer_memory_usage / 1024);
+  }
 }
 
 void WebMediaPlayerImpl::ScheduleIdlePauseTimer() {