1130534 - There is no threadsafe way to attach/detach media pipelines from media engines while media is flowing

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Comment 1

[Byron Campen [:bwc]]

We need to be able to detach or attach either a MediaPipelineReceive or MediaPipelineTransmit from a MediaEngine at any time. Right now, the attachment is indirect via a WebrtcAudioConduit or WebrtcVideoConduit. The conduits are not safe to init or destroy while media is flowing, because we'll race on accesses to mOtherDirection, mVoiceEngine, mVideoEngine, and probably other stuff. It is not clear whether we can simply create both conduits from the get-go, whether we need them or not for a given m-line, and attach/detach MediaPipelines to them as needed.

Comment 2

[Byron Campen [:bwc]]

So, it looks possible to create both conduits from the outset, but we would need to alter how we route stuff like the routing for SendPacket:

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp#792

Note that this assumes that both conduits are always attached to a pipeline transport (mTransport), this may not be the case if their lifetime is made independent of the pipelines.

There may be other gotchas too. Will keep looking.

Comment 3

[Byron Campen [:bwc]]

What would happen if we moved away from having two conduits, to having just one, with the following properties:

1. Conduits would have mTransmitterTransport and mReceiverTransport, set by MediaPipelineTransmit and MediaPipelineReceive respectively.
2. RTP data would be passed to mTransmitterTransport, if set.
3. RTCP data would be passed to mReceiverTransport if set, otherwise mTransmitterTransport if set (I think this is functionally equivalent to what we do now, even though it isn't quite right for sender reports).
3a. Eventually it would be very nice to check whether the RTCP data is a sender or receiver report, and route it appropriately.
4. If/when an m-section is disabled, the associated conduit and media engine can be disposed of.
5. If/when a MediaPipelineTransmit goes away, the PipelineTransport will be detached on STS, and the conduit's |mTransmitterTransport| unset on the webrtc.org thread. Same thing goes for Receive.
6. If/when a MediaPipelineTransmit is created, it will attach a PipelineTransport on STS, and set the conduit's |mTransmitterTransport| on the webrtc.org thread. Same thing goes for Receive.
7. Creation/destruction of MediaPipelineReceiveVideo would result in the conduit's |mRenderer| being set and unset.

Comment 4

[Byron Campen [:bwc]]

Hmm, I guess there's no straightforward way to run an arbitrary command on a webrtc.org thread, so we may need some locking for setting/unsetting the transport and renderer. Probably no big deal.

Comment 5

[Byron Campen [:bwc]]

One thing that worries me is whether it will be easy/possible to update the codecs on a pre-existing conduit.

Comment 6

[Byron Campen [:bwc]]

Another potential gotcha. With video conduits, we do not always attach as an external renderer here:

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#469

Not sure whether we need to be able to set/unset this on the fly.

Comment 7

[Byron Campen [:bwc]]

Attachment: (WIP) Use a single bidirectional media conduit that MediaPipelines can attach/detach at will

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8561727 [details] [diff] [review]
(WIP) Use a single bidirectional media conduit that MediaPipelines can attach/detach at will

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

Seems to work for audio, but video is unhappy for some reason. Not sure why.

Comment 9

[Byron Campen [:bwc]]

Attachment: (WIP) Use a single bidirectional media conduit that MediaPipelines can attach/detach at will

Fix some crashes and deadlocks.

Comment 10

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

Comment on attachment 8562208 [details] [diff] [review]
(WIP) Use a single bidirectional media conduit that MediaPipelines can attach/detach at will

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

Definitely looks reasonable; went over it in detail and compared to bug 864654's old patch

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +815,2 @@
>  //WebRTC::RTP Callback Implementation
>  int WebrtcAudioConduit::SendPacket(int channel, const void* data, int len)

Add // Called on AudioGUM or MSG thread

@@ +841,4 @@
>    }
>  }
>  
>  int WebrtcAudioConduit::SendRTCPPacket(int channel, const void* data, int len)

Add // Called on WebRTC Process thread

@@ +842,5 @@
>  }
>  
>  int WebrtcAudioConduit::SendRTCPPacket(int channel, const void* data, int len)
>  {
>    CSFLogDebug(logTag,  "%s : channel %d", __FUNCTION__, channel);

Change to:  
CSFLogDebug(logTag,  "%s : channel %d , len %d, first rtcp = %u ",
            __FUNCTION__, channel, len, ((uint8_t *) data)[1]);

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1179,2 @@
>  //WebRTC::RTP Callback Implementation
>  int WebrtcVideoConduit::SendPacket(int channel, const void* data, int len)

Add // Called on MSG thread

@@ +1193,4 @@
>    }
>  }
>  
>  int WebrtcVideoConduit::SendRTCPPacket(int channel, const void* data, int len)

Add // Called on webrtc Process thread

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +136,5 @@
> +      conduit_->StopReceiving();
> +    } else {
> +      conduit_->SetTransmitterTransport(nullptr);
> +      conduit_->StopTransmitting();
> +    }

Should this be done before or after DetachMediaStream?

Comment 11

[Byron Campen [:bwc]]

Attachment: (WIP) Use a single bidirectional media conduit that MediaPipelines can attach/detach at will

Fixing test bustage.

Comment 12

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #10)
> Comment on attachment 8562208 [details] [diff] [review]
> (WIP) Use a single bidirectional media conduit that MediaPipelines can
> attach/detach at will
> 
> Review of attachment 8562208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Definitely looks reasonable; went over it in detail and compared to bug
> 864654's old patch
> 
> ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
> @@ +815,2 @@
> >  //WebRTC::RTP Callback Implementation
> >  int WebrtcAudioConduit::SendPacket(int channel, const void* data, int len)
> 
> Add // Called on AudioGUM or MSG thread
> 
> @@ +841,4 @@
> >    }
> >  }
> >  
> >  int WebrtcAudioConduit::SendRTCPPacket(int channel, const void* data, int len)
> 
> Add // Called on WebRTC Process thread
> 
> @@ +842,5 @@
> >  }
> >  
> >  int WebrtcAudioConduit::SendRTCPPacket(int channel, const void* data, int len)
> >  {
> >    CSFLogDebug(logTag,  "%s : channel %d", __FUNCTION__, channel);
> 
> Change to:  
> CSFLogDebug(logTag,  "%s : channel %d , len %d, first rtcp = %u ",
>             __FUNCTION__, channel, len, ((uint8_t *) data)[1]);
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +1179,2 @@
> >  //WebRTC::RTP Callback Implementation
> >  int WebrtcVideoConduit::SendPacket(int channel, const void* data, int len)
> 
> Add // Called on MSG thread
> 
> @@ +1193,4 @@
> >    }
> >  }
> >  
> >  int WebrtcVideoConduit::SendRTCPPacket(int channel, const void* data, int len)
> 
> Add // Called on webrtc Process thread
> 

   I would expect these two to be called on the same thread/s...

> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> @@ +136,5 @@
> > +      conduit_->StopReceiving();
> > +    } else {
> > +      conduit_->SetTransmitterTransport(nullptr);
> > +      conduit_->StopTransmitting();
> > +    }
> 
> Should this be done before or after DetachMediaStream?

   I think before. I've changed it.

Comment 13

[Byron Campen [:bwc]]

Attachment: (WIP) Use a single bidirectional media conduit that MediaPipelines can attach/detach at will

Incorporate feedback

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8563164 [details] [diff] [review]
(WIP) Use a single bidirectional media conduit that MediaPipelines can attach/detach at will

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

For some reason, mozreview won't let me put just this patch up for review, it insists on grabbing the parent changeset (part 2 of the renegotiation bug) no matter what revset syntax I use (but not the parent's parent, which is part 1 of the renegotiation bug).

Comment 15

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

> > >  //WebRTC::RTP Callback Implementation
> > >  int WebrtcVideoConduit::SendPacket(int channel, const void* data, int len)
> > 
> > Add // Called on MSG thread
> > 

> > >  int WebrtcVideoConduit::SendRTCPPacket(int channel, const void* data, int len)
> > 
> > Add // Called on webrtc Process thread
> > 
> 
>    I would expect these two to be called on the same thread/s...

RTCP (at least periodic RTCP) is generated off the webrtc Process timer thread; in reality I suspect RTCP may also be generated directly on other threads such as MSG for things like NACK.  Might be worth checking (breakpoint it or print the tread ID), so we can annotate it correctly.

Comment 16

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

Comment on attachment 8563164 [details] [diff] [review]
(WIP) Use a single bidirectional media conduit that MediaPipelines can attach/detach at will

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

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +103,5 @@
> +  mPtrVoEVideoSync = nullptr;
> +  mPtrVoERTP_RTCP = nullptr;
> +  mPtrRTP = nullptr;
> +
> +  // only one opener can call Delete.  Have it be the last to close.

This comment might no longer be relevant

@@ +465,5 @@
>      CSFLogError(logTag, "%s Setting Receive Codec Failed ", __FUNCTION__);
>      return kMediaConduitInvalidReceiveCodec;
>    }
>  
> +  condError = StartReceiving();

Maybe keep the comment

@@ +761,5 @@
> +MediaConduitErrorCode
> +WebrtcAudioConduit::StopReceiving()
> +{
> +  // Are we receiving already? If so, stop receiving and playout
> +  // since we can't apply new recv codec when the engine is playing.

This comment should likely move back to where this was copied from

@@ +842,4 @@
>    }
>  }
>  
> +// Called on WebRTC Process thread

See VideoConduit.cpp discussion; change to "called on webrtc Process thread and perhaps on other threads".  Audio is mostly just timed RR/SR RTCP's.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1199,4 @@
>    }
>  }
>  
> +// Called on webrtc Process thread

I realize I said Process thread, but I think this may also be called from other WebRTC code (NACK, etc), which means MSG or other random WebRTC threads.  So "called from multiple threads including Process"

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +1,2 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
> + * mNumStreams = numStreams;

remove....

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ +360,5 @@
> +                          " to level " << level <<
> +                          ". This requires re-creating the MediaPipeline.");
> +    // Since we do not support changing the conduit on a pre-existing
> +    // MediaPipeline (the conduit is where stuff like echo cancellation and RTCP
> +    // happens, which is determined by what tracks appear at the same level).

echo cancellation no longer is done at the conduit level; that's done in getUserMedia now.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +1429,5 @@
>      }
>  
>  
>      // Check feedback method for video
> +    if (flags & PIPELINE_VIDEO && !(flags & PIPELINE_SEND)) {

Add parens around bit ops!

Comment 17

[Byron Campen [:bwc]]

Have incorporated feedback, and try looks great:

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

Comment 18

[Byron Campen [:bwc]]

Have done some testing on TSAN, and not seeing any new races or lock-order problems.

Comment 19

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95b94b9901f1

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95b94b9901f1