Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 1 month ago by Taylor Brandstetter Modified:
5 years, 1 month ago Reviewers:
pthatcher, pthatcher1 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 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix for scenario where m-line is revived after being set to port 0.
When this is detected, we'll now "reconfigure" the senders and
receivers, which will reconnect the capturers/renderers to the
underlying streams which have been recreated.
BUG=webrtc:2136
Committed: https://crrev.com/faac497af560ece34301343eb40377fd5503f7a0
Cr-Commit-Position: refs/heads/master@{#10628}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressing comments. #Patch Set 3 : Going with alternate solution, as discussed, that deals with remote tracks getting ended. #Patch Set 4 : Making things a bit cleaner #
Total comments: 2
Patch Set 5 : Fixing typo. #
Created: 5 years, 1 month ago
Messages
Total messages: 15 (5 generated)
deadbeef@webrtc.org changed reviewers: + pthatcher@webrtc.org
Hi Peter; this is basically the same as the fix attempted initially, but instead of reconfiguring the senders/receivers EVERY time a description is set, we'll now only do it when the m-line goes from zero to non-zero. Which will hopefully fix the Android test that broke (it's passing locally).
https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... File talk/app/webrtc/peerconnection.cc (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... talk/app/webrtc/peerconnection.cc:954: } else if (audio_rejected_) { Can we get the equivalent of audio_rejected_ by looking at local_description_ before line 934? Then we wouldn't need the extra member variable. https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... talk/app/webrtc/peerconnection.cc:959: ReconfigureReceivers(cricket::MEDIA_TYPE_AUDIO); Should we have a ReconfigureSenderAndReceivers method? https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... talk/app/webrtc/peerconnection_unittest.cc:280: // Defaults to true. in offer => an offer? https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpreceiverin... File talk/app/webrtc/rtpreceiverinterface.h (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpreceiverin... talk/app/webrtc/rtpreceiverinterface.h:57: virtual void Reconfigure() = 0; Can you add a comment describing what effect this has and when it is appropriate to call it? https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.cc File talk/app/webrtc/rtpsender.cc (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.cc#... talk/app/webrtc/rtpsender.cc:197: provider_->SetCaptureDevice(ssrc_, source->GetVideoCapturer()); Is there any downside to calling this all the time? In other words, to just call Reconfigure() all the time and not have a separate SetVideoSend? https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.cc#... talk/app/webrtc/rtpsender.cc:202: void VideoRtpSender::SetVideoSend() { Actually, if this is separate, calling something like "ReconfigureJustX" or "ReconfigureEverythingExceptX" might make sense. Perhaps "ReconfigureEverythingExceptCaptureDevice". It's verbose, but it's at least clear what the intention is. https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.h File talk/app/webrtc/rtpsender.h (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.h#n... talk/app/webrtc/rtpsender.h:140: // Only calls SetVideoSend. Perhaps this would be more clear as "Only calls VideoChannel::SetVideoSend" Saying "SetVideoSend calls SetVideoSend" makes it sound recursive.
https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... File talk/app/webrtc/peerconnection.cc (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... talk/app/webrtc/peerconnection.cc:954: } else if (audio_rejected_) { On 2015/11/11 00:45:02, pthatcher1 wrote: > Can we get the equivalent of audio_rejected_ by looking at local_description_ > before line 934? Then we wouldn't need the extra member variable. It gets more complex if we go that route. We have to check both the remote and local description. But really what we're interested in is the last answer description (assuming port 0 offers must be met with port 0 answers). I started implementing this but it started to feel like overkill for a termporary hack. https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... talk/app/webrtc/peerconnection.cc:959: ReconfigureReceivers(cricket::MEDIA_TYPE_AUDIO); On 2015/11/11 00:45:02, pthatcher1 wrote: > Should we have a ReconfigureSenderAndReceivers method? Done. https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/peerconnectio... talk/app/webrtc/peerconnection_unittest.cc:280: // Defaults to true. On 2015/11/11 00:45:02, pthatcher1 wrote: > in offer => an offer? Done. https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpreceiverin... File talk/app/webrtc/rtpreceiverinterface.h (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpreceiverin... talk/app/webrtc/rtpreceiverinterface.h:57: virtual void Reconfigure() = 0; On 2015/11/11 00:45:02, pthatcher1 wrote: > Can you add a comment describing what effect this has and when it is appropriate > to call it? Done. Just to note, I'm planning to move this method off of the "interface" classes in a future CL, like we've discussed. https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.cc File talk/app/webrtc/rtpsender.cc (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.cc#... talk/app/webrtc/rtpsender.cc:197: provider_->SetCaptureDevice(ssrc_, source->GetVideoCapturer()); On 2015/11/11 00:45:02, pthatcher1 wrote: > Is there any downside to calling this all the time? In other words, to just > call Reconfigure() all the time and not have a separate SetVideoSend? If you trace your way down to WebRtcVideoChannel2::WebRtcVideoSendStream::SetCapturer, it definitely seems to have some undesired effects. I plan to merge SetVideoSend and SetCapturer in a later CL though, and then we can get rid of "ReconfigureEverythingButCapturer" https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.cc#... talk/app/webrtc/rtpsender.cc:202: void VideoRtpSender::SetVideoSend() { On 2015/11/11 00:45:02, pthatcher1 wrote: > Actually, if this is separate, calling something like "ReconfigureJustX" or > "ReconfigureEverythingExceptX" might make sense. Perhaps > "ReconfigureEverythingExceptCaptureDevice". It's verbose, but it's at least > clear what the intention is. Done. https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.h File talk/app/webrtc/rtpsender.h (right): https://codereview.webrtc.org/1428243005/diff/1/talk/app/webrtc/rtpsender.h#n... talk/app/webrtc/rtpsender.h:140: // Only calls SetVideoSend. On 2015/11/11 00:45:02, pthatcher1 wrote: > Perhaps this would be more clear as "Only calls VideoChannel::SetVideoSend" > > Saying "SetVideoSend calls SetVideoSend" makes it sound recursive. Done.
Description was changed from ========== Fix for scenario where m-line is revived after being set to port 0. When this is detected, we'll now "reconfigure" the senders and receivers, which will reconnect the capturers/renderers to the underlying streams which have been recreated. BUG=webrtc:2136 ========== to ========== Fix for scenario where m-line is revived after being set to port 0. When this is detected, we'll now "reconfigure" the senders and receivers, which will reconnect the capturers/renderers to the underlying streams which have been recreated. BUG=webrtc:2136 ==========
Here's the solution we discussed. I also needed to change peerconnection_unittest to account for the fact that tracks are now being added/removed from a stream.
pthatcher@google.com changed reviewers: + pthatcher@google.com
lgtm https://codereview.webrtc.org/1428243005/diff/60001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnection.h (right): https://codereview.webrtc.org/1428243005/diff/60001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection.h:240: // Called when a media type is rejected (m-line set to port 0)t. What's the ")t."? Did you mean ")."?
https://codereview.webrtc.org/1428243005/diff/60001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnection.h (right): https://codereview.webrtc.org/1428243005/diff/60001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection.h:240: // Called when a media type is rejected (m-line set to port 0)t. On 2015/11/12 19:44:19, pthatcher wrote: > What's the ")t."? Did you mean ")."? Yeah, not sure how that happened.
lgtm
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@google.com Link to the patchset: https://codereview.webrtc.org/1428243005/#ps80001 (title: "Fixing typo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1428243005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1428243005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/faac497af560ece34301343eb40377fd5503f7a0 Cr-Commit-Position: refs/heads/master@{#10628} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
