Include playbacks which never underflow in underflow metrics.

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>
@@ skipping 1066 matching lines @@
 void WebMediaPlayerImpl::OnBufferingStateChange(BufferingState state) {
   DVLOG(1) << __func__ << "(" << state << ")";
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   // Ignore buffering state changes until we've completed all outstanding
   // operations.
   if (!pipeline_controller_.IsStable())
     return;
 
   if (state == BUFFERING_HAVE_ENOUGH) {
+    if (data_source_ &&
+        highest_ready_state_ < WebMediaPlayer::ReadyStateHaveEnoughData) {
+      DCHECK_EQ(underflow_count_, 0);
+      // Record a zero value for underflow histograms so that the histogram
+      // includes playbacks which never encounter an underflow event.
+      UMA_HISTOGRAM_COUNTS_100("Media.UnderflowCount", 0);

[Steven Holte, 2016/09/07 00:24:26]

You should probably consolidate the macro calls for these histograms into a
single location, using a helper function, since each call will create a local
static variable.

+      UMA_HISTOGRAM_TIMES("Media.UnderflowDuration", base::TimeDelta());
+    }
+
     // TODO(chcunningham): Monitor playback position vs buffered. Potentially
     // transition to HAVE_FUTURE_DATA here if not enough is buffered.
     SetReadyState(WebMediaPlayer::ReadyStateHaveEnoughData);
 
     // Let the DataSource know we have enough data. It may use this information
     // to release unused network connections.
     if (data_source_)
       data_source_->OnBufferingHaveEnough(false);
 
     // Blink expects a timeChanged() in response to a seek().
@@ skipping 15 matching lines @@
     // Buffering has underflowed.
     DCHECK_EQ(state, BUFFERING_HAVE_NOTHING);
     // It shouldn't be possible to underflow if we've not advanced past
     // HAVE_CURRENT_DATA.
     DCHECK_GT(highest_ready_state_, WebMediaPlayer::ReadyStateHaveCurrentData);
     SetReadyState(WebMediaPlayer::ReadyStateHaveCurrentData);
 
     // Report the number of times we've entered the underflow state. Only report
     // for src= playback since for MSE it's out of our control.
     if (data_source_) {
       UMA_HISTOGRAM_COUNTS_100("Media.UnderflowCount", ++underflow_count_);

[Steven Holte, 2016/09/07 00:24:26]

Generally not a good idea to have side-effecting like ++x in a macro call.  It
should behave correctly in this case, since there is only 1 reference to this
argument, but would be a good thing to fix.

       underflow_timer_.reset(new base::ElapsedTimer());
     }
   }
 
   UpdatePlayState();
 }
 
 void WebMediaPlayerImpl::OnDurationChange() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
@@ skipping 675 matching lines @@
       pipeline_metadata_.natural_size,
       base::Bind(&GetCurrentTimeInternal, this)));
   watch_time_reporter_->OnVolumeChange(volume_);
   if (delegate_ && delegate_->IsHidden())
     watch_time_reporter_->OnHidden();
   else
     watch_time_reporter_->OnShown();
 }
 
 }  // namespace media