Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641763003:
RTCMediaStreamTrackStats collected on a per-attachment basis. (Closed)
DescriptionRTCMediaStreamTrackStats collected on a per-attachment basis.
According to recent spec change:
https://github.com/w3c/webrtc-stats/pull/138/files
This establishes the relationship between tracks and
[Voice/Video][Sender/Receiver]Info(s). Follow-up CLs will easily be able
to collect more stats from them.
BUG=webrtc:6757, chromium:659137, chromium:627816
Review-Url: https://codereview.webrtc.org/2641763003
Cr-Commit-Position: refs/heads/master@{#16181}
Committed: https://chromium.googlesource.com/external/webrtc/+/9e30274c03c8e812abeb37c7876648e3cae0805a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments #
Total comments: 1
Patch Set 3 : Multiple senders for local video track too, not just for local audio track #
Total comments: 4
Patch Set 4 : Added comments #
Messages
Total messages: 27 (18 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== RTCMediaStreamTrackStats collected on a per-attachment basis. BUG= ========== to ========== RTCMediaStreamTrackStats collected on a per-attachment basis. BUG=chromium:659137, webrtc:6757 ==========
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/...
Description was changed from ========== RTCMediaStreamTrackStats collected on a per-attachment basis. BUG=chromium:659137, webrtc:6757 ========== to ========== RTCMediaStreamTrackStats collected on a per-attachment basis. According to recent spec change: https://github.com/w3c/webrtc-stats/pull/138/files This establishes the relationship between tracks and [Voice/Video][Sender/Receiver]Info(s). Follow-up CLs will easily be able to collect more stats from them. BUG=webrtc:6757, chromium:659137, chromium:627816 ==========
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
Please take a look, hta and deadbeef. (This makes this CL obsolete: https://codereview.webrtc.org/2602033002)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:433: video_receiver_info.frame_width); If a frame hasn't been decoded yet, the width and height here will be 0. Should there be a "> 0" check similar to the ones above? https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:384: Could you add a comment above this, explaining that it sets up mock RtpSender/RtpReceiver/VoiceChannel/VideoChannels? Maybe just call the method "CreateMockRtpSendersReceiversAndChannels" (though that's a little long). https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:2213: } // namespace Since this CL adds the ability to have multiple track stats for a single track being sent, can one of the tests cover that?
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 hta, deadbeef. https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:433: video_receiver_info.frame_width); On 2017/01/18 20:10:27, Taylor Brandstetter wrote: > If a frame hasn't been decoded yet, the width and height here will be 0. Should > there be a "> 0" check similar to the ones above? Done. Also testing the default in a unittest. https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:384: On 2017/01/18 20:10:27, Taylor Brandstetter wrote: > Could you add a comment above this, explaining that it sets up mock > RtpSender/RtpReceiver/VoiceChannel/VideoChannels? Maybe just call the method > "CreateMockRtpSendersReceiversAndChannels" (though that's a little long). Done. https://codereview.webrtc.org/2641763003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:2213: } // namespace On 2017/01/18 20:10:27, Taylor Brandstetter wrote: > Since this CL adds the ability to have multiple track stats for a single track > being sent, can one of the tests cover that? Done. https://codereview.webrtc.org/2641763003/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (left): https://codereview.webrtc.org/2641763003/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1448: // |expected_local_audio_track.echo_return_loss_enhancement|. Test removed because this case is now covered by the previous test.
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.
lgtm
lgtm https://codereview.webrtc.org/2641763003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2641763003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1394: nit: Could add comment here that this VoiceSenderInfo uses default values and expects the relevant stats to be undefined. https://codereview.webrtc.org/2641763003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1537: // Remote audio track with undefined (default) values Typo, should say "remote video track"
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, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2641763003/#ps80001 (title: "Added comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2641763003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2641763003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1394: On 2017/01/19 17:20:32, Taylor Brandstetter wrote: > nit: Could add comment here that this VoiceSenderInfo uses default values and > expects the relevant stats to be undefined. Done. https://codereview.webrtc.org/2641763003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1537: // Remote audio track with undefined (default) values On 2017/01/19 17:20:32, Taylor Brandstetter wrote: > Typo, should say "remote video track" Done.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484905121956300,
"parent_rev": "fd6c94d3114d3a3e5db262110735919c7da3571b", "commit_rev":
"9e30274c03c8e812abeb37c7876648e3cae0805a"}
Message was sent while issue was closed.
Description was changed from ========== RTCMediaStreamTrackStats collected on a per-attachment basis. According to recent spec change: https://github.com/w3c/webrtc-stats/pull/138/files This establishes the relationship between tracks and [Voice/Video][Sender/Receiver]Info(s). Follow-up CLs will easily be able to collect more stats from them. BUG=webrtc:6757, chromium:659137, chromium:627816 ========== to ========== RTCMediaStreamTrackStats collected on a per-attachment basis. According to recent spec change: https://github.com/w3c/webrtc-stats/pull/138/files This establishes the relationship between tracks and [Voice/Video][Sender/Receiver]Info(s). Follow-up CLs will easily be able to collect more stats from them. BUG=webrtc:6757, chromium:659137, chromium:627816 Review-Url: https://codereview.webrtc.org/2641763003 Cr-Commit-Position: refs/heads/master@{#16181} Committed: https://chromium.googlesource.com/external/webrtc/+/9e30274c03c8e812abeb37c78... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/9e30274c03c8e812abeb37c78...
Message was sent while issue was closed.
On 2017/01/20 10:47:19, commit-bot: I haz the power wrote: > Committed patchset #4 (id:80001) as > https://chromium.googlesource.com/external/webrtc/+/9e30274c03c8e812abeb37c78... Caused flake fixed by https://codereview.webrtc.org/2640743007/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
