Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2675923002:
RTCIceCandidatePairStats.available[Outgoing/Incoming]Bitrate collected. (Closed)
DescriptionRTCIceCandidatePairStats.available[Outgoing/Incoming]Bitrate collected.
Collected for current pairs, undefined for other pairs. This is the
same as the old stats' VideoBwe.googAvailable[Send/Receive]Bandwidth.
NOTE: The value this is based on for incoming bitrate is not set. This
CL wires it up but has a TODO that the incoming bitrate needs to be
collected properly. (Same problem for both old and new stats.)
Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-availableoutgoingbitrate
Discussion: https://github.com/w3c/webrtc-stats/issues/112#issuecomment-277167781
BUG=webrtc:7062
Review-Url: https://codereview.webrtc.org/2675923002
Cr-Commit-Position: refs/heads/master@{#16472}
Committed: https://chromium.googlesource.com/external/webrtc/+/338f78ac95addee5f3540407037d745e7ceca827
Patch Set 1 #
Total comments: 5
Patch Set 2 : Make sure outgoing bitrate is defined for selected pair. TODO for incoming bitrate. #Patch Set 3 : Rebase with master #
Messages
Total messages: 37 (21 generated)
Patchset #1 (id:1) has been deleted
hbos@webrtc.org changed reviewers: + hta@webrtc.org, stefan@webrtc.org
Please take a look stefan, hta.
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: This issue passed the CQ dry run.
The CQ bit was checked by stefan@webrtc.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
On 2017/02/06 16:05:13, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > Committers are members of the group "project-webrtc-committers". > Note that this has nothing to do with OWNERS files. sorry, i accidentally pressed commit on the wrong CL :)
This mostly looks good, but I think someone with better knowledge of the details should take a look too. https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:860: if (info.best_connection && video_media_info && Is best_connection always the connection in used?
On 2017/02/06 16:05:34, stefan-webrtc wrote: > On 2017/02/06 16:05:13, commit-bot: I haz the power wrote: > > No L-G-T-M from a valid reviewer yet. > > CQ run can only be started by full committers or once the patch has > > received an L-G-T-M from a full committer. > > Even if an L-G-T-M may have been provided, it was from a non-committer, > > _not_ a full super star committer. > > Committers are members of the group "project-webrtc-committers". > > Note that this has nothing to do with OWNERS files. > > sorry, i accidentally pressed commit on the wrong CL :) I don't mind ((((:
Description was changed from ========== RTCIceCandidatePairStats.available[Outgoing/Incoming]Bitrate collected. Collected for current pairs, undefined for other pairs. This is the same as the old stats' VideoBwe.googAvailable[Send/Receive]Bandwidth. Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-availableout... Discussion: https://github.com/w3c/webrtc-stats/issues/112#issuecomment-277167781 BUG=webrtc:7062 ========== to ========== RTCIceCandidatePairStats.available[Outgoing/Incoming]Bitrate collected. Collected for current pairs, undefined for other pairs. This is the same as the old stats' VideoBwe.googAvailable[Send/Receive]Bandwidth. Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-availableout... Discussion: https://github.com/w3c/webrtc-stats/issues/112#issuecomment-277167781 BUG=webrtc:7062 ==========
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org
https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:860: if (info.best_connection && video_media_info && On 2017/02/06 16:08:17, stefan-webrtc wrote: > Is best_connection always the connection in used? To my understanding, yes. +deadbeef, do you know?
https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:860: if (info.best_connection && video_media_info && On 2017/02/06 16:12:37, hbos wrote: > On 2017/02/06 16:08:17, stefan-webrtc wrote: > > Is best_connection always the connection in used? > > To my understanding, yes. +deadbeef, do you know? Yes, it is. It was even renamed from "best_connection" to "selected_connection" in some places, but not here.
lgtm from my side then, but please get someone else's opinion too as I haven't worked with most of these files.
Alrighty :) Please take a look, hta.
lgtm https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstats_integr... File webrtc/pc/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:372: if (candidate_pair.available_outgoing_bitrate.is_defined()) { This isn't quite nice, because any implementation that doesn't populate the avaiable bitrates will pass. Is it possible to define conditions under which this *should* be defined, and test for those rather than on whether it's present?
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Description was changed from ========== RTCIceCandidatePairStats.available[Outgoing/Incoming]Bitrate collected. Collected for current pairs, undefined for other pairs. This is the same as the old stats' VideoBwe.googAvailable[Send/Receive]Bandwidth. Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-availableout... Discussion: https://github.com/w3c/webrtc-stats/issues/112#issuecomment-277167781 BUG=webrtc:7062 ========== to ========== RTCIceCandidatePairStats.available[Outgoing/Incoming]Bitrate collected. Collected for current pairs, undefined for other pairs. This is the same as the old stats' VideoBwe.googAvailable[Send/Receive]Bandwidth. NOTE: The value this is based on for incoming bitrate is not set. This CL wires it up but has a TODO that the incoming bitrate needs to be collected properly. (Same problem for both old and new stats.) Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-availableout... Discussion: https://github.com/w3c/webrtc-stats/issues/112#issuecomment-277167781 BUG=webrtc:7062 ==========
https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstats_integr... File webrtc/pc/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2675923002/diff/20001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:372: if (candidate_pair.available_outgoing_bitrate.is_defined()) { On 2017/02/07 11:34:37, hta-webrtc wrote: > This isn't quite nice, because any implementation that doesn't populate the > avaiable bitrates will pass. > Is it possible to define conditions under which this *should* be defined, and > test for those rather than on whether it's present? > Yes! Updated the code to check if (selected pair) defined; else undefined; This worked for outgoing bitrate, but incoming is always undefined. Digging deeper, this value is a variable that is never set under any circumstances (and indeed, the value in old stats is 0 on sample websites). So... I'm leaving it wired up in case the incoming bitrate is fixed for the old stats, but I added a TODO about it and the integration test expects it to be undefined.
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, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2675923002/#ps100001 (title: "Rebase with master")
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
Try jobs failed on following builders: ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17028) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
Patchset #3 (id:100001) has been deleted
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, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2675923002/#ps120001 (title: "Rebase with master")
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": 120001, "attempt_start_ts": 1486474310502880,
"parent_rev": "3443bb75a08811a16fd2dd406f72878703f4a861", "commit_rev":
"338f78ac95addee5f3540407037d745e7ceca827"}
Message was sent while issue was closed.
Description was changed from ========== RTCIceCandidatePairStats.available[Outgoing/Incoming]Bitrate collected. Collected for current pairs, undefined for other pairs. This is the same as the old stats' VideoBwe.googAvailable[Send/Receive]Bandwidth. NOTE: The value this is based on for incoming bitrate is not set. This CL wires it up but has a TODO that the incoming bitrate needs to be collected properly. (Same problem for both old and new stats.) Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-availableout... Discussion: https://github.com/w3c/webrtc-stats/issues/112#issuecomment-277167781 BUG=webrtc:7062 ========== to ========== RTCIceCandidatePairStats.available[Outgoing/Incoming]Bitrate collected. Collected for current pairs, undefined for other pairs. This is the same as the old stats' VideoBwe.googAvailable[Send/Receive]Bandwidth. NOTE: The value this is based on for incoming bitrate is not set. This CL wires it up but has a TODO that the incoming bitrate needs to be collected properly. (Same problem for both old and new stats.) Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-availableout... Discussion: https://github.com/w3c/webrtc-stats/issues/112#issuecomment-277167781 BUG=webrtc:7062 Review-Url: https://codereview.webrtc.org/2675923002 Cr-Commit-Position: refs/heads/master@{#16472} Committed: https://chromium.googlesource.com/external/webrtc/+/338f78ac95addee5f35404070... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/338f78ac95addee5f35404070... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
