Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(316)

Issue 2761143002: Support encrypted RTP extensions (RFC 6904) (Closed)

Created:
3 years, 9 months ago by joachim
Modified:
3 years, 5 months ago
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Support encrypted RTP extensions (RFC 6904) Can be enabled by setting "enable_encrypted_rtp_header_extensions" in "crypto_options" of "PeerConnectionFactoryInterface::Options" and will only be used if both peers support it. BUG=webrtc:3411 Review-Url: https://codereview.webrtc.org/2761143002 Cr-Commit-Position: refs/heads/master@{#18842} Committed: https://chromium.googlesource.com/external/webrtc/+/5869f50f7a7079055fde7320a3babc5f26134f1c

Patch Set 1 #

Total comments: 77

Patch Set 2 : Various changes based on feedback from Peter and Taylor. #

Total comments: 12

Patch Set 3 : More updates + support for adding/changing encrypted extensions. #

Total comments: 30

Patch Set 4 : Don't negotiate extension ids in SrtpFilter, more changes after feedback from Taylor. #

Total comments: 8

Patch Set 5 : More feedback from Taylor. #

Total comments: 3

Patch Set 6 : Filter encrypted extensions. #

Patch Set 7 : Add different test scenarios, fix send/recv direction for extensions in SRTP filter. #

Patch Set 8 : Updated comment. #

Total comments: 22

Patch Set 9 : Feedback from Peter. #

Patch Set 10 : Rebased #

Patch Set 11 : Added missing deps for tests. #

Patch Set 12 : Updated condition when to update SRTP filter, fixes error in CallTransferredForXXX tests. #

Patch Set 13 : Added missing include for compiling on Windows. #

Total comments: 3

Patch Set 14 : Only update SRTP filter if DTLS is connected. #

Total comments: 2

Patch Set 15 : Don't set state to "DTLS_TRANSPORT_CLOSED". #

Patch Set 16 : Another rebase. #

Patch Set 17 : Fix compile error on win_x64 bots. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1404 lines, -116 lines) Patch
M webrtc/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +7 lines, -0 lines 0 comments Download
M webrtc/config.h View 1 2 3 4 5 6 7 8 9 2 chunks +24 lines, -3 lines 0 comments Download
M webrtc/config.cc View 1 2 3 4 5 6 7 8 9 4 chunks +62 lines, -0 lines 0 comments Download
A webrtc/config_unittest.cc View 1 2 3 1 chunk +49 lines, -0 lines 0 comments Download
M webrtc/media/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/media/base/fakertp.h View 1 2 3 4 5 6 7 8 3 chunks +51 lines, -0 lines 0 comments Download
A webrtc/media/base/fakertp.cc View 1 2 3 4 5 6 7 8 1 chunk +66 lines, -0 lines 0 comments Download
M webrtc/media/engine/webrtcmediaengine.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +8 lines, -4 lines 0 comments Download
M webrtc/media/engine/webrtcmediaengine_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +53 lines, -0 lines 0 comments Download
M webrtc/p2p/base/dtlstransportchannel.h View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -0 lines 0 comments Download
M webrtc/p2p/base/dtlstransportchannel.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -1 line 0 comments Download
M webrtc/p2p/base/dtlstransportinternal.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/p2p/base/fakedtlstransport.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +13 lines, -1 line 0 comments Download
M webrtc/pc/channel.h View 1 2 3 4 5 6 7 8 9 2 chunks +12 lines, -6 lines 0 comments Download
M webrtc/pc/channel.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 20 chunks +114 lines, -42 lines 0 comments Download
M webrtc/pc/channel_unittest.cc View 1 2 3 4 5 6 7 8 9 7 chunks +313 lines, -0 lines 0 comments Download
M webrtc/pc/mediasession.h View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -0 lines 0 comments Download
M webrtc/pc/mediasession.cc View 1 2 3 4 5 6 7 8 9 9 chunks +101 lines, -16 lines 0 comments Download
M webrtc/pc/mediasession_unittest.cc View 1 2 3 4 5 6 7 8 9 5 chunks +187 lines, -0 lines 0 comments Download
M webrtc/pc/srtpfilter.h View 1 2 3 4 5 6 7 8 9 6 chunks +23 lines, -0 lines 0 comments Download
M webrtc/pc/srtpfilter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 10 chunks +120 lines, -25 lines 0 comments Download
M webrtc/pc/srtpfilter_unittest.cc View 1 2 3 4 5 6 7 8 9 7 chunks +98 lines, -0 lines 0 comments Download
M webrtc/pc/webrtcsdp.cc View 1 2 3 4 5 6 7 8 9 2 chunks +25 lines, -3 lines 0 comments Download
M webrtc/pc/webrtcsdp_unittest.cc View 1 2 3 4 5 6 7 8 9 8 chunks +52 lines, -15 lines 0 comments Download
M webrtc/pc/webrtcsession.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/pc/webrtcsessiondescriptionfactory.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/rtc_base/sslstreamadapter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 51 (17 generated)
joachim
I finally had some time to continue working on this. Tested compatibility between Chrome 57 ...
3 years, 9 months ago (2017-03-21 00:18:27 UTC) #2
pthatcher1
I'm sure Taylor can give this a more thorough review than I can. https://codereview.webrtc.org/2761143002/diff/1/webrtc/api/peerconnectioninterface.h File ...
3 years, 9 months ago (2017-03-21 07:07:07 UTC) #4
Taylor Brandstetter
Mostly looks good. Most of my concerns are just around the BaseChannel code. https://codereview.webrtc.org/2761143002/diff/1/webrtc/api/peerconnectioninterface.h File ...
3 years, 9 months ago (2017-03-22 18:00:12 UTC) #5
joachim
Thank you both for the detailed feedback! Uploaded a new version that addresses all comments ...
3 years, 9 months ago (2017-03-23 00:04:34 UTC) #7
Taylor Brandstetter
New patchset looks great. I think the only remaining thing to resolve is what happens ...
3 years, 9 months ago (2017-03-23 20:10:57 UTC) #8
joachim
Here is an updated version that now supports adding/changing encrypted extensions when receiving additional offers/answers ...
3 years, 8 months ago (2017-03-30 22:43:49 UTC) #10
Taylor Brandstetter
I still don't think the offer/answer logic handles all cases. Let's take a step back ...
3 years, 8 months ago (2017-04-01 00:28:59 UTC) #11
joachim
Sorry for the slow response, I had too much other (personal) stuff to look into ...
3 years, 8 months ago (2017-04-17 10:46:10 UTC) #12
Taylor Brandstetter
I only have some minor comments left; great work on this. https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): ...
3 years, 8 months ago (2017-04-19 06:48:40 UTC) #13
joachim
All comments addressed, ptal. https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittest.cc File webrtc/pc/channel_unittest.cc (right): https://codereview.webrtc.org/2761143002/diff/40001/webrtc/pc/channel_unittest.cc#newcode1069 webrtc/pc/channel_unittest.cc:1069: } On 2017/04/19 06:48:39, Taylor ...
3 years, 8 months ago (2017-04-19 23:40:19 UTC) #14
Taylor Brandstetter
Just a couple small questions, otherwise lgtm https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc#newcode1492 webrtc/pc/channel.cc:1492: return extensions; ...
3 years, 8 months ago (2017-04-20 06:37:28 UTC) #15
joachim
Thanks, adressed your comments. https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/60001/webrtc/pc/channel.cc#newcode1492 webrtc/pc/channel.cc:1492: return extensions; On 2017/04/20 06:37:28, ...
3 years, 8 months ago (2017-04-24 22:07:48 UTC) #16
Taylor Brandstetter
https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittest.cc File webrtc/pc/channel_unittest.cc (right): https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittest.cc#newcode1019 webrtc/pc/channel_unittest.cc:1019: WaitForThreads(); On 2017/04/24 22:07:48, joachim wrote: > On 2017/04/20 ...
3 years, 8 months ago (2017-04-24 23:08:17 UTC) #17
joachim
On 2017/04/24 23:08:17, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittest.cc > File webrtc/pc/channel_unittest.cc (right): > > https://codereview.webrtc.org/2761143002/diff/80001/webrtc/pc/channel_unittest.cc#newcode1019 ...
3 years, 7 months ago (2017-04-26 22:42:10 UTC) #18
Taylor Brandstetter
On 2017/04/26 22:42:10, joachim wrote: > > While sending packets from channel 2 to 1 ...
3 years, 7 months ago (2017-04-26 23:49:18 UTC) #19
joachim
Thanks for being so patient and sorry for the noise ;-) I found the error ...
3 years, 7 months ago (2017-05-02 23:01:56 UTC) #20
Taylor Brandstetter
lgtm
3 years, 7 months ago (2017-05-05 01:52:54 UTC) #22
Taylor Brandstetter
Oh, you'll also need an owner for the root webrtc/ directory. stefan@?
3 years, 7 months ago (2017-05-05 02:08:59 UTC) #24
pthatcher1
Just some readability suggestions https://codereview.webrtc.org/2761143002/diff/140001/webrtc/base/sslstreamadapter.h File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/2761143002/diff/140001/webrtc/base/sslstreamadapter.h#newcode86 webrtc/base/sslstreamadapter.h:86: bool enable_encrypted_rtp_header_extensions = false; be ...
3 years, 7 months ago (2017-05-05 21:26:39 UTC) #25
joachim
Thanks a lot, all comments addressed. https://codereview.webrtc.org/2761143002/diff/140001/webrtc/base/sslstreamadapter.h File webrtc/base/sslstreamadapter.h (right): https://codereview.webrtc.org/2761143002/diff/140001/webrtc/base/sslstreamadapter.h#newcode86 webrtc/base/sslstreamadapter.h:86: bool enable_encrypted_rtp_header_extensions = ...
3 years, 7 months ago (2017-05-06 14:52:33 UTC) #26
pthatcher1
lgtm
3 years, 7 months ago (2017-05-09 03:25:02 UTC) #27
joachim
On 2017/05/05 02:08:59, Taylor Brandstetter wrote: > Oh, you'll also need an owner for the ...
3 years, 7 months ago (2017-05-11 10:40:09 UTC) #28
stefan-webrtc
webrtc/ lgtm
3 years, 7 months ago (2017-05-18 14:08:38 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2761143002/160001
3 years, 7 months ago (2017-05-18 19:36:53 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/builds/4431) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, ...
3 years, 7 months ago (2017-05-18 19:38:23 UTC) #34
joachim
On 2017/05/18 19:38:23, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 7 months ago (2017-05-18 20:11:01 UTC) #35
joachim
Finally found some time for the rebase. Notable changes since the CL I failed to ...
3 years, 6 months ago (2017-06-23 23:59:16 UTC) #36
Taylor Brandstetter
lgtm again, with a minor comment https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc#newcode1164 webrtc/pc/channel.cc:1164: (rtp_dtls_transport_->dtls_state() == DTLS_TRANSPORT_NEW ...
3 years, 5 months ago (2017-06-28 11:28:21 UTC) #37
joachim
Ptal https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc#newcode1164 webrtc/pc/channel.cc:1164: (rtp_dtls_transport_->dtls_state() == DTLS_TRANSPORT_NEW || On 2017/06/28 11:28:20, Taylor ...
3 years, 5 months ago (2017-06-28 17:03:06 UTC) #38
Taylor Brandstetter
https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2761143002/diff/240001/webrtc/pc/channel.cc#newcode1164 webrtc/pc/channel.cc:1164: (rtp_dtls_transport_->dtls_state() == DTLS_TRANSPORT_NEW || On 2017/06/28 17:03:05, joachim wrote: ...
3 years, 5 months ago (2017-06-29 06:59:09 UTC) #39
joachim
Changed and rebased again to resolve the "base" -> "rtc_base" conflicts. https://codereview.webrtc.org/2761143002/diff/260001/webrtc/p2p/base/fakedtlstransport.h File webrtc/p2p/base/fakedtlstransport.h (right): ...
3 years, 5 months ago (2017-06-29 17:20:59 UTC) #40
Taylor Brandstetter
lgtm again
3 years, 5 months ago (2017-06-29 17:38:14 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2761143002/320001
3 years, 5 months ago (2017-06-29 19:01:21 UTC) #48
commit-bot: I haz the power
3 years, 5 months ago (2017-06-29 19:31:45 UTC) #51
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/external/webrtc/+/5869f50f7a7079055fde7320a...

Powered by Google App Engine
This is Rietveld 408576698