786307 - Implement RTCP MUX in MediaPipeline

Status: RESOLVED FIXED

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

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

call setup time optimization (strongly wanted!  but we'd pref on without it imho)
Needs coordinating bug in signaling

Comment 2

[Ethan Hugg [:ehugg]]

Looking at the MediaPipeline code it appears that this is already in.  I think the next step would be to add tests for this into the mediapipline_unittest.

When a mediapipeline is created there are three options each of which should have a test:

1. Separate TransportFlow given for rtcp - no mux.
2. RTCP TransportFlow is set to the same as RTP - should mux.
3. RTCP TransportFlow is set to NULL - should mux.

I didn't see anything in the test yet that verifies RTCP end-end either so that should be added as well.  Then we can find out if anything is broken in MediaPipeline for rtcp-mux.

Comment 3

[Eric Rescorla (:ekr)]

This sounds exactly right.

Comment 4

[Eric Rescorla (:ekr)]

... at least as a strategy. I can't remember why there are both the null and same options... But why don't you add a test.

Comment 5

[Suhas]

Bug 859971 does test RTCP Tx/Rx ..

Comment 6

[Eric Rescorla (:ekr)]

As far as I can tell it does not check mux. And what is needed here is a pipeline test.

Comment 7

[Suhas]

Agreed ..

Comment 8

[Suhas]

Attachment: Part1 Enable RTP/RTCP Tx/Rx for non mux scenario

Patch to verify RTP and RTCP Tx/Rx in the test-case. Also fixes couple of minor bugs in MediaPipeline.cpp

Comment 9

[Suhas]

Attachment: Verify RTCP Mux support with different transport settings

Adds few test cases to verify RTCP Mux functionality. Also fixes minor bugs in Mediapipeline.cpp for mux scenario.

Comment 10

[Ethan Hugg [:ehugg]]

Comment on attachment 745616 [details] [diff] [review]
Part1 Enable RTP/RTCP Tx/Rx for non mux scenario

Review of attachment 745616 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +86,5 @@
>      MOZ_MTLOG(PR_LOG_ERROR, "RTP transport is already in error state");
>      TransportFailed_s(rtp_transport_);
>      return NS_ERROR_FAILURE;
> +  } 
> +  //Setup RTCP 

remove trailing whitespace used to be set in your sublime prefs.

@@ +88,5 @@
>      return NS_ERROR_FAILURE;
> +  } 
> +  //Setup RTCP 
> +  if (!muxed_) {
> +    rtcp_transport_->SignalStateChange.connect(this,

We have a MOZ_ASSERT for rtp_transport at the top of this function, I'm guessing we need a MOZ_ASSERT for rtcp_transport before this call, or a runtime check or both.

Comment 11

[Ethan Hugg [:ehugg]]

Comment on attachment 745617 [details] [diff] [review]
Verify RTCP Mux support with different transport settings

Review of attachment 745617 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +272,5 @@
>    State *state = rtcp ? &rtcp_state_ : &rtp_state_;
>  
>    *state = MP_CLOSED;
>  
> +  if(muxed_) 

trailing ws

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +265,5 @@
> +    } else if(option == MUX_NULL_TRANSPORT) {
> +      audio_rtcp_flow_ = nullptr;
> +    } else {
> +      // do nothing. since the rtcp flow should be setup already
> +    }

There is too much duplicate code between TestAgentSend/Receive and you are adding more here.  Need to probably move some of this up to superclass.

Comment 12

[Suhas]

Attachment: Fix and Verify RTCP Mux Support

Comment 13

[Suhas]

Attachment: Fix and Verify RTCP Mux Support

Comment 14

[Suhas]

Attachment: Fix and Verify RTCP Mux Support

Comment 15

[Suhas]

Attachment: Fix and verify RTCP Mux support

This is same patch as 2 patches reviewed earlier. But instead combined into a single patch and re-verified on the latest m-c. Also this patch incorporates comments from earlier R+.

Comment 16

[Suhas]

Comment on attachment 768380 [details] [diff] [review]
Fix and verify RTCP Mux support

Taking R+ from Ethan and Randell. Moving r? to Ekr.

Comment 17

[Ethan Hugg [:ehugg]]

Attachment: Implement RTCP MUX in MediaPipeline

Comment 18

[Ethan Hugg [:ehugg]]

Attachment: Implement RTCP MUX in MediaPipeline

Comment 19

[Ethan Hugg [:ehugg]]

Comment on attachment 779911 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline

Review of attachment 779911 [details] [diff] [review]:
-----------------------------------------------------------------

Un-bitrotted and removed a bit of redundant code.

Comment 20

[Eric Rescorla (:ekr)]

Comment on attachment 779911 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline

Review of attachment 779911 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with comments below.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +96,5 @@
>  
> +    if (rtcp_transport_->state() == TransportLayer::TS_OPEN) {
> +      res = TransportReady_s(rtcp_transport_);
> +      if (NS_FAILED(res)) {
> +        MOZ_MTLOG(PR_LOG_ERROR, "Error calling TransportReady(); res="

PR_LOG_ERROR -> ML_ERROR here andbelow.

@@ +275,5 @@
>  
>    *state = MP_CLOSED;
>  
> +  if(muxed_) {
> +    rtcp_state_ = MP_CLOSED;

MOZ_ASSERT() here that state != &rtcp_state.

Also assert in StateChange() that if (muxed_) the stream is the RTP stream.

Comment 21

[Eric Rescorla (:ekr)]

Comment on attachment 779911 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline

Re-marking r? because I didn't review the unit test yet.

Comment 22

[Eric Rescorla (:ekr)]

Comment on attachment 779911 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline

Review of attachment 779911 [details] [diff] [review]:
-----------------------------------------------------------------

please clean up unit test and the API a bit.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +87,5 @@
> +  }
> +
> +  // Setup RTCP
> +  if (muxed_) {
> +    rtcp_transport_ = rtp_transport_;

Let's either indicate muxed_ with null or with the same. And check/assert on the other.

@@ +96,5 @@
>  
> +    if (rtcp_transport_->state() == TransportLayer::TS_OPEN) {
> +      res = TransportReady_s(rtcp_transport_);
> +      if (NS_FAILED(res)) {
> +        MOZ_MTLOG(PR_LOG_ERROR, "Error calling TransportReady(); res="

PR_LOG_ERROR -> ML_ERROR here andbelow.

@@ +275,5 @@
>  
>    *state = MP_CLOSED;
>  
> +  if(muxed_) {
> +    rtcp_state_ = MP_CLOSED;

MOZ_ASSERT() here that state != &rtcp_state.

Also assert in StateChange() that if (muxed_) the stream is the RTP stream.

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +53,5 @@
>   public:
>    TestAgent() :
>        audio_flow_(new TransportFlow()),
>        audio_prsock_(new TransportLayerPrsock()),
>        audio_dtls_(new TransportLayerDtls()),

This seems like a lot of cut-and-paste. Let's make a struct that represents these and then we can pass that to Connectsocket.

@@ +94,5 @@
> +      layers->push(audio_prsock_);
> +      layers->push(audio_dtls_);
> +      ASSERT_EQ((nsresult)NS_OK, audio_flow_->PushLayers(layers));
> +    } else {
> +      if(!audio_rtcp_flow_)

You should be able to refactor this so it has much less cut-and-paste.

@@ +189,5 @@
>    mozilla::RefPtr<mozilla::MediaSessionConduit> audio_conduit_;
>    nsRefPtr<DOMMediaStream> audio_;
>    mozilla::RefPtr<mozilla::MediaPipeline> audio_pipeline_;
>    mozilla::RefPtr<TransportFlow> video_flow_;
>    TransportLayerPrsock *video_prsock_;

Do we no longer need the video flow?

@@ +227,5 @@
> +    return audio_pipeline_->rtp_packets_sent();
> +  }
> +
> +  int GetRtcpCount() {
> +    return audio_pipeline_->rtcp_packets_received();

Rename these to be GetAudio....()

@@ +273,5 @@
> +  int GetRtpCount() {
> +    return audio_pipeline_->rtp_packets_received();
> +  }
> +
> +  int GetRtcpCount() {

GetAudio*

@@ +305,5 @@
> +      test_utils->sts_target(),
> +      WrapRunnable(&p2_, &TestAgent::ConnectSocket, rtp_fds_[1], true, false));
> +
> +    // create rtcp flows separately as we are not muxing them.
> +    if(!aIsRtcpMux) {

Please clean up the comments here. Capitalization and if you're going to have numbers, have numbers.

Comment 23

[Ethan Hugg [:ehugg]]

Comment on attachment 779911 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline

Review of attachment 779911 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +87,5 @@
> +  }
> +
> +  // Setup RTCP
> +  if (muxed_) {
> +    rtcp_transport_ = rtp_transport_;

OK, I will change it to send same if you want mux and assert if you send in NULL on the premise that the latter is more likely to be done by accident.

Comment 24

[Eric Rescorla (:ekr)]

Actually now that I think about it, isn't same more likely by accident?

Comment 25

[Ethan Hugg [:ehugg]]

OK, I will assert on same and mux on NULL.  Personally I am accident-prone enough to do either.

Comment 26

[Ethan Hugg [:ehugg]]

Attachment: Implement RTCP MUX in MediaPipeline

Comment 27

[Ethan Hugg [:ehugg]]

Attachment: Implement RTCP MUX in MediaPipeline

Comment 28

[Ethan Hugg [:ehugg]]

Comment on attachment 786324 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline

Review of attachment 786324 [details] [diff] [review]:
-----------------------------------------------------------------

This one should address the review comments:

I changed the API a bit so that creating a MediaPipeline with rtcp_transport==NULL is the way to signal that you want rtcp-mux.  Accordingly, I have removed the muxed_ data member and check rtcp_transport==NULL in all cases instead.  We assert in debug builds if you send in the same flow for both rtp and rtcp.

In the unittests I eliminated the duplicate code by creating a struct for transport flow info that we select at the beginning of CreateSocket.  Rather than trying to update unused code, I removed all the code related to video.

Comment 29

[Eric Rescorla (:ekr)]

Comment on attachment 786324 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline

Review of attachment 786324 [details] [diff] [review]:
-----------------------------------------------------------------

I think rather than having conditionals it would make more sense to *check* for rtcp_transport != NULL in the ctor but
still set rtcp_transport_ internally. Do you have a reason why that's less good?

Also, looks like the unit tests still need some code cleanup.

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +45,5 @@
> +
> +struct TransportInfo {
> +  mozilla::RefPtr<TransportFlow> flow_;
> +  TransportLayerPrsock *prsock_;
> +  TransportLayerDtls *dtls_;

Members first, then ctor.

@@ +49,5 @@
> +  TransportLayerDtls *dtls_;
> +
> +  TransportInfo(TransportFlow *flow,
> +    TransportLayerPrsock *prsock,
> +    TransportLayerDtls *dtls) {

Is this ctor ever called with nonzero values? If so, suggest just having the no-arg ctor.

@@ +52,5 @@
> +    TransportLayerPrsock *prsock,
> +    TransportLayerDtls *dtls) {
> +    flow_ = flow;
> +    prsock_ = prsock;
> +    dtls_ = dtls;

Use initializers.

@@ +64,5 @@
>        audio_conduit_(mozilla::AudioSessionConduit::Create(NULL)),
>        audio_(),
>        audio_pipeline_(),
> +      audio_rtp_transport_(nullptr, nullptr, nullptr),
> +      audio_rtcp_transport_(nullptr, nullptr, nullptr) {

Is this ctor ever called with nonzero values?

@@ +72,2 @@
>      nsresult res;
> +    TransportInfo *transport_p = isRtcp ?

s/transport_p/transport/

@@ +74,5 @@
> +      &audio_rtcp_transport_ : &audio_rtp_transport_;
> +
> +    transport_p->flow_ = new TransportFlow();
> +    transport_p->prsock_ = new TransportLayerPrsock();
> +    transport_p->dtls_ = new TransportLayerDtls();

Let's make an init function to do this stuff.

@@ +90,5 @@
> +    transport_p->dtls_->SetSrtpCiphers(ciphers);
> +    transport_p->dtls_->SetIdentity(DtlsIdentity::Generate());
> +    transport_p->dtls_->SetRole(client ? TransportLayerDtls::CLIENT :
> +                          TransportLayerDtls::SERVER);
> +    transport_p->dtls_->SetVerificationAllowAll();

And this stuff as well, perhaps.

@@ +208,5 @@
>  
>      std::string test_pc("PC");
> +
> +    if (aIsRtcpMux) {
> +        ASSERT_FALSE(audio_rtcp_transport_.flow_);

two space indent.

Comment 30

[Ethan Hugg [:ehugg]]

>I think rather than having conditionals it would make more sense to *check* for rtcp_transport 
>!= NULL in the ctor but
>still set rtcp_transport_ internally. Do you have a reason why that's less good?

It seemed confusing to assert on equals and then set them equal a couple lines later so I thought this would be clearer.  I can change to setting them equal on the mux case and check for equality to determine when we're muxing.  It would be fewer lines of changes.

Comment 31

[Ethan Hugg [:ehugg]]

Attachment: Implement RTCP MUX in MediaPipeline

Comment 32

[Ethan Hugg [:ehugg]]

Attachment: Implement RTCP MUX in MediaPipeline

Comment 33

[Ethan Hugg [:ehugg]]

Comment on attachment 786734 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline


I now set rtcp_transport_ = rtp_transport_ when muxing.  I did not revive the 
muxed_ data member since I didn't want to sprinkle in ASSERTs for when it should 
be the same as rtcp_transport_ == rtp_transport_.  I also expanded the 
TransportInfo by adding an Init() and simplified its constructor.

Comment 34

[Eric Rescorla (:ekr)]

Comment on attachment 786734 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline

Review of attachment 786734 [details] [diff] [review]:
-----------------------------------------------------------------

This lgtm with nits below.

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +43,5 @@
>  
>  namespace {
> +
> +class TransportInfo {
> +public:

public is indented one space;

@@ +47,5 @@
> +public:
> +  TransportInfo() :
> +    flow_(nullptr),
> +    prsock_(nullptr),
> +    dtls_(nullptr) {}

This code uses NULL

@@ +70,5 @@
> +  }
> +
> +  mozilla::RefPtr<TransportFlow> flow_;
> +  TransportLayerPrsock *prsock_;
> +  TransportLayerDtls *dtls_;

This appears to leak memory if, for instance, the Init() fails.

Not a crisis in a unit test, but might be worth clenaing up

@@ +98,5 @@
> +    nsAutoPtr<std::queue<TransportLayer *> > layers(
> +      new std::queue<TransportLayer *>);
> +    layers->push(transport->prsock_);
> +    layers->push(transport->dtls_);
> +    ASSERT_EQ((nsresult)NS_OK, transport->flow_->PushLayers(layers));

Maybe we should just push this into a member fxn since I don't see much evidence that we're looking into these.

@@ +118,5 @@
>  
>    void StopInt() {
>      audio_->GetStream()->Stop();
> +    audio_rtp_transport_.flow_ = nullptr;
> +    audio_rtcp_transport_.flow_ = nullptr;

Let's make this a member fxn too.

Comment 35

[Ethan Hugg [:ehugg]]

Attachment: Implement RTCP MUX in MediaPipeline

Comment 36

[Ethan Hugg [:ehugg]]

Comment on attachment 787063 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline


Addressed nits in last review - Only unittest code changed.  Bringing forward r+ from EKR.

Comment 37

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3483fe77b6d

Comment 38

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b994417e2fc3 because I couldn't see anything else that might be the cause of the OS X make check crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=26280112&tree=Mozilla-Inbound

Comment 39

[Ethan Hugg [:ehugg]]

A quick count of M-I builds shows this happened 4/15 times and only on OSX 10.7 debug builds.  Investigating.

Comment 40

[Ethan Hugg [:ehugg]]

I set the sleep where we wait for data down to 1ms and I got this same symptom so I'm guessing this is timing related.  Here is a try with a longer wait:

https://tbpl.mozilla.org/?tree=Try&rev=16cb70cf9558

We ended up putting signaling_unittest behind a env var because of timing issues, hopefully we don't have to resort to that here.  

Other ideas and opinions welcome.

Comment 41

[Suhas]

5seconds should typically generate at least one RTCP packet per vanilla RTCP timing calculations
If not, either we are not generating RTCP packet or the test ended soon

Comment 42

[Ethan Hugg [:ehugg]]

Trying again with a longer timeout

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1defc12ddaf

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1defc12ddaf