Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562703007:
RTCOutboundRTPStreamStats.roundTripTime: Only report non-negative values. (Closed)
DescriptionRTCOutboundRTPStreamStats.roundTripTime: Only report non-negative values.
Underlying stats gatherers may otherwise default it to -1.
BUG=chromium:669877, chromium:627816
Committed: https://crrev.com/e10e6d1f47bd3e9570b5c91bfe9b9f6015124c96
Cr-Commit-Position: refs/heads/master@{#15625}
Patch Set 1 #Patch Set 2 : Rebase with master #Patch Set 3 : MarkMemberTested in integration test. Bug revealed, TODO comment added. #Patch Set 4 : Rebase with master #Patch Set 5 : Unittest testing that it defaults to undefined #
Messages
Total messages: 31 (20 generated)
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, hta and deadbeef. I wanted to refactor underlying code to use rtc::Optional, but it lead to changes all over the place, with assumptions about a value being available (e.g. after change do we DCHECK, add an if or use 0 as default). I had modified 13 files and was still not done yet when I decided that this cleanup probably isn't worth it right now. So instead I'm just checking that it's non-negative, which fixes the bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/17985)
lgtm agree that rtc::optional is cleaner, but it's not free, and as you've discovered, it tends to making all the pieces of code care whether the code is present or not.
deadbeef@google.com changed reviewers: + deadbeef@google.com
When you say "only report positive values", does this mean the stat will be absent from the dictionary when a value hasn't been obtained yet? At TPAC, there was discussion about how we should report default values (empty string, -1 or 0 as applicable) in this case, to differentiate between "stat not available yet" vs "stat not supported at all by this browser". Or does that rule only apply to stats added to the spec in the future?
Description was changed from ========== RTCOutboundRTPStreamStats.roundTripTime: Only report positive values. Underlying stats gatherers may default it to -1. BUG=chromium:669877, chromium:627816 ========== to ========== RTCOutboundRTPStreamStats.roundTripTime: Only report non-negative values. Underlying stats gatherers may otherwise default it to -1. BUG=chromium:669877, 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/...
On 2016/12/09 19:28:32, Taylor wrote: > When you say "only report positive values", does this mean the stat will be > absent from the dictionary when a value hasn't been obtained yet? > > At TPAC, there was discussion about how we should report default values (empty > string, -1 or 0 as applicable) in this case, to differentiate between "stat not > available yet" vs "stat not supported at all by this browser". Or does that rule > only apply to stats added to the spec in the future? I would like to avoid magical values because they would be different for different stats (not just type, but -1 or 0 may or may not be possible values for a numeric stat). The spec-compliant thing to do, and what I do here, is to not define a stat if we don't have a value for it yet. (In C++ you can check is_defined(), in JavaScript it will be missing and therefore |undefined|.) But it would be nice to know what is browser-supported. I suppose for numerical values you could use NaN and for strings you could use empty string, but this is not spec-compliant and I'm not sure I like it. And you would only be able to check this for RTCStats dictionaries that were returned. I think the spec needs to be updated if we want to support this and to be addressed separately if we decide to do that. Any thoughts, Harald?
PTAL deadbeef and hta.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm There are 3 cases that are "unusual" for stats: - Not supported by platform. Omitting them is the right thing - because that's the only thing that's consistent with handling of future stats that we haven't invented yet. - No event of this type has happened yet. Returning zero is the right thing. - In this particular state (such as RTT before we've had a round trip happen), the question "what is this value" is meaningless. It seems to me that in the last case, omitting the value is more consistent and easier for the user to handle than inventing magical values that have to be different on a case by case basis. But it means that differentiating "question isn't meaningful" from "hasn't been implemented" gets harder. My current thinking is that if we need to have that, we need a separate API surface to indicate "these are the supported stats", just like we have the "get supported constraints" interface. Issue should be solved in spec, not in code. Until solved in spec, I think omitting for "isn't meaningful yet" is the right thing to do.
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/...
PTAL deadbeef. Given the above I have not changed this, but I did add a unittest.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hbos@webrtc.org
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/2562703007/#ps80001 (title: "Unittest testing that it defaults to undefined")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481795562424170,
"parent_rev": "e6d8c8bfad20a18236f7e5eae6574a28e53aef99", "commit_rev":
"2ee1813cd22454c52e87faaf12029a697b86d462"}
Message was sent while issue was closed.
Description was changed from ========== RTCOutboundRTPStreamStats.roundTripTime: Only report non-negative values. Underlying stats gatherers may otherwise default it to -1. BUG=chromium:669877, chromium:627816 ========== to ========== RTCOutboundRTPStreamStats.roundTripTime: Only report non-negative values. Underlying stats gatherers may otherwise default it to -1. BUG=chromium:669877, chromium:627816 Review-Url: https://codereview.webrtc.org/2562703007 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== RTCOutboundRTPStreamStats.roundTripTime: Only report non-negative values. Underlying stats gatherers may otherwise default it to -1. BUG=chromium:669877, chromium:627816 Review-Url: https://codereview.webrtc.org/2562703007 ========== to ========== RTCOutboundRTPStreamStats.roundTripTime: Only report non-negative values. Underlying stats gatherers may otherwise default it to -1. BUG=chromium:669877, chromium:627816 Committed: https://crrev.com/e10e6d1f47bd3e9570b5c91bfe9b9f6015124c96 Cr-Commit-Position: refs/heads/master@{#15625} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e10e6d1f47bd3e9570b5c91bfe9b9f6015124c96 Cr-Commit-Position: refs/heads/master@{#15625} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
