Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2610843003:
RTC[In/Out]boundRTPStreamStats.mediaTrackId collected. (Closed)
DescriptionRTC[In/Out]boundRTPStreamStats.mediaTrackId collected.
Based on the mapping between [Audio/Video]TrackInterface and
[Voice/Video][Sender/Receiver]Info.
The IDs of RTCMediaStreamTrackStats are updated to distinguish between
local and remote cases. Previously, if local and remote cases had the
same label only one of them would be included in the report (bug).
BUG=webrtc:6758, chromium:657854, chromium:657855, chromium:657856, chromium:627816
Review-Url: https://codereview.webrtc.org/2610843003
Cr-Commit-Position: refs/heads/master@{#16095}
Committed: https://chromium.googlesource.com/external/webrtc/+/84abeb1d37ee27147674cd94a5606bd4ac725d9a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #Patch Set 3 : Addressed comments #
Total comments: 2
Patch Set 4 : Addressed nit #Patch Set 5 : track_to_id_ for thread-safety and 'if (track)' on inbound #
Total comments: 1
Patch Set 6 : Rebase with master #
Messages
Total messages: 47 (34 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/...
Patchset #1 (id:1) has been deleted
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
Please take a look, hta and deadbeef. (Follow-up to https://codereview.webrtc.org/2611983002/)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (left): https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:335: } I think you should continue to check for duplicates. In theory a track can be part of multiple streams, though we don't handle that correctly at the moment. https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:757: if (audio_track) { There should always be a track for a receiver; you could DCHECK here. https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:2016: } // namespace webrtc Does this test need to be updated?
lgtm lgtm once deadbeef's comment has been addressed.
of course the "duplicate track" stuff is affected by the spec item under discussion on "what to do with multi-attached tracks", but let's land it this way until we address that.
PTAL deadbeef. https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (left): https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:335: } On 2017/01/04 23:58:00, Taylor Brandstetter wrote: > I think you should continue to check for duplicates. In theory a track can be > part of multiple streams, though we don't handle that correctly at the moment. Done. https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:757: if (audio_track) { On 2017/01/04 23:58:00, Taylor Brandstetter wrote: > There should always be a track for a receiver; you could DCHECK here. Done. https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2610843003/diff/20001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:2016: } // namespace webrtc On 2017/01/04 23:58:00, Taylor Brandstetter wrote: > Does this test need to be updated? Oh yes of course! 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/...
lgtm https://codereview.webrtc.org/2610843003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2610843003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:825: track_media_info_map_->GetVideoTrack(video_receiver_info); nit: If you add a DCHECK for remote audio tracks' existence, should add one for video as well for consistency.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/9664)
https://codereview.webrtc.org/2610843003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2610843003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:825: track_media_info_map_->GetVideoTrack(video_receiver_info); On 2017/01/09 18:58:10, Taylor Brandstetter wrote: > nit: If you add a DCHECK for remote audio tracks' existence, should add one for > video as well for consistency. Nice catch. 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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/21506)
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/...
Patchset #5 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
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...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/21563)
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Patchset #5 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Please take another look (even though you LGTM'd). See diff between Patch Set 5 and 4. I started a Hangouts call on Chromium with this patch applied (and fiddling to make new getStats be invoked alongside old getStats) and found two problems. 1. Chromium uses proxy tracks that invoke on the signaling thread. This could cause deadlocks when "track->id()" is invoked on the network thread (in ProduceRTPStreamStats_n). Solution: prepare a track to ID map on the signaling thread before ProduceRTPStreamStats_n. 2. The RTC_DCHECK for inbound tracks does not initially hold on Hangouts. Replaced by "if". https://codereview.webrtc.org/2610843003/diff/130001/webrtc/api/rtcstatscolle... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2610843003/diff/130001/webrtc/api/rtcstatscolle... webrtc/api/rtcstatscollector.cc:768: if (audio_track) { I started a Hangouts call on Chromium and found that RTC_DCHECK(audio_track) would be triggered and crash. I looked at the stats before ending the call and there was no instance of mediaTrackId that was undefined, so this may only be a problem initially when the call is being set up or in special cases. Either way I can't DCHECK this. Using "if" for inbound audio and video.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. The issue you saw in testing may be due to SSRCs being unsignaled, and no packets received yet? In which case it may make sense to have no RTCInboundRTPStreamStats yet. Anyway, nice job finding those issues.
On 2017/01/10 17:35:05, Taylor Brandstetter wrote: > lgtm. The issue you saw in testing may be due to SSRCs being unsignaled, and no > packets received yet? In which case it may make sense to have no > RTCInboundRTPStreamStats yet. Anyway, nice job finding those issues. Interesting, that may be the case. I documented this in the RTCInboundRTPStreamStats bug (crbug.com/657855#c13). I'll land this CL as-is when the CL it depends on lands.
On 2017/01/11 09:34:42, hbos wrote: > On 2017/01/10 17:35:05, Taylor Brandstetter wrote: > > lgtm. The issue you saw in testing may be due to SSRCs being unsignaled, and > no > > packets received yet? In which case it may make sense to have no > > RTCInboundRTPStreamStats yet. Anyway, nice job finding those issues. > > Interesting, that may be the case. I documented this in the > RTCInboundRTPStreamStats bug (crbug.com/657855#c13). > I'll land this CL as-is when the CL it depends on lands. Actually, I don't think my theory's right, since it looks like we don't even handle the unsignaled SSRC case (https://bugs.chromium.org/p/webrtc/issues/detail?id=6971). So we may want to look into this.
Patchset #6 (id:150001) has been deleted
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 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/2610843003/#ps170001 (title: "Rebase with master")
The CQ bit was unchecked by hbos@webrtc.org
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/2610843003/#ps170001 (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": 170001, "attempt_start_ts": 1484571837608690,
"parent_rev": "93f16d74fc1249d574ae3504945985970a10975c", "commit_rev":
"84abeb1d37ee27147674cd94a5606bd4ac725d9a"}
Message was sent while issue was closed.
Description was changed from ========== RTC[In/Out]boundRTPStreamStats.mediaTrackId collected. Based on the mapping between [Audio/Video]TrackInterface and [Voice/Video][Sender/Receiver]Info. The IDs of RTCMediaStreamTrackStats are updated to distinguish between local and remote cases. Previously, if local and remote cases had the same label only one of them would be included in the report (bug). BUG=webrtc:6758, chromium:657854, chromium:657855, chromium:657856, chromium:627816 ========== to ========== RTC[In/Out]boundRTPStreamStats.mediaTrackId collected. Based on the mapping between [Audio/Video]TrackInterface and [Voice/Video][Sender/Receiver]Info. The IDs of RTCMediaStreamTrackStats are updated to distinguish between local and remote cases. Previously, if local and remote cases had the same label only one of them would be included in the report (bug). BUG=webrtc:6758, chromium:657854, chromium:657855, chromium:657856, chromium:627816 Review-Url: https://codereview.webrtc.org/2610843003 Cr-Commit-Position: refs/heads/master@{#16095} Committed: https://chromium.googlesource.com/external/webrtc/+/84abeb1d37ee27147674cd94a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:170001) as https://chromium.googlesource.com/external/webrtc/+/84abeb1d37ee27147674cd94a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
