Move SRTP logic out of MediaPipeline, and into mtransport

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
Rank:
15
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

(Blocks 1 bug)

60 Branch
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

a year ago
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.
Rank: 15
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

11 months ago
mozreview-review
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.
Attachment #8980672 - Flags: review?(drno) → review+

Comment 28

11 months ago
mozreview-review
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?
Attachment #8980673 - Flags: review?(drno) → review+

Comment 29

11 months ago
mozreview-review
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 :)
Attachment #8980674 - Flags: review?(drno) → review+

Comment 30

11 months ago
mozreview-review
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.
Attachment #8980675 - Flags: review?(drno) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

11 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 41

11 months ago
Pushed by bcampen@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.