Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814233002:
Delete default_send_ssrc_. (Closed)
|
Created:
4 years, 9 months ago by nisse-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete default_send_ssrc_.
BUG=webrtc:5426
Committed: https://crrev.com/a4f07887c7bfcc2db1826a1fe3e068fd8604d1cd
Cr-Commit-Position: refs/heads/master@{#12112}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Delete test TwoStreamsReUseFirstStream. Tweak check in RemoveSendStream. #
Total comments: 3
Patch Set 3 : Make RemoveSendStream tolerate ssrc == 0, and return false. #
Total comments: 3
Patch Set 4 : Delete ssrc==0 check in RemoveSendStream. #
Messages
Total messages: 32 (10 generated)
nisse@webrtc.org changed reviewers: + pbos@webrtc.org, pthatcher@webrtc.org
The default_send_ssrc_ appears to be a remnant of no longer used features. Can we delete it?
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814233002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814233002/1
https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/base/videoengine... File webrtc/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/base/videoengine... webrtc/media/base/videoengine_unittest.h:1146: // TODO(nisse): Delete test? "default channel" concept is now gone. This test is not just verifying that we can add and remove streams, which I believe is covered by other tests. So, yes, please delete it. By the way, there is now no longer any reason to have this be a separate file from webrtcvideoengine2_unittest.cc. If you feel inclined, please merge the two and delete this file. https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1043: RTC_CHECK(ssrc != 0); Technically, 0 is a valid SSRC. We were using it as a special value before, but that was technically incorrect. If we've fixed that now, I see no reason to not allow SSRC 0 to be used.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gn_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1043: RTC_CHECK(ssrc != 0); On 2016/03/18 16:55:48, pthatcher1 wrote: > Technically, 0 is a valid SSRC. We were using it as a special value before, but > that was technically incorrect. If we've fixed that now, I see no reason to not > allow SSRC 0 to be used. I think it might hit some DCHECKs lower.
On 2016/03/20 21:19:20, pbos-webrtc wrote: > https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/engine/webrtcvid... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/engine/webrtcvid... > webrtc/media/engine/webrtcvideoengine2.cc:1043: RTC_CHECK(ssrc != 0); > On 2016/03/18 16:55:48, pthatcher1 wrote: > > Technically, 0 is a valid SSRC. We were using it as a special value before, > but > > that was technically incorrect. If we've fixed that now, I see no reason to > not > > allow SSRC 0 to be used. > > I think it might hit some DCHECKs lower. Should this fail right now? I'm not sure if this gets filtered in an above layer.
https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/base/videoengine... File webrtc/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/base/videoengine... webrtc/media/base/videoengine_unittest.h:1146: // TODO(nisse): Delete test? "default channel" concept is now gone. On 2016/03/18 16:55:48, pthatcher1 wrote: > This test is not just verifying that we can add and remove streams, which I > believe is covered by other tests. So, yes, please delete it. > > By the way, there is now no longer any reason to have this be a separate file > from webrtcvideoengine2_unittest.cc. If you feel inclined, please merge the two > and delete this file. Done. https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1814233002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1043: RTC_CHECK(ssrc != 0); On 2016/03/18 16:55:48, pthatcher1 wrote: > Technically, 0 is a valid SSRC. We were using it as a special value before, but > that was technically incorrect. If we've fixed that now, I see no reason to not > allow SSRC 0 to be used. I don't think ssrc == 0 works yet. There are RTC_DCHECKs in AddSendStream and AddRecvStream, and 0 is handled specially by default_unsignalled_ssrc handler. We should never have RTC_DCHECK crash on invalid values provided by the peer. Not sure if that can happen here (where are ssrc:s allocated?), but it makes some sense to keep the a check here to match the check in AddSendStream. But should change to RTC_DCHECK to really match that one.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814233002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814233002/20001
https://codereview.webrtc.org/1814233002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1814233002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1044: RTC_DCHECK(ssrc != 0); I think this should return false and not crash, because I think we can add these from munged SDP.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/1814233002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1814233002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1044: RTC_DCHECK(ssrc != 0); On 2016/03/21 09:54:04, pbos-webrtc wrote: > I think this should return false and not crash, because I think we can add these > from munged SDP. Does that apply to AddSendStream too?
On 2016/03/21 14:23:06, nisse-webrtc wrote: > https://codereview.webrtc.org/1814233002/diff/20001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/1814233002/diff/20001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:1044: RTC_DCHECK(ssrc != 0); > On 2016/03/21 09:54:04, pbos-webrtc wrote: > > I think this should return false and not crash, because I think we can add > these > > from munged SDP. > > Does that apply to AddSendStream too? Hoping pthatcher@ can answer that better, but probably yes, I don't think either of these are filtered.
https://codereview.webrtc.org/1814233002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1814233002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1044: RTC_DCHECK(ssrc != 0); On 2016/03/21 14:23:06, nisse-webrtc wrote: > On 2016/03/21 09:54:04, pbos-webrtc wrote: > > I think this should return false and not crash, because I think we can add > these > > from munged SDP. > > Does that apply to AddSendStream too? RemoveSendStream will only have passed to it the SSRC that was passed into AddSendStream. And, yes, the SDP can have SSRC=0: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... I'm guessing that we blow up in many places if that happens, but there's really no reason that we should. So we ought to be moving in the direction of removing those blow ups, not adding them. https://codereview.webrtc.org/1814233002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1814233002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1045: return false; {}s please.
https://codereview.webrtc.org/1814233002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1814233002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1045: return false; On 2016/03/22 21:30:28, pthatcher1 wrote: > {}s please. We don't need this here, because what will be passed in here is only what was passed into AddSendStream. So we should just allow ssrc == 0 and let AddSendStream control whether or not ssrc == 0 is supported.
lgtm Even though I would like the two things I mentioned fixed, I'll lgtm this and trust you'll do the right thing.
https://codereview.webrtc.org/1814233002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1814233002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1045: return false; On 2016/03/22 21:31:38, pthatcher1 wrote: > On 2016/03/22 21:30:28, pthatcher1 wrote: > > {}s please. > > We don't need this here, because what will be passed in here is only what was > passed into AddSendStream. So we should just allow ssrc == 0 and let > AddSendStream control whether or not ssrc == 0 is supported. Ok, deleted this check and comment.
lgtm
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1814233002/#ps60001 (title: "Delete ssrc==0 check in RemoveSendStream.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814233002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814233002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814233002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814233002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Delete default_send_ssrc_. BUG=webrtc:5426 ========== to ========== Delete default_send_ssrc_. BUG=webrtc:5426 Committed: https://crrev.com/a4f07887c7bfcc2db1826a1fe3e068fd8604d1cd Cr-Commit-Position: refs/heads/master@{#12112} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a4f07887c7bfcc2db1826a1fe3e068fd8604d1cd Cr-Commit-Position: refs/heads/master@{#12112} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
