Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Where "TRANSPORT attributes" refers to:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-mux-attributes-16

The BUNDLE draft now says that these attributes can
(in fact, MUST) be omitted when m= sections are bundled
(they only need to go in one of the bundled m= sections),
so we should start accepting that SDP.

This CL doesn't fix "a=rtcp-mux", unfortunately. That will be easier
to fix once we've split apart an "RtpTransport" object from
BaseChannel.

BUG=webrtc:6351
Review-Url: https://codereview.webrtc.org/2647593003
Cr-Commit-Position: refs/heads/master@{#16782}
Committed: https://chromium.googlesource.com/external/webrtc/+/b7892536616aae4582ac6edb76bac14a00ea7937

[Taylor Brandstetter, 2017-01-19 03:42:34]

Description was changed from

==========
Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Where "TRANSPORT attributes" refers to:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-mux-attributes-16

The BUNDLE draft now says that these attributes can be ommitted when m=
sections are bundled (they only need to go in one of the bundled m=
sections), so we should start accepting that SDP.

BUG=webrtc:6351
==========

to

==========
Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Where "TRANSPORT attributes" refers to:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-mux-attributes-16

The BUNDLE draft now says that these attributes can (in fact, MUST) be ommitted
when m=
sections are bundled (they only need to go in one of the bundled m=
sections), so we should start accepting that SDP.

BUG=webrtc:6351
==========

[Taylor Brandstetter, 2017-01-19 03:43:17]

Description was changed from

==========
Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Where "TRANSPORT attributes" refers to:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-mux-attributes-16

The BUNDLE draft now says that these attributes can (in fact, MUST) be ommitted
when m=
sections are bundled (they only need to go in one of the bundled m=
sections), so we should start accepting that SDP.

BUG=webrtc:6351
==========

to

==========
Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Where "TRANSPORT attributes" refers to:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-mux-attributes-16

The BUNDLE draft now says that these attributes can
(in fact, MUST) be omitted when m= sections are bundled
(they only need to go in one of the bundled m= sections),
so we should start accepting that SDP.

BUG=webrtc:6351
==========

[Taylor Brandstetter, 2017-01-20 22:25:20]

Description was changed from

==========
Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Where "TRANSPORT attributes" refers to:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-mux-attributes-16

The BUNDLE draft now says that these attributes can
(in fact, MUST) be omitted when m= sections are bundled
(they only need to go in one of the bundled m= sections),
so we should start accepting that SDP.

BUG=webrtc:6351
==========

to

==========
Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Where "TRANSPORT attributes" refers to:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-mux-attributes-16

The BUNDLE draft now says that these attributes can
(in fact, MUST) be omitted when m= sections are bundled
(they only need to go in one of the bundled m= sections),
so we should start accepting that SDP.

This CL doesn't fix "a=rtcp-mux", unfortunately. That will be easier
to fix once we've split apart an "RtpTransport" object from
BaseChannel.

BUG=webrtc:6351
==========

[Taylor Brandstetter, 2017-01-21 05:46:17]

I realize the mediasession.cc code is ugly. I have a CL in the works that does a
lot of refactoring of it, so I'm trying to do the simplest thing here so that
merging will be less painful.

[Taylor Brandstetter, 2017-02-16 01:57:05]

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

[pthatcher1, 2017-02-17 19:17:33]

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:529: make_real_max_bundle_offer_ = real;
A better name might be:

modify_remote_offers_to_spec_compliant_bundle_

and 

ModifyRemoteOffersToSpecCompliantBundle

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1906: TEST_F(P2PTestConductor,
LocalP2PTestWithRealMaxBundleOffer) {
A better test name might be IncomingOfferWithSpecCompliantBundle

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:168: // BUNDLE-tag section needs a ufrag and pwd.
Mismatches, such as replying with
I translation from spec speak to understandable text would be nice, perhaps
something like:

BUNDLE-tag section (the first media section of the BUNDLE group)

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:184: continue;
A comment here like "It's not the first, so it spec-compliant sections may not
have any attributes".

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:220: // ufrag and pwd.
Like the comment above, please translate "BUNDLE-tag section" (that might not be
the worse name I've seen from a spec, but it's up there).

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescriptionfactory.h (right):

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.h:57: // for an m= section that's
being bundled into another m= section, in
might be more clear as 

"|bundled| indicates that this transport description is in an m= section that's
in a bundle group and isn't the first m= section in the bundle group, so it
doesn't need transport-level attributes".

Better yet, why not just call the variable |include_transport_level_attributes|
or some shorter version of that?  Then you don't need the comment.

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.h:64: bool bundled) const;
It ought to be the 3rd argument, not the last.  And maybe it should be part of
TransportOptions.

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1499: new
TransportInfo(*answer->GetTransportInfoByName(added.name)));
Why do you need to set the bundle_transport here?  Why not do it after the for
loop, like so?

if (answer_bundle.FirstContentName()) {
  bundle_transport.reset(new
TransportInfo(*answer->GetTransportInfoByName(answer_bundle.FirstContentName))
}

That would put less code into the for loop, which would, I think, make it more
clear.  And you  might not need the std::unique_ptr.

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1501: }
Might be more clear with one if statement:

ContentInfo& added = answer->contents().back();
if (!added.rejected && options.bundled_enabled && 
    offer_bundle && offer_bundle->HasContentName(added.name)) {
  answer_bundle.AddContentName(added.name);
  bundle_transport.reset(new
TransportInfo(*answer->GetTransportInfoByName(added.name));
}

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1502: }
If the offer has this:
a=bundle b a
m=
a=mid:a
m=
a=mid:b


Do we response with this?
a=bundle a b
m=
a=mid:a
m=
a=mid:b


It looks like we already do that, but it seems wrong to me now.  Is it wrong?

[Taylor Brandstetter, 2017-02-17 21:49:15]

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:529: make_real_max_bundle_offer_ = real;
On 2017/02/17 19:17:32, pthatcher1 wrote:
> A better name might be:
> 
> modify_remote_offers_to_spec_compliant_bundle_
> 
> and 
> 
> ModifyRemoteOffersToSpecCompliantBundle

Replaced "real" with "spec compliant".

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:168: // BUNDLE-tag section needs a ufrag and pwd.
Mismatches, such as replying with
On 2017/02/17 19:17:32, pthatcher1 wrote:
> I translation from spec speak to understandable text would be nice, perhaps
> something like:
> 
> BUNDLE-tag section (the first media section of the BUNDLE group)

Done.

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:184: continue;
On 2017/02/17 19:17:32, pthatcher1 wrote:
> A comment here like "It's not the first, so it spec-compliant sections may not
> have any attributes".

Done.

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:220: // ufrag and pwd.
On 2017/02/17 19:17:32, pthatcher1 wrote:
> Like the comment above, please translate "BUNDLE-tag section" (that might not
be
> the worse name I've seen from a spec, but it's up there).

Done.

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescriptionfactory.h (right):

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.h:57: // for an m= section that's
being bundled into another m= section, in
On 2017/02/17 19:17:32, pthatcher1 wrote:
> might be more clear as 
> 
> "|bundled| indicates that this transport description is in an m= section
that's
> in a bundle group and isn't the first m= section in the bundle group, so it
> doesn't need transport-level attributes".
> 
> Better yet, why not just call the variable
|include_transport_level_attributes|
> or some shorter version of that?  Then you don't need the comment.

Done.

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.h:64: bool bundled) const;
On 2017/02/17 19:17:32, pthatcher1 wrote:
> It ought to be the 3rd argument, not the last.  And maybe it should be part of
> TransportOptions.

Done (though not the TransportOptions part).

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1499: new
TransportInfo(*answer->GetTransportInfoByName(added.name)));
On 2017/02/17 19:17:32, pthatcher1 wrote:
> Why do you need to set the bundle_transport here?  Why not do it after the for
> loop?

Because it's needed inside the loop, for AddXContentForAnswer.

> And you  might not need the std::unique_ptr.

What's the alternative? I think it's most clean with unique_ptr.

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1501: }
On 2017/02/17 19:17:32, pthatcher1 wrote:
> Might be more clear with one if statement:
> 
> ContentInfo& added = answer->contents().back();
> if (!added.rejected && options.bundled_enabled && 
>     offer_bundle && offer_bundle->HasContentName(added.name)) {
>   answer_bundle.AddContentName(added.name);
>   bundle_transport.reset(new
> TransportInfo(*answer->GetTransportInfoByName(added.name));
> }

Done.

https://codereview.webrtc.org/2647593003/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1502: }
On 2017/02/17 19:17:32, pthatcher1 wrote:
> If the offer has this:
> a=bundle b a
> m=
> a=mid:a
> m=
> a=mid:b
> 
> 
> Do we response with this?
> a=bundle a b
> m=
> a=mid:a
> m=
> a=mid:b
> 
> 
> It looks like we already do that, but it seems wrong to me now.  Is it wrong?

Yes we already do that. Yes, it's technically wrong, though I didn't intend to
change that in this CL; this CL is just about parsing spec-compliant SDP, not
generating it.

[pthatcher1, 2017-02-22 18:42:45]

lgtm

[Taylor Brandstetter, 2017-02-22 23:52:39]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-02-22 23:52:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-22 23:54:18]

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

[commit-bot: I haz the power, 2017-02-22 23:54:19]

Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23089)

[Taylor Brandstetter, 2017-02-23 01:53:28]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-02-23 01:53:28]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2647593003/#ps80001 (title:
"Merge with master")

[commit-bot: I haz the power, 2017-02-23 01:53:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-23 02:06:13]

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

[commit-bot: I haz the power, 2017-02-23 02:06:14]

Try jobs failed on following builders:
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/22367)

[Taylor Brandstetter, 2017-02-23 02:20:51]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-02-23 02:20:52]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2647593003/#ps100001 (title:
"Fixing tests due to bool's meaning being reversed in patchset 4")

[commit-bot: I haz the power, 2017-02-23 02:20:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-23 03:35:19]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487816451844810,
"parent_rev": "3fa87f782e0a11f1ce24e56a61f0b926e99670dc", "commit_rev":
"b7892536616aae4582ac6edb76bac14a00ea7937"}

[commit-bot: I haz the power, 2017-02-23 03:35:24]

Description was changed from

==========
Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Where "TRANSPORT attributes" refers to:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-mux-attributes-16

The BUNDLE draft now says that these attributes can
(in fact, MUST) be omitted when m= sections are bundled
(they only need to go in one of the bundled m= sections),
so we should start accepting that SDP.

This CL doesn't fix "a=rtcp-mux", unfortunately. That will be easier
to fix once we've split apart an "RtpTransport" object from
BaseChannel.

BUG=webrtc:6351
==========

to

==========
Accept SDP with TRANSPORT attributes missing from bundled m= sections.

Where "TRANSPORT attributes" refers to:
https://tools.ietf.org/html/draft-ietf-mmusic-sdp-mux-attributes-16

The BUNDLE draft now says that these attributes can
(in fact, MUST) be omitted when m= sections are bundled
(they only need to go in one of the bundled m= sections),
so we should start accepting that SDP.

This CL doesn't fix "a=rtcp-mux", unfortunately. That will be easier
to fix once we've split apart an "RtpTransport" object from
BaseChannel.

BUG=webrtc:6351

Review-Url: https://codereview.webrtc.org/2647593003
Cr-Commit-Position: refs/heads/master@{#16782}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b7892536616aae4582ac6edb7...
==========

[commit-bot: I haz the power, 2017-02-23 03:35:25]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/b7892536616aae4582ac6edb7...