Properly handle different transports having different SSL roles.

Properly handle different transports having different SSL roles.

This meant splitting "transport_options" into audio/video/data options,
for when creating the answer, and giving "GetSslRole" a "transport_name"
parameter so we can retrieve the current role on a per-transport basis.

BUG=webrtc:4525
R=pthatcher@webrtc.org

Committed: https://crrev.com/f475d365a25036725c3f545f57de59d2cc902d17
Cr-Commit-Position: refs/heads/master@{#11192}

[Taylor Brandstetter, 2015-12-10 21:59:02]

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

[Taylor Brandstetter, 2015-12-10 21:59:02]

Note that it may seem redundant how "ice_restart" is duplicated 3 times even
though it's always the same for audio/video/data. But this will change in the
near future, after I refactor this CL: https://codereview.webrtc.org/1453813005/

[pthatcher1, 2015-12-11 03:16:09]

https://codereview.webrtc.org/1516993002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1516993002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:1065: session_->data_channel() &&
This is duplicated a lot.  Perhaps make a GetSslRole(base_channel, &role) helper
method?

https://codereview.webrtc.org/1516993002/diff/1/talk/app/webrtc/webrtcsession...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1516993002/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:419: &ssl_role)) {
Again, a helper method for GetSslRole(base_channel, &ssl_role) might be nice.

https://codereview.webrtc.org/1516993002/diff/1/talk/session/media/mediasessi...
File talk/session/media/mediasession.h (right):

https://codereview.webrtc.org/1516993002/diff/1/talk/session/media/mediasessi...
talk/session/media/mediasession.h:149: TransportOptions data_transport_options;
At first pass, this feels ugly, especially if we want to move to having Unified
Plan and having multiple audio, video, and data transports.  If anything, this
should be a vector of (transport_name, transport_options).

It also feels ugly for the bundle case, where there aren't separate audio,
video, and data transports.  

Could you try with a vector/map approach and see where that heads?


I also have been reluctant to approve https://codereview.webrtc.org/1453813005/
for the same reason, but I hadn't yet thought of a good way to approach it.  I
think a vector/map of options keyed by transport name is a good thing to try.

[Taylor Brandstetter, 2015-12-11 18:40:02]

https://codereview.webrtc.org/1516993002/diff/1/talk/session/media/mediasessi...
File talk/session/media/mediasession.h (right):

https://codereview.webrtc.org/1516993002/diff/1/talk/session/media/mediasessi...
talk/session/media/mediasession.h:149: TransportOptions data_transport_options;
On 2015/12/11 03:16:09, pthatcher1 wrote:
> At first pass, this feels ugly, especially if we want to move to having
Unified
> Plan and having multiple audio, video, and data transports.  If anything, this
> should be a vector of (transport_name, transport_options).
> 
> It also feels ugly for the bundle case, where there aren't separate audio,
> video, and data transports.  
> 
> Could you try with a vector/map approach and see where that heads?
> 
> 
> I also have been reluctant to approve
https://codereview.webrtc.org/1453813005/
> for the same reason, but I hadn't yet thought of a good way to approach it.  I
> think a vector/map of options keyed by transport name is a good thing to try.

I had the same thought. But my reasons for not doing a map were:

* It would be inconsistent with the current form of MediaSessionOptions (we
already have recv_audio/recv_video, video_bandwidth/data_bandwidth, etc.)
* It seems like it would be really messy to get the content name propagated to
the right places with the current status of our code.

But I'll try, and see where it heads.

[Taylor Brandstetter, 2016-01-07 20:57:06]

On 2015/12/11 18:40:02, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/1516993002/diff/1/talk/session/media/mediasessi...
> File talk/session/media/mediasession.h (right):
> 
>
https://codereview.webrtc.org/1516993002/diff/1/talk/session/media/mediasessi...
> talk/session/media/mediasession.h:149: TransportOptions
data_transport_options;
> On 2015/12/11 03:16:09, pthatcher1 wrote:
> > At first pass, this feels ugly, especially if we want to move to having
> Unified
> > Plan and having multiple audio, video, and data transports.  If anything,
this
> > should be a vector of (transport_name, transport_options).
> > 
> > It also feels ugly for the bundle case, where there aren't separate audio,
> > video, and data transports.  
> > 
> > Could you try with a vector/map approach and see where that heads?
> > 
> > 
> > I also have been reluctant to approve
> https://codereview.webrtc.org/1453813005/
> > for the same reason, but I hadn't yet thought of a good way to approach it. 
I
> > think a vector/map of options keyed by transport name is a good thing to
try.
> 
> I had the same thought. But my reasons for not doing a map were:
> 
> * It would be inconsistent with the current form of MediaSessionOptions (we
> already have recv_audio/recv_video, video_bandwidth/data_bandwidth, etc.)
> * It seems like it would be really messy to get the content name propagated to
> the right places with the current status of our code.
> 
> But I'll try, and see where it heads.

Ok; converting to a map got pretty messy. I think it will be a lot easier to do
that once we start doing the work to support unified plan SDP. Right now, all
over the place we have code that looks like:

DoStuffForAudio();
DoStuffForVideo();
DoStuffForData();

So audio_value/video_value/data_value makes sense. I think having a map will
only make sense when we have code that looks like:

for (each content) {
  DoStuffForContent(content.name);
}

When we make that change, we already know we'll have to turn
"recv_audio"/"recv_video"/"audio_bandwidth"/"video_bandwidth" into a map of
names to "content options". So it seems like it would be easy to clean up the
transport options in that same future CL.

[pthatcher1, 2016-01-07 21:12:03]

https://codereview.webrtc.org/1516993002/diff/60001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1516993002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:974:
session_->GetSslRole(session_->data_channel()->transport_name(), &role)) {
Can you make a method so you can call like this?

session_->GetSslRole(session_->data_channel(), &role)?  That could be used in
three places here.

https://codereview.webrtc.org/1516993002/diff/60001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1516993002/diff/60001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:989: }
What effect does moving this down have?  Can you leave a comment explaining why
it needs to be down here?

https://codereview.webrtc.org/1516993002/diff/60001/talk/session/media/medias...
File talk/session/media/mediasession.h (right):

https://codereview.webrtc.org/1516993002/diff/60001/talk/session/media/medias...
talk/session/media/mediasession.h:149: TransportOptions data_transport_options;
Can you put a TODO explaining why we have this and how we're going to fix it
later?

[Taylor Brandstetter, 2016-01-07 22:25:55]

https://codereview.webrtc.org/1516993002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1516993002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:1065: session_->data_channel() &&
On 2015/12/11 03:16:09, pthatcher1 wrote:
> This is duplicated a lot.  Perhaps make a GetSslRole(base_channel, &role)
helper
> method?

Done.

https://codereview.webrtc.org/1516993002/diff/1/talk/app/webrtc/webrtcsession...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1516993002/diff/1/talk/app/webrtc/webrtcsession...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:419: &ssl_role)) {
On 2015/12/11 03:16:09, pthatcher1 wrote:
> Again, a helper method for GetSslRole(base_channel, &ssl_role) might be nice.

Done.

https://codereview.webrtc.org/1516993002/diff/60001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1516993002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:974:
session_->GetSslRole(session_->data_channel()->transport_name(), &role)) {
On 2016/01/07 21:12:02, pthatcher1 wrote:
> Can you make a method so you can call like this?
> 
> session_->GetSslRole(session_->data_channel(), &role)?  That could be used in
> three places here.

This method will probably be refactored away when we support unified plan, but
sure.

https://codereview.webrtc.org/1516993002/diff/60001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1516993002/diff/60001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:989: }
On 2016/01/07 21:12:03, pthatcher1 wrote:
> What effect does moving this down have?  Can you leave a comment explaining
why
> it needs to be down here?

If somehow the DTLS roles were "active" for audio and "passive" for video, then
you start BUNDLEing on video, the answer will have "active" for both. But if you
try to push down the transport description before enabling BUNDLE (and
destroying the video transport), it will complain about a DTLS role conflict.

Bottom line is: we shouldn't be pushing down a transport description for a
transport that's about to be destroyed, especially since that description is now
referring to a DIFFERENT transport.

https://codereview.webrtc.org/1516993002/diff/60001/talk/session/media/medias...
File talk/session/media/mediasession.h (right):

https://codereview.webrtc.org/1516993002/diff/60001/talk/session/media/medias...
talk/session/media/mediasession.h:149: TransportOptions data_transport_options;
On 2016/01/07 21:12:03, pthatcher1 wrote:
> Can you put a TODO explaining why we have this and how we're going to fix it
> later?

Done.

[pthatcher1, 2016-01-07 23:53:53]

lgtm

[Taylor Brandstetter, 2016-01-08 23:36:10]

Description was changed from

==========
Properly handle different transports having different SSL roles.

This meant splitting "transport_options" into audio/video/data options,
for when creating the answer, and giving "GetSslRole" a "transport_name"
parameter so we can retrieve the current role on a per-transport basis.

BUG=webrtc:4525
==========

to

==========
Properly handle different transports having different SSL roles.

This meant splitting "transport_options" into audio/video/data options,
for when creating the answer, and giving "GetSslRole" a "transport_name"
parameter so we can retrieve the current role on a per-transport basis.

BUG=webrtc:4525
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/f475d365a25036725c3f545f5...
==========

[Taylor Brandstetter, 2016-01-08 23:36:11]

Committed patchset #5 (id:80001) manually as
f475d365a25036725c3f545f57de59d2cc902d17 (presubmit successful).

[commit-bot: I haz the power, 2016-01-08 23:36:11]

Description was changed from

==========
Properly handle different transports having different SSL roles.

This meant splitting "transport_options" into audio/video/data options,
for when creating the answer, and giving "GetSslRole" a "transport_name"
parameter so we can retrieve the current role on a per-transport basis.

BUG=webrtc:4525
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/f475d365a25036725c3f545f5...
==========

to

==========
Properly handle different transports having different SSL roles.

This meant splitting "transport_options" into audio/video/data options,
for when creating the answer, and giving "GetSslRole" a "transport_name"
parameter so we can retrieve the current role on a per-transport basis.

BUG=webrtc:4525
R=pthatcher@webrtc.org

Committed: https://crrev.com/f475d365a25036725c3f545f57de59d2cc902d17
Cr-Commit-Position: refs/heads/master@{#11192}
==========

[commit-bot: I haz the power, 2016-01-08 23:36:12]

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