RTCInboundRTPStreamStats's [fir/pli/nack]_count are collected for video.

RTCInboundRTPStreamStats's [fir/pli/nack]_count are collected for video.

Previously this was only collected for RTCOutboundRTPStreamStats video,
with no comment saying it was missing for Inbound. (nack_count should be
collected vor audio as well but this is currently not available - there
is already an existing comment about this in rtcstats_objects.h.)

BUG=chromium:657855, chromium:657854, chromium:627816
Committed: https://crrev.com/820f578f2bb3585cc48876fa9e594a6a6b0dd174
Cr-Commit-Position: refs/heads/master@{#15185}

[hbos, 2016-11-21 14:56:50]

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

[commit-bot: I haz the power, 2016-11-21 14:56:57]

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

[hbos, 2016-11-21 14:56:59]

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

[hbos, 2016-11-21 14:57:00]

Please take a look, hta.

[commit-bot: I haz the power, 2016-11-21 15:11:39]

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

[commit-bot: I haz the power, 2016-11-21 15:11:41]

Dry run: This issue passed the CQ dry run.

[hta-webrtc, 2016-11-22 09:51:30]

lgtm, just nits.

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:110: void
SetInboundRTPStreamStatsFromMediaReceiverInfo(
Add a comment to this function to say that this provides the media independent
counters. It's obvious when you inspect it, but nice to be explicit.

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:151:
static_cast<uint32_t>(video_receiver_info.nacks_sent);
Are we sure these are actually counted (ie never -1)?
If they're -1, we should skip generating the stat, but if they're never -1, we
don't need to bother.

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector_...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:1369:
EXPECT_TRUE(report->Get(*expected_audio.transport_id));
Can you add a test line here that checks that fir_count is not set?

[hbos, 2016-11-22 11:02:45]

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:110: void
SetInboundRTPStreamStatsFromMediaReceiverInfo(
On 2016/11/22 09:51:29, hta-webrtc wrote:
> Add a comment to this function to say that this provides the media independent
> counters. It's obvious when you inspect it, but nice to be explicit.

Done.

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:151:
static_cast<uint32_t>(video_receiver_info.nacks_sent);
On 2016/11/22 09:51:29, hta-webrtc wrote:
> Are we sure these are actually counted (ie never -1)?
> If they're -1, we should skip generating the stat, but if they're never -1, we
> don't need to bother.

Looks like it, they start at 0 and there is code for increasing them.

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector_...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2515293002/diff/1/webrtc/api/rtcstatscollector_...
webrtc/api/rtcstatscollector_unittest.cc:1369:
EXPECT_TRUE(report->Get(*expected_audio.transport_id));
On 2016/11/22 09:51:30, hta-webrtc wrote:
> Can you add a test line here that checks that fir_count is not set?

Because we do EXPECT_EQ between expected and actual RTCStats, and the expected
stats does not have these members set, the test already makes sure that they are
not set. Same for the other tests.

[hbos, 2016-11-22 11:02:51]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-11-22 11:02:51]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2515293002/#ps20001 (title:
"Added comments")

[commit-bot: I haz the power, 2016-11-22 11:02:57]

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-11-22 11:16:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479812571409540,
"parent_rev": "3f3c782ba328ceb8cdb241b6b4eb36444ea9f0b1", "commit_rev":
"3de48d1cd348701d5ddc35fa6d44ca910e11efb5"}

[commit-bot: I haz the power, 2016-11-22 11:16:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-22 11:17:02]

Description was changed from

==========
RTCInboundRTPStreamStats's [fir/pli/nack]_count are collected for video.

Previously this was only collected for RTCOutboundRTPStreamStats video,
with no comment saying it was missing for Inbound. (nack_count should be
collected vor audio as well but this is currently not available - there
is already an existing comment about this in rtcstats_objects.h.)

BUG=chromium:657855, chromium:657854, chromium:627816
==========

to

==========
RTCInboundRTPStreamStats's [fir/pli/nack]_count are collected for video.

Previously this was only collected for RTCOutboundRTPStreamStats video,
with no comment saying it was missing for Inbound. (nack_count should be
collected vor audio as well but this is currently not available - there
is already an existing comment about this in rtcstats_objects.h.)

BUG=chromium:657855, chromium:657854, chromium:627816

Committed: https://crrev.com/820f578f2bb3585cc48876fa9e594a6a6b0dd174
Cr-Commit-Position: refs/heads/master@{#15185}
==========

[commit-bot: I haz the power, 2016-11-22 11:17:02]

Patchset 2 (id:??) landed as
https://crrev.com/820f578f2bb3585cc48876fa9e594a6a6b0dd174
Cr-Commit-Position: refs/heads/master@{#15185}