Added UMA metrics for MediaSourcePlayer and MediaCodecPlayer

Added UMA metrics for MediaSourcePlayer and MediaCodecPlayer

Added 4 new histograms:
  Media.MSE.LateAudioFrames,
  Media.MSE.LateVideoFrames,
  Media.MSE.PlaybackDuration,
  Media.MSE.Starvations.

Each player holds MediaStatistics object which gathers events
during playback and calculates and reports results after the
playback is stopped.

BUG=407577
Committed: https://crrev.com/1ea2b2ca612e9889a5638b83f355176e7ea2c990
Cr-Commit-Position: refs/heads/master@{#351975}

[Tima Vaisburd, 2015-09-29 05:06:25]

timav@chromium.org changed reviewers:
+ isherman@chromium.org, qinmin@chromium.org, xhwang@chromium.org

[Tima Vaisburd, 2015-09-29 05:06:26]

PTAL

xhwang@ for additions to directory
isherman@ for histograms.xml

[qinmin, 2015-09-29 23:47:44]

https://codereview.chromium.org/1367403003/diff/1/media/base/android/audio_de...
File media/base/android/audio_decoder_job.cc (right):

https://codereview.chromium.org/1367403003/diff/1/media/base/android/audio_de...
media/base/android/audio_decoder_job.cc:128: if (frame_statistics_ &&
!next_frame_time_limit_.is_null() &&
I think we can just check the head_position variable to determine if audio
underruns, no?

https://codereview.chromium.org/1367403003/diff/1/media/base/android/audio_de...
media/base/android/audio_decoder_job.cc:130:
frame_statistics_->AddLateFrame(current_time - next_frame_time_limit_);
{} needed

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
File media/base/android/media_statistics.cc (right):

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.cc:62: base::TimeDelta duration =
base::TimeTicks::Now() - start_time_;
The duration is calculated from system time. Should we use the playback duration
instead?

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
File media/base/android/media_statistics.h (right):

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.h:18: uint32_t total;
provide comments on these variables.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.h:41: FrameStatistics& AudioFrames() {
return audio_frames_; }
audio_frames();

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.h:42: FrameStatistics& VideoFrames() {
return video_frames_; }
video_frames()

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.h:43: void Start();
comments on member functions

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:17686: +<histogram
name="Media.MSE.LateAudioFrames">
I think LateAudioFramesPerMillion is better

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:17695: +<histogram
name="Media.MSE.LateVideoFrames">
ditto

[Ilya Sherman, 2015-09-30 04:20:03]

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
File media/base/android/media_statistics.cc (right):

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.cc:18: const int kNumDurationSlots = 1800;
nit: Please leave a blank line after this one, for consistency with line 10.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.cc:18: const int kNumDurationSlots = 1800;
Sorry, 1800 buckets is far too many for a histogram.  The typical number is 50. 
If you'd like to use more, could you please help me understand why -- i.e. what
you're trying to measure, that the standard number of buckets is insufficient
for?

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:17686: +<histogram
name="Media.MSE.LateAudioFrames">
nit; You might also add units="frames per million" or something.

[Tima Vaisburd, 2015-09-30 18:50:10]

https://codereview.chromium.org/1367403003/diff/1/media/base/android/audio_de...
File media/base/android/audio_decoder_job.cc (right):

https://codereview.chromium.org/1367403003/diff/1/media/base/android/audio_de...
media/base/android/audio_decoder_job.cc:128: if (frame_statistics_ &&
!next_frame_time_limit_.is_null() &&
On 2015/09/29 23:47:44, qinmin wrote:
> I think we can just check the head_position variable to determine if audio
> underruns, no?

Having the delay makes it possible to track it down if we want to do it later,
e.g. make a histogram of delay. It also make it similar to video frame
statistics so we can pass a smaller frame statistics object into decoders. What
do you think?

https://codereview.chromium.org/1367403003/diff/1/media/base/android/audio_de...
media/base/android/audio_decoder_job.cc:130:
frame_statistics_->AddLateFrame(current_time - next_frame_time_limit_);
On 2015/09/29 23:47:44, qinmin wrote:
> {} needed

Done.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
File media/base/android/media_statistics.cc (right):

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.cc:18: const int kNumDurationSlots = 1800;
On 2015/09/30 04:20:03, Ilya Sherman wrote:
> nit: Please leave a blank line after this one, for consistency with line 10.

Done.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.cc:18: const int kNumDurationSlots = 1800;
On 2015/09/30 04:20:03, Ilya Sherman wrote:
> Sorry, 1800 buckets is far too many for a histogram.  The typical number is
50. 
> If you'd like to use more, could you please help me understand why -- i.e.
what
> you're trying to measure, that the standard number of buckets is insufficient
> for?

Sorry, it was my bad thinking. Changed to 60 instead of standard 50, one slot
per minute.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
File media/base/android/media_statistics.h (right):

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.h:18: uint32_t total;
On 2015/09/29 23:47:44, qinmin wrote:
> provide comments on these variables.

Done.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.h:41: FrameStatistics& AudioFrames() {
return audio_frames_; }
On 2015/09/29 23:47:44, qinmin wrote:
> audio_frames();

Done.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.h:42: FrameStatistics& VideoFrames() {
return video_frames_; }
On 2015/09/29 23:47:44, qinmin wrote:
> video_frames()

Done.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.h:43: void Start();
On 2015/09/29 23:47:44, qinmin wrote:
> comments on member functions

Done.

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:17686: +<histogram
name="Media.MSE.LateAudioFrames">
On 2015/09/30 04:20:03, Ilya Sherman wrote:
> nit; You might also add units="frames per million" or something.

Added units but did not changes the histogram names, is this sufficient?

[xhwang, 2015-09-30 21:02:58]

My comments are split into two PS again. Sorry for the confusion.

https://chromiumcodereview.appspot.com/1367403003/diff/1/media/base/android/m...
File media/base/android/media_codec_decoder.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/1/media/base/android/m...
media/base/android/media_codec_decoder.cc:524: return last_frame_posted_;
nit: a boolean variable should be named as is_* or has_*. When looking at this
my first reaction is that this returns the last frame that is posted.

https://chromiumcodereview.appspot.com/1367403003/diff/1/media/base/android/m...
File media/base/android/media_statistics.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/1/media/base/android/m...
media/base/android/media_statistics.cc:45: DVLOG(1) << "MediaStatistics::" <<
__FUNCTION__;
nit: class name not necessary in the log..

https://chromiumcodereview.appspot.com/1367403003/diff/1/media/base/android/m...
media/base/android/media_statistics.cc:103: 1000000 * video_frames_.late /
video_frames_.total);
hmm, I wonder why we report frames per milliion frames? Can't we just report
this as a percentage?

https://chromiumcodereview.appspot.com/1367403003/diff/1/tools/metrics/histog...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/1367403003/diff/1/tools/metrics/histog...
tools/metrics/histograms/histograms.xml:17683: +  <summary>Duration of an
uninterrupted MSE playback in seconds.</summary>
+wolenetz as Media.MSE.Duration could be a general histogram not limited to
Clank. Matt: How is duration defined for MSE in general?

What's the exact meaning of "uninterrupted MSE playback"? What if the user seeks
during playback? Is this in wall clock time, or in media time?

In media, "Duration" has a clean meaning [1]. The "Media.Duration" UMA follows
this meaning. Here it seems we are recording something different. Can we use a
different name to avoid confusion? e.g. *.PlaybackDuration.

[1]
http://www.w3.org/html/wg/drafts/html/master/semantics.html#dom-media-duration

https://chromiumcodereview.appspot.com/1367403003/diff/1/tools/metrics/histog...
tools/metrics/histograms/histograms.xml:17690: +    Relative number of late
audio frames wrt total number of audio frames in MSE
Could you please elaborate what is exactly a "late audio frame"?

https://chromiumcodereview.appspot.com/1367403003/diff/1/tools/metrics/histog...
tools/metrics/histograms/histograms.xml:17699: +    Relative number of late
video frames wrt total number of video frames in MSE
ditto

https://chromiumcodereview.appspot.com/1367403003/diff/1/tools/metrics/histog...
tools/metrics/histograms/histograms.xml:17727: +    Relative number of
starvations wrt total number of frames in MSE playback,
ditto, define "starvation".

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
File media/base/android/media_codec_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_codec_audio_decoder.cc:214:
frame_statistics_->AddFrame();
When can frame_statistics_ be null? Is it only in unit tests? If so, can we fix
the test and always make sure frame_statistics_ is non-null?

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
File media/base/android/media_codec_decoder.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_codec_decoder.cc:46:
frame_statistics_(frame_statistics),
nit: Make the initialization order consistent with the parameter order.

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_codec_player.cc:1256: // This methos needs to be called
when decoder threads are not processing
s/methos/method

Also, I don't quite understand this comment. Could you please elaborate?

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
File media/base/android/media_source_player.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_source_player.cc:529:
media_stat_->StopAndReport(GetCurrentTime());
Why should we StopAndReport() after ResumePlaybackAfterKeyAdded() on l.524?

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
File media/base/android/media_statistics.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.cc:1: #include
"media/base/android/media_statistics.h"
Add license header

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.cc:30: << " total:" << total;
This seems to be the only place to use |stream_type|, if you move the logging to
the caller you can remove this member variable.

With that, you can also omit the |delay| parameter.

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.cc:38: num_starvations_(0) {
nit: consider using Non-Static Class Member Initializers:

https://chromium-cpp.appspot.com/

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.cc:54: return;  // skip if there was no
prior Start().
Should this ever happen?

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
File media/base/android/media_statistics.h (right):

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.h:25: // A number of late frames. Late
frames are a subset of the total frames.
What's the definition of being late?

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.h:26: uint32_t late;
nit: consider using Non-Static Class Member Initializers:

https://chromium-cpp.appspot.com/

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.h:54: FrameStatistics& audio_frames() {
return audio_frames_; }
nit: s/audio_frame/audio_frame_statistic

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.h:57: FrameStatistics& video_frames() {
return video_frames_; }
ditto

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.h:65: void StopAndReport(base::TimeDelta
current_playback_time);
When called in a row, will only the first call take effect?

[Ilya Sherman, 2015-10-01 07:19:20]

LGTM, though I'd still recommend 50 buckets rather than 60.  (Also, I have a
couple of nits inline.)

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
File media/base/android/media_statistics.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.cc:19: const int kNumDurationSlots = 60;
Please keep in mind that histogram buckets are sized logarithmically, so even
with 60 buckets, you won't get one per minute.  Rather, the smallest buckets
will be very fine-grained, and the larger buckets will be less fine grained.

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
File media/base/android/media_statistics.h (right):

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.h:20: // Audio: total number of frame that
has been rendered.
nit: "has" -> "have"

https://chromiumcodereview.appspot.com/1367403003/diff/20001/media/base/andro...
media/base/android/media_statistics.h:21: // Video: total amount of frames
thatwere supposed to be rendered. Late video
nit: "thatwere" -> "that were"

[Tima Vaisburd, 2015-10-01 20:05:16]

Xiaohan, I think I did not miss any of your comments. PTAL.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_co...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_co...
media/base/android/media_codec_decoder.cc:524: return last_frame_posted_;
On 2015/09/30 21:02:57, xhwang wrote:
> nit: a boolean variable should be named as is_* or has_*. When looking at this
> my first reaction is that this returns the last frame that is posted.

This change is undone.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
File media/base/android/media_statistics.cc (right):

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.cc:45: DVLOG(1) << "MediaStatistics::" <<
__FUNCTION__;
On 2015/09/30 21:02:57, xhwang wrote:
> nit: class name not necessary in the log..

Removed everywhere in this class.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.cc:62: base::TimeDelta duration =
base::TimeTicks::Now() - start_time_;
On 2015/09/29 23:47:44, qinmin wrote:
> The duration is calculated from system time. Should we use the playback
duration
> instead?

Done.

https://codereview.chromium.org/1367403003/diff/1/media/base/android/media_st...
media/base/android/media_statistics.cc:103: 1000000 * video_frames_.late /
video_frames_.total);
On 2015/09/30 21:02:57, xhwang wrote:
> hmm, I wonder why we report frames per milliion frames? Can't we just report
> this as a percentage?

We expect these values to be less than 1%, so parts per million seem more
appropriate here. I modified the code though and changed the upper limit from
10^6 to 10^4

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:17683: +  <summary>Duration of an
uninterrupted MSE playback in seconds.</summary>
On 2015/09/30 21:02:57, xhwang wrote:
> +wolenetz as Media.MSE.Duration could be a general histogram not limited to
> Clank. Matt: How is duration defined for MSE in general?
> 
> What's the exact meaning of "uninterrupted MSE playback"? What if the user
seeks
> during playback? Is this in wall clock time, or in media time?
> 
> In media, "Duration" has a clean meaning [1]. The "Media.Duration" UMA follows
> this meaning. Here it seems we are recording something different. Can we use a
> different name to avoid confusion? e.g. *.PlaybackDuration.
> 
> [1]
> http://www.w3.org/html/wg/drafts/html/master/semantics.html#dom-media-duration

Added more explanation. This is the interval between playback actually starts
till pause, seek, release, etc. in media seconds.

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:17690: +    Relative number of late
audio frames wrt total number of audio frames in MSE
On 2015/09/30 21:02:57, xhwang wrote:
> Could you please elaborate what is exactly a "late audio frame"?

Added an explanation, this is related to potential underruns.

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:17699: +    Relative number of late
video frames wrt total number of video frames in MSE
On 2015/09/30 21:02:57, xhwang wrote:
> ditto

Added an explanation here as well.

https://codereview.chromium.org/1367403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:17727: +    Relative number of
starvations wrt total number of frames in MSE playback,
On 2015/09/30 21:02:57, xhwang wrote:
> ditto, define "starvation".

Added here too.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_codec_audio_decoder.cc:214:
frame_statistics_->AddFrame();
On 2015/09/30 21:02:57, xhwang wrote:
> When can frame_statistics_ be null? Is it only in unit tests? If so, can we
fix
> the test and always make sure frame_statistics_ is non-null?

Done.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:46:
frame_statistics_(frame_statistics),
On 2015/09/30 21:02:57, xhwang wrote:
> nit: Make the initialization order consistent with the parameter order.

Done.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_codec_player.cc:1256: // This methos needs to be called
when decoder threads are not processing
On 2015/09/30 21:02:57, xhwang wrote:
> s/methos/method
> 
> Also, I don't quite understand this comment. Could you please elaborate?

Changed to:
  At this point decoder threads are either not running at all or their
  message pumps are in the idel state after the preroll is done.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_source_player.cc:529:
media_stat_->StopAndReport(GetCurrentTime());
On 2015/09/30 21:02:57, xhwang wrote:
> Why should we StopAndReport() after ResumePlaybackAfterKeyAdded() on l.524?

Aha, this is the place that you explained to me!
Unfortunately in the current simple implementation we will wipe out the
statistics accumulated before this point by calling media_stat_->Start:
ResumePlaybackAfterkeyAdded() will call StartInternal which does prefetch and at
the end of prefetch we start collecting again.
This StopAndReport() gathers the prior portion.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
File media/base/android/media_statistics.cc (right):

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.cc:1: #include
"media/base/android/media_statistics.h"
On 2015/09/30 21:02:57, xhwang wrote:
> Add license header

Done.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.cc:19: const int kNumDurationSlots = 60;
On 2015/10/01 07:19:20, Ilya Sherman wrote:
> Please keep in mind that histogram buckets are sized logarithmically, so even
> with 60 buckets, you won't get one per minute.  Rather, the smallest buckets
> will be very fine-grained, and the larger buckets will be less fine grained.

Wow, that's great! Changed to 50.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.cc:30: << " total:" << total;
On 2015/09/30 21:02:57, xhwang wrote:
> This seems to be the only place to use |stream_type|, if you move the logging
to
> the caller you can remove this member variable.
> 
> With that, you can also omit the |delay| parameter.

It would be in 4 places instead of one though: audio/video and two players. I
removed the variable for now, but still thinking.

The delay is interesting on its own, maybe we need two more histograms.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.cc:38: num_starvations_(0) {
On 2015/09/30 21:02:57, xhwang wrote:
> nit: consider using Non-Static Class Member Initializers:
> 
> https://chromium-cpp.appspot.com/

Done.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.cc:54: return;  // skip if there was no
prior Start().
On 2015/09/30 21:02:57, xhwang wrote:
> Should this ever happen?

Yes, in MediaCodecPlayer this method is called from methods that might come one
after another, e.g. Seek and Release. The Seek might come several times in a row
without a true start in between.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
File media/base/android/media_statistics.h (right):

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.h:20: // Audio: total number of frame that
has been rendered.
On 2015/10/01 07:19:20, Ilya Sherman wrote:
> nit: "has" -> "have"

Done.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.h:21: // Video: total amount of frames
thatwere supposed to be rendered. Late video
On 2015/10/01 07:19:20, Ilya Sherman wrote:
> nit: "thatwere" -> "that were"

Done.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.h:25: // A number of late frames. Late
frames are a subset of the total frames.
On 2015/09/30 21:02:58, xhwang wrote:
> What's the definition of being late?

Copied the text from histogram.xml

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.h:26: uint32_t late;
On 2015/09/30 21:02:57, xhwang wrote:
> nit: consider using Non-Static Class Member Initializers:
> 
> https://chromium-cpp.appspot.com/

Done.

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.h:54: FrameStatistics& audio_frames() {
return audio_frames_; }
On 2015/09/30 21:02:58, xhwang wrote:
> nit: s/audio_frame/audio_frame_statistic 

The only place in the code it is being called has the form
media_stat_->audio_frames(), so it seems it's clear that it refers to statistics
about frames. What do you think?

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.h:57: FrameStatistics& video_frames() {
return video_frames_; }
On 2015/09/30 21:02:58, xhwang wrote:
> ditto

See above

https://codereview.chromium.org/1367403003/diff/20001/media/base/android/medi...
media/base/android/media_statistics.h:65: void StopAndReport(base::TimeDelta
current_playback_time);
On 2015/09/30 21:02:58, xhwang wrote:
> When called in a row, will only the first call take effect?

This is what I wanted to say. Changed the comment.

[xhwang, 2015-10-01 21:58:58]

lgtm with some nits

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
File media/base/android/audio_decoder_job.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
media/base/android/audio_decoder_job.cc:131:
frame_statistics_->AddLateFrame(current_time - next_frame_time_limit_);
This DCHECK is not necessary. In debug build l.131 will crash if DCHECK fails,
which is pretty much equivalent to the DCHECK above.

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
File media/base/android/media_codec_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
media/base/android/media_codec_audio_decoder.cc:215: DCHECK(frame_statistics_);
ditto

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
File media/base/android/media_codec_decoder.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
media/base/android/media_codec_decoder.cc:522: last_frame_posted_ = true;
The naming of this member variable still can be improved. See my previous
comment. You can do it later of course.

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
File media/base/android/media_decoder_job.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
media/base/android/media_decoder_job.cc:492: DCHECK(frame_statistics_);
ditto

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
File media/base/android/media_statistics.cc (right):

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
media/base/android/media_statistics.cc:17: namespace {
nit: no need to have anonymous namespace here. namespace scope const variables
are automatically static.

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
File media/base/android/media_statistics.h (right):

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
media/base/android/media_statistics.h:18: // Audio: total number of frames that
have been rendered.
How about late frames, as described in l.24.

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
media/base/android/media_statistics.h:34: void AddFrame() { ++total; }
To be accurate, all Add*() functions should really be Increment*().

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
media/base/android/media_statistics.h:38: void AddLateFrame(base::TimeDelta /*
delay */) { ++late; }
Why do you still pass in |delay| here?

https://chromiumcodereview.appspot.com/1367403003/diff/40001/media/base/andro...
media/base/android/media_statistics.h:54: FrameStatistics& audio_frames() {
return audio_frames_; }
I would recommend to be as clear as possible. audio_frame_stats() would be much
better in this case.

[qinmin, 2015-10-01 23:04:56]

lgtm

[Tima Vaisburd, 2015-10-02 00:44:33]

The Shaka player testing forced me to do some more changes, you might want to
take another look.

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/audi...
File media/base/android/audio_decoder_job.cc (right):

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/audi...
media/base/android/audio_decoder_job.cc:131:
frame_statistics_->AddLateFrame(current_time - next_frame_time_limit_);
On 2015/10/01 21:58:57, xhwang wrote:
> This DCHECK is not necessary. In debug build l.131 will crash if DCHECK fails,
> which is pretty much equivalent to the DCHECK above.

I thought DCHECK will provide extra information. Removed.

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
media/base/android/media_codec_audio_decoder.cc:215: DCHECK(frame_statistics_);
On 2015/10/01 21:58:57, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:522: last_frame_posted_ = true;
On 2015/10/01 21:58:57, xhwang wrote:
> The naming of this member variable still can be improved. See my previous
> comment. You can do it later of course.

Acknowledged.

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
File media/base/android/media_decoder_job.cc (right):

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
media/base/android/media_decoder_job.cc:492: DCHECK(frame_statistics_);
On 2015/10/01 21:58:57, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
File media/base/android/media_statistics.cc (right):

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
media/base/android/media_statistics.cc:13: #define LIMIT_10000(a) std::min(a,
9999U)
The Widevine testing on Shaka player shows up to 7% of late video frames for the
new player. The old one (MediaSource) has low rate, but its starvation rate is
50%. After each starvation there is time resetting, that explains the absence of
late frames.
So we cannot limit the results to 1%, I removed this.

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
media/base/android/media_statistics.cc:17: namespace {
On 2015/10/01 21:58:58, xhwang wrote:
> nit: no need to have anonymous namespace here. namespace scope const variables
> are automatically static.

Done.

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
File media/base/android/media_statistics.h (right):

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
media/base/android/media_statistics.h:18: // Audio: total number of frames that
have been rendered.
On 2015/10/01 21:58:58, xhwang wrote:
> How about late frames, as described in l.24.

Late frames are also rendered. I'm not sure I got this remark.

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
media/base/android/media_statistics.h:34: void AddFrame() { ++total; }
On 2015/10/01 21:58:58, xhwang wrote:
> To be accurate, all Add*() functions should really be Increment*().

Renamed to IncrementFrameCount() and IncrementLateFrameCount().

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
media/base/android/media_statistics.h:38: void AddLateFrame(base::TimeDelta /*
delay */) { ++late; }
On 2015/10/01 21:58:58, xhwang wrote:
> Why do you still pass in |delay| here?

Sorry, removed.

https://codereview.chromium.org/1367403003/diff/40001/media/base/android/medi...
media/base/android/media_statistics.h:54: FrameStatistics& audio_frames() {
return audio_frames_; }
On 2015/10/01 21:58:58, xhwang wrote:
> I would recommend to be as clear as possible. audio_frame_stats() would be
much
> better in this case.

Done.

https://codereview.chromium.org/1367403003/diff/60001/media/base/android/medi...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/1367403003/diff/60001/media/base/android/medi...
media/base/android/media_source_player.cc:504: return;
Removed the call to MediaStatistics::StopAndReport() from here since we may have
frequent prefetch events dues to frequent starvations.

https://codereview.chromium.org/1367403003/diff/60001/media/base/android/medi...
media/base/android/media_source_player.cc:527:
media_stat_->StopAndReport(GetCurrentTime());
With the new MediaStatistics::Start() semantics I can move the call here as you
recommended.

https://codereview.chromium.org/1367403003/diff/60001/media/base/android/medi...
File media/base/android/media_statistics.cc (right):

https://codereview.chromium.org/1367403003/diff/60001/media/base/android/medi...
media/base/android/media_statistics.cc:31: if (total == 1)
I accidently deleted this check in the prior PS, restored.

https://codereview.chromium.org/1367403003/diff/60001/media/base/android/medi...
media/base/android/media_statistics.cc:44: if (start_time_ == kNoTimestamp()) {
I changed the Start() semantic to follow StopAndReport(): only the first call in
a row takes effect. With this we can keep the Start() calls where they are even
if MediaSourcePlayer reporting starvations very frequently.

[Tima Vaisburd, 2015-10-02 00:57:09]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-10-02 00:57:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, xhwang@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/1367403003/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2015-10-02 00:57:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1367403003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1367403003/100001

[commit-bot: I haz the power, 2015-10-02 01:02:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 01:02:45]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tima Vaisburd, 2015-10-02 01:18:33]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2015-10-02 01:19:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1367403003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1367403003/100001

[commit-bot: I haz the power, 2015-10-02 01:26:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 01:26:29]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Tima Vaisburd, 2015-10-02 01:28:39]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2015-10-02 01:29:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1367403003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1367403003/100001

[commit-bot: I haz the power, 2015-10-02 01:36:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 01:36:16]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Tima Vaisburd, 2015-10-02 01:43:17]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-10-02 01:43:17]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
qinmin@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1367403003/#ps120001
(title: "Attempt to fix clang compilation")

[commit-bot: I haz the power, 2015-10-02 01:43:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1367403003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1367403003/120001

[commit-bot: I haz the power, 2015-10-02 06:40:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 06:40:17]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Tima Vaisburd, 2015-10-02 07:06:03]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-10-02 07:06:39]

The CQ bit was unchecked by timav@chromium.org

[commit-bot: I haz the power, 2015-10-02 07:06:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1367403003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1367403003/120001

[Tima Vaisburd, 2015-10-02 07:06:41]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2015-10-02 07:07:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1367403003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1367403003/120001

[commit-bot: I haz the power, 2015-10-02 08:26:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-02 08:27:39]

Patchset 7 (id:??) landed as
https://crrev.com/1ea2b2ca612e9889a5638b83f355176e7ea2c990
Cr-Commit-Position: refs/heads/master@{#351975}