Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1455923003:
Allow default audio receive channel to receive on any unsignalled SSRC. (Closed)
|
Created:
5 years, 1 month ago by the sun Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAllow default audio receive channel to receive on any unsignalled SSRC.
BUG=webrtc:5208
Committed: https://crrev.com/7e63ef0e8f3baf832005e2e378b6834c0d005f12
Cr-Commit-Position: refs/heads/master@{#10723}
Patch Set 1 #Patch Set 2 : Add unit tests, fix one more case where we didn't do as before. #Patch Set 3 : rebase #
Total comments: 5
Patch Set 4 : better logic and grammar #Patch Set 5 : revert logic change #Patch Set 6 : rebase #Messages
Total messages: 18 (7 generated)
pthatcher@google.com changed reviewers: + pthatcher@google.com
lgtm
solenberg@webrtc.org changed reviewers: + hta@webrtc.org
Hi, before I request a downmerge of this fix, can you please take a close look?
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
Thank you for adding the tests, they are thorough. I have a suggestion for making the code more readable, but otherwise it looks good. https://codereview.webrtc.org/1455923003/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1455923003/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2450: if (default_recv_ssrc_ == -1 && GetReceiveChannelId(ssrc) == -1) { This might be more clear as: bool known_ssrc = (GetReceiveChannelId(ssrc) >= 0); bool have_default_recv_channel = (default_recv_ssrc_ >= 0); if (!known_ssrc && !have_default_recv_channel) { // Create a default recv channel ... call to AddRecvStream ... } // If the SSRC is unknown here, route it to the default channel. // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5208 int channel = GetReceiveChannelId(known_ssrc ? ssrc : default_recv_ssrc_); RTC_DCHECK(channel != -1); ... call to call_->Receiver()->DeliverPacket ... if (known_ssrc && webrtc::PacketReceiver::DELIVERY_OK != delivery_result) { return; } ... call to engine()->voe()->network()->ReceivedRTPPacket ... https://codereview.webrtc.org/1455923003/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1455923003/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:2494: // Note that the first unknown SSRC cannot 0, because we only support creating cannot 0 => cannot be 0
https://codereview.chromium.org/1455923003/diff/40001/talk/media/webrtc/webrt... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1455923003/diff/40001/talk/media/webrtc/webrt... talk/media/webrtc/webrtcvoiceengine.cc:2450: if (default_recv_ssrc_ == -1 && GetReceiveChannelId(ssrc) == -1) { On 2015/11/19 17:16:57, pthatcher1 wrote: > This might be more clear as: > > bool known_ssrc = (GetReceiveChannelId(ssrc) >= 0); > bool have_default_recv_channel = (default_recv_ssrc_ >= 0); > > if (!known_ssrc && !have_default_recv_channel) { > // Create a default recv channel > ... call to AddRecvStream ... > } > > // If the SSRC is unknown here, route it to the default channel. > // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5208 > int channel = GetReceiveChannelId(known_ssrc ? ssrc : default_recv_ssrc_); > RTC_DCHECK(channel != -1); > > ... call to call_->Receiver()->DeliverPacket ... > > if (known_ssrc && webrtc::PacketReceiver::DELIVERY_OK != delivery_result) { > return; > } > > ... call to engine()->voe()->network()->ReceivedRTPPacket ... > > > I tried this, mainly to get rid of the extra map lookup (which both mine and your solution requires), but I felt the whole thing turning into an uncomfortably large change to downstream. So if it is ok with you I'd rather keep it this simple and try to land the changes required to move packet delivery in under webrtc::AudioReceiveStream for the next cut. https://codereview.chromium.org/1455923003/diff/40001/talk/media/webrtc/webrt... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.chromium.org/1455923003/diff/40001/talk/media/webrtc/webrt... talk/media/webrtc/webrtcvoiceengine_unittest.cc:2494: // Note that the first unknown SSRC cannot 0, because we only support creating On 2015/11/19 17:16:58, pthatcher1 wrote: > cannot 0 => cannot be 0 Done, and below.
lgtm https://codereview.chromium.org/1455923003/diff/40001/talk/media/webrtc/webrt... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1455923003/diff/40001/talk/media/webrtc/webrt... talk/media/webrtc/webrtcvoiceengine.cc:2450: if (default_recv_ssrc_ == -1 && GetReceiveChannelId(ssrc) == -1) { On 2015/11/19 20:33:22, the sun wrote: > On 2015/11/19 17:16:57, pthatcher1 wrote: > > This might be more clear as: > > > > bool known_ssrc = (GetReceiveChannelId(ssrc) >= 0); > > bool have_default_recv_channel = (default_recv_ssrc_ >= 0); > > > > if (!known_ssrc && !have_default_recv_channel) { > > // Create a default recv channel > > ... call to AddRecvStream ... > > } > > > > // If the SSRC is unknown here, route it to the default channel. > > // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5208 > > int channel = GetReceiveChannelId(known_ssrc ? ssrc : default_recv_ssrc_); > > RTC_DCHECK(channel != -1); > > > > ... call to call_->Receiver()->DeliverPacket ... > > > > if (known_ssrc && webrtc::PacketReceiver::DELIVERY_OK != delivery_result) { > > return; > > } > > > > ... call to engine()->voe()->network()->ReceivedRTPPacket ... > > > > > > > > I tried this, mainly to get rid of the extra map lookup (which both mine and > your solution requires), but I felt the whole thing turning into an > uncomfortably large change to downstream. So if it is ok with you I'd rather > keep it this simple and try to land the changes required to move packet delivery > in under webrtc::AudioReceiveStream for the next cut. So you're suggesting: 1. Land the minimal diff that changes behavior and then pull over to chromium. 2. Land a "make it more readable" CL that doesn't change behavior. That sounds good to me, as long as we remember to do #2.
On 2015/11/19 21:01:22, pthatcher1 wrote: > lgtm > > https://codereview.chromium.org/1455923003/diff/40001/talk/media/webrtc/webrt... > File talk/media/webrtc/webrtcvoiceengine.cc (right): > > https://codereview.chromium.org/1455923003/diff/40001/talk/media/webrtc/webrt... > talk/media/webrtc/webrtcvoiceengine.cc:2450: if (default_recv_ssrc_ == -1 && > GetReceiveChannelId(ssrc) == -1) { > On 2015/11/19 20:33:22, the sun wrote: > > On 2015/11/19 17:16:57, pthatcher1 wrote: > > > This might be more clear as: > > > > > > bool known_ssrc = (GetReceiveChannelId(ssrc) >= 0); > > > bool have_default_recv_channel = (default_recv_ssrc_ >= 0); > > > > > > if (!known_ssrc && !have_default_recv_channel) { > > > // Create a default recv channel > > > ... call to AddRecvStream ... > > > } > > > > > > // If the SSRC is unknown here, route it to the default channel. > > > // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5208 > > > int channel = GetReceiveChannelId(known_ssrc ? ssrc : default_recv_ssrc_); > > > RTC_DCHECK(channel != -1); > > > > > > ... call to call_->Receiver()->DeliverPacket ... > > > > > > if (known_ssrc && webrtc::PacketReceiver::DELIVERY_OK != delivery_result) { > > > return; > > > } > > > > > > ... call to engine()->voe()->network()->ReceivedRTPPacket ... > > > > > > > > > > > > > I tried this, mainly to get rid of the extra map lookup (which both mine and > > your solution requires), but I felt the whole thing turning into an > > uncomfortably large change to downstream. So if it is ok with you I'd rather > > keep it this simple and try to land the changes required to move packet > delivery > > in under webrtc::AudioReceiveStream for the next cut. > > So you're suggesting: > > 1. Land the minimal diff that changes behavior and then pull over to chromium. > 2. Land a "make it more readable" CL that doesn't change behavior. > > That sounds good to me, as long as we remember to do #2. We (or I) can't forget since that's very much part of the VoE refactoring... :)
The CQ bit was checked by solenberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@google.com, pthatcher@webrtc.org Link to the patchset: https://codereview.chromium.org/1455923003/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455923003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455923003/100001
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)
The CQ bit was checked by solenberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1455923003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1455923003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7e63ef0e8f3baf832005e2e378b6834c0d005f12 Cr-Commit-Position: refs/heads/master@{#10723} |
