RTCInboundRTPStreamStats.qpSum collected.

RTCInboundRTPStreamStats.qpSum collected.

This was previously only collected for local tracks
(RTCOutboundRTPStreamStats.qpSum).

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-qpsum

This CL also improves some testing in rtcstatscollector_unittest.cc.
Default and non-default values are tested in the same unittests,
removing the test that was specific to default-values, which was
otherwise code duplication.

BUG=webrtc:7065
Review-Url: https://codereview.webrtc.org/2675943002
Cr-Commit-Position: refs/heads/master@{#16477}
Committed: https://chromium.googlesource.com/external/webrtc/+/cd195bea5e38a61e73b08f82445560d9136cd9da

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

Patchset #1 (id:1) has been deleted

[hbos, 2017-02-03 14:12:12]

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

[hbos, 2017-02-03 14:12:12]

Please take a look, sakal and hta.

(Based on https://codereview.webrtc.org/2649133005/, I think I need to rebase
with master before I can run trybots)

[hbos, 2017-02-03 14:12:35]

Description was changed from

==========
RTCInboundRTPStreamStats.qpSum collected.

This was previously only collected for local tracks
(RTCOutboundRTPStreamStats.qpSum).

BUG=webrtc:7065
==========

to

==========
RTCInboundRTPStreamStats.qpSum collected.

This was previously only collected for local tracks
(RTCOutboundRTPStreamStats.qpSum).

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-qpsum

BUG=webrtc:7065
==========

[sakal, 2017-02-03 15:13:02]

https://codereview.webrtc.org/2675943002/diff/20001/webrtc/api/stats/rtcstats.h
File webrtc/api/stats/rtcstats.h (right):

https://codereview.webrtc.org/2675943002/diff/20001/webrtc/api/stats/rtcstats...
webrtc/api/stats/rtcstats.h:63: void set_timestamp_us_for_testing(int64_t
timestamp_us) {
I would rather just do a test for the qp_sum undefined case that ignores all
other values.

Alternatively, how about just constructing a completely new object? I don't
think it would add too much code duplication.

[hbos, 2017-02-03 16:07:32]

PTAL. Removed set_timestamp_us_for_testing and a bit of unittest refactoring.

I've changed the unittests so that any optional values are first undefined, we
getStats and check the result, then we change so that nothing is undefined and
we getStats and check the result again.

So the RTCStatsCollectorTest.CollectRTC[In/Out]boundRTPStreamStats_[Audio/Video]
tests covers both the undefined and defined cases.
With that I was able to remove a separate "..._Default" test and get the same
coverage.

https://codereview.webrtc.org/2675943002/diff/20001/webrtc/api/stats/rtcstats.h
File webrtc/api/stats/rtcstats.h (right):

https://codereview.webrtc.org/2675943002/diff/20001/webrtc/api/stats/rtcstats...
webrtc/api/stats/rtcstats.h:63: void set_timestamp_us_for_testing(int64_t
timestamp_us) {
On 2017/02/03 15:13:02, sakal wrote:
> I would rather just do a test for the qp_sum undefined case that ignores all
> other values.
> 
> Alternatively, how about just constructing a completely new object? I don't
> think it would add too much code duplication.

Oops, I actually don't need this, comparison operator ignores timestamp.

[hbos, 2017-02-03 16:08:42]

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

[commit-bot: I haz the power, 2017-02-03 16:08:51]

Dry run: 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-02-03 16:12:42]

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

[commit-bot: I haz the power, 2017-02-03 16:12:43]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21119)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[hbos, 2017-02-03 16:18:16]

(Ignore red bots, it has to do with dependent CL not having landed)

[sakal, 2017-02-06 08:52:21]

lgtm

[hta-webrtc, 2017-02-07 12:19:41]

lgtm

https://codereview.webrtc.org/2675943002/diff/40001/webrtc/pc/rtcstats_integr...
File webrtc/pc/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2675943002/diff/40001/webrtc/pc/rtcstats_integr...
webrtc/pc/rtcstats_integrationtest.cc:529: // if-statement needs to be included.
Nit: Since this is the incoming stream, it's the decoder, not the encoder that
needs to provide the value.

Writing it like this is good, since it will trigger an error the first time
someone actually provides a value in test.

https://codereview.webrtc.org/2675943002/diff/40001/webrtc/pc/rtcstatscollect...
File webrtc/pc/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2675943002/diff/40001/webrtc/pc/rtcstatscollect...
webrtc/pc/rtcstatscollector_unittest.cc:1913: voice_media_info.senders[0].rtt_ms
= 4500;
Is this an unrelated change that has snuck in, does the desription need to
change, or is this part of the non-submitted dependency CL?

[hbos, 2017-02-07 15:36:52]

Description was changed from

==========
RTCInboundRTPStreamStats.qpSum collected.

This was previously only collected for local tracks
(RTCOutboundRTPStreamStats.qpSum).

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-qpsum

BUG=webrtc:7065
==========

to

==========
RTCInboundRTPStreamStats.qpSum collected.

This was previously only collected for local tracks
(RTCOutboundRTPStreamStats.qpSum).

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-qpsum

This CL also improves some testing in rtcstatscollector_unittest.cc.
Default and non-default values are tested in the same unittests,
removing the test that was specific to default-values, which was
otherwise code duplication.

BUG=webrtc:7065
==========

[hbos, 2017-02-07 15:37:04]

https://codereview.webrtc.org/2675943002/diff/40001/webrtc/pc/rtcstats_integr...
File webrtc/pc/rtcstats_integrationtest.cc (right):

https://codereview.webrtc.org/2675943002/diff/40001/webrtc/pc/rtcstats_integr...
webrtc/pc/rtcstats_integrationtest.cc:529: // if-statement needs to be included.
On 2017/02/07 12:19:41, hta-webrtc wrote:
> Nit: Since this is the incoming stream, it's the decoder, not the encoder that
> needs to provide the value.
> 
> Writing it like this is good, since it will trigger an error the first time
> someone actually provides a value in test.

Done.

https://codereview.webrtc.org/2675943002/diff/40001/webrtc/pc/rtcstatscollect...
File webrtc/pc/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2675943002/diff/40001/webrtc/pc/rtcstatscollect...
webrtc/pc/rtcstatscollector_unittest.cc:1913: voice_media_info.senders[0].rtt_ms
= 4500;
On 2017/02/07 12:19:41, hta-webrtc wrote:
> Is this an unrelated change that has snuck in, does the desription need to
> change, or is this part of the non-submitted dependency CL?

This is from me testing default and non-default values in the same test,
removing the need for an independent default test case which is otherwise
duplicated code. CL description updated.

[hbos, 2017-02-07 15:37:09]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2017-02-07 15:37:10]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org,
sakal@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2675943002/#ps80001 (title:
"Addressed nit")

[commit-bot: I haz the power, 2017-02-07 15:37:15]

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-02-07 16:31:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486481829786010,
"parent_rev": "c16fa5ea692e06992c30c8b9853356b90da74e2a", "commit_rev":
"cd195bea5e38a61e73b08f82445560d9136cd9da"}

[commit-bot: I haz the power, 2017-02-07 16:31:31]

Description was changed from

==========
RTCInboundRTPStreamStats.qpSum collected.

This was previously only collected for local tracks
(RTCOutboundRTPStreamStats.qpSum).

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-qpsum

This CL also improves some testing in rtcstatscollector_unittest.cc.
Default and non-default values are tested in the same unittests,
removing the test that was specific to default-values, which was
otherwise code duplication.

BUG=webrtc:7065
==========

to

==========
RTCInboundRTPStreamStats.qpSum collected.

This was previously only collected for local tracks
(RTCOutboundRTPStreamStats.qpSum).

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-qpsum

This CL also improves some testing in rtcstatscollector_unittest.cc.
Default and non-default values are tested in the same unittests,
removing the test that was specific to default-values, which was
otherwise code duplication.

BUG=webrtc:7065

Review-Url: https://codereview.webrtc.org/2675943002
Cr-Commit-Position: refs/heads/master@{#16477}
Committed:
https://chromium.googlesource.com/external/webrtc/+/cd195bea5e38a61e73b08f824...
==========

[commit-bot: I haz the power, 2017-02-07 16:31:31]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/cd195bea5e38a61e73b08f824...

[skvlad, 2017-02-07 18:44:21]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.webrtc.org/2687483002/ by skvlad@webrtc.org.

The reason for reverting is: Breaks downstream build..