Report WMPI components memory usage to UMA

media/blink/webmediaplayer_impl.cc

 // Copyright 2013 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/blink/webmediaplayer_impl.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #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"
 #include "base/task_runner_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "cc/blink/web_layer_impl.h"
 #include "cc/layers/video_layer.h"
 #include "media/audio/null_audio_sink.h"
@@ skipping 1042 matching lines @@
 #endif
 
   // If we're not in an aggressive buffering state, tell the data source we have
   // enough data so that it may release the connection.
   if (buffering_strategy_ !=
       MultibufferDataSource::BUFFERING_STRATEGY_AGGRESSIVE) {
     if (data_source_)
       data_source_->OnBufferingHaveEnough(true);
   }
 
-  ReportMemoryUsage();
+  ReportMemoryUsage(MaybeMemPressureLvl());
 
   if (pending_suspend_resume_cycle_) {
     pending_suspend_resume_cycle_ = false;
     UpdatePlayState();
   }
 }
 
 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) {
   DVLOG(1) << __func__;
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK_NE(status, PIPELINE_OK);
 
   if (suppress_destruction_errors_)
     return;
 
@@ skipping 87 matching lines @@
     // to release unused network connections.
     if (data_source_)
       data_source_->OnBufferingHaveEnough(false);
 
     // Blink expects a timeChanged() in response to a seek().
     if (should_notify_time_changed_)
       client_->timeChanged();
 
     // 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.
     if (underflow_timer_ && data_source_) {
       UMA_HISTOGRAM_TIMES("Media.UnderflowDuration",
                           underflow_timer_->Elapsed());
       underflow_timer_.reset();
     }
   } else {
     // Buffering has underflowed.
@@ skipping 544 matching lines @@
 }
 
 void WebMediaPlayerImpl::SetMemoryReportingState(
     bool is_memory_reporting_enabled) {
   if (memory_usage_reporting_timer_.IsRunning() ==
       is_memory_reporting_enabled) {
     return;
   }
 
   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());
   }
 }
 
 void WebMediaPlayerImpl::SetSuspendState(bool is_suspended) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   // Do not change the state after an error has occurred.
   // TODO(sandersd): Update PipelineController to remove the need for this.
   if (IsNetworkStateError(network_state_))
     return;
@@ skipping 120 matching lines @@
   }
 
   // It's not critical if some cases where memory usage can change are missed,
   // since media memory changes are usually gradual.
   result.is_memory_reporting_enabled =
       can_play && !result.is_suspended && !paused_;
 
   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
   // 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()));
+        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() {
   // Only schedule the pause timer if we're playing and are suspended.
   if (paused_ || !pipeline_controller_.IsSuspended())
     return;
 
 #if defined(OS_ANDROID)
   // Remote players will be suspended and locally paused.
   if (isRemote())
@@ skipping 18 matching lines @@
     watch_time_reporter_->OnShown();
 }
 
 bool WebMediaPlayerImpl::IsHidden() const {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   return delegate_ && delegate_->IsHidden();
 }
 
 }  // namespace media