Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390693003:
RTCIceCandidatePairStats added. (Closed)
DescriptionRTCIceCandidatePairStats[1] added.
Note: In this initial CL most stats members are missing. This needs to
be addressed before closing the RTCIceCandidatePairStats bug
(crbug.com/633550).
[1] https://w3c.github.io/webrtc-stats/#candidatepair-dict*
BUG=chromium:633550, chromium:627816
Committed: https://crrev.com/c47a0c3ac42ba4a5582187acc3ad3a762c323e33
Cr-Commit-Position: refs/heads/master@{#14604}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Updated ID and comments #Patch Set 3 : Set requests_received and update unittests with non-default ConnectionInfo values #
Total comments: 2
Patch Set 4 : responses_received, not requests_received #
Total comments: 10
Patch Set 5 : Addressed comments (added requests_sent/responses_sent) #
Messages
Total messages: 34 (20 generated)
Description was changed from ========== RTCIceCandidatePairStats added. BUG=chromium:627816, chromium:633550 ========== to ========== RTCIceCandidatePairStats[1] added. [1] https://w3c.github.io/webrtc-stats/#candidatepair-dict* BUG=chromium:627816, chromium:633550 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== RTCIceCandidatePairStats[1] added. [1] https://w3c.github.io/webrtc-stats/#candidatepair-dict* BUG=chromium:627816, chromium:633550 ========== to ========== RTCIceCandidatePairStats[1] added. Note: In this initial CL most stats members are missing. This needs to be addressed before closing the RTCIceCandidatePairStats bug (crbug.com/633550). [1] https://w3c.github.io/webrtc-stats/#candidatepair-dict* BUG=chromium:633550, chromium:627816 ==========
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
Please take a look, deadbeef and hta. I suspect we might want to address more of the TODOs before landing this (or else do it in a follow-up)? Do you know where to get ahold of the missing stats? I'm not very familiar with this part of the code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/10 16:40:13, hbos wrote: > Please take a look, deadbeef and hta. > > I suspect we might want to address more of the TODOs before landing this (or > else do it in a follow-up)? > Do you know where to get ahold of the missing stats? I'm not very familiar with > this part of the code. I think I should update ConnectionInfo to contain more of the stats in a separate CL. Could land in either order.
We should document the discrepancies between our stats and the official definitions in a bug somewhere. I think this CL could still be landed though. hta, maybe you remember the exact meaning of "currentRtt" that was decided. But as I recall, it was only the very last RTT measurement (not smoothed). With the reasoning that if the application wanted to do averaging, it could do that with two getStats calls and "totalRtt". https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:233: // subsequent connections. crbug.com/633550 A pair can be uniquely identified by the candidates that form it. So you should be able to form the ID by appending the IDs of the candidates. https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:259: candidate_pair_stats->writable = info.writable; Our "writable" doesn't currently match the definition in the spec. Our "writable" goes to false after a certain amount of time without a response passes. https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:267: static_cast<double>(info.rtt) / 1000.0; I'm pretty sure "current_rtt" means the very last RTT measurement, but our RTT is smoothed. So there's also a discrepancy here. https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:368: rtc::ToString<>(connection_id); Should the test be relying on a specific ID format? Or just for the ID to remain consistent over successive calls to GetStats?
https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:233: // subsequent connections. crbug.com/633550 On 2016/10/10 21:45:37, Taylor Brandstetter wrote: > A pair can be uniquely identified by the candidates that form it. So you should > be able to form the ID by appending the IDs of the candidates. Good thinkin'! Done. https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:259: candidate_pair_stats->writable = info.writable; On 2016/10/10 21:45:38, Taylor Brandstetter wrote: > Our "writable" doesn't currently match the definition in the spec. Our > "writable" goes to false after a certain amount of time without a response > passes. Acknowledged. Wrote a TODO comment. https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:267: static_cast<double>(info.rtt) / 1000.0; On 2016/10/10 21:45:37, Taylor Brandstetter wrote: > I'm pretty sure "current_rtt" means the very last RTT measurement, but our RTT > is smoothed. So there's also a discrepancy here. Ack. Wrote a comment. https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:368: rtc::ToString<>(connection_id); On 2016/10/10 21:45:38, Taylor Brandstetter wrote: > Should the test be relying on a specific ID format? Or just for the ID to remain > consistent over successive calls to GetStats? I think testing the ID format is a good way to ensure it will stay consistent and unique.
lgtm https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2390693003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:368: rtc::ToString<>(connection_id); On 2016/10/10 22:39:56, hbos wrote: > On 2016/10/10 21:45:38, Taylor Brandstetter wrote: > > Should the test be relying on a specific ID format? Or just for the ID to > remain > > consistent over successive calls to GetStats? > > I think testing the ID format is a good way to ensure it will stay consistent > and unique. It is, but it means the test will need to be updated whenever the format changes. Which I guess is unlikely to happen, so it's not a big deal.
Please take a look, hta.
PTAL hta and deadbeef (even though you already lgtm'd) - I added another stat and updated the unittest.
https://codereview.webrtc.org/2390693003/diff/100001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2390693003/diff/100001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:269: static_cast<uint64_t>(info.recv_ping_responses); "requests received" = "responses received"?
https://codereview.webrtc.org/2390693003/diff/100001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2390693003/diff/100001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:269: static_cast<uint64_t>(info.recv_ping_responses); On 2016/10/11 17:26:50, Taylor Brandstetter wrote: > "requests received" = "responses received"? Oops, brainfart. Done.
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9140)
lgtm There are a few more stats that look like really low hanging fruit (already defined in the cricket stats). Are they defined wrong? A few more comment comments. But time to land. https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:263: // smoothed according to the spec. crbug.com/633550 Yes, the definition at https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-currentrtt says that it's the last measurement only. https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:269: // TODO(hbos): Set candidate_pair_stats->requests_sent. info.sent_ping_requests_total? https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:272: // TODO(hbos): Set candidate_pair_stats->responses_sent. info.sent_ping_responses? https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:383: EXPECT_FALSE(candidate_pair_stats.state.is_defined()); Please add a comment saying that the EXPECT_FALSE(....is_defined()) are all "not implemented yet" stats, and that the bugs and TODOs are listed in rtcstatscollector.cc https://codereview.webrtc.org/2390693003/diff/120001/webrtc/stats/rtcstats_ob... File webrtc/stats/rtcstats_objects.cc (right): https://codereview.webrtc.org/2390693003/diff/120001/webrtc/stats/rtcstats_ob... webrtc/stats/rtcstats_objects.cc:22: const char* RTCIceCandidateType::kHost = "host"; Add a comment saying where these strings come from (what spec). (I think it's the ICE spec's SDP mapping stats, it's not in the stats doc).
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:263: // smoothed according to the spec. crbug.com/633550 On 2016/10/11 20:40:26, hta-webrtc wrote: > Yes, the definition at > https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-currentrtt says > that it's the last measurement only. Acknowledged. https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:269: // TODO(hbos): Set candidate_pair_stats->requests_sent. On 2016/10/11 20:40:26, hta-webrtc wrote: > info.sent_ping_requests_total? Done. https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:272: // TODO(hbos): Set candidate_pair_stats->responses_sent. On 2016/10/11 20:40:26, hta-webrtc wrote: > info.sent_ping_responses? Done. https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2390693003/diff/120001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector_unittest.cc:383: EXPECT_FALSE(candidate_pair_stats.state.is_defined()); On 2016/10/11 20:40:26, hta-webrtc wrote: > Please add a comment saying that the EXPECT_FALSE(....is_defined()) are all "not > implemented yet" stats, and that the bugs and TODOs are listed in > rtcstatscollector.cc Done. https://codereview.webrtc.org/2390693003/diff/120001/webrtc/stats/rtcstats_ob... File webrtc/stats/rtcstats_objects.cc (right): https://codereview.webrtc.org/2390693003/diff/120001/webrtc/stats/rtcstats_ob... webrtc/stats/rtcstats_objects.cc:22: const char* RTCIceCandidateType::kHost = "host"; On 2016/10/11 20:40:26, hta-webrtc wrote: > Add a comment saying where these strings come from (what spec). (I think it's > the ICE spec's SDP mapping stats, it's not in the stats doc). > Done.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/2390693003/#ps180001 (title: "Addressed comments (added requests_sent/responses_sent)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== RTCIceCandidatePairStats[1] added. Note: In this initial CL most stats members are missing. This needs to be addressed before closing the RTCIceCandidatePairStats bug (crbug.com/633550). [1] https://w3c.github.io/webrtc-stats/#candidatepair-dict* BUG=chromium:633550, chromium:627816 ========== to ========== RTCIceCandidatePairStats[1] added. Note: In this initial CL most stats members are missing. This needs to be addressed before closing the RTCIceCandidatePairStats bug (crbug.com/633550). [1] https://w3c.github.io/webrtc-stats/#candidatepair-dict* BUG=chromium:633550, chromium:627816 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== RTCIceCandidatePairStats[1] added. Note: In this initial CL most stats members are missing. This needs to be addressed before closing the RTCIceCandidatePairStats bug (crbug.com/633550). [1] https://w3c.github.io/webrtc-stats/#candidatepair-dict* BUG=chromium:633550, chromium:627816 ========== to ========== RTCIceCandidatePairStats[1] added. Note: In this initial CL most stats members are missing. This needs to be addressed before closing the RTCIceCandidatePairStats bug (crbug.com/633550). [1] https://w3c.github.io/webrtc-stats/#candidatepair-dict* BUG=chromium:633550, chromium:627816 Committed: https://crrev.com/c47a0c3ac42ba4a5582187acc3ad3a762c323e33 Cr-Commit-Position: refs/heads/master@{#14604} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c47a0c3ac42ba4a5582187acc3ad3a762c323e33 Cr-Commit-Position: refs/heads/master@{#14604} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
