Log statistics of TabCapture API through UMA

Log statistics of TabCapture API through UMA

Log the following data through UMA:
* Frame rate over 10s
* Histogram of capture time
* Percentage of frame drops over 5mins
* Histogram of view size (per kilopixels)

BUG=163469
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175265

[miu, 2012-12-14 23:28:35]

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:98:
const int kFrameDropReportInterval = 9000;
I think you can remove this (see comments below).

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:357:
"TabCapture.ViewKiloPixels",
Since a sample is only recorded when the size changes, should we name this
something like TabCapture.ViewChangeKiloPixels?

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:596: if
(frame_number % kFrameDropReportInterval ==
Rather than do this mod kFrameDropReportInterval stuff, why not just record the
FrameDropPercentage below (i.e., where TabCaputre.FrameRate is recorded)?

Then, instead of incrementing count_frames_missed_, you can also easily
calculate it as:

  const int count_frames_dropped =
      frame_number_ - last_frame_number_ - count_frames_rendered_
  DCHECK_LE(0, count_frames_dropped);
  last_frame_number_ = frame_number_;  // Update last_frame_number_ where
count_frames_rendered_ = 0;

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:601:
count_frames_missed_ * 100 / kFrameDropReportInterval);
What if the frames missed is 0.9%?  It seems we'd want to know that (e.g., on
fast hardware).  UMA_HISTOGRAM_COUNTS_10000 would let you do basis points for
more precision.

Also/Or, consider rounding to int: static_cast<int>(<<calculation>> + 0.5)

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:628:
const int measured_fps =
IMHO, you're going to want more precision here.  Suggest you make measured_fps
double like it was, or consider changing this metric to frames-per-minute or
some such.

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:630:
UMA_HISTOGRAM_COUNTS("TabCapture.FrameRate", measured_fps);
Ditto on considering rounding the value (assuming you make measured_fps a
double).  You may want to increase the precision being recorded: Meaning, is
29.0 vs. 29.5 vs 30.0 FPS something we want measured?

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:701:
base::Time start_time,
Should be const ref type.

[Alpha Left Google, 2013-01-04 00:00:38]

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:98:
const int kFrameDropReportInterval = 9000;
On 2012/12/14 23:28:35, Yuri wrote:
> I think you can remove this (see comments below).

Done.

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:357:
"TabCapture.ViewKiloPixels",
On 2012/12/14 23:28:35, Yuri wrote:
> Since a sample is only recorded when the size changes, should we name this
> something like TabCapture.ViewChangeKiloPixels?

Done.

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:596: if
(frame_number % kFrameDropReportInterval ==
On 2012/12/14 23:28:35, Yuri wrote:
> Rather than do this mod kFrameDropReportInterval stuff, why not just record
the
> FrameDropPercentage below (i.e., where TabCaputre.FrameRate is recorded)?
> 
> Then, instead of incrementing count_frames_missed_, you can also easily
> calculate it as:
> 
>   const int count_frames_dropped =
>       frame_number_ - last_frame_number_ - count_frames_rendered_
>   DCHECK_LE(0, count_frames_dropped);
>   last_frame_number_ = frame_number_;  // Update last_frame_number_ where
> count_frames_rendered_ = 0;
> 

Done.

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:601:
count_frames_missed_ * 100 / kFrameDropReportInterval);
On 2012/12/14 23:28:35, Yuri wrote:
> What if the frames missed is 0.9%?  It seems we'd want to know that (e.g., on
> fast hardware).  UMA_HISTOGRAM_COUNTS_10000 would let you do basis points for
> more precision.
> 
> Also/Or, consider rounding to int: static_cast<int>(<<calculation>> + 0.5)

Done.

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:628:
const int measured_fps =
On 2012/12/14 23:28:35, Yuri wrote:
> IMHO, you're going to want more precision here.  Suggest you make measured_fps
> double like it was, or consider changing this metric to frames-per-minute or
> some such.

I think a rough number is enough, 29 or 30 are both fine.

https://codereview.chromium.org/11565052/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:701:
base::Time start_time,
On 2012/12/14 23:28:35, Yuri wrote:
> Should be const ref type.

Done.

[miu, 2013-01-04 18:05:27]

lgtm

https://codereview.chromium.org/11565052/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/11565052/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:604:
const int measured_fps =
nit: It was useful to see the fractional part in the VLOG statement.  Can you
put this back to double, and static_cast<int>(measured_fps) for the
UMA_HISTOGRAM_COUNTS statement?

[commit-bot: I haz the power, 2013-01-04 20:17:54]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Alpha Left Google, 2013-01-04 21:02:48]

wjia: Hi Wei, can you help review this? Yuri isn't a full committer yet.

[wjia(left Chromium), 2013-01-04 22:01:36]

lgtm

[commit-bot: I haz the power, 2013-01-04 22:03:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/11565052/11001

[commit-bot: I haz the power, 2013-01-05 02:23:15]

Change committed as 175265