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

Issue 3012953002: Created the DtlsSrtpTransport.

Created:
1 year, 3 months ago by Zhi Huang
Modified:
1 year, 2 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Created the DtlsSrtpTransport. The DtlsSrtpTransport takes DTLS responsibilities from BaseChannel. DtlsSrtpTransport is responsible for exporting keys from DtlsTransport and setting up the wrapped SrtpTransport. BUG=webrtc:7013

Patch Set 1 : Initial review. #

Total comments: 29

Patch Set 2 : Resolved the comments. #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+663 lines, -204 lines) Patch
M pc/BUILD.gn View 1 2 chunks +3 lines, -0 lines 0 comments Download
M pc/channel.h View 1 7 chunks +26 lines, -5 lines 1 comment Download
M pc/channel.cc View 1 14 chunks +163 lines, -177 lines 1 comment Download
A pc/dtlssrtptransport.h View 1 1 chunk +117 lines, -0 lines 2 comments Download
A pc/dtlssrtptransport.cc View 1 1 chunk +238 lines, -0 lines 3 comments Download
A pc/dtlssrtptransport_unittest.cc View 1 1 chunk +99 lines, -0 lines 1 comment Download
M pc/srtptransport.h View 1 1 chunk +11 lines, -3 lines 0 comments Download
M pc/srtptransport.cc View 1 1 chunk +0 lines, -10 lines 0 comments Download
M pc/srtptransport_unittest.cc View 1 1 chunk +4 lines, -8 lines 0 comments Download
M pc/webrtcsession_unittest.cc View 1 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 12 (8 generated)
Zhi Huang
PTAL. Thanks. https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/peerconnectioninterface_unittest.cc File webrtc/pc/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/peerconnectioninterface_unittest.cc#newcode2268 webrtc/pc/peerconnectioninterface_unittest.cc:2268: // CreateAnswerAsLocalDescription(); This would be removed once ...
1 year, 3 months ago (2017-09-10 22:58:01 UTC) #6
pthatcher
https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newcode944 webrtc/pc/channel.cc:944: if (!srtp_transport_) { This could use an early return. ...
1 year, 2 months ago (2017-09-12 00:24:44 UTC) #8
Zhi Huang
PTAL. Thanks. https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newcode944 webrtc/pc/channel.cc:944: if (!srtp_transport_) { On 2017/09/12 00:24:44, pthatcher ...
1 year, 2 months ago (2017-09-27 00:46:33 UTC) #11
Taylor Brandstetter
1 year, 2 months ago (2017-09-27 23:54:48 UTC) #12
In retrospect, I think it would have been a lot easier to do this after (or in
combination with) the CL that moves responsibility to TransportController. Then
a lot of the "enabling"/"upgrading"/"switching" kind of code wouldn't be
necessary.

https://codereview.webrtc.org/3012953002/diff/140001/pc/channel.cc
File pc/channel.cc (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/channel.cc#newcode995
pc/channel.cc:995: // certficate is ready and the |rtp_dtls_transport_| is
active. The
certificate

https://codereview.webrtc.org/3012953002/diff/140001/pc/channel.h
File pc/channel.h (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/channel.h#newcode384
pc/channel.h:384: // Create an SrtpTransport and wrap it in an
DtlsSrptTransport.
DtlsSrtpTransport

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.cc
File pc/dtlssrtptransport.cc (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.cc#...
pc/dtlssrtptransport.cc:142: void
DtlsSrtpTransport::SetSendEncryptedHeaderExtensionIds(
In a previous code review I mentioned that it may be useful to make an
RtpTransportAdapter helper class, similar to AsyncSocketAdapter. Now that we
have two classes that do this forwarding, it may now be a good time. Could you
at least add a TODO, if you agree?

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.cc#...
pc/dtlssrtptransport.cc:144: RTC_DCHECK(srtp_transport_);
nit: I don't know if adding these DCHECKS everywhere adds much; the only real
place I'd say it's necessary is the constructor, which doesn't have one.

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.cc#...
pc/dtlssrtptransport.cc:222: if (srtp_transport_) {
When will it be null?

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.h
File pc/dtlssrtptransport.h (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.h#n...
pc/dtlssrtptransport.h:59: void SetRtcpMuxEnabled(bool enable) override;
nit: I'd remove the newlines between all the override methods.

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.h#n...
pc/dtlssrtptransport.h:108: bool active_ = false;
Is this ever different than srtp_transport_->IsActive()?

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport_uni...
File pc/dtlssrtptransport_unittest.cc (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport_uni...
pc/dtlssrtptransport_unittest.cc:99: }
Should also test sending some packets; this doesn't actually verify that the
keying material was extracted correctly, just that both sides *think* they're
working.

Powered by Google App Engine
This is Rietveld 408576698