Fix for scenario where m-line is revived after being set to port 0.

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
Committed: https://crrev.com/faac497af560ece34301343eb40377fd5503f7a0
Cr-Commit-Position: refs/heads/master@{#10628}

[Taylor Brandstetter, 2015-11-10 20:28:00]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2015-11-10 20:28:00]

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).

[pthatcher1, 2015-11-11 00:45:02]

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.

[Taylor Brandstetter, 2015-11-11 01:33:30]

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.

[Taylor Brandstetter, 2015-11-11 22:48:38]

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
==========

[Taylor Brandstetter, 2015-11-11 22:50:26]

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, 2015-11-12 19:44:19]

pthatcher@google.com changed reviewers:
+ pthatcher@google.com

[pthatcher, 2015-11-12 19:44:19]

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 ")."?

[Taylor Brandstetter, 2015-11-12 19:54:11]

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.

[pthatcher1, 2015-11-12 20:00:47]

lgtm

[Taylor Brandstetter, 2015-11-12 23:25:44]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-11-12 23:25:44]

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.")

[commit-bot: I haz the power, 2015-11-12 23:26:01]

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

[commit-bot: I haz the power, 2015-11-12 23:33:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-12 23:33:19]

Patchset 5 (id:??) landed as
https://crrev.com/faac497af560ece34301343eb40377fd5503f7a0
Cr-Commit-Position: refs/heads/master@{#10628}