Add stats (histograms) for vp8 screenshare layers

Add stats (histograms) for vp8 screenshare layers

BUG=

Committed: https://crrev.com/b0fdfea9e8c51cfb5be852e9026e8cbe1a9c2c99
Cr-Commit-Position: refs/heads/master@{#11830}

[sprang_webrtc, 2016-02-25 13:58:56]

sprang@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2016-02-29 09:10:18]

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:83: if
(stats_.first_framt_time_ms_ == -1)
first_frame_time_ms_

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:316:
(stats_.num_tl1_frames_ + (duration_sec / 2)) / duration_sec);
Should this report tl0+tl1 instead, since that's the effective framerate the
user will get?

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc
(right):

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:479:
dropped_frame = true;
What is it in the test code that makes sure this actually happens? Is it the
overshoot?

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:507: 
Can we express some of these expectations using constants used above?

If too complex it may not be worth it.

[sprang_webrtc, 2016-02-29 12:43:17]

Addressed comment. I also renamed most of the histograms to make them easier to
organize.

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:83: if
(stats_.first_framt_time_ms_ == -1)
On 2016/02/29 09:10:18, stefan-webrtc (holmer) wrote:
> first_frame_time_ms_

Done.

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:316:
(stats_.num_tl1_frames_ + (duration_sec / 2)) / duration_sec);
On 2016/02/29 09:10:18, stefan-webrtc (holmer) wrote:
> Should this report tl0+tl1 instead, since that's the effective framerate the
> user will get?

I think I'd rather keep them separate. The total frame rate has its own
histogram already (and it's sliced on screen content too).

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc
(right):

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:479:
dropped_frame = true;
On 2016/02/29 09:10:18, stefan-webrtc (holmer) wrote:
> What is it in the test code that makes sure this actually happens? Is it the
> overshoot?

No, it's the too large frame (line 470).

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:507: 
On 2016/02/29 09:10:18, stefan-webrtc (holmer) wrote:
> Can we express some of these expectations using constants used above?
> 
> If too complex it may not be worth it.

I think FrameRateLayerX, FramesPerDrop and FramesPerOvershoot will be difficult
to infer. That's why I just sanity check (and did manual inspection to see that
the values make sense).

[stefan-webrtc, 2016-02-29 15:04:51]

lgtm

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:316:
(stats_.num_tl1_frames_ + (duration_sec / 2)) / duration_sec);
On 2016/02/29 12:43:17, språng wrote:
> On 2016/02/29 09:10:18, stefan-webrtc (holmer) wrote:
> > Should this report tl0+tl1 instead, since that's the effective framerate the
> > user will get?
> 
> I think I'd rather keep them separate. The total frame rate has its own
> histogram already (and it's sliced on screen content too).

ack

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc
(right):

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:479:
dropped_frame = true;
On 2016/02/29 12:43:17, språng wrote:
> On 2016/02/29 09:10:18, stefan-webrtc (holmer) wrote:
> > What is it in the test code that makes sure this actually happens? Is it the
> > overshoot?
> 
> No, it's the too large frame (line 470).

Acknowledged.

https://codereview.webrtc.org/1734793003/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc:507: 
On 2016/02/29 12:43:17, språng wrote:
> On 2016/02/29 09:10:18, stefan-webrtc (holmer) wrote:
> > Can we express some of these expectations using constants used above?
> > 
> > If too complex it may not be worth it.
> 
> I think FrameRateLayerX, FramesPerDrop and FramesPerOvershoot will be
difficult
> to infer. That's why I just sanity check (and did manual inspection to see
that
> the values make sense).

Acknowledged.

[sprang_webrtc, 2016-03-01 08:45:40]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-03-01 08:45:40]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1734793003/#ps40001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-03-01 08:45:50]

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

[commit-bot: I haz the power, 2016-03-01 08:56:13]

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

[commit-bot: I haz the power, 2016-03-01 08:56:14]

Try jobs failed on following builders:
  linux_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/12825)
  linux_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/13081)

[sprang_webrtc, 2016-03-01 13:45:16]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-03-01 13:45:17]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1734793003/#ps60001 (title:
"Prevent incorrect teardown order")

[commit-bot: I haz the power, 2016-03-01 13:45:22]

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

[commit-bot: I haz the power, 2016-03-01 13:51:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-01 13:51:24]

Description was changed from

==========
Add stats (histograms) for vp8 screenshare layers

BUG=
==========

to

==========
Add stats (histograms) for vp8 screenshare layers

BUG=

Committed: https://crrev.com/b0fdfea9e8c51cfb5be852e9026e8cbe1a9c2c99
Cr-Commit-Position: refs/heads/master@{#11830}
==========

[commit-bot: I haz the power, 2016-03-01 13:51:25]

Patchset 4 (id:??) landed as
https://crrev.com/b0fdfea9e8c51cfb5be852e9026e8cbe1a9c2c99
Cr-Commit-Position: refs/heads/master@{#11830}