Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2742903002:
Parse the connection data in SDP (c= line). (Closed)
|
Created:
3 years, 9 months ago by Zhi Huang Modified:
3 years, 9 months ago Reviewers:
Taylor Brandstetter CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionParse the connection data in SDP (c= line).
Extract the remote addresses from SDP c= line on both session level and
media level. The media level address will overwrite the session level one if
exists.
WebRTC is not using c= and this is used for new SDP parsing API.
BUG=webrtc:7311
Review-Url: https://codereview.webrtc.org/2742903002
Cr-Commit-Position: refs/heads/master@{#17326}
Committed: https://chromium.googlesource.com/external/webrtc/+/38989e593c994b6865f2b2cc496abf59d5925eb8
Patch Set 1 : Parse the c= line. #
Total comments: 21
Patch Set 2 : Remove the session level addr from SessionDescription. Modified the unit tests. #
Total comments: 7
Patch Set 3 : Move the GetDefaultDestination logic to JsepSessionDescription. #
Total comments: 24
Patch Set 4 : Refactoring the tests. Resolve the comments. #
Total comments: 20
Patch Set 5 : Make the unit tests cleaner. #
Total comments: 8
Patch Set 6 : Add a test for candidate removal. Solve the comments. #
Total comments: 1
Patch Set 7 : Monior fix according to the style guide. #
Messages
Total messages: 40 (27 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
https://codereview.webrtc.org/2742903002/diff/40001/webrtc/p2p/base/sessionde... File webrtc/p2p/base/sessiondescription.h (right): https://codereview.webrtc.org/2742903002/diff/40001/webrtc/p2p/base/sessionde... webrtc/p2p/base/sessiondescription.h:185: void set_session_connection_addr( I think it would be simpler to only store this address in MediaContentDescription. If a "c=" is found at the session level, its value can just be copied to the media level, since the meaning is the same. This is what currently happens with ice-ufrag at the session level (for example): https://cs.chromium.org/chromium/src/third_party/webrtc/pc/webrtcsdp.cc?type=... https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/mediasession.h File webrtc/pc/mediasession.h (right): https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/mediasession.h#... webrtc/pc/mediasession.h:316: rtc::Optional<rtc::SocketAddress> media_connection_addr() { This doesn't need to be an Optional since a value should always exist. But even if no value is set, this can be detected using SocketAddress::IsNil. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/mediasession.h#... webrtc/pc/mediasession.h:335: // https://tools.ietf.org/html/rfc4566#page-21 nit: Can link directly to the relevant section: https://tools.ietf.org/html/rfc4566#section-5.7 https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc File webrtc/pc/webrtcsdp.cc (right): https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1903: bool ParseConnectionData(const std::string line, rtc::SocketAddress* addr) { Meant to be const ref string? https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1909: // Skip "c=<nettype>". We should verify that "nettype" is "IN" since that's all we support. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1913: // Skip the "<addrtype>". To be more thorough, we could verify that addrtype matches the type of the parsed address (IP4 or IP6), if the address is a literal IP address and not a domain name. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1920: LOG(LS_ERROR) << "Failed to parse the connection data. Multicast is not " This method should take an error output parameter and do: return ParseFailed( line, "Failed to parse the connection data. Multicast is not currently supported.", error); In order to return errors. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1993: LOG(LS_ERROR) << "Failed to parse the session level connection data:" This doesn't need to log an error if ParseConnectionData already did. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp_unitt... File webrtc/pc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp_unitt... webrtc/pc/webrtcsdp_unittest.cc:871: audio_desc_ = CreateAudioContentDescription(); To be consistent with how most things in this file are tested, you could set the connection_address for the audio/video descriptions here... https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp_unitt... webrtc/pc/webrtcsdp_unittest.cc:1154: const MCD* cd2) { ... and add code to compare them in this method. Though you would still need additional tests for "c=" at the session level, and for IPv6. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp_unitt... webrtc/pc/webrtcsdp_unittest.cc:3557: // Test that the invalid connection data cannot be parsed. I'd use the term "unsupported" rather than "invalid", since multicast addresses are valid from an SDP perspective.
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Please take another look. Thanks. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/p2p/base/sessionde... File webrtc/p2p/base/sessiondescription.h (right): https://codereview.webrtc.org/2742903002/diff/40001/webrtc/p2p/base/sessionde... webrtc/p2p/base/sessiondescription.h:185: void set_session_connection_addr( On 2017/03/10 22:41:17, Taylor Brandstetter wrote: > I think it would be simpler to only store this address in > MediaContentDescription. If a "c=" is found at the session level, its value can > just be copied to the media level, since the meaning is the same. > > This is what currently happens with ice-ufrag at the session level (for > example): > https://cs.chromium.org/chromium/src/third_party/webrtc/pc/webrtcsdp.cc?type=... Done. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/mediasession.h File webrtc/pc/mediasession.h (right): https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/mediasession.h#... webrtc/pc/mediasession.h:316: rtc::Optional<rtc::SocketAddress> media_connection_addr() { On 2017/03/10 22:41:17, Taylor Brandstetter wrote: > This doesn't need to be an Optional since a value should always exist. But even > if no value is set, this can be detected using SocketAddress::IsNil. Done. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/mediasession.h#... webrtc/pc/mediasession.h:335: // https://tools.ietf.org/html/rfc4566#page-21 On 2017/03/10 22:41:17, Taylor Brandstetter wrote: > nit: Can link directly to the relevant section: > https://tools.ietf.org/html/rfc4566#section-5.7 Done. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc File webrtc/pc/webrtcsdp.cc (right): https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1903: bool ParseConnectionData(const std::string line, rtc::SocketAddress* addr) { On 2017/03/10 22:41:18, Taylor Brandstetter wrote: > Meant to be const ref string? Ah, yes. I missed the &. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1909: // Skip "c=<nettype>". On 2017/03/10 22:41:18, Taylor Brandstetter wrote: > We should verify that "nettype" is "IN" since that's all we support. Done. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1913: // Skip the "<addrtype>". On 2017/03/10 22:41:18, Taylor Brandstetter wrote: > To be more thorough, we could verify that addrtype matches the type of the > parsed address (IP4 or IP6), if the address is a literal IP address and not a > domain name. Done. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1920: LOG(LS_ERROR) << "Failed to parse the connection data. Multicast is not " On 2017/03/10 22:41:18, Taylor Brandstetter wrote: > This method should take an error output parameter and do: > > return ParseFailed( > line, > "Failed to parse the connection data. Multicast is not currently > supported.", > error); > > In order to return errors. Done. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp.cc#ne... webrtc/pc/webrtcsdp.cc:1993: LOG(LS_ERROR) << "Failed to parse the session level connection data:" On 2017/03/10 22:41:18, Taylor Brandstetter wrote: > This doesn't need to log an error if ParseConnectionData already did. Done. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp_unitt... File webrtc/pc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp_unitt... webrtc/pc/webrtcsdp_unittest.cc:871: audio_desc_ = CreateAudioContentDescription(); On 2017/03/10 22:41:18, Taylor Brandstetter wrote: > To be consistent with how most things in this file are tested, you could set the > connection_address for the audio/video descriptions here... Done. https://codereview.webrtc.org/2742903002/diff/40001/webrtc/pc/webrtcsdp_unitt... webrtc/pc/webrtcsdp_unittest.cc:1154: const MCD* cd2) { On 2017/03/10 22:41:18, Taylor Brandstetter wrote: > ... and add code to compare them in this method. Though you would still need > additional tests for "c=" at the session level, and for IPv6. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Two things I forgot about in the initial review were the port from the "m=" line and the "default destination" logic; I think we should address those before landing. https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/mediasession.h File webrtc/pc/mediasession.h (right): https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:314: // overwrite the session level address. This comment could be updated. Something like "May be present at the media or session level of SDP. If present at both levels, the media-level attribute overwrites the session-level one." https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:315: void set_connection_addr(const rtc::SocketAddress& addr) { nit: I'd prefer spelling out "connection_address" https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/webrtcsdp.cc File webrtc/pc/webrtcsdp.cc (left): https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/webrtcsdp.cc#o... webrtc/pc/webrtcsdp.cc:753: AddLine(os.str(), &new_lines); We shouldn't remove this "default destination" logic, since it's required by the ICE standard and may be needed by legacy endpoints. Two things I can think of doing: * Change the logic to: "If connection_address was set, use that. Otherwise, use default destination from ICE candidates." One oddity with this is that if you serialize a description with the address unset, then parse it, the address becomes set. Which breaks the serialize/deserialize symmetry. * Update the default destination in JsepSessionDescription::AddCandidate. Assuming this works, this seems cleaner in my opinion. The tests for this in webrtcsdp.cc would then move to jsepsessiondescription_unittest.cc. https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/webrtcsdp.cc File webrtc/pc/webrtcsdp.cc (right): https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:2387: if (fields[1] == kMediaPortRejected) { Something I just realized: the port on the m= line itself should probably be copied into the connection_address for convenience. So that with the combination of the "m=" and "c=" lines, you have a full IP/port pair.
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Please take another look. Thanks. https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/mediasession.h File webrtc/pc/mediasession.h (right): https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:314: // overwrite the session level address. On 2017/03/15 17:57:20, Taylor Brandstetter wrote: > This comment could be updated. Something like "May be present at the media or > session level of SDP. If present at both levels, the media-level attribute > overwrites the session-level one." Done. https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:315: void set_connection_addr(const rtc::SocketAddress& addr) { On 2017/03/15 17:57:20, Taylor Brandstetter wrote: > nit: I'd prefer spelling out "connection_address" Done. https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/webrtcsdp.cc File webrtc/pc/webrtcsdp.cc (right): https://codereview.webrtc.org/2742903002/diff/100001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:2387: if (fields[1] == kMediaPortRejected) { On 2017/03/15 17:57:20, Taylor Brandstetter wrote: > Something I just realized: the port on the m= line itself should probably be > copied into the connection_address for convenience. So that with the combination > of the "m=" and "c=" lines, you have a full IP/port pair. Done.
Just minor comments at this point https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/jsepsessiondes... File webrtc/pc/jsepsessiondescription_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/jsepsessiondes... webrtc/pc/jsepsessiondescription_unittest.cc:237: TEST_F(JsepSessionDescriptionTest, SerializeSessionDescriptionWithIPv6Only) { nit: These tests don't need to even call "Serialize" now, since the serialization is tested in webrtcsdp_unittest.cc. They can just verify that after adding the candidates, the connection_address field is set correctly. This could be left as a TODO. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/mediasession.h File webrtc/pc/mediasession.h (right): https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:316: connection_addr_ = addr; nit: May be confusing that the variable name is "connection_addr" and the method is "connection_address" https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc File webrtc/pc/webrtcsdp.cc (right): https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:722: static void UpdateMediaDefaultDestination( Since this is now done in JsepSessionDescription, this method isn't necessary, is it? https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:2391: int port = atoi(fields[1].c_str()); Can use "rtc::FromString<int>" and check for parse failure. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:2424: message, cricket::MEDIA_TYPE_VIDEO, mline_index, port, protocol, To avoid having to add the port as an argument everywhere, could the port be set after "ParseContentDescription" is called, below in this method? https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:2496: content->set_connection_address(session_connection_addr); If this condition is hit, the port from the m= line won't be set. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... File webrtc/pc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:198: "c=IN IP4 74.125.127.126\r\n" Since you changed the tests back to how they were initially, this is probably not needed; same with the "set_connection_address" calls below. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3410: // The port is expected to be the default value 0. This comment about the port can probably be removed. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3438: EXPECT_EQ("192.168.0.3:0", These should actually expect a port, since there should still be a port from the m= line. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3477: // unsupported IPv4 address. nit: Capitalize first word of comments https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3492: sdp = kSdpFullString; nit: Is this also "unsupported IPv6 address"? Might also be helpful to say "Unsupported multicast IPv6 address", to be more specific. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3496: } There should also be a test for the serialization. The test could just create a SessionDescription, call "set_connection_address", then serialize and deserialize and make sure the address is the same. That's typically how we test it. Or it could search for the string, but that seems more fragile.
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Patchset #4 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Please take another look. Thanks. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/jsepsessiondes... File webrtc/pc/jsepsessiondescription_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/jsepsessiondes... webrtc/pc/jsepsessiondescription_unittest.cc:237: TEST_F(JsepSessionDescriptionTest, SerializeSessionDescriptionWithIPv6Only) { On 2017/03/17 00:20:42, Taylor Brandstetter wrote: > nit: These tests don't need to even call "Serialize" now, since the > serialization is tested in webrtcsdp_unittest.cc. They can just verify that > after adding the candidates, the connection_address field is set correctly. This > could be left as a TODO. Done. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/mediasession.h File webrtc/pc/mediasession.h (right): https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/mediasession.h... webrtc/pc/mediasession.h:316: connection_addr_ = addr; On 2017/03/17 00:20:42, Taylor Brandstetter wrote: > nit: May be confusing that the variable name is "connection_addr" and the method > is "connection_address" Done. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc File webrtc/pc/webrtcsdp.cc (right): https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:722: static void UpdateMediaDefaultDestination( On 2017/03/17 00:20:42, Taylor Brandstetter wrote: > Since this is now done in JsepSessionDescription, this method isn't necessary, > is it? You are right. I should remove this. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:2391: int port = atoi(fields[1].c_str()); On 2017/03/17 00:20:42, Taylor Brandstetter wrote: > Can use "rtc::FromString<int>" and check for parse failure. Done. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:2424: message, cricket::MEDIA_TYPE_VIDEO, mline_index, port, protocol, On 2017/03/17 00:20:42, Taylor Brandstetter wrote: > To avoid having to add the port as an argument everywhere, could the port be set > after "ParseContentDescription" is called, below in this method? Yes, but I need to create a copy of those since the getter will return a const ref. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:2496: content->set_connection_address(session_connection_addr); On 2017/03/17 00:20:42, Taylor Brandstetter wrote: > If this condition is hit, the port from the m= line won't be set. Fixed. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... File webrtc/pc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:198: "c=IN IP4 74.125.127.126\r\n" On 2017/03/17 00:20:43, Taylor Brandstetter wrote: > Since you changed the tests back to how they were initially, this is probably > not needed; same with the "set_connection_address" calls below. Since the way of generating the c= line has been changed, we can't go back and also fixing all the related tests seems to be the only option then. This change is just to make the fix easier. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3410: // The port is expected to be the default value 0. On 2017/03/17 00:20:43, Taylor Brandstetter wrote: > This comment about the port can probably be removed. Done. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3438: EXPECT_EQ("192.168.0.3:0", On 2017/03/17 00:20:43, Taylor Brandstetter wrote: > These should actually expect a port, since there should still be a port from the > m= line. Done. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3477: // unsupported IPv4 address. On 2017/03/17 00:20:43, Taylor Brandstetter wrote: > nit: Capitalize first word of comments Done. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3492: sdp = kSdpFullString; On 2017/03/17 00:20:43, Taylor Brandstetter wrote: > nit: Is this also "unsupported IPv6 address"? Might also be helpful to say > "Unsupported multicast IPv6 address", to be more specific. Done. https://codereview.webrtc.org/2742903002/diff/160001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3496: } On 2017/03/17 00:20:43, Taylor Brandstetter wrote: > There should also be a test for the serialization. The test could just create a > SessionDescription, call "set_connection_address", then serialize and > deserialize and make sure the address is the same. That's typically how we test > it. Or it could search for the string, but that seems more fragile. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/7468)
https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/jsepsessiondes... File webrtc/pc/jsepsessiondescription_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/jsepsessiondes... webrtc/pc/jsepsessiondescription_unittest.cc:235: // TODO(zhihuang): Rename these tests. These are used to verify that after Not just rename, but also modify to test the "connection_address" field directly. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp.cc File webrtc/pc/webrtcsdp.cc (right): https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:1288: : std::to_string(media_desc->connection_address().port())); Can use rtc::ToString; std::to_string may not be available on some platforms (I ran into this recently with something). Also, may want to change this to an "if/else" to avoid the nested ternary expression. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:1298: InitLine(kLineTypeConnection, kConnectionNettype, &os); Should handle the "IsNil" case explicitly here, and generate "0.0.0.0". Similar to how IsNil results in kDummyPort above. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... File webrtc/pc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:198: "c=IN IP4 74.125.127.126\r\n" I still think, since this is intended to be "reference WebRTC SDP", it should have "0.0.0.0" since that's what WebRTC actually uses. If the issue is find/replacing in tests like ParseConnectionDataSessionLevelOnly, the find/replace string could just include the "m=" line above as well. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:879: audio_desc_->set_connection_address(audio_addr); This seems unnecessary since AddCandidate will do it. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:1087: rtc::SocketAddress connection_address("0.0.0.0", 9); This shouldn't be necessary if an unset address results in "0.0.0.0:9" by default. Same with the "add data channel" methods below. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:1832: video_desc_->set_connection_address(video_addr); I'd suggest replacing this pattern with a method that's just called "MakeDescriptionWithoutCandidates", and mention in a comment that it creates a description that corresponds to kSdpString. That way the test can just read like: MakeDescriptionWithoutCandidates(); AddFingerprint(); std::string message = webrtc::SdpSerialize(jdesc_, false); ... https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3451: // Remove the media level addresses. This comment can be removed. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3476: // Remove the media level addresses. This comment should be moved to above the "Replace" calls. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3538: TEST_F(WebRtcSdpTest, SerializeAndDeserializeWithConneceionAddress) { Spelling of "connection"
Patchset #5 (id:220001) has been deleted
Patchset #5 (id:240001) has been deleted
Please take another look. Thanks! https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/jsepsessiondes... File webrtc/pc/jsepsessiondescription_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/jsepsessiondes... webrtc/pc/jsepsessiondescription_unittest.cc:235: // TODO(zhihuang): Rename these tests. These are used to verify that after On 2017/03/18 00:46:07, Taylor Brandstetter wrote: > Not just rename, but also modify to test the "connection_address" field > directly. Done. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp.cc File webrtc/pc/webrtcsdp.cc (right): https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:1288: : std::to_string(media_desc->connection_address().port())); On 2017/03/18 00:46:07, Taylor Brandstetter wrote: > Can use rtc::ToString; std::to_string may not be available on some platforms (I > ran into this recently with something). > > Also, may want to change this to an "if/else" to avoid the nested ternary > expression. Good to know that. But if use the if/else, the port cannot be a const ref anymore. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp.cc#n... webrtc/pc/webrtcsdp.cc:1298: InitLine(kLineTypeConnection, kConnectionNettype, &os); On 2017/03/18 00:46:07, Taylor Brandstetter wrote: > Should handle the "IsNil" case explicitly here, and generate "0.0.0.0". Similar > to how IsNil results in kDummyPort above. Done. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... File webrtc/pc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:198: "c=IN IP4 74.125.127.126\r\n" On 2017/03/18 00:46:07, Taylor Brandstetter wrote: > I still think, since this is intended to be "reference WebRTC SDP", it should > have "0.0.0.0" since that's what WebRTC actually uses. > > If the issue is find/replacing in tests like > ParseConnectionDataSessionLevelOnly, the find/replace string could just include > the "m=" line above as well. Done. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:879: audio_desc_->set_connection_address(audio_addr); On 2017/03/18 00:46:07, Taylor Brandstetter wrote: > This seems unnecessary since AddCandidate will do it. I thought about this and I think AddCandidate won't do it because the |jdesc_| is created based on a copy of |desc_|. When doing the serialization in the test, we'll use the |desc_|. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:1087: rtc::SocketAddress connection_address("0.0.0.0", 9); On 2017/03/18 00:46:08, Taylor Brandstetter wrote: > This shouldn't be necessary if an unset address results in "0.0.0.0:9" by > default. Same with the "add data channel" methods below. Done. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:1832: video_desc_->set_connection_address(video_addr); On 2017/03/18 00:46:07, Taylor Brandstetter wrote: > I'd suggest replacing this pattern with a method that's just called > "MakeDescriptionWithoutCandidates", and mention in a comment that it creates a > description that corresponds to kSdpString. > > That way the test can just read like: > > MakeDescriptionWithoutCandidates(); > AddFingerprint(); > std::string message = webrtc::SdpSerialize(jdesc_, false); > ... Since I won't add this MakeDescriptionWIthoutCandidate() to every test case, I think it would better to create a new local jsep_desc for some test cases(for example, this test case.) so that the tests won't affect each other. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3451: // Remove the media level addresses. On 2017/03/18 00:46:08, Taylor Brandstetter wrote: > This comment can be removed. Done. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3476: // Remove the media level addresses. On 2017/03/18 00:46:08, Taylor Brandstetter wrote: > This comment should be moved to above the "Replace" calls. Done. https://codereview.webrtc.org/2742903002/diff/200001/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:3538: TEST_F(WebRtcSdpTest, SerializeAndDeserializeWithConneceionAddress) { On 2017/03/18 00:46:08, Taylor Brandstetter wrote: > Spelling of "connection" Done.
lgtm with nits https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/jsepsessiondes... File webrtc/pc/jsepsessiondescription.cc (right): https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/jsepsessiondes... webrtc/pc/jsepsessiondescription.cc:71: const std::vector<JsepCandidateCollection>& candidate_collections, nit: I believe this method could take a single JsepCandidateCollection and MediaContentDescription. https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/jsepsessiondes... File webrtc/pc/jsepsessiondescription_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/jsepsessiondes... webrtc/pc/jsepsessiondescription_unittest.cc:358: // Tests the candidate removal. nit: The candidate removal case would be better as its own test. https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/webrtcsdp_unit... File webrtc/pc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:879: audio_desc_->set_connection_address(audio_addr); nit (slash question): I still don't see that this is necessary, since AddCandidate will do it. https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:1771: rtc::SocketAddress audio_addr("0.0.0.0", 9); nit: Could add a comment explaining this. "Calling 'Initialize' with a copy of the inner SessionDescription will create a copy of the JsepSessionDescription without candidates. The 'connection address' field, previously set from the candidates, must also be reset."
Patchset #6 (id:270001) has been deleted
https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/jsepsessiondes... File webrtc/pc/jsepsessiondescription.cc (right): https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/jsepsessiondes... webrtc/pc/jsepsessiondescription.cc:71: const std::vector<JsepCandidateCollection>& candidate_collections, On 2017/03/20 18:29:58, Taylor Brandstetter wrote: > nit: I believe this method could take a single JsepCandidateCollection and > MediaContentDescription. I'll change it to take a single JsepCandidateCollection and a ContentDescription so that we don't have to downcast it whenever the method is called. https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/jsepsessiondes... File webrtc/pc/jsepsessiondescription_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/jsepsessiondes... webrtc/pc/jsepsessiondescription_unittest.cc:358: // Tests the candidate removal. On 2017/03/20 18:29:58, Taylor Brandstetter wrote: > nit: The candidate removal case would be better as its own test. Done. https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/webrtcsdp_unit... File webrtc/pc/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:879: audio_desc_->set_connection_address(audio_addr); On 2017/03/20 18:29:58, Taylor Brandstetter wrote: > nit (slash question): I still don't see that this is necessary, since > AddCandidate will do it. The jdesc_ is initialized based on a copy of desc_ so when the AddCandidate is called, the desc_ will not be changed. When serialize the sdp in other test cases, the jsep_.Initialize will be called again based on another copy of desc_. So I need to change the desc_ explicitly. https://codereview.webrtc.org/2742903002/diff/250009/webrtc/pc/webrtcsdp_unit... webrtc/pc/webrtcsdp_unittest.cc:1771: rtc::SocketAddress audio_addr("0.0.0.0", 9); On 2017/03/20 18:29:58, Taylor Brandstetter wrote: > nit: Could add a comment explaining this. "Calling 'Initialize' with a copy of > the inner SessionDescription will create a copy of the JsepSessionDescription > without candidates. The 'connection address' field, previously set from the > candidates, must also be reset." Done.
Recent changes lgtm https://codereview.webrtc.org/2742903002/diff/290001/webrtc/pc/jsepsessiondes... File webrtc/pc/jsepsessiondescription.cc (right): https://codereview.webrtc.org/2742903002/diff/290001/webrtc/pc/jsepsessiondes... webrtc/pc/jsepsessiondescription.cc:70: cricket::ContentDescription* content_description, nit: const parameters appear first in the parameter list according to the style guide
The CQ bit was checked by zhihuang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2742903002/#ps310001 (title: "Monior fix according to the style guide.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 310001, "attempt_start_ts": 1490117336851870,
"parent_rev": "d19bcb7116d527b5d0b7426468490e72b03358ca", "commit_rev":
"38989e593c994b6865f2b2cc496abf59d5925eb8"}
Message was sent while issue was closed.
Description was changed from ========== Parse the connection data in SDP (c= line). Extract the remote addresses from SDP c= line on both session level and media level. The media level address will overwrite the session level one if exists. WebRTC is not using c= and this is used for new SDP parsing API. BUG=webrtc:7311 ========== to ========== Parse the connection data in SDP (c= line). Extract the remote addresses from SDP c= line on both session level and media level. The media level address will overwrite the session level one if exists. WebRTC is not using c= and this is used for new SDP parsing API. BUG=webrtc:7311 Review-Url: https://codereview.webrtc.org/2742903002 Cr-Commit-Position: refs/heads/master@{#17326} Committed: https://chromium.googlesource.com/external/webrtc/+/38989e593c994b6865f2b2cc4... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:310001) as https://chromium.googlesource.com/external/webrtc/+/38989e593c994b6865f2b2cc4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
