Closed Bug 786307 Opened 8 years ago Closed 7 years ago

Implement RTCP MUX in MediaPipeline

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: ekr, Assigned: ehugg)

References

Details

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

Attachments

(1 file, 12 obsolete files)

21.42 KB, patch
ehugg
: review+
Details | Diff | Splinter Review
No description provided.
Whiteboard: [WebRTC]
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-]
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: nobody → ethanhugg
This sounds exactly right.
... 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.
Bug 859971 does test RTCP Tx/Rx ..
As far as I can tell it does not check mux. And what is needed here is a pipeline test.
Agreed ..
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)
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)
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+
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+
Attachment #745616 - Flags: review?(rjesup)
Attachment #745617 - Flags: review?(rjesup)
Attachment #745616 - Flags: review?(rjesup) → review+
Attachment #745617 - Flags: review?(rjesup) → review+
Blocks: 863306
Attached patch Fix and Verify RTCP Mux Support (obsolete) — Splinter Review
Attachment #745616 - Attachment is obsolete: true
Attachment #745616 - Flags: review?(ekr)
Attached patch Fix and Verify RTCP Mux Support (obsolete) — Splinter Review
Attachment #768373 - Attachment is obsolete: true
Attached patch Fix and Verify RTCP Mux Support (obsolete) — Splinter Review
Attachment #768376 - Attachment is obsolete: true
Attached patch Fix and verify RTCP Mux support (obsolete) — Splinter Review
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 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)
Attachment #768380 - Attachment is obsolete: true
Attachment #768380 - Flags: review?(ekr)
Attachment #779879 - Attachment is obsolete: true
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)
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+
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)
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-
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.
Actually now that I think about it, isn't same more likely by accident?
OK, I will assert on same and mux on NULL.  Personally I am accident-prone enough to do either.
Attachment #779911 - Attachment is obsolete: true
Attachment #786026 - Attachment is obsolete: true
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)
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-
>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.
Attachment #786324 - Attachment is obsolete: true
Attachment #786724 - Attachment is obsolete: true
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)
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+
Attachment #786734 - Attachment is obsolete: true
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
A quick count of M-I builds shows this happened 4/15 times and only on OSX 10.7 debug builds.  Investigating.
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.
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
https://hg.mozilla.org/mozilla-central/rev/e1defc12ddaf
Status: NEW → RESOLVED
Closed: 7 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.