Implement qpSum stat for video send ssrc stats.

Implement qpSum stat for video send ssrc stats.

Implemented as defined by this pull request: https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
Committed: https://crrev.com/87da40488330817c483e2f27848f73ea730a1c4f
Cr-Commit-Position: refs/heads/master@{#14851}

[sakal, 2016-10-18 11:19:31]

Patchset #1 (id:1) has been deleted

[sakal, 2016-10-18 11:24:17]

sakal@webrtc.org changed reviewers:
+ hbos@webrtc.org, hta@webrtc.org, tommi@webrtc.org

[sakal, 2016-10-18 11:24:18]

PTAL

qpSum will overflow if we have a video stream open for more than a few days (16
days with average QP of 50). I am not sure if this warrants an use of bigger
datatype though.

[tommi, 2016-10-19 09:24:54]

Lgtm. I think it might be good to think about if overflow will be a problem for
e.g security cameras or if can be easily handled.

[sakal, 2016-10-20 12:16:58]

Description was changed from

==========
Implement qpSum stat for send streams.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

to

==========
Implement qpSum stat for video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

[hbos, 2016-10-20 17:08:21]

https://codereview.webrtc.org/2430603003/diff/80001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2430603003/diff/80001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:689: int qp_sum;
Should be uint32_t.

What about the decoded case? I think there should be a qp_sum in
VideoReceiverInfo as well.

From spec:
  The sum of the QP values of frames passed. The count
  of frames is in framesDecoded for inbound
  stream stats, and in framesEncoded for
  outbound stream stats.

This only covers the RTCOutboundRTPStreamStats case.

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

https://codereview.webrtc.org/2430603003/diff/80001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:513: }
We should perhaps add QP counters for H264 as well. Not in this CL though. Care
to file a bug?

https://codereview.webrtc.org/2430603003/diff/80001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/2430603003/diff/80001/webrtc/video_send_stream....
webrtc/video_send_stream.h:60: int qp_sum = 0;
Should be uint32_t

[sakal, 2016-10-21 08:59:43]

https://codereview.chromium.org/2430603003/diff/80001/webrtc/media/base/media...
File webrtc/media/base/mediachannel.h (right):

https://codereview.chromium.org/2430603003/diff/80001/webrtc/media/base/media...
webrtc/media/base/mediachannel.h:689: int qp_sum;
On 2016/10/20 17:08:21, hbos wrote:
> Should be uint32_t.
> 
> What about the decoded case? I think there should be a qp_sum in
> VideoReceiverInfo as well.
> 
> From spec:
>   The sum of the QP values of frames passed. The count
>   of frames is in framesDecoded for inbound
>   stream stats, and in framesEncoded for
>   outbound stream stats.
> 
> This only covers the RTCOutboundRTPStreamStats case.

I'd prefer to implement it in another CL. It will be considerably more work and
adding this for the encoding case satisfies our immediate use case.

https://codereview.chromium.org/2430603003/diff/80001/webrtc/video/send_stati...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.chromium.org/2430603003/diff/80001/webrtc/video/send_stati...
webrtc/video/send_statistics_proxy.cc:513: }
On 2016/10/20 17:08:21, hbos wrote:
> We should perhaps add QP counters for H264 as well. Not in this CL though.
Care
> to file a bug?

I think it would be better you filed the bug since you could provide more
information on why adding it would be useful. Personally, I don't know what
these stats are used for.

https://codereview.chromium.org/2430603003/diff/80001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.chromium.org/2430603003/diff/80001/webrtc/video_send_strea...
webrtc/video_send_stream.h:60: int qp_sum = 0;
On 2016/10/20 17:08:21, hbos wrote:
> Should be uint32_t

Done.

[hta-webrtc, 2016-10-21 09:13:12]

I don't think this is ready; I don't understand from the tests what will happen
with qpSum for other codecs than VP8 (it should be omitted from stats).

Re overflow: I have filed a bug against the spec to change the data type for the
qpSum field to "unsigned long long" (53 bits; thank you Javascript); this CL
should use uint_64 for the datatype.

https://codereview.chromium.org/2430603003/diff/100001/webrtc/api/statscollec...
File webrtc/api/statscollector_unittest.cc (right):

https://codereview.chromium.org/2430603003/diff/100001/webrtc/api/statscollec...
webrtc/api/statscollector_unittest.cc:1924: video_sender_info.qp_sum = 11;
This is a clash with the other CL (frames_decoded stat). Pick a different
number?

https://codereview.chromium.org/2430603003/diff/100001/webrtc/api/statscollec...
webrtc/api/statscollector_unittest.cc:1937: ExtractSsrcStatsValue(reports,
StatsReport::kStatsValueNameQpSum));
Can you add a test that verifies that when qp_sum is -1, the field is NOT
generated?

https://codereview.chromium.org/2430603003/diff/100001/webrtc/video/send_stat...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.chromium.org/2430603003/diff/100001/webrtc/video/send_stat...
webrtc/video/send_statistics_proxy_unittest.cc:323: EXPECT_EQ(130u,
statistics_proxy_->GetStats().qp_sum);
Can you add an unittest that chekcs that if codec is not VP8, the qp_sum will
start at -1 and remain at -1?

https://codereview.chromium.org/2430603003/diff/100001/webrtc/video_send_stre...
File webrtc/video_send_stream.h (right):

https://codereview.chromium.org/2430603003/diff/100001/webrtc/video_send_stre...
webrtc/video_send_stream.h:60: uint32_t qp_sum = 0;
Should this initialize to -1, since we don't know at this time whether it will
be possible to generate qp_sum or not?

[sakal, 2016-10-21 10:43:24]

Patchset #6 (id:120001) has been deleted

[sakal, 2016-10-21 14:13:42]

Patchset #7 (id:160001) has been deleted

[sakal, 2016-10-21 14:17:26]

Currently, qpSum works for VP8 and VP9. It will also work for H264 once
kthelgason@ lands his CL.

https://codereview.chromium.org/2430603003/diff/100001/webrtc/api/statscollec...
File webrtc/api/statscollector_unittest.cc (right):

https://codereview.chromium.org/2430603003/diff/100001/webrtc/api/statscollec...
webrtc/api/statscollector_unittest.cc:1924: video_sender_info.qp_sum = 11;
On 2016/10/21 09:13:12, hta-webrtc wrote:
> This is a clash with the other CL (frames_decoded stat). Pick a different
> number?

Does it really matter since it is in a different test?

https://codereview.chromium.org/2430603003/diff/100001/webrtc/api/statscollec...
webrtc/api/statscollector_unittest.cc:1937: ExtractSsrcStatsValue(reports,
StatsReport::kStatsValueNameQpSum));
On 2016/10/21 09:13:12, hta-webrtc wrote:
> Can you add a test that verifies that when qp_sum is -1, the field is NOT
> generated?

Done.

https://codereview.chromium.org/2430603003/diff/100001/webrtc/video/send_stat...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.chromium.org/2430603003/diff/100001/webrtc/video/send_stat...
webrtc/video/send_statistics_proxy_unittest.cc:323: EXPECT_EQ(130u,
statistics_proxy_->GetStats().qp_sum);
On 2016/10/21 09:13:12, hta-webrtc wrote:
> Can you add an unittest that chekcs that if codec is not VP8, the qp_sum will
> start at -1 and remain at -1?

Since QP sum is an unsigned type, it cannot be -1. I made it rtc::Optional
instead. Done.

https://codereview.chromium.org/2430603003/diff/100001/webrtc/video_send_stre...
File webrtc/video_send_stream.h (right):

https://codereview.chromium.org/2430603003/diff/100001/webrtc/video_send_stre...
webrtc/video_send_stream.h:60: uint32_t qp_sum = 0;
On 2016/10/21 09:13:12, hta-webrtc wrote:
> Should this initialize to -1, since we don't know at this time whether it will
> be possible to generate qp_sum or not?

Made rtc::Optional.

[hta-webrtc, 2016-10-27 12:22:36]

lgtm

One comment on code I don't understand, but it's not new in this CL, so
shouldn't block this CL.

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

https://codereview.webrtc.org/2430603003/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:515: }
I don't understand the code here - it seems that for the non-simulcast casel
(num_spatial_layers == 1), we're indexing qp_counters_ with the index -1. That
seems weird.

Since this is old code, I expect that it has worked. But it looks strange.

[hbos, 2016-10-28 09:25:14]

lgtm

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

https://codereview.webrtc.org/2430603003/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:515: }
On 2016/10/27 12:22:35, hta-webrtc wrote:
> I don't understand the code here - it seems that for the non-simulcast casel
> (num_spatial_layers == 1), we're indexing qp_counters_ with the index -1. That
> seems weird.
> 
> Since this is old code, I expect that it has worked. But it looks strange.

This confuses me too.

[sakal, 2016-10-31 13:26:11]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-10-31 13:26:11]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2430603003/#ps180001 (title:
"Change qp_sum to rtc::Optional<uint64_t>.")

[commit-bot: I haz the power, 2016-10-31 13:26:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-31 13:53:49]

Description was changed from

==========
Implement qpSum stat for video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

to

==========
Implement qpSum stat for video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

[commit-bot: I haz the power, 2016-10-31 13:53:50]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-10-31 13:53:53]

Description was changed from

==========
Implement qpSum stat for video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

to

==========
Implement qpSum stat for video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541

Committed: https://crrev.com/87da40488330817c483e2f27848f73ea730a1c4f
Cr-Commit-Position: refs/heads/master@{#14851}
==========

[commit-bot: I haz the power, 2016-10-31 13:53:55]

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