|
|
Created:
5 years, 1 month ago by the sun Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, pthatcher1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionChange default SSRC for RTCP receiver reports to not collide with video.
BUG=chromium:547661
TBR=pthatcher@webrtc.org
Committed: https://crrev.com/8093d5442e4c365bfebc07abcf5fb653bd7a1d57
Cr-Commit-Position: refs/heads/master@{#10621}
Patch Set 1 #Patch Set 2 : Change default SSRC to something more obviously wrong. #
Total comments: 1
Patch Set 3 : removed comment #
Total comments: 1
Messages
Total messages: 25 (9 generated)
solenberg@webrtc.org changed reviewers: + mflodman@webrtc.org, pbos@webrtc.org
PTAL, this needs landing before today's roll to chromium.
Description was changed from ========== Change default SSRC for RTCP receiver reports to not collide with video. BUG=547661 ========== to ========== Change default SSRC for RTCP receiver reports to not collide with video. BUG=547661 ==========
On 2015/11/12 10:47:30, the sun wrote: > PTAL, this needs landing before today's roll to chromium. This requires bundlefilter to be updated though? It only lets SSRC=1 through as a special case?
On 2015/11/12 10:53:36, pbos-webrtc wrote: > On 2015/11/12 10:47:30, the sun wrote: > > PTAL, this needs landing before today's roll to chromium. > > This requires bundlefilter to be updated though? It only lets SSRC=1 through as > a special case? Not for the system to work as before, but yes, BundleFilter needs fixing as well. See https://code.google.com/p/chromium/issues/detail?id=547661
On 2015/11/12 10:54:48, the sun wrote: > On 2015/11/12 10:53:36, pbos-webrtc wrote: > > On 2015/11/12 10:47:30, the sun wrote: > > > PTAL, this needs landing before today's roll to chromium. > > > > This requires bundlefilter to be updated though? It only lets SSRC=1 through > as > > a special case? > > Not for the system to work as before, but yes, BundleFilter needs fixing as > well. See https://code.google.com/p/chromium/issues/detail?id=547661 If this gets stuck in BundleFilter (and dropped), what's the point of fixing what SSRC it contains? Decryption failure vs. dropped in BundleFilter, isn't that equivalent from a "no packets arriving" perspective? Am I misunderstanding something. I'm also wondering whether you need to fix: https://bugs.chromium.org/p/webrtc/issues/detail?id=4883#c9 but for audio (switching the receiver SSRC when removing senders, including back to SSRC=2 when all senders are gone).
Description was changed from ========== Change default SSRC for RTCP receiver reports to not collide with video. BUG=547661 ========== to ========== Change default SSRC for RTCP receiver reports to not collide with video. BUG=chromium:547661 ==========
solenberg@webrtc.org changed reviewers: + hta@webrtc.org
LGTM. This is the wrong long-term fix. But the fix itself does not increase our technical debt. The real fix is to make recipient of RTCP accept all RTCP packets independent of SSRC, and figure out from the SSRCs being reported on (within the packet) where they're supposed to go. Also, RTCP with Bundle should report on both audio and video streams, not separate reports for each.
On 2015/11/12 10:59:27, pbos-webrtc wrote: > On 2015/11/12 10:54:48, the sun wrote: > > On 2015/11/12 10:53:36, pbos-webrtc wrote: > > > On 2015/11/12 10:47:30, the sun wrote: > > > > PTAL, this needs landing before today's roll to chromium. > > > > > > This requires bundlefilter to be updated though? It only lets SSRC=1 through > > as > > > a special case? > > > > Not for the system to work as before, but yes, BundleFilter needs fixing as > > well. See https://code.google.com/p/chromium/issues/detail?id=547661 > > If this gets stuck in BundleFilter (and dropped), what's the point of fixing > what SSRC it contains? Decryption failure vs. dropped in BundleFilter, isn't > that equivalent from a "no packets arriving" perspective? Am I misunderstanding > something. The point is it won't get stuck in BundleFilter since in the test case where the issue was discovered, we have no receive streams at the sender (BundleFilter doesn't filter RTCP then). > I'm also wondering whether you need to fix: > https://bugs.chromium.org/p/webrtc/issues/detail?id=4883#c9 but for audio > (switching the receiver SSRC when removing senders, including back to SSRC=2 > when all senders are gone). It is true that we'll keep sending RRs on a stale SSRC, however that is still nothing new and should be fixed elsewhere.
https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.h:321: // This should really be fixed in the RTP/RTCP module. Why in the RTP/RTCP module?
On 2015/11/12 12:48:12, pbos-webrtc wrote: > https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv... > File talk/media/webrtc/webrtcvoiceengine.h (right): > > https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv... > talk/media/webrtc/webrtcvoiceengine.h:321: // This should really be fixed in the > RTP/RTCP module. > Why in the RTP/RTCP module? See Harald's comment.
On 2015/11/12 12:48:40, the sun wrote: > On 2015/11/12 12:48:12, pbos-webrtc wrote: > > > https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv... > > File talk/media/webrtc/webrtcvoiceengine.h (right): > > > > > https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv... > > talk/media/webrtc/webrtcvoiceengine.h:321: // This should really be fixed in > the > > RTP/RTCP module. > > Why in the RTP/RTCP module? > > See Harald's comment. Probably means that RTCP should be aggregated across Call, can we put that there instead of within which module the work should be done? We have more than one RTP/RTCP module, hence the confusion.
solenberg@webrtc.org changed reviewers: - mflodman@webrtc.org
On 2015/11/12 12:50:05, pbos-webrtc wrote: > On 2015/11/12 12:48:40, the sun wrote: > > On 2015/11/12 12:48:12, pbos-webrtc wrote: > > > > > > https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv... > > > File talk/media/webrtc/webrtcvoiceengine.h (right): > > > > > > > > > https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv... > > > talk/media/webrtc/webrtcvoiceengine.h:321: // This should really be fixed in > > the > > > RTP/RTCP module. > > > Why in the RTP/RTCP module? > > > > See Harald's comment. > > Probably means that RTCP should be aggregated across Call, can we put that there > instead of within which module the work should be done? We have more than one > RTP/RTCP module, hence the confusion. There, removed the comment. Linking to the bugs should be enough.
lgtm, please tbr pthatcher so he's aware of it
Description was changed from ========== Change default SSRC for RTCP receiver reports to not collide with video. BUG=chromium:547661 ========== to ========== Change default SSRC for RTCP receiver reports to not collide with video. BUG=chromium:547661 TBR=pthatcher@webrtc.org ==========
The CQ bit was checked by solenberg@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/1438183002/#ps40001 (title: "removed comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1438183002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1438183002/40001
pbos@webrtc.org changed reviewers: + pthatcher@webrtc.org
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8093d5442e4c365bfebc07abcf5fb653bd7a1d57 Cr-Commit-Position: refs/heads/master@{#10621}
Message was sent while issue was closed.
https://codereview.webrtc.org/1438183002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1438183002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.h:321: uint32_t receiver_reports_ssrc_ = 0xFA17FA17u; Sorry for being late to the bug. Changing the RR SSRC to something other than 1 may cause RRs to not be received if the SSRCs aren't signalled because the BundleFilter won't know what to do with them. I see there is a CL for updating the BundleFilter to let in all RRs, regardless of SSRC=1, but that's not submitted yet. Until it has been submitted and rolled out for a while, I think the fix here to change the SSRC is risky.
Message was sent while issue was closed.
On 2015/11/12 20:46:40, pthatcher1 wrote: > https://codereview.webrtc.org/1438183002/diff/40001/talk/media/webrtc/webrtcv... > File talk/media/webrtc/webrtcvoiceengine.h (right): > > https://codereview.webrtc.org/1438183002/diff/40001/talk/media/webrtc/webrtcv... > talk/media/webrtc/webrtcvoiceengine.h:321: uint32_t receiver_reports_ssrc_ = > 0xFA17FA17u; > Sorry for being late to the bug. > > Changing the RR SSRC to something other than 1 may cause RRs to not be received > if the SSRCs aren't signalled because the BundleFilter won't know what to do > with them. I see there is a CL for updating the BundleFilter to let in all RRs, > regardless of SSRC=1, but that's not submitted yet. Until it has been submitted > and rolled out for a while, I think the fix here to change the SSRC is risky. Back 3 weeks ago, when we had the default send channel in WVoMC, a *random* SSRC was picked for RRs in the case we're addressing in this isssue. That case being 1 audio/video sender, 1 audio/video receiver. Since there are no streams in the opposite direction, the sender's BundleFilter has not been populated with any receive streams and therefore will let through all RTCP. So the fix provided avoids the SRTCP index collision and allows RRs to flow *as before*. The result can be seen clearly here: https://chromeperf.appspot.com/group_report?bug_id=555023, i.e. the stats are back to nominal levels. I realize that in a more realistic use case, we still have the same problem as before: audio RRs will be dropped by BundleFilter (until the mentioned CL is landed). |