1455647 - Move SRTP logic out of MediaPipeline, and into mtransport

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, enhancement, P2)

Description

[Byron Campen [:bwc]]

The SRTP logic lives in MediaPipeline right now, which is not ideal. This probably belongs in mtransport, as a TransportLayer. This will probably be important for the effort of moving mtransport off into a separate STS process.

Comment 1

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6be05b9fc8e7e31c3d2fa7e34551f953ee4d912

Comment 2

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a616d8eff597629238d5c35be66edd03b7e67d5

Comment 3

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=898ff125cbf85de65434a90ab12a8e27ca100071

Comment 4

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51ae596e178b5030793f5c45b842c4766c630802

Comment 5

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bb9ab8927f35898be59e3230e512385d45d8dd8

Comment 6

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe88d8c5760fe8d439f600ad2db3250429c9c03b

Comment 7

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c57ea0da4c55f6c2816b5334d108e992a515665c

Comment 8

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c44e340c02a5f6a7132e737281aa1b2762c84ee1

Comment 9

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55ae92a9a88858466c631cc7f79e968a9d2633c3

Comment 10

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e488b9e3c7afcd27e2aa72d6740869a6e5fc91

Comment 11

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb5289763bdf18ed55b1d411415b7ed241516600

Comment 12

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1659b5fcce1badd1ee18dfe23a0bcc5c3bcea815

Comment 13

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=113617a1c07cb42a1197426d2feefb57efce9a17

Comment 14

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d0023694ed7

Comment 15

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba8c7d5d8ba4

Comment 16

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dba34b332eb9

Comment 17

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e1a8109a832

Comment 18

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e5e0b9e54d3

Comment 19

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f10d4393886

Comment 20

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f82c6f1898f

Comment 21

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea9ee7e5690a

Comment 22

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=504d1d0e4e7b

Comment 23

[Byron Campen [:bwc]]

Attachment: Bug 1455647 - Part 1: Simplify TransportFlow. r+drno

Review commit: https://reviewboard.mozilla.org/r/246842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246842/

Comment 24

[Byron Campen [:bwc]]

Attachment: Bug 1455647 - Part 2: Allow TransportLayers to be arranged in trees.

Review commit: https://reviewboard.mozilla.org/r/246844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246844/

Comment 25

[Byron Campen [:bwc]]

Attachment: Bug 1455647 - Part 3: Move SRTP into a TransportLayer.

Review commit: https://reviewboard.mozilla.org/r/246846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246846/

Comment 26

[Byron Campen [:bwc]]

Attachment: Bug 1455647 - Part 4: Make a place to live for context about media packets, to fix packet dump hooks. r+drno

Review commit: https://reviewboard.mozilla.org/r/246848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246848/

Comment 27

[Nils Ohlmeier [:drno]]

Comment on attachment 8980672 [details]
Bug 1455647 - Part 1: Simplify TransportFlow. r+drno

https://reviewboard.mozilla.org/r/246842/#review253060

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:209
(Diff revision 1)
>    }
>  
>    void PushLayers() {
>      nsresult res;
>  
> -    nsAutoPtr<std::queue<TransportLayer *> > layers(
> +    (void)flow_->PushLayer(loopback_);

Do you ignore a failure here on purpose (oppose to checking for errors when pushing the dtls_ layer)?
If so a comment might help.

Comment 28

[Nils Ohlmeier [:drno]]

Comment on attachment 8980673 [details]
Bug 1455647 - Part 2: Allow TransportLayers to be arranged in trees.

https://reviewboard.mozilla.org/r/246844/#review253906

::: media/mtransport/test/sctp_unittest.cpp:141
(Diff revision 1)
>      loopback_->Connect(peer->loopback_);
> +    ASSERT_EQ((nsresult)NS_OK, loopback_->Init());

I find the order of first connecting and then initing strange. But it seems to work...

::: media/mtransport/transportflow.cpp
(Diff revision 1)
>  #include "transportflow.h"
>  #include "transportlayer.h"
>  
>  namespace mozilla {
>  
> -MOZ_MTLOG_MODULE("mtransport")

Was this removed on purpose?
What does this log on now, without this being set?

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:207
(Diff revision 1)
>        TransportLayerDtls::SERVER);
>      dtls_->SetVerificationAllowAll();
>    }
>  
>    void PushLayers() {
> -    nsresult res;
> +    loopback_->Init();

We should probably also check the return value here.

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:209
(Diff revision 1)
>    }
>  
>    void PushLayers() {
> -    nsresult res;
> -
> -    (void)flow_->PushLayer(loopback_);
> +    loopback_->Init();
> +    nsresult res = dtls_->Init();
> +    dtls_->Chain(loopback_);

I would prefer if the error handling happens before the chaining and pushing.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:538
(Diff revision 1)
> -  TransportLayerIce* ice =
> +  aIceLayer->SetParameters(aPCMedia->ice_media_stream(aLevel),
> -      static_cast<TransportLayerIce*>(aLayerList->values.front());
> -  ice->SetParameters(aPCMedia->ice_media_stream(aLevel),
> -                     aIsRtcp ? 2 : 1);
> +                           aIsRtcp ? 2 : 1);
> -  for (auto& value : aLayerList->values) {
> -    (void)aFlow->PushLayer(value); // TODO(bug 854518): Process errors.
> +  aIceLayer->Init();
> +  aDtlsLayer->Init();

Can bug 854518 be closed after this lands?

Comment 29

[Nils Ohlmeier [:drno]]

Comment on attachment 8980674 [details]
Bug 1455647 - Part 3: Move SRTP into a TransportLayer.

https://reviewboard.mozilla.org/r/246846/#review254820

::: media/mtransport/transportlayersrtp.cpp:114
(Diff revision 1)
> +  if (IsRtp(data, len)) {
> +    MOZ_MTLOG(ML_INFO, "Attempting to protect RTP...");
> +    res = mSendSrtp->ProtectRtp(
> +      buf->data(), buf->len(), buf->capacity(), &out_len);
> +  } else {
> +    MOZ_MTLOG(ML_INFO, "Attempting to protect SRTP...");

s/SRTP/RTCP/

::: media/mtransport/transportlayersrtp.cpp:170
(Diff revision 1)
> +      MOZ_CRASH(); // TODO: Remove once we have enough field experience to
> +                   // know it doesn't happen. bug 798797. Note that the
> +                   // code after this never executes.

I think after 6 years we can remove this now and close the bug.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:349
(Diff revision 1)
>    // Replace a track with a different one
>    // In non-compliance with the likely final spec, allow the new
>    // track to be part of a different stream (since we don't support
>    // multiple tracks of a type in a stream yet).  bug 1056650
> -  virtual nsresult ReplaceTrack(RefPtr<dom::MediaStreamTrack>& aDomTrack);
> +  virtual nsresult SetTrack(dom::MediaStreamTrack* aDomTrack);

Not sure if the comment should get updated.

::: media/mtransport/SrtpFlow.cpp:20
(Diff revision 1)
> -
>  using namespace mozilla;
>  
>  namespace mozilla {
>  
> +MOZ_MTLOG_MODULE("mtransport")

Ok now the logging change from the previous patch makes sense :)

Comment 30

[Nils Ohlmeier [:drno]]

Comment on attachment 8980675 [details]
Bug 1455647 - Part 4: Make a place to live for context about media packets, to fix packet dump hooks. r+drno

https://reviewboard.mozilla.org/r/246848/#review254860

Couple of smaller suggestions and question if we can even optimize further. Feel free to postpone further optimizations into follow up bugs.
Otherwise LGTM.

::: media/mtransport/mediapacket.h:111
(Diff revision 1)
> +    // Level that this packet belongs to, if known.
> +    Maybe<size_t> level_;

If this refers to the SDP msection level I think SDP should at least be mentioned in the coment, if not even in the name of the variable e.g. sdp_level_ (because I would have not expected to have SDP information to be present in a media packet container).

::: media/mtransport/test/transport_unittests.cpp:298
(Diff revision 1)
>  
>    virtual void OnRecord(TransportLayer* layer,
>                          uint8_t content_type,
>                          const unsigned char *data, size_t len) {
>      // Only inject once.
> -    if (injected_) {
> +    if (!packet_.data()) {

|injected_| never got set anywhere if I'm not mistaken. Is this change in behavior intentionally?

::: media/mtransport/transportlayerdtls.cpp:1021
(Diff revision 1)
> -    unsigned char buf[9216];
>      int32_t rv;
>      // One packet might contain several DTLS packets
>      do {
> -      rv = PR_Recv(ssl_fd_.get(), buf, sizeof(buf), 0, PR_INTERVAL_NO_WAIT);
> +      // nICEr uses a 9216 bytes buffer to allow support for jumbo frames
> +      // Can we peek to get a better idea of the actual size?

Not sure if PR_Available64() works on network sockets, or only on files only.
But then this also involved decrypting the packet via NSS. Not sure how reliable we could predict the size of the decrypted packet.

::: media/mtransport/transportlayerloopback.h
(Diff revision 1)
>    TRANSPORT_LAYER_ID("loopback")
>  
>   private:
>    DISALLOW_COPY_ASSIGN(TransportLayerLoopback);
>  
> -  // A queued packet

Nice a second definition for QueuedPacket... good that we remove that duplicated code.

::: media/webrtc/signaling/src/mediapipeline/TransportLayerPacketDumper.h:16
(Diff revision 1)
> -#include "mozilla/RefPtr.h"
> -#include "SrtpFlow.h"
> +#include "signaling/src/peerconnection/PacketDumper.h"
> +#include "mozilla/dom/RTCPeerConnectionBinding.h"
>  
>  namespace mozilla {
>  
> -class TransportLayerDtls;
> +class TransportLayerPacketDumper final : public TransportLayer {

This makes wonder if this class should also handle the functionality of the RtpLogger class?!

::: media/mtransport/transportlayersrtp.cpp:59
(Diff revision 1)
>    downward_->SignalPacketReceived.connect(this, &TransportLayerSrtp::PacketReceived);
>  
>    return true;
>  }
>  
> -static bool
> +static bool IsRtp(const unsigned char* data, size_t len)

Why doesn't this take a MediaPacket now as well?

::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h:28
(Diff revision 1)
> +#include "mediapacket.h"
> +

Is this new include really needed, or maybe a left over from previous changes?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1080
(Diff revision 1)
>    // Filter out everything but RTP/RTCP
> -  if (aData[0] < 128 || aData[0] > 191) {
> +  if (packet.data()[0] < 128 || packet.data()[0] > 191) {
>      return;
>    }

Is this safety check still needed, or can only valid RTP get here now?
And if the check is still needed, could this be made more strict to not let pass RTCP here?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1136
(Diff revision 1)
>    RtpLogger::LogPacket(
> -    aData, aLen, true, true, header.headerLength, mDescription);
> +    packet.data(), packet.len(), true, true, header.headerLength, mDescription);
>  
> +  // Might be nice to pass ownership of the buffer in this case, but it is a
> +  // small optimization in a rare case.
>    mPacketDumper->Dump(
> -    mLevel, dom::mozPacketDumpType::Rtp, false, aData, aLen);
> +    mLevel, dom::mozPacketDumpType::Srtp, false, packet.encrypted_data(), packet.encrypted_len());
> +
> +  mPacketDumper->Dump(
> +    mLevel, dom::mozPacketDumpType::Rtp, false, packet.data(), packet.len());

Would it be possible to insert the RtpLogger and/or the PacketDumper now into the tree as plugable transportflow services on a per need basis (instead of calling them here for every single packet)?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1179
(Diff revision 1)
> -  if (!aLen) {
> +  if (!packet.len()) {
>      return;
>    }
>  
>    // Filter out everything but RTP/RTCP
> -  if (aData[0] < 128 || aData[0] > 191) {
> +  if (packet.data()[0] < 128 || packet.data()[0] > 191) {

Same here: still needed? If needed can it at least drop RTP here?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1194
(Diff revision 1)
> -  mPacketDumper->Dump(mLevel, dom::mozPacketDumpType::Srtcp, false, aData, aLen);
> -
>    CSFLogDebug(LOGTAG, "%s received RTCP packet.", mDescription.c_str());
>    IncrementRtcpPacketsReceived();
>  
> -  RtpLogger::LogPacket(aData, aLen, true, false, 0, mDescription);
> +  RtpLogger::LogPacket(packet.data(), packet.len(), true, false, 0, mDescription);

Shouldn't the RtpLogger also simply take MediaPacket's?
That would also allow to drop the |isRtp| argument on this function call.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1222
(Diff revision 1)
> -    RtcpPacketReceived(aLayer, aData, aLen);
> +      break;
> +    case MediaPacket::RTCP:
> +      RtcpPacketReceived(aLayer, packet);
> +      break;
> +    default:
> +      MOZ_CRASH();

Comment for the Moz crash might be helpful.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1623
(Diff revision 1)
> -  nsAutoPtr<DataBuffer> buf(new DataBuffer(aData, aLen));
> +  packet->Copy(aData, aLen, aLen + SRTP_MAX_EXPANSION);
>  

packet->setType(MediaPacket::RTP) should be called here already.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1639
(Diff revision 1)
>  }
>  
>  nsresult
>  MediaPipeline::PipelineTransport::SendRtpRtcpPacket_s(
> -  nsAutoPtr<DataBuffer> aData,
> +  nsAutoPtr<MediaPacket> aPacket,
>    bool aIsRtp)

I think the |aIsRtp| argument should get removed and use the MediaPacket internal type instead.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1705
(Diff revision 1)
> -  nsAutoPtr<DataBuffer> buf(new DataBuffer(aData, aLen));
> +  packet->Copy(aData, aLen, aLen + SRTP_MAX_EXPANSION);
>  

Same here: lets call packet->setType() here already.

Comment 31

[Byron Campen [:bwc]]

Comment on attachment 8980672 [details]
Bug 1455647 - Part 1: Simplify TransportFlow. r+drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246842/diff/1-2/

Comment 32

[Byron Campen [:bwc]]

Comment on attachment 8980673 [details]
Bug 1455647 - Part 2: Allow TransportLayers to be arranged in trees.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246844/diff/1-2/

Comment 33

[Byron Campen [:bwc]]

Comment on attachment 8980674 [details]
Bug 1455647 - Part 3: Move SRTP into a TransportLayer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246846/diff/1-2/

Comment 34

[Byron Campen [:bwc]]

Comment on attachment 8980675 [details]
Bug 1455647 - Part 4: Make a place to live for context about media packets, to fix packet dump hooks. r+drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246848/diff/1-2/

Comment 35

[Byron Campen [:bwc]]

Comment on attachment 8980675 [details]
Bug 1455647 - Part 4: Make a place to live for context about media packets, to fix packet dump hooks. r+drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246848/diff/2-3/

Comment 36

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2296c093e015a0ce422ff72c918a94399ee92fbe -d 9f0fd50fb6d9: rebasing 467709:2296c093e015 "Bug 1455647 - Part 1: Simplify TransportFlow. r+drno r=drno"
merging media/mtransport/test/transport_unittests.cpp
merging media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
merging netwerk/sctp/datachannel/DataChannel.cpp
merging netwerk/sctp/datachannel/DataChannel.h
warning: conflicts while merging netwerk/sctp/datachannel/DataChannel.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 37

[Byron Campen [:bwc]]

Comment on attachment 8980672 [details]
Bug 1455647 - Part 1: Simplify TransportFlow. r+drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246842/diff/2-3/

Comment 38

[Byron Campen [:bwc]]

Comment on attachment 8980673 [details]
Bug 1455647 - Part 2: Allow TransportLayers to be arranged in trees.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246844/diff/2-3/

Comment 39

[Byron Campen [:bwc]]

Comment on attachment 8980674 [details]
Bug 1455647 - Part 3: Move SRTP into a TransportLayer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246846/diff/2-3/

Comment 40

[Byron Campen [:bwc]]

Comment on attachment 8980675 [details]
Bug 1455647 - Part 4: Make a place to live for context about media packets, to fix packet dump hooks. r+drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246848/diff/3-4/

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ae266dfb8ea
Part 1: Simplify TransportFlow. r+drno r=drno
https://hg.mozilla.org/integration/autoland/rev/19e0229159de
Part 2: Allow TransportLayers to be arranged in trees. r=drno
https://hg.mozilla.org/integration/autoland/rev/cbac90b0c34d
Part 3: Move SRTP into a TransportLayer. r=drno
https://hg.mozilla.org/integration/autoland/rev/8ab6afabc78c
Part 4: Make a place to live for context about media packets, to fix packet dump hooks. r+drno r=drno

Comment 42

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/0ae266dfb8ea
https://hg.mozilla.org/mozilla-central/rev/19e0229159de
https://hg.mozilla.org/mozilla-central/rev/cbac90b0c34d
https://hg.mozilla.org/mozilla-central/rev/8ab6afabc78c