Implementing unified plan encoding of msid.

Implementing unified plan encoding of msid.

Meaning "a=msid:...", instead of "a=ssrc:X msid:...".
An additional option to SdpSerialize determines if the
"a=msid" attribute is used.

Committed: https://crrev.com/9d3584c47ef1c73475dfa88e4fef802e3314815b
Cr-Commit-Position: refs/heads/master@{#11644}

[Taylor Brandstetter, 2016-02-12 02:10:48]

Description was changed from

==========
Implementing unified plan encoding of msid.

Meaning "a=msid:...", instead of "a=ssrc:X msid:...".
An additional option to SdpSerialize determines which encoding is used.
==========

to

==========
Implementing unified plan encoding of msid.

Meaning "a=msid:...", instead of "a=ssrc:X msid:...".
An additional option to SdpSerialize determines if the
"a=msid" attribute is used.
==========

[Taylor Brandstetter, 2016-02-12 02:20:10]

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

[Taylor Brandstetter, 2016-02-12 02:20:10]

PTAL. You'll notice that when the "unified_plan_sdp" flag is set, I'm encoding
the MSID in both the new and old ways. My thought is that this will make the
transition to unified plan smoother. When we finally remove support for plan B
altogether, we can stop encoding the MSID the old way.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepiceca...
File talk/app/webrtc/jsepicecandidate.h (right):

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepiceca...
talk/app/webrtc/jsepicecandidate.h:75: : candidates_(std::move(o.candidates_))
{}
This is needed because JsepSessionDescription::Initialize resizes a vector of
JsepCandidateCollections, so if the size changes, they need to get moved, or
else the candidates will get double-deleted.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp.cc
File talk/app/webrtc/webrtcsdp.cc (right):

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:1415: << "one track in a media section. Omitting
'a=msid'.";
You may ask "why not throw an exception/return a warning here?", but this could
happen if you theoretically deserialize a Plan B description, then try to
serialize it as unified plan.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
File talk/app/webrtc/webrtcsdp_unittest.cc (left):

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp_unittest.cc:175: "a=ssrc:4 label:audio_track_id_2\r\n"
I removed the extra tracks from the base reference SDP, so that it's only 1
video track and 1 audio track. This means the tests for things unrelated to the
number of tracks (like ICE stuff, content direction fingerprint/crypto, etc.)
can remain unaffected.

There are now additional tests (at the bottom of this file) that specifically
test serializing and deserializing plan B and unified plan descriptions.

[pthatcher1, 2016-02-12 03:54:35]

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepiceca...
File talk/app/webrtc/jsepicecandidate.h (right):

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepiceca...
talk/app/webrtc/jsepicecandidate.h:75: : candidates_(std::move(o.candidates_))
{}
On 2016/02/12 02:20:10, Taylor Brandstetter wrote:
> This is needed because JsepSessionDescription::Initialize resizes a vector of
> JsepCandidateCollections, so if the size changes, they need to get moved, or
> else the candidates will get double-deleted.

Can you make that a comment?

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp.cc
File talk/app/webrtc/webrtcsdp.cc (right):

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:1415: << "one track in a media section. Omitting
'a=msid'.";
On 2016/02/12 02:20:10, Taylor Brandstetter wrote:
> You may ask "why not throw an exception/return a warning here?", but this
could
> happen if you theoretically deserialize a Plan B description, then try to
> serialize it as unified plan.

Should we at least serialize the first media description, then?

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:1418: std::string appdata = track->id;
1.  Why is this "appdata" instead of track_id?
2.  Why are you copying it?  Why not just use track->id below?
3.  Why not do "const std::string& stream_id = track->sync_label" to make it
more readable?

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:2074: // msid-appdata = 1*64token-char  ; see RFC
4566
The parsing code and the serializing code don't seem to have the same comment. 
I think the other one was more clear.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:2692: ssrc_info.msid_appdata = track_id;
Can we rename ssrc_info.msid_identifier and .msid_appdata to .stream_id and
.track_id?  It seems like it would make a lot more sense.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:2693: }
Do we need to merge all of the ssrc_infos into one big ssrc_info or does
CreateTracksFromSsrcInfos take care of that?  If it does, can you put a comment
here explaining so?

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp.h
File talk/app/webrtc/webrtcsdp.h (right):

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.h:56: bool unified_plan_sdp = false);
Rather than have a default variable, can we make it explicit?  

In fact, it might even be nice to just have two functions:

SdpSerliazePlanB
SdpSerliazeUnifiedPlan

[Taylor Brandstetter, 2016-02-12 20:44:57]

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepiceca...
File talk/app/webrtc/jsepicecandidate.h (right):

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepiceca...
talk/app/webrtc/jsepicecandidate.h:75: : candidates_(std::move(o.candidates_))
{}
On 2016/02/12 03:54:34, pthatcher1 wrote:
> On 2016/02/12 02:20:10, Taylor Brandstetter wrote:
> > This is needed because JsepSessionDescription::Initialize resizes a vector
of
> > JsepCandidateCollections, so if the size changes, they need to get moved, or
> > else the candidates will get double-deleted.
> 
> Can you make that a comment?

Done.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp.cc
File talk/app/webrtc/webrtcsdp.cc (right):

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:1415: << "one track in a media section. Omitting
'a=msid'.";
On 2016/02/12 03:54:35, pthatcher1 wrote:
> On 2016/02/12 02:20:10, Taylor Brandstetter wrote:
> > You may ask "why not throw an exception/return a warning here?", but this
> could
> > happen if you theoretically deserialize a Plan B description, then try to
> > serialize it as unified plan.
> 
> Should we at least serialize the first media description, then?

We do.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:1418: std::string appdata = track->id;
On 2016/02/12 03:54:34, pthatcher1 wrote:
> 1.  Why is this "appdata" instead of track_id?
> 2.  Why are you copying it?  Why not just use track->id below?
> 3.  Why not do "const std::string& stream_id = track->sync_label" to make it
> more readable?

1. That's what the code below does for the SSRC line. I assumed there was a
reason.
2. Same as above.
3. Same as above.

I cleaned it up in both places.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:2074: // msid-appdata = 1*64token-char  ; see RFC
4566
On 2016/02/12 03:54:34, pthatcher1 wrote:
> The parsing code and the serializing code don't seem to have the same comment.

> I think the other one was more clear.

Again, was just copying the way it was done elsewhere. I included the other
comment, but kept this one, since it's how the attribute is actually defined in
the spec.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:2692: ssrc_info.msid_appdata = track_id;
On 2016/02/12 03:54:34, pthatcher1 wrote:
> Can we rename ssrc_info.msid_identifier and .msid_appdata to .stream_id and
> .track_id?  It seems like it would make a lot more sense.

Done.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.cc:2693: }
On 2016/02/12 03:54:35, pthatcher1 wrote:
> Do we need to merge all of the ssrc_infos into one big ssrc_info or does
> CreateTracksFromSsrcInfos take care of that?  If it does, can you put a
comment
> here explaining so?

Yes, CreateTracksFromSsrcInfos does that. I meant for the comment above to
convey that, but I made it more clear.

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp.h
File talk/app/webrtc/webrtcsdp.h (right):

https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/webrtcsdp...
talk/app/webrtc/webrtcsdp.h:56: bool unified_plan_sdp = false);
On 2016/02/12 03:54:35, pthatcher1 wrote:
> Rather than have a default variable, can we make it explicit?  
> 
> In fact, it might even be nice to just have two functions:
> 
> SdpSerliazePlanB
> SdpSerliazeUnifiedPlan

Done.

[pthatcher1, 2016-02-12 22:19:42]

lgtm

[Taylor Brandstetter, 2016-02-17 01:32:31]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-02-17 01:33:57]

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

[commit-bot: I haz the power, 2016-02-17 01:54:14]

Description was changed from

==========
Implementing unified plan encoding of msid.

Meaning "a=msid:...", instead of "a=ssrc:X msid:...".
An additional option to SdpSerialize determines if the
"a=msid" attribute is used.
==========

to

==========
Implementing unified plan encoding of msid.

Meaning "a=msid:...", instead of "a=ssrc:X msid:...".
An additional option to SdpSerialize determines if the
"a=msid" attribute is used.
==========

[commit-bot: I haz the power, 2016-02-17 01:54:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-17 01:54:20]

Description was changed from

==========
Implementing unified plan encoding of msid.

Meaning "a=msid:...", instead of "a=ssrc:X msid:...".
An additional option to SdpSerialize determines if the
"a=msid" attribute is used.
==========

to

==========
Implementing unified plan encoding of msid.

Meaning "a=msid:...", instead of "a=ssrc:X msid:...".
An additional option to SdpSerialize determines if the
"a=msid" attribute is used.

Committed: https://crrev.com/9d3584c47ef1c73475dfa88e4fef802e3314815b
Cr-Commit-Position: refs/heads/master@{#11644}
==========

[commit-bot: I haz the power, 2016-02-17 01:54:20]

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

[brucedawson, 2016-02-19 19:23:38]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2016-02-19 19:23:39]

It's not a bug, but the /analyze builder pointed out some new variable shadowing
which then points out that the 'os' variable is being declared in nested scopes.

Variable shadowing can be mildly confusing, and in this case it also causes some
modest extra overhead for more constructors/destructors. I believe that the
three internal 'os' declarations can be deleted.

FWIW.

https://codereview.webrtc.org/1688383002/diff/80001/webrtc/api/webrtcsdp.cc
File webrtc/api/webrtcsdp.cc (right):

https://codereview.webrtc.org/1688383002/diff/80001/webrtc/api/webrtcsdp.cc#n...
webrtc/api/webrtcsdp.cc:1398: std::ostringstream os;
Unnecessarily duplicated variable, also variable shadowing.

https://codereview.webrtc.org/1688383002/diff/80001/webrtc/api/webrtcsdp.cc#n...
webrtc/api/webrtcsdp.cc:1453: std::ostringstream os;
Ditto (but not from this CL).

https://codereview.webrtc.org/1688383002/diff/80001/webrtc/api/webrtcsdp.cc#n...
webrtc/api/webrtcsdp.cc:1476: std::ostringstream os;
Ditto (but not from this CL).

[Taylor Brandstetter, 2016-02-19 21:15:59]

On 2016/02/19 19:23:39, brucedawson wrote:
> It's not a bug, but the /analyze builder pointed out some new variable
shadowing
> which then points out that the 'os' variable is being declared in nested
scopes.
> 
> Variable shadowing can be mildly confusing, and in this case it also causes
some
> modest extra overhead for more constructors/destructors. I believe that the
> three internal 'os' declarations can be deleted.
> 
> FWIW.
> 
> https://codereview.webrtc.org/1688383002/diff/80001/webrtc/api/webrtcsdp.cc
> File webrtc/api/webrtcsdp.cc (right):
> 
>
https://codereview.webrtc.org/1688383002/diff/80001/webrtc/api/webrtcsdp.cc#n...
> webrtc/api/webrtcsdp.cc:1398: std::ostringstream os;
> Unnecessarily duplicated variable, also variable shadowing.
> 
>
https://codereview.webrtc.org/1688383002/diff/80001/webrtc/api/webrtcsdp.cc#n...
> webrtc/api/webrtcsdp.cc:1453: std::ostringstream os;
> Ditto (but not from this CL).
> 
>
https://codereview.webrtc.org/1688383002/diff/80001/webrtc/api/webrtcsdp.cc#n...
> webrtc/api/webrtcsdp.cc:1476: std::ostringstream os;
> Ditto (but not from this CL).

Thanks for pointing that out, I'll make a CL to fix it.