RTC[In/Out]boundRTPStreamStats: qpSum,framesDecoded,framesEncoded added.

RTC[In/Out]boundRTPStreamStats: qpSum,framesDecoded,framesEncoded added.

Recently added to the spec:
RTCRTPStreamStats.qpSum - https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-qpsum
RTCInboundRTPStreamStats.framesDecoded - https://w3c.github.io/webrtc-stats/#dom-rtcinboundrtpstreamstats-framesdecoded
RTCOutboundRTPStreamStats.framesEncoded - https://w3c.github.io/webrtc-stats/#dom-rtcoutboundrtpstreamstats-framesencoded

These are added and collected. However, the qpSum is only collected in
the outbound case. It should be collected in the inbound case before
closing crbug.com/657855

BUG=chromium:657854, chromium:657855, chromium:657856
Review-Url: https://codereview.webrtc.org/2588373005
Cr-Commit-Position: refs/heads/master@{#15872}
Committed: https://chromium.googlesource.com/external/webrtc/+/6769c4941817ac3d5b967d0aafa1304c06d0672e

[hbos, 2016-12-21 14:09:43]

Patchset #1 (id:1) has been deleted

[hbos, 2016-12-21 14:11:18]

hbos@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, hta@webrtc.org

[hbos, 2016-12-21 14:11:19]

Please take a look, hta and deadbeef.

[hbos, 2016-12-21 14:11:28]

The CQ bit was checked by hbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 14:11:38]

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

[hbos, 2016-12-21 14:12:59]

hbos@webrtc.org changed reviewers:
+ sakal@webrtc.org

[hbos, 2016-12-21 14:12:59]

And +sakal for question (but is OOO until Jan 8 so won't be able to review): Is
qp_sum planned to be added to the VideoReceiverInfo as well (for inbound case)?
Is that easy to add?

[commit-bot: I haz the power, 2016-12-21 14:30:08]

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

[commit-bot: I haz the power, 2016-12-21 14:30:09]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2016-12-21 18:49:09]

lgtm

[hbos, 2016-12-29 09:00:11]

Please take a look, hta

[hta-webrtc, 2017-01-02 14:29:12]

lgtm

One test coverage question, otherwise lgtm.

https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.cc:229: if (video_sender_info.qp_sum)
This means that we have two cases to test: qp_sum not set and qp_sum set.

https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector_unittest.cc:1669:
video_media_info.senders[0].qp_sum = rtc::Optional<uint64_t>(16);
How much work is it to add a test case for the case where qp_sum is not set?

[hbos, 2017-01-02 15:34:39]

https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector_unittest.cc:1669:
video_media_info.senders[0].qp_sum = rtc::Optional<uint64_t>(16);
On 2017/01/02 14:29:12, hta-webrtc wrote:
> How much work is it to add a test case for the case where qp_sum is not set?

Already covered :) Tested being set here in
CollectRTCOutboundRTPStreamStats_Video, tested not being set in
CollectRTCOutboundRTPStreamStats_Default below.

https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector_unittest.cc:1820: // |expected_video.qp_sum| should
be undefined.
^-- Here in CollectRTCOutboundRTPStreamStats_Default

[hbos, 2017-01-02 15:35:22]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2017-01-02 15:35:22]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org,
deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2588373005/#ps40001 (title:
"Rebase with master")

[commit-bot: I haz the power, 2017-01-02 15:35:27]

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, 2017-01-02 16:35:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483371322244140,
"parent_rev": "d17a5a7709fcef98bb6fcd0d2a99e11dfcc38bb7", "commit_rev":
"6769c4941817ac3d5b967d0aafa1304c06d0672e"}

[commit-bot: I haz the power, 2017-01-02 16:35:16]

Description was changed from

==========
RTC[In/Out]boundRTPStreamStats: qpSum,framesDecoded,framesEncoded added.

Recently added to the spec:
RTCRTPStreamStats.qpSum -
https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-qpsum
RTCInboundRTPStreamStats.framesDecoded -
https://w3c.github.io/webrtc-stats/#dom-rtcinboundrtpstreamstats-framesdecoded
RTCOutboundRTPStreamStats.framesEncoded -
https://w3c.github.io/webrtc-stats/#dom-rtcoutboundrtpstreamstats-framesencoded

These are added and collected. However, the qpSum is only collected in
the outbound case. It should be collected in the inbound case before
closing crbug.com/657855

BUG=chromium:657854, chromium:657855, chromium:657856
==========

to

==========
RTC[In/Out]boundRTPStreamStats: qpSum,framesDecoded,framesEncoded added.

Recently added to the spec:
RTCRTPStreamStats.qpSum -
https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-qpsum
RTCInboundRTPStreamStats.framesDecoded -
https://w3c.github.io/webrtc-stats/#dom-rtcinboundrtpstreamstats-framesdecoded
RTCOutboundRTPStreamStats.framesEncoded -
https://w3c.github.io/webrtc-stats/#dom-rtcoutboundrtpstreamstats-framesencoded

These are added and collected. However, the qpSum is only collected in
the outbound case. It should be collected in the inbound case before
closing crbug.com/657855

BUG=chromium:657854, chromium:657855, chromium:657856

Review-Url: https://codereview.webrtc.org/2588373005
Cr-Commit-Position: refs/heads/master@{#15872}
Committed:
https://chromium.googlesource.com/external/webrtc/+/6769c4941817ac3d5b967d0aa...
==========

[commit-bot: I haz the power, 2017-01-02 16:35:17]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/6769c4941817ac3d5b967d0aa...

[sakal, 2017-01-09 07:13:55]

lgtm

https://codereview.webrtc.org/2588373005/diff/40001/webrtc/api/rtcstats_integ...
File webrtc/api/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2588373005/diff/40001/webrtc/api/rtcstats_integ...
webrtc/api/rtcstats_integrationtest.cc:549:
verifier.TestMemberIsUndefined(outbound_stream.qp_sum);
nit: QP sum should also be undefined if frames_encoded is zero. This might not
ever be used in that case though?

[hbos, 2017-01-09 08:47:57]

https://codereview.webrtc.org/2588373005/diff/40001/webrtc/api/rtcstats_integ...
File webrtc/api/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2588373005/diff/40001/webrtc/api/rtcstats_integ...
webrtc/api/rtcstats_integrationtest.cc:549:
verifier.TestMemberIsUndefined(outbound_stream.qp_sum);
On 2017/01/09 07:13:55, sakal wrote:
> nit: QP sum should also be undefined if frames_encoded is zero. This might not
> ever be used in that case though?

Ah, I should change below:
verifier.TestMemberIsDefined(outbound_stream.frames_encoded);
To:
verifier.TestMemberIsPositive(outbound_stream.frames_encoded);
But no biggie.

[joe.palmer, 2017-03-13 19:26:33]

How do I know which version of Chrome supports this feature?

I've been looking but can't find a way to determine the minimum Chrome version
that I would be able to get this info.

Many thanks.

[hbos, 2017-03-14 09:16:25]

On 2017/03/13 19:26:33, joe.palmer wrote:
> How do I know which version of Chrome supports this feature?
> 
> I've been looking but can't find a way to determine the minimum Chrome version
> that I would be able to get this info.
> 
> Many thanks.

This is in the promise-based getStats API in M57, where the promise-based
getStats API is behind flag as an experimental web platform feature
(chrome://flags/#enable-experimental-web-platform-features). This is unflagged
in M58 (enabled by default).

[hbos, 2017-03-14 09:17:41]

Not to be confused with the callback-based (non-spec compliant) getStats API.

[hbos, 2017-03-14 09:20:09]

The availability of the QP values are encoder/decoder specific so I'm not
entirely sure for which that is available or in what version.

[joe.palmer, 2017-03-14 20:58:17]

Thanks for your reply. I thought this was only just coming in but the
framesDecoded stat seems to be available in at least Chrome v56.0.2924.87 (and
earlier if I remember correctly) shown by this screenshot showing my current
version and framesDecoded being returned:

https://www.dropbox.com/s/ojh8puh2j4lzjva/Screenshot%202017-03-14%2020.47.33....

Even though the commits seems to show that this feature first landed in Chrome
v57.0.297(0/1).0:

https://storage.googleapis.com/chromium-find-releases-static/a3a.html#a3a849d...
https://storage.googleapis.com/chromium-find-releases-static/b9d.html#b9df69c...

Is there any why to know for sure which version of Chrome framesEncoded is
available?

Many thanks.

[hbos, 2017-03-15 08:22:20]

On 2017/03/14 20:58:17, joe.palmer wrote:
> Thanks for your reply. I thought this was only just coming in but the
> framesDecoded stat seems to be available in at least Chrome v56.0.2924.87 (and
> earlier if I remember correctly) shown by this screenshot showing my current
> version and framesDecoded being returned:
> 
>
https://www.dropbox.com/s/ojh8puh2j4lzjva/Screenshot%202017-03-14%2020.47.33....
> 
> Even though the commits seems to show that this feature first landed in Chrome
> v57.0.297(0/1).0:
> 
>
https://storage.googleapis.com/chromium-find-releases-static/a3a.html#a3a849d...
>
https://storage.googleapis.com/chromium-find-releases-static/b9d.html#b9df69c...
> 
> Is there any why to know for sure which version of Chrome framesEncoded is
> available?
> 
> Many thanks.

Ah, I see that
https://webrtc.github.io/samples/src/content/peerconnection/constraints/ is
using the old callback-based getStats API.
Because of the big differences between it and the spec, the spec-compliant stats
collector was rewritten and it returns a different set of stats.

framesEncoded was added to the old getStats API here:
- WebRTC CL
https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/53f7ed93...
- Roll of this into Chromium:
https://chromium.googlesource.com/chromium/src/+/7bda5b2db9ce07f1b2d62f576f75...
- Meaning this was initially in version 56.0.2901.0 of Chrome.

[joe.palmer, 2017-03-16 16:22:36]

That's really helpful, thanks very much!

[hbos, 2017-03-17 09:49:36]

On 2017/03/16 16:22:36, joe.palmer wrote:
> That's really helpful, thanks very much!

Cheers!