Add histogram stats for average QP per frame for VP8 (for sent video streams):

Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.Qp.Vp8"
- "WebRTC.Video.Encoded.Qp.Vp8.S0"
- "WebRTC.Video.Encoded.Qp.Vp8.S1"
- "WebRTC.Video.Encoded.Qp.Vp8.S2"

BUG=

Committed: https://crrev.com/118ef0059468fc09e60f3e7dda1cf1c94eb5ec65
Cr-Commit-Position: refs/heads/master@{#12174}

[åsapersson, 2015-12-15 15:55:11]

Description was changed from

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.VP8.Qp"

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

to

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.VP8.Qp"

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

[åsapersson, 2015-12-15 15:55:11]

asapersson@webrtc.org changed reviewers:
+ mflodman@webrtc.org, stefan@webrtc.org

[åsapersson, 2015-12-15 15:55:56]

Split this to a sepatate CL from https://codereview.webrtc.org/1264693003/.

[åsapersson, 2016-03-22 14:33:16]

Description was changed from

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.VP8.Qp"

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

to

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.Qp.Vp8"

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

[stefan-webrtc, 2016-03-22 15:12:46]

LG, just one question about the test

https://codereview.webrtc.org/1523293002/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1523293002/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy_unittest.cc:350: EXPECT_EQ(kQpIdx1,
test::LastHistogramSample("WebRTC.Video.Encoded.Qp.Vp8"));
Shouldn't this be the average of 21 and 39?

[åsapersson, 2016-03-22 15:29:46]

https://codereview.webrtc.org/1523293002/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1523293002/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy_unittest.cc:350: EXPECT_EQ(kQpIdx1,
test::LastHistogramSample("WebRTC.Video.Encoded.Qp.Vp8"));
On 2016/03/22 15:12:46, stefan-webrtc (holmer) wrote:
> Shouldn't this be the average of 21 and 39?

QP is stored per ssrc e.g.
two qp samples should be recorded (one for each stream): 
simulcast_idx: 0 -> ssrcs[0] average of 21.
simulcast_idx: 1 -> ssrcs[1] average of 39.

[stefan-webrtc, 2016-03-22 15:31:45]

lgtm

[pbos-webrtc, 2016-03-22 15:35:11]

pbos@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2016-03-22 15:35:12]

https://codereview.webrtc.org/1523293002/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1523293002/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:198: RTC_HISTOGRAMS_COUNTS_200(kIndex,
uma_prefix_ + "Encoded.Qp.Vp8", qp);
Should these be reported separately per simulcast layer? (Like this when we only
send one layer, but per-layer when we do simulcast)

[åsapersson, 2016-03-23 10:57:33]

https://codereview.webrtc.org/1523293002/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1523293002/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:198: RTC_HISTOGRAMS_COUNTS_200(kIndex,
uma_prefix_ + "Encoded.Qp.Vp8", qp);
On 2016/03/22 15:35:12, pbos-webrtc wrote:
> Should these be reported separately per simulcast layer? (Like this when we
only
> send one layer, but per-layer when we do simulcast)

Have updated to use a separate histogram per spatial layer. PTAL.

[åsapersson, 2016-03-23 10:58:05]

Description was changed from

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.Qp.Vp8"

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

to

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.Qp.Vp8"
- "WebRTC.Video.Encoded.Qp.Vp8.S0"
- "WebRTC.Video.Encoded.Qp.Vp8.S1"
- "WebRTC.Video.Encoded.Qp.Vp8.S2"

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

[mflodman, 2016-03-23 12:25:20]

Logic LG, two code related comments.

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:200: if (spatial_idx == -1) {
I think it would be nice to rather build a string based on 'spatial_idx' and
only have one call to RTC_HISTOGRAMS_COUNT. That would mean less duplicated
code.

wdyt?

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:450: if (rtp_video_header->codec ==
kRtpVideoVp8) {
This can be an additional condition in the if above rather than having two ifs.

[pbos-webrtc, 2016-03-23 12:34:03]

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:200: if (spatial_idx == -1) {
On 2016/03/23 12:25:20, mflodman wrote:
> I think it would be nice to rather build a string based on 'spatial_idx' and
> only have one call to RTC_HISTOGRAMS_COUNT. That would mean less duplicated
> code.
> 
> wdyt?

We can't since RTC_HISTOGRAMS_COUNTS requires the string to be unique per macro
for performance reasons.

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:211: }
Should we have an else case here that should do something in case spatial_idx
goes above 2 to prevent dropping some unknown stats in the future?

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:450: if (rtp_video_header->codec ==
kRtpVideoVp8) {
On 2016/03/23 12:25:20, mflodman wrote:
> This can be an additional condition in the if above rather than having two
ifs.

I think so as well, if ssrcs.size() is 1 then we ignore what spatial_idx
actually is

[åsapersson, 2016-03-23 13:09:27]

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:200: if (spatial_idx == -1) {
On 2016/03/23 12:34:03, pbos-webrtc wrote:
> On 2016/03/23 12:25:20, mflodman wrote:
> > I think it would be nice to rather build a string based on 'spatial_idx' and
> > only have one call to RTC_HISTOGRAMS_COUNT. That would mean less duplicated
> > code.
> > 
> > wdyt?
> 
> We can't since RTC_HISTOGRAMS_COUNTS requires the string to be unique per
macro
> for performance reasons.

Right the name should not vary for a macro.

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:211: }
On 2016/03/23 12:34:03, pbos-webrtc wrote:
> Should we have an else case here that should do something in case spatial_idx
> goes above 2 to prevent dropping some unknown stats in the future?

Added a warning...

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:450: if (rtp_video_header->codec ==
kRtpVideoVp8) {
On 2016/03/23 12:25:20, mflodman wrote:
> This can be an additional condition in the if above rather than having two
ifs.

Done.

https://codereview.webrtc.org/1523293002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:450: if (rtp_video_header->codec ==
kRtpVideoVp8) {
On 2016/03/23 12:34:03, pbos-webrtc wrote:
> On 2016/03/23 12:25:20, mflodman wrote:
> > This can be an additional condition in the if above rather than having two
> ifs.
> 
> I think so as well, if ssrcs.size() is 1 then we ignore what spatial_idx
> actually is

-1 might be used when not using spatial layers for VP9 (therefore used here to
keep it similar..).

[pbos-webrtc, 2016-03-29 17:46:48]

lgtm

[mflodman, 2016-03-30 13:08:28]

lgtm

[åsapersson, 2016-03-31 06:58:57]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2016-03-31 06:58:58]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
pbos@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1523293002/#ps160001 (title:
"rebase")

[commit-bot: I haz the power, 2016-03-31 06:59:04]

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

[commit-bot: I haz the power, 2016-03-31 07:00:22]

Description was changed from

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.Qp.Vp8"
- "WebRTC.Video.Encoded.Qp.Vp8.S0"
- "WebRTC.Video.Encoded.Qp.Vp8.S1"
- "WebRTC.Video.Encoded.Qp.Vp8.S2"

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

to

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.Qp.Vp8"
- "WebRTC.Video.Encoded.Qp.Vp8.S0"
- "WebRTC.Video.Encoded.Qp.Vp8.S1"
- "WebRTC.Video.Encoded.Qp.Vp8.S2"

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

[commit-bot: I haz the power, 2016-03-31 07:00:25]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-31 07:00:32]

Description was changed from

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.Qp.Vp8"
- "WebRTC.Video.Encoded.Qp.Vp8.S0"
- "WebRTC.Video.Encoded.Qp.Vp8.S1"
- "WebRTC.Video.Encoded.Qp.Vp8.S2"

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

to

==========
Add histogram stats for average QP per frame for VP8 (for sent video streams):
- "WebRTC.Video.Encoded.Qp.Vp8"
- "WebRTC.Video.Encoded.Qp.Vp8.S0"
- "WebRTC.Video.Encoded.Qp.Vp8.S1"
- "WebRTC.Video.Encoded.Qp.Vp8.S2"

BUG=

Committed: https://crrev.com/118ef0059468fc09e60f3e7dda1cf1c94eb5ec65
Cr-Commit-Position: refs/heads/master@{#12174}
==========

[commit-bot: I haz the power, 2016-03-31 07:00:34]

Patchset 9 (id:??) landed as
https://crrev.com/118ef0059468fc09e60f3e7dda1cf1c94eb5ec65
Cr-Commit-Position: refs/heads/master@{#12174}