Do not create a temporary transport channel when using max-bundle

Do not create a temporary transport channel when using max-bundle

With this change, when max-bundle and rtcp-mux are both enabled, we no
longer create and destroy a temporary transport channel when a media
channel gets added. Instead, the media channel uses the correct bundled
transport channel from the start.

This fixes a bug where adding a media type would cause the ICE state to
briefly become Disconnected and then immediately recover. The temporary
channel was created in a non-writable state, which caused the
TransportController to declare the ICE state to be Disconnected (as not
all transport channels were writable). Right after creation, the
temporary channel was then destroyed and the ICE state went back to the
correct one.

BUG=webrtc:5856
Committed: https://crrev.com/6c87a67b63cee30e007a64b8f1de0aede93ea0da
Cr-Commit-Position: refs/heads/master@{#12781}

[skvlad, 2016-05-11 02:39:11]

skvlad@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, honghaiz@webrtc.org, pthatcher@webrtc.org

[pthatcher1, 2016-05-11 06:20:39]

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1731: const std::string&
WebRtcSession::GetTransportForChannel(
Why not, instead, return a const pointer that is NULL when it's not bundled?

Also, should we call this GetBundleTransportName?

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
PeerConnectionInterface::kBundlePolicyMaxBundle) {
If the policy is max bundle, then all the channels must be in the bundle and
that bundle must have a content name.  So you could probably just use
bundle->FirstContentName()  in all of the calls below and you won't need
GetTranpsortForChannel.

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1800: PeerConnectionInterface::kRtcpMuxPolicyRequire
/* rtcp */,
Can you put this on a separate line?

bool rtcp_mux = (rtcp_mux_policy_ ==
PeerConnectionInterface::kRtcpMuxPolicyPeerConnectionInterface::kRtcpMuxPolicyRequire);

Then you could use it here and below.

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1824: PeerConnectionInterface::kRtcpMuxPolicyRequire
/* rtcp */,
Same thing here.

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1849: data_channel_type_));
And here.

[Taylor Brandstetter, 2016-05-11 15:32:23]

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
PeerConnectionInterface::kBundlePolicyMaxBundle) {
On 2016/05/11 06:20:39, pthatcher1 wrote:
> If the policy is max bundle, then all the channels must be in the bundle and
> that bundle must have a content name.  So you could probably just use
> bundle->FirstContentName()  in all of the calls below and you won't need
> GetTranpsortForChannel.

JSEP does say the application can't munge BUNDLE groups or "a=bundle-only" (once
we implement that). But these channels could be created from a *remote*
description, and a remote peer doesn't have the same restrictions as a JSEP
implementation (it could decide to use bundle-only on a subset of media
descriptions). So I think this should be left as-is. Unless we start rejecting
non-BUNDLE media descriptions (working group hasn't decided on that yet).

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1800: PeerConnectionInterface::kRtcpMuxPolicyRequire
/* rtcp */,
This means that if we receive a remote description with no "a=rtcp-mux" and our
policy is "require", RTCP will silently fail to work as opposed to us rejecting
the media description (which JSEP says we should do). Could you add a TODO
comment to reject media descriptions without "a=rtcp-mux" when in "require"
mode?

Oh, and the "bool rtcp" parameter tells whether RTCP is enabled *at all*. So
this would turn off RTCP for anyone not using "require mux". I think there need
to be two separate parameters: "RTCP enabled" and "RTCP mux required".

[honghaiz3, 2016-05-11 16:42:11]

On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
> File webrtc/api/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
> webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
> PeerConnectionInterface::kBundlePolicyMaxBundle) {
> On 2016/05/11 06:20:39, pthatcher1 wrote:
> > If the policy is max bundle, then all the channels must be in the bundle and
> > that bundle must have a content name.  So you could probably just use
> > bundle->FirstContentName()  in all of the calls below and you won't need
> > GetTranpsortForChannel.
> 
> JSEP does say the application can't munge BUNDLE groups or "a=bundle-only"
(once
> we implement that). But these channels could be created from a *remote*
> description, and a remote peer doesn't have the same restrictions as a JSEP
> implementation (it could decide to use bundle-only on a subset of media
> descriptions). So I think this should be left as-is. Unless we start rejecting
> non-BUNDLE media descriptions (working group hasn't decided on that yet).
> 
>
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
> webrtc/api/webrtcsession.cc:1800:
PeerConnectionInterface::kRtcpMuxPolicyRequire
> /* rtcp */,
> This means that if we receive a remote description with no "a=rtcp-mux" and
our
> policy is "require", RTCP will silently fail to work as opposed to us
rejecting
> the media description (which JSEP says we should do). Could you add a TODO
> comment to reject media descriptions without "a=rtcp-mux" when in "require"
> mode?
> 
> Oh, and the "bool rtcp" parameter tells whether RTCP is enabled *at all*. So
> this would turn off RTCP for anyone not using "require mux". I think there
need
> to be two separate parameters: "RTCP enabled" and "RTCP mux required".

I wonder why 
when a new transport channel is added (typically without writability) and all
existing transport channels are writable, 
the transport controller declare ICE_DISCONNECT and triggering ICE restart.
Even with this fix, the problem may still exist for unbundled case. 
When that happened, we really should not need an ICE restart, right?
If a new transport channel is added, it should be either removed quickly if it
is for temporary use, or it should start ICE to establish connections.

[Taylor Brandstetter, 2016-05-11 19:54:47]

On 2016/05/11 16:42:11, honghaiz3 wrote:
> On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> > https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
> > File webrtc/api/webrtcsession.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
> > webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
> > PeerConnectionInterface::kBundlePolicyMaxBundle) {
> > On 2016/05/11 06:20:39, pthatcher1 wrote:
> > > If the policy is max bundle, then all the channels must be in the bundle
and
> > > that bundle must have a content name.  So you could probably just use
> > > bundle->FirstContentName()  in all of the calls below and you won't need
> > > GetTranpsortForChannel.
> > 
> > JSEP does say the application can't munge BUNDLE groups or "a=bundle-only"
> (once
> > we implement that). But these channels could be created from a *remote*
> > description, and a remote peer doesn't have the same restrictions as a JSEP
> > implementation (it could decide to use bundle-only on a subset of media
> > descriptions). So I think this should be left as-is. Unless we start
rejecting
> > non-BUNDLE media descriptions (working group hasn't decided on that yet).
> > 
> >
>
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
> > webrtc/api/webrtcsession.cc:1800:
> PeerConnectionInterface::kRtcpMuxPolicyRequire
> > /* rtcp */,
> > This means that if we receive a remote description with no "a=rtcp-mux" and
> our
> > policy is "require", RTCP will silently fail to work as opposed to us
> rejecting
> > the media description (which JSEP says we should do). Could you add a TODO
> > comment to reject media descriptions without "a=rtcp-mux" when in "require"
> > mode?
> > 
> > Oh, and the "bool rtcp" parameter tells whether RTCP is enabled *at all*. So
> > this would turn off RTCP for anyone not using "require mux". I think there
> need
> > to be two separate parameters: "RTCP enabled" and "RTCP mux required".
> 
> I wonder why 
> when a new transport channel is added (typically without writability) and all
> existing transport channels are writable, 
> the transport controller declare ICE_DISCONNECT and triggering ICE restart.
> Even with this fix, the problem may still exist for unbundled case. 
> When that happened, we really should not need an ICE restart, right?
> If a new transport channel is added, it should be either removed quickly if it
> is for temporary use, or it should start ICE to establish connections.

Going to "disconnected" when adding a new transport channel is a longstanding
bug (since before the TransportController refactoring). The state should really
be either "new" or "connected". I was just waiting for the working group to
decide what it should be before I fixed the bug.

[pthatcher1, 2016-05-11 22:23:20]

On 2016/05/11 19:54:47, Taylor Brandstetter wrote:
> On 2016/05/11 16:42:11, honghaiz3 wrote:
> > On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> > >
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
> > > File webrtc/api/webrtcsession.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
> > > webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
> > > PeerConnectionInterface::kBundlePolicyMaxBundle) {
> > > On 2016/05/11 06:20:39, pthatcher1 wrote:
> > > > If the policy is max bundle, then all the channels must be in the bundle
> and
> > > > that bundle must have a content name.  So you could probably just use
> > > > bundle->FirstContentName()  in all of the calls below and you won't need
> > > > GetTranpsortForChannel.
> > > 
> > > JSEP does say the application can't munge BUNDLE groups or "a=bundle-only"
> > (once
> > > we implement that). But these channels could be created from a *remote*
> > > description, and a remote peer doesn't have the same restrictions as a
JSEP
> > > implementation (it could decide to use bundle-only on a subset of media
> > > descriptions). So I think this should be left as-is. Unless we start
> rejecting
> > > non-BUNDLE media descriptions (working group hasn't decided on that yet).
> > > 
> > >
> >
>
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
> > > webrtc/api/webrtcsession.cc:1800:
> > PeerConnectionInterface::kRtcpMuxPolicyRequire
> > > /* rtcp */,
> > > This means that if we receive a remote description with no "a=rtcp-mux"
and
> > our
> > > policy is "require", RTCP will silently fail to work as opposed to us
> > rejecting
> > > the media description (which JSEP says we should do). Could you add a TODO
> > > comment to reject media descriptions without "a=rtcp-mux" when in
"require"
> > > mode?
> > > 
> > > Oh, and the "bool rtcp" parameter tells whether RTCP is enabled *at all*.
So
> > > this would turn off RTCP for anyone not using "require mux". I think there
> > need
> > > to be two separate parameters: "RTCP enabled" and "RTCP mux required".
> > 
> > I wonder why 
> > when a new transport channel is added (typically without writability) and
all
> > existing transport channels are writable, 
> > the transport controller declare ICE_DISCONNECT and triggering ICE restart.
> > Even with this fix, the problem may still exist for unbundled case. 
> > When that happened, we really should not need an ICE restart, right?
> > If a new transport channel is added, it should be either removed quickly if
it
> > is for temporary use, or it should start ICE to establish connections.
> 
> Going to "disconnected" when adding a new transport channel is a longstanding
> bug (since before the TransportController refactoring). The state should
really
> be either "new" or "connected". I was just waiting for the working group to
> decide what it should be before I fixed the bug.

Why don't we just pick one and then change it later if the WG picks the other
one?  It can't be that hard to change, can it?

I'd say we go with "new" for now.

[pthatcher1, 2016-05-11 22:23:47]

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
PeerConnectionInterface::kBundlePolicyMaxBundle) {
On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> On 2016/05/11 06:20:39, pthatcher1 wrote:
> > If the policy is max bundle, then all the channels must be in the bundle and
> > that bundle must have a content name.  So you could probably just use
> > bundle->FirstContentName()  in all of the calls below and you won't need
> > GetTranpsortForChannel.
> 
> JSEP does say the application can't munge BUNDLE groups or "a=bundle-only"
(once
> we implement that). But these channels could be created from a *remote*
> description, and a remote peer doesn't have the same restrictions as a JSEP
> implementation (it could decide to use bundle-only on a subset of media
> descriptions). So I think this should be left as-is. Unless we start rejecting
> non-BUNDLE media descriptions (working group hasn't decided on that yet).

If we are in max-bundle mode, we already reject descriptions that don't have a
bundle group.  But you're right that we don't verify that all the m-lines are in
the max-bundle.  But I think we should reject descriptions that don't have
everything bundled when in max-bundle mode, unless the standards body decides
otherwise (and I'll argue in the group that max-bundle should mean we reject
non-bundled m-lines).

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1800: PeerConnectionInterface::kRtcpMuxPolicyRequire
/* rtcp */,
On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> This means that if we receive a remote description with no "a=rtcp-mux" and
our
> policy is "require", RTCP will silently fail to work as opposed to us
rejecting
> the media description (which JSEP says we should do). Could you add a TODO
> comment to reject media descriptions without "a=rtcp-mux" when in "require"
> mode?
> 
> Oh, and the "bool rtcp" parameter tells whether RTCP is enabled *at all*. So
> this would turn off RTCP for anyone not using "require mux". I think there
need
> to be two separate parameters: "RTCP enabled" and "RTCP mux required".

The "rtcp" parameter here ultimately gets passed into the BaseChannel parameters
"rtcp_tranpsort_enabled", so it might make sense to call it that here as well
and everywhere from here to there (instead of calling it "rtcp").  That would
make it more clear what this value means.  

And having rtcp_transport_enabled = (rtcp_mux_policy_ ==
PeerConnectionInterface::kRtcpMuxPolicyNegotiate), I believe, works.

You're right that we should reject the remote description if a=rtcp-mux is not
in it and rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire. 
We should fix that in another CL.  I don't think it impacts this CL.

[skvlad, 2016-05-11 22:24:20]

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1751: // TODO(mallinath) - Add a correct error code
if the channels are not created
This TODO seems to not apply - there is a check for this condition in
ValidateSessionDescription(). We should be rejecting descriptions which have
bundled channels without RTCP mux. 

There doesn't seem to be a check for the case where the offerer requires
RTCP-mux and the answer does not enable it. I've added a TODO to add one.

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
PeerConnectionInterface::kBundlePolicyMaxBundle) {
On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> On 2016/05/11 06:20:39, pthatcher1 wrote:
> > If the policy is max bundle, then all the channels must be in the bundle and
> > that bundle must have a content name.  So you could probably just use
> > bundle->FirstContentName()  in all of the calls below and you won't need
> > GetTranpsortForChannel.
> 
> JSEP does say the application can't munge BUNDLE groups or "a=bundle-only"
(once
> we implement that). But these channels could be created from a *remote*
> description, and a remote peer doesn't have the same restrictions as a JSEP
> implementation (it could decide to use bundle-only on a subset of media
> descriptions). So I think this should be left as-is. Unless we start rejecting
> non-BUNDLE media descriptions (working group hasn't decided on that yet).

Added a TODO to reject the offer in this case when the JSEP change is accepted.

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1800: PeerConnectionInterface::kRtcpMuxPolicyRequire
/* rtcp */,
On 2016/05/11 06:20:39, pthatcher1 wrote:
> Can you put this on a separate line?
> 
> bool rtcp_mux = (rtcp_mux_policy_ ==
>
PeerConnectionInterface::kRtcpMuxPolicyPeerConnectionInterface::kRtcpMuxPolicyRequire);
> 
> Then you could use it here and below.

Done.

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1800: PeerConnectionInterface::kRtcpMuxPolicyRequire
/* rtcp */,
On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> This means that if we receive a remote description with no "a=rtcp-mux" and
our
> policy is "require", RTCP will silently fail to work as opposed to us
rejecting
> the media description (which JSEP says we should do). Could you add a TODO
> comment to reject media descriptions without "a=rtcp-mux" when in "require"
> mode?
Added a comment in ValidateSessionDescription. 

> 
> Oh, and the "bool rtcp" parameter tells whether RTCP is enabled *at all*. So
> this would turn off RTCP for anyone not using "require mux". I think there
need
> to be two separate parameters: "RTCP enabled" and "RTCP mux required".
If RTCP mux is enabled, CreateVoiceChannel calls ActivateRtcpMux right after
creating the channel. This creates the RtcpMuxFilter which appears to be enough
to send and receive muxed RTCP.

Setting rtcp=false - if my understanding is correct - only prevents us from
creating and then destroying the non-muxed RTCP transport channel.

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1824: PeerConnectionInterface::kRtcpMuxPolicyRequire
/* rtcp */,
On 2016/05/11 06:20:39, pthatcher1 wrote:
> Same thing here.

Acknowledged.

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1849: data_channel_type_));
On 2016/05/11 06:20:39, pthatcher1 wrote:
> And here.

Done.

[pthatcher1, 2016-05-11 22:25:47]

On 2016/05/11 22:23:47, pthatcher1 wrote:
> https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
> File webrtc/api/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
> webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
> PeerConnectionInterface::kBundlePolicyMaxBundle) {
> On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> > On 2016/05/11 06:20:39, pthatcher1 wrote:
> > > If the policy is max bundle, then all the channels must be in the bundle
and
> > > that bundle must have a content name.  So you could probably just use
> > > bundle->FirstContentName()  in all of the calls below and you won't need
> > > GetTranpsortForChannel.
> > 
> > JSEP does say the application can't munge BUNDLE groups or "a=bundle-only"
> (once
> > we implement that). But these channels could be created from a *remote*
> > description, and a remote peer doesn't have the same restrictions as a JSEP
> > implementation (it could decide to use bundle-only on a subset of media
> > descriptions). So I think this should be left as-is. Unless we start
rejecting
> > non-BUNDLE media descriptions (working group hasn't decided on that yet).
> 
> If we are in max-bundle mode, we already reject descriptions that don't have a
> bundle group.  But you're right that we don't verify that all the m-lines are
in
> the max-bundle.  But I think we should reject descriptions that don't have
> everything bundled when in max-bundle mode, unless the standards body decides
> otherwise (and I'll argue in the group that max-bundle should mean we reject
> non-bundled m-lines).

Sorry, I should have been more clear.  Long-term (as we become more standards
compliant), we should reject m-lines (with port 0) that are not bundled. 
Short-term (while we reject the whole description instead), we should verify
that all m-lines are in the bundle and reject if they aren't.

> 
>
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
> webrtc/api/webrtcsession.cc:1800:
PeerConnectionInterface::kRtcpMuxPolicyRequire
> /* rtcp */,
> On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> > This means that if we receive a remote description with no "a=rtcp-mux" and
> our
> > policy is "require", RTCP will silently fail to work as opposed to us
> rejecting
> > the media description (which JSEP says we should do). Could you add a TODO
> > comment to reject media descriptions without "a=rtcp-mux" when in "require"
> > mode?
> > 
> > Oh, and the "bool rtcp" parameter tells whether RTCP is enabled *at all*. So
> > this would turn off RTCP for anyone not using "require mux". I think there
> need
> > to be two separate parameters: "RTCP enabled" and "RTCP mux required".
> 
> The "rtcp" parameter here ultimately gets passed into the BaseChannel
parameters
> "rtcp_tranpsort_enabled", so it might make sense to call it that here as well
> and everywhere from here to there (instead of calling it "rtcp").  That would
> make it more clear what this value means.  
> 
> And having rtcp_transport_enabled = (rtcp_mux_policy_ ==
> PeerConnectionInterface::kRtcpMuxPolicyNegotiate), I believe, works.
> 
> You're right that we should reject the remote description if a=rtcp-mux is not
> in it and rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire. 
> We should fix that in another CL.  I don't think it impacts this CL.

[skvlad, 2016-05-11 22:34:25]

On 2016/05/11 22:25:47, pthatcher1 wrote:
> Sorry, I should have been more clear.  Long-term (as we become more standards
> compliant), we should reject m-lines (with port 0) that are not bundled. 
> Short-term (while we reject the whole description instead), we should verify
> that all m-lines are in the bundle and reject if they aren't.

So do you want to do it as part of this CL?

[Taylor Brandstetter, 2016-05-11 22:35:55]

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
PeerConnectionInterface::kBundlePolicyMaxBundle) {
On 2016/05/11 22:23:47, pthatcher1 wrote:
> On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> > On 2016/05/11 06:20:39, pthatcher1 wrote:
> > > If the policy is max bundle, then all the channels must be in the bundle
and
> > > that bundle must have a content name.  So you could probably just use
> > > bundle->FirstContentName()  in all of the calls below and you won't need
> > > GetTranpsortForChannel.
> > 
> > JSEP does say the application can't munge BUNDLE groups or "a=bundle-only"
> (once
> > we implement that). But these channels could be created from a *remote*
> > description, and a remote peer doesn't have the same restrictions as a JSEP
> > implementation (it could decide to use bundle-only on a subset of media
> > descriptions). So I think this should be left as-is. Unless we start
rejecting
> > non-BUNDLE media descriptions (working group hasn't decided on that yet).
> 
> If we are in max-bundle mode, we already reject descriptions that don't have a
> bundle group.  But you're right that we don't verify that all the m-lines are
in
> the max-bundle.  But I think we should reject descriptions that don't have
> everything bundled when in max-bundle mode, unless the standards body decides
> otherwise (and I'll argue in the group that max-bundle should mean we reject
> non-bundled m-lines).

I'm of the opposite opinion... I think we should do what the standard says until
the standard is changed. Here's a PR where you can argue, though:
https://github.com/rtcweb-wg/jsep/pull/279

[pthatcher1, 2016-05-12 03:47:50]

On 2016/05/11 22:35:55, Taylor Brandstetter wrote:
> https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc
> File webrtc/api/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1972493002/diff/1/webrtc/api/webrtcsession.cc#n...
> webrtc/api/webrtcsession.cc:1755: if (bundle_policy_ ==
> PeerConnectionInterface::kBundlePolicyMaxBundle) {
> On 2016/05/11 22:23:47, pthatcher1 wrote:
> > On 2016/05/11 15:32:23, Taylor Brandstetter wrote:
> > > On 2016/05/11 06:20:39, pthatcher1 wrote:
> > > > If the policy is max bundle, then all the channels must be in the bundle
> and
> > > > that bundle must have a content name.  So you could probably just use
> > > > bundle->FirstContentName()  in all of the calls below and you won't need
> > > > GetTranpsortForChannel.
> > > 
> > > JSEP does say the application can't munge BUNDLE groups or "a=bundle-only"
> > (once
> > > we implement that). But these channels could be created from a *remote*
> > > description, and a remote peer doesn't have the same restrictions as a
JSEP
> > > implementation (it could decide to use bundle-only on a subset of media
> > > descriptions). So I think this should be left as-is. Unless we start
> rejecting
> > > non-BUNDLE media descriptions (working group hasn't decided on that yet).
> > 
> > If we are in max-bundle mode, we already reject descriptions that don't have
a
> > bundle group.  But you're right that we don't verify that all the m-lines
are
> in
> > the max-bundle.  But I think we should reject descriptions that don't have
> > everything bundled when in max-bundle mode, unless the standards body
decides
> > otherwise (and I'll argue in the group that max-bundle should mean we reject
> > non-bundled m-lines).
> 
> I'm of the opposite opinion... I think we should do what the standard says
until
> the standard is changed. Here's a PR where you can argue, though:
> https://github.com/rtcweb-wg/jsep/pull/279

Why don't we just keep the existing behavior with this CL (reject if not BUNDLE,
but don't verify they are all bundled) and change it to match the spec later, at
which point the standard should be less ambiguous?

[pthatcher1, 2016-05-12 03:50:24]

On 2016/05/11 22:34:25, skvlad wrote:
> On 2016/05/11 22:25:47, pthatcher1 wrote:
> > Sorry, I should have been more clear.  Long-term (as we become more
standards
> > compliant), we should reject m-lines (with port 0) that are not bundled. 
> > Short-term (while we reject the whole description instead), we should verify
> > that all m-lines are in the bundle and reject if they aren't.
> 
> So do you want to do it as part of this CL?

No, we can leave it out of this CL.

[pthatcher1, 2016-05-12 03:56:52]

I'm happy with this as long as you make two minor readability/comment
improvements.

https://codereview.webrtc.org/1972493002/diff/40001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1972493002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1794: rtcp_mux_policy_ ==
PeerConnectionInterface::kRtcpMuxPolicyRequire;
This might be a little more clear as:

bool require_rtcp_mux = (rtcp_mux_policy_ ==
PeerConnectionInterface::kRtcpMuxPolicyRequire);
bool create_rtcp_transport_channel = !require_rtcp_mux;
voice_channel_.reset(channel_manager_->CreateVoiceChannel(...,
create_rtcp_transport_channel)
...
if (require_rtcp_mux) {
  voice_channel_->ActivateRtcpMux();
}

https://codereview.webrtc.org/1972493002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1963: // remote description enables RTCP mux.
Or, rather, reject any m= sections that do not have rtcp-mux enabled.

[skvlad, 2016-05-13 19:17:51]

https://codereview.webrtc.org/1972493002/diff/40001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1972493002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1794: rtcp_mux_policy_ ==
PeerConnectionInterface::kRtcpMuxPolicyRequire;
On 2016/05/12 03:56:52, pthatcher1 wrote:
> This might be a little more clear as:
> 
> bool require_rtcp_mux = (rtcp_mux_policy_ ==
> PeerConnectionInterface::kRtcpMuxPolicyRequire);
> bool create_rtcp_transport_channel = !require_rtcp_mux;
> voice_channel_.reset(channel_manager_->CreateVoiceChannel(...,
> create_rtcp_transport_channel)
> ...
> if (require_rtcp_mux) {
>   voice_channel_->ActivateRtcpMux();
> }

Done.

https://codereview.webrtc.org/1972493002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1963: // remote description enables RTCP mux.
On 2016/05/12 03:56:52, pthatcher1 wrote:
> Or, rather, reject any m= sections that do not have rtcp-mux enabled.

Done.

[skvlad, 2016-05-16 18:43:32]

Ping. Please take a look, the bug is targeted for Chrome M52 which branches out
soon.

[pthatcher1, 2016-05-16 19:59:06]

lgtm

[Taylor Brandstetter, 2016-05-16 19:59:48]

lgtm

https://codereview.webrtc.org/1972493002/diff/60001/webrtc/api/webrtcsession_...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1972493002/diff/60001/webrtc/api/webrtcsession_...
webrtc/api/webrtcsession_unittest.cc:3291: offer->ToString(&sdp);
nit: I know the old tests don't do this, but it would be better to just make a
copy of the session description rather than converting it to SDP and back. No
need to change this CL, but in the future that's what we should do.

[skvlad, 2016-05-16 20:01:25]

The CQ bit was checked by skvlad@webrtc.org

[commit-bot: I haz the power, 2016-05-16 20:01:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972493002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972493002/60001

[commit-bot: I haz the power, 2016-05-16 20:02:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-16 20:02:21]

Try jobs failed on following builders:
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15045)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/13696)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3192)

[skvlad, 2016-05-17 20:05:47]

The CQ bit was checked by skvlad@webrtc.org

[skvlad, 2016-05-17 20:05:48]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1972493002/#ps80001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-05-17 20:05:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972493002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972493002/80001

[commit-bot: I haz the power, 2016-05-17 22:06:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-17 22:06:17]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[skvlad, 2016-05-17 22:24:59]

The CQ bit was checked by skvlad@webrtc.org

[commit-bot: I haz the power, 2016-05-17 22:25:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972493002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972493002/80001

[commit-bot: I haz the power, 2016-05-18 00:49:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-18 00:50:07]

Description was changed from

==========
Do not create a temporary transport channel when using max-bundle

With this change, when max-bundle and rtcp-mux are both enabled, we no
longer create and destroy a temporary transport channel when a media
channel gets added. Instead, the media channel uses the correct bundled
transport channel from the start.

This fixes a bug where adding a media type would cause the ICE state to
briefly become Disconnected and then immediately recover. The temporary
channel was created in a non-writable state, which caused the
TransportController to declare the ICE state to be Disconnected (as not
all transport channels were writable). Right after creation, the
temporary channel was then destroyed and the ICE state went back to the
correct one.

BUG=webrtc:5856
==========

to

==========
Do not create a temporary transport channel when using max-bundle

With this change, when max-bundle and rtcp-mux are both enabled, we no
longer create and destroy a temporary transport channel when a media
channel gets added. Instead, the media channel uses the correct bundled
transport channel from the start.

This fixes a bug where adding a media type would cause the ICE state to
briefly become Disconnected and then immediately recover. The temporary
channel was created in a non-writable state, which caused the
TransportController to declare the ICE state to be Disconnected (as not
all transport channels were writable). Right after creation, the
temporary channel was then destroyed and the ICE state went back to the
correct one.

BUG=webrtc:5856

Committed: https://crrev.com/6c87a67b63cee30e007a64b8f1de0aede93ea0da
Cr-Commit-Position: refs/heads/master@{#12781}
==========

[commit-bot: I haz the power, 2016-05-18 00:50:08]

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