Implement RTCP MUX in MediaPipeline

RESOLVED FIXED in mozilla26

Status

()

P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ekr, Assigned: ehugg)

Tracking

unspecified
mozilla26
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC], [blocking-webrtc-])

Attachments

(1 attachment, 12 obsolete attachments)

21.42 KB, patch
ehugg
: review+
Details | Diff | Splinter Review
Comment hidden (empty)

Updated

6 years ago
Whiteboard: [WebRTC]

Updated

6 years ago
Whiteboard: [WebRTC] → [WebRTC], [blocking-webrtc+]
call setup time optimization (strongly wanted!  but we'd pref on without it imho)
Needs coordinating bug in signaling
Priority: -- → P2
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc-]
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → ethanhugg
(Reporter)

Comment 3

6 years ago
This sounds exactly right.
(Reporter)

Comment 4

6 years ago
... 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

6 years ago
Bug 859971 does test RTCP Tx/Rx ..
(Reporter)

Comment 6

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

Comment 7

6 years ago
Agreed ..

Comment 8

6 years ago
Created attachment 745616 [details] [diff] [review]
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
Attachment #745616 - Flags: review?(ethanhugg)
Attachment #745616 - Flags: review?(ekr)

Comment 9

6 years ago
Created attachment 745617 [details] [diff] [review]
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.
Attachment #745617 - Flags: review?(ethanhugg)
Attachment #745617 - Flags: review?(ekr)
(Assignee)

Comment 10

6 years ago
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.
Attachment #745616 - Flags: review?(ethanhugg) → review+
(Assignee)

Comment 11

6 years ago
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.
Attachment #745617 - Flags: review?(ethanhugg) → review+

Updated

6 years ago
Attachment #745616 - Flags: review?(rjesup)

Updated

6 years ago
Attachment #745617 - Flags: review?(rjesup)

Updated

6 years ago
Attachment #745616 - Flags: review?(rjesup) → review+

Updated

6 years ago
Attachment #745617 - Flags: review?(rjesup) → review+

Updated

5 years ago
Blocks: 863306

Comment 12

5 years ago
Created attachment 768373 [details] [diff] [review]
Fix and Verify RTCP Mux Support

Updated

5 years ago
Attachment #745616 - Attachment is obsolete: true
Attachment #745616 - Flags: review?(ekr)

Comment 13

5 years ago
Created attachment 768376 [details] [diff] [review]
Fix and Verify RTCP Mux Support

Updated

5 years ago
Attachment #768373 - Attachment is obsolete: true

Comment 14

5 years ago
Created attachment 768377 [details] [diff] [review]
Fix and Verify RTCP Mux Support

Updated

5 years ago
Attachment #768376 - Attachment is obsolete: true

Comment 15

5 years ago
Created attachment 768380 [details] [diff] [review]
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+.
Attachment #745617 - Attachment is obsolete: true
Attachment #768377 - Attachment is obsolete: true
Attachment #745617 - Flags: review?(ekr)

Comment 16

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

Taking R+ from Ethan and Randell. Moving r? to Ekr.
Attachment #768380 - Flags: review?(ekr)
(Assignee)

Comment 17

5 years ago
Created attachment 779879 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline
(Assignee)

Updated

5 years ago
Attachment #768380 - Attachment is obsolete: true
Attachment #768380 - Flags: review?(ekr)
(Assignee)

Comment 18

5 years ago
Created attachment 779911 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline
(Assignee)

Updated

5 years ago
Attachment #779879 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
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.
Attachment #779911 - Flags: review?(ekr)
(Reporter)

Comment 20

5 years ago
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.
Attachment #779911 - Flags: review?(ekr) → review+
(Reporter)

Comment 21

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

Re-marking r? because I didn't review the unit test yet.
Attachment #779911 - Flags: review+ → review?(ekr)
(Reporter)

Comment 22

5 years ago
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.
Attachment #779911 - Flags: review?(ekr) → review-
(Assignee)

Comment 23

5 years ago
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.
(Reporter)

Comment 24

5 years ago
Actually now that I think about it, isn't same more likely by accident?
(Assignee)

Comment 25

5 years ago
OK, I will assert on same and mux on NULL.  Personally I am accident-prone enough to do either.
(Assignee)

Comment 26

5 years ago
Created attachment 786026 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline
(Assignee)

Updated

5 years ago
Attachment #779911 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #786026 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
Created attachment 786324 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline
(Assignee)

Comment 28

5 years ago
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.
Attachment #786324 - Flags: review?(ekr)
(Reporter)

Comment 29

5 years ago
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.
Attachment #786324 - Flags: review?(ekr) → review-
(Assignee)

Comment 30

5 years ago
>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.
(Assignee)

Comment 31

5 years ago
Created attachment 786724 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline
(Assignee)

Updated

5 years ago
Attachment #786324 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #786724 - Attachment is obsolete: true
(Assignee)

Comment 32

5 years ago
Created attachment 786734 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline
(Assignee)

Comment 33

5 years ago
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.
Attachment #786734 - Flags: review?(ekr)
(Reporter)

Comment 34

5 years ago
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.
Attachment #786734 - Flags: review?(ekr) → review+
(Assignee)

Comment 35

5 years ago
Created attachment 787063 [details] [diff] [review]
Implement RTCP MUX in MediaPipeline
(Assignee)

Updated

5 years ago
Attachment #786734 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
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.
Attachment #787063 - Flags: review+
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
(Assignee)

Comment 39

5 years ago
A quick count of M-I builds shows this happened 4/15 times and only on OSX 10.7 debug builds.  Investigating.
(Assignee)

Comment 40

5 years ago
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

5 years ago
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
(Assignee)

Comment 42

5 years ago
Trying again with a longer timeout

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1defc12ddaf
https://hg.mozilla.org/mozilla-central/rev/e1defc12ddaf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.