Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 1688383002: Implementing unified plan encoding of msid. (Closed)

Created:
1 year, 9 months ago by Taylor Brandstetter
Modified:
1 year, 9 months ago
Reviewers:
pthatcher1, brucedawson
CC:
webrtc-reviews_webrtc.org
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Fixing unit test and making it pass. Also, restoring generation of "a=ssrc:X msid:Y" for backwards … #

Patch Set 3 : Fixing a comment. #

Total comments: 17

Patch Set 4 : Addressing pthatcher@'s comments. #

Patch Set 5 : Merging with master. #

Total comments: 3

Messages

Total messages: 15 (6 generated)
Taylor Brandstetter
PTAL. You'll notice that when the "unified_plan_sdp" flag is set, I'm encoding the MSID in ...
1 year, 9 months ago (2016-02-12 02:20:10 UTC) #3
pthatcher1
https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepicecandidate.h File talk/app/webrtc/jsepicecandidate.h (right): https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepicecandidate.h#newcode75 talk/app/webrtc/jsepicecandidate.h:75: : candidates_(std::move(o.candidates_)) {} On 2016/02/12 02:20:10, Taylor Brandstetter wrote: ...
1 year, 9 months ago (2016-02-12 03:54:35 UTC) #4
Taylor Brandstetter
https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepicecandidate.h File talk/app/webrtc/jsepicecandidate.h (right): https://codereview.webrtc.org/1688383002/diff/40001/talk/app/webrtc/jsepicecandidate.h#newcode75 talk/app/webrtc/jsepicecandidate.h:75: : candidates_(std::move(o.candidates_)) {} On 2016/02/12 03:54:34, pthatcher1 wrote: > ...
1 year, 9 months ago (2016-02-12 20:44:57 UTC) #5
pthatcher1
lgtm
1 year, 9 months ago (2016-02-12 22:19:42 UTC) #6
commit-bot: I haz the power
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
1 year, 9 months ago (2016-02-17 01:33:57 UTC) #8
commit-bot: I haz the power
Committed patchset #5 (id:80001)
1 year, 9 months ago (2016-02-17 01:54:15 UTC) #10
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/9d3584c47ef1c73475dfa88e4fef802e3314815b Cr-Commit-Position: refs/heads/master@{#11644}
1 year, 9 months ago (2016-02-17 01:54:20 UTC) #12
brucedawson
It's not a bug, but the /analyze builder pointed out some new variable shadowing which ...
1 year, 9 months ago (2016-02-19 19:23:39 UTC) #14
Taylor Brandstetter
1 year, 9 months ago (2016-02-19 21:15:59 UTC) #15
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld efc10ee0f