1032839 - Implement Track replace for RTPSender

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

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

Works with the basic framework of RTPSender/RTPReceiver in bug 1032835

This will allow camera-switching without renegotiation

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: replaceTrack API

Adds the API skeleton and a test to demonstrate use and ensure the callbacks work.

Try - https://tbpl.mozilla.org/?tree=Try&rev=ecd4c43031b0

Comment 2

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

Comment on attachment 8471994 [details] [diff] [review]
Part 1: replaceTrack API

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1661,5 @@
> +  // TODO: Do an aStream.HasTrack() check on both track args someday.
> +  //
> +  // Since MediaStreams are currently limited to one track of each kind, we
> +  // allow replacement with an outside track not already in the same stream.
> +  // Since a track may be replaced more than once we can check neither arg.

rephrase; a little confusing

@@ +1666,5 @@
> +  //
> +  // This works as sync happens receiver-side and timestamps are tied to capture
> +
> +  // Insert magic here.
> +  bool success = true;

i.e. patch 2 with the backend/pipeline code which I'll write.

Note: the backend must keep itself linked to any MediaStreams it's using any tracks from, and handle the combination as if they were one.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +483,5 @@
> +NS_IMETHODIMP
> +TestObserver::OnReplaceTrackError(uint32_t code, const char *message, ER&)
> +{
> +  return NS_OK;
> +}

I presume more is planned for here?  Either flash this out in another patch, or file a followup

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Randell Jesup [:jesup] from comment 2)
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +483,5 @@
> > +NS_IMETHODIMP
> > +TestObserver::OnReplaceTrackError(uint32_t code, const char *message, ER&)
> > +{
> > +  return NS_OK;
> > +}
> 
> I presume more is planned for here?  Either flash this out in another patch,
> or file a followup

I have absolutely nothing planned. This boilerplate is needed to make the code in PeerConnectionImpl.cpp compile without breaking it up with #ifdef MOZ_INTERNAL_API (it satisfies pure virtual methods in FakePCObserver.h)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8471994 [details] [diff] [review]
Part 1: replaceTrack API

Do we have a bug filed to convert  RTCError to something sane.
(Use of __exposedProps__ is scary.)

+  void replaceTrack(MediaStreamTrack track,
+                    VoidFunction successCallback,
+                    RTCPeerConnectionErrorCallback failureCallback);
Does that mean the existing .track should be replaced with 'track'?
I think 'track' should be renamed, perhaps 'newTrack'


This is clearly missing some additional patch, but apparently that will be 
media/webrtc/* thingie.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Do we have a bug filed to convert  RTCError to something sane.
> (Use of __exposedProps__ is scary.)

I've opened Bug 1053407.

> +  void replaceTrack(MediaStreamTrack track,
> +                    VoidFunction successCallback,
> +                    RTCPeerConnectionErrorCallback failureCallback);
> Does that mean the existing .track should be replaced with 'track'?
> I think 'track' should be renamed, perhaps 'newTrack'

Thanks, I'm calling it withTrack elsewhere, but missed this one.

> This is clearly missing some additional patch, but apparently that will be 
> media/webrtc/* thingie.

Yes that's Part 2 which jesup is writing.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: replaceTrack API (2) r=smaug, r=jesup

Incorporated feedback and unbitrotted. Carrying forward r=smaug, jesup.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: replaceTrack API (3) r=smaug, r=jesup

Unbitrot.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: replaceTrack API (4) r=smaug, r=jesup

Unbitrot.

Comment 9

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

Attachment: WIP patch to implement ReplaceTrack backend

Comment 10

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

Attachment: pc_test.html

Hacky test for ReplaceTrack.

Switches between real and fake video for pc1 when you toggle the "Use Fake Video for one stream" (which I advise toggling once before the call to get it in the right state).

Comment 11

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

Attachment: pc_test.html

Updated test; tries to replace both audio and video; only audio gets replaced.

Comment 12

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

Attachment: WIP backend part 2

Comment 13

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

Attachment: WIP patch to implement ReplaceTrack backend

reupload in case I modified it

Comment 14

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

Attachment: pc_test.html

Better test..  Change senders1 to senders2 in the replacetracks stuff to swap the other pane

Comment 15

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

Attachment: Backend support for PeerConnection ReplaceTrack()

tested and works well, including under ASAN

Comment 16

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

Attachment: Deal with PCImpl's ReplaceStream returning synchronously to PeerConnection.js

Comment 17

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

Attachment: pc_test.html

Updated test.  We can't rely on the order of tracks in getTracks(); key off type of track.

Click the "use fake video for on stream" button to swap from fake to real (video and audio).

Comment 18

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

Comment on attachment 8476584 [details] [diff] [review]
Backend support for PeerConnection ReplaceTrack()

r? jib for upper-level pieces; r? padenot for MSG bits

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8476584 [details] [diff] [review]
Backend support for PeerConnection ReplaceTrack()

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

lgtm for the parts I know, with fix to compile, but I'm unsure about the locking and curious what Paul thinks.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +631,5 @@
> +
> +  return MediaPipeline::Init();
> +}
> +
> +void MediaPipelineTransmit::AttachToTrack(TrackID track_id) {

Seems like AttachToTrack basically re-inits the pipeline on main-thread while things are running. Is that thread-safe?

@@ +701,5 @@
> +nsresult MediaPipelineTransmit::ReplaceTrack(DOMMediaStream *domstream,
> +                                             TrackID track_id) {
> +  // MainThread, checked in calls we make
> +  MOZ_MTLOG(ML_DEBUG, "Reattaching pipeline to stream "
> +            << static_cast<void *>(domstream->GetStream()) << " conduit type=" <<

Don't need static_cast with <<

@@ +990,5 @@
> +  } else if (conduit_->type() !=
> +             (media.GetType() == MediaSegment::AUDIO ? MediaSessionConduit::AUDIO :
> +                                                       MediaSessionConduit::VIDEO)) {
> +    // Ignore data in case we have a muxed stream
> +    return;

I find this if-else-else with returns mixed in hard to follow.

Could this middle "return"-else be turned into an if-return inside the last else (ahead of the mutex)? That would also let you flip the remaining if-else upside down, if that reads better.

@@ +995,5 @@
> +  } else {
> +    // Don't lock during normal media flow except on first sample
> +    MutexAutoLock lock(mMutex);
> +    track_id_ = track_id_external_ = tid;
> +  }

This auto-lock ends here, is that what you intended?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +392,5 @@
>  
>    // Initialize (stuff here may fail)
>    virtual nsresult Init() MOZ_OVERRIDE;
>  
> +  virtual void AttachToTrack();

virtual void AttachToTrack(TrackID track_id);

@@ +466,5 @@
>      void SetActive(bool active) { active_ = active; }
>      void SetEnabled(bool enabled) { enabled_ = enabled; }
> +    TrackID trackid() {
> +      MutexAutoLock lock(mMutex);
> +      return track_id_external_;

What does this lock accomplish? track_id_external_ may have changed by the time the caller sees the return value. Is that a problem?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1597,5 @@
> +  TrackID withID = aWithTrack.GetTrackID();
> +
> +  bool success = false;
> +  for(uint32_t i = 0; i < media()->LocalStreamsLength(); ++i) {
> +    LocalSourceStreamInfo *info = media()->GetLocalStream(i);

or auto *

@@ +1599,5 @@
> +  bool success = false;
> +  for(uint32_t i = 0; i < media()->LocalStreamsLength(); ++i) {
> +    LocalSourceStreamInfo *info = media()->GetLocalStream(i);
> +    // XXX use type instead of TrackID - bug 1056650
> +    int pipeline = info->HasTrackType(&aStream, !!(aThisTrack.AsVideoStreamTrack()));

Excessive parens.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +104,5 @@
>  
> +#if 0
> +// XXX  bug 1056652 makes this not very useful for transmit streams
> +// NOTE: index is != the trackid in the MediaStream
> +int LocalSourceStreamInfo::HasTrack(DOMMediaStream* aStream, TrackID aTrack)

Name seems problematic since if(HasTrack(track)) yields true for not found.

Maybe IndexOfTrack?

Comment 20

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #19)
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +631,5 @@
> > +
> > +  return MediaPipeline::Init();
> > +}
> > +
> > +void MediaPipelineTransmit::AttachToTrack(TrackID track_id) {
> 
> Seems like AttachToTrack basically re-inits the pipeline on main-thread
> while things are running. Is that thread-safe?

It un-inited it first by DetachMediaStream() (which already purposely runs on MainThread, and wasn't changed).
Since this all works by listener callbacks, and the listener changes are proxied to MSG thread as commands, yes it's safe.

> @@ +701,5 @@
> > +nsresult MediaPipelineTransmit::ReplaceTrack(DOMMediaStream *domstream,
> > +                                             TrackID track_id) {
> > +  // MainThread, checked in calls we make
> > +  MOZ_MTLOG(ML_DEBUG, "Reattaching pipeline to stream "
> > +            << static_cast<void *>(domstream->GetStream()) << " conduit type=" <<
> 
> Don't need static_cast with <<

Perhaps not; this was cut-and-pasted from other logs here.

> 
> @@ +990,5 @@
> > +  } else if (conduit_->type() !=
> > +             (media.GetType() == MediaSegment::AUDIO ? MediaSessionConduit::AUDIO :
> > +                                                       MediaSessionConduit::VIDEO)) {
> > +    // Ignore data in case we have a muxed stream
> > +    return;
> 
> I find this if-else-else with returns mixed in hard to follow.
> 
> Could this middle "return"-else be turned into an if-return inside the last
> else (ahead of the mutex)? That would also let you flip the remaining
> if-else upside down, if that reads better.

This logically flows better; I can add a comment explaining what's going on

> 
> @@ +995,5 @@
> > +  } else {
> > +    // Don't lock during normal media flow except on first sample
> > +    MutexAutoLock lock(mMutex);
> > +    track_id_ = track_id_external_ = tid;
> > +  }
> 
> This auto-lock ends here, is that what you intended?

Very much so; the lock only exists so we can see a version of the track_id from other threads (and have two copies so we don't need to lock when reading it on every 10ms media poll to avoid a TSAN violation.

> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> @@ +392,5 @@
> >  
> >    // Initialize (stuff here may fail)
> >    virtual nsresult Init() MOZ_OVERRIDE;
> >  
> > +  virtual void AttachToTrack();
> 
> virtual void AttachToTrack(TrackID track_id);

Huh; odd that compiled.

> @@ +466,5 @@
> >      void SetActive(bool active) { active_ = active; }
> >      void SetEnabled(bool enabled) { enabled_ = enabled; }
> > +    TrackID trackid() {
> > +      MutexAutoLock lock(mMutex);
> > +      return track_id_external_;
> 
> What does this lock accomplish? track_id_external_ may have changed by the
> time the caller sees the return value. Is that a problem?

This will only change once (or once initially (from TRACK_INVALID) and once per replaceTrack).
The lock is to avoid TSAN issues.  The value trackid for a transmit track will not generally be known when the pipeline is created until the first media sample is received.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1597,5 @@
> > +  TrackID withID = aWithTrack.GetTrackID();
> > +
> > +  bool success = false;
> > +  for(uint32_t i = 0; i < media()->LocalStreamsLength(); ++i) {
> > +    LocalSourceStreamInfo *info = media()->GetLocalStream(i);
> 
> or auto *
> 
> @@ +1599,5 @@
> > +  bool success = false;
> > +  for(uint32_t i = 0; i < media()->LocalStreamsLength(); ++i) {
> > +    LocalSourceStreamInfo *info = media()->GetLocalStream(i);
> > +    // XXX use type instead of TrackID - bug 1056650
> > +    int pipeline = info->HasTrackType(&aStream, !!(aThisTrack.AsVideoStreamTrack()));
> 
> Excessive parens.

Habit; I prefer to paren ! to make it clear what is being referred to and avoid operation precedence surprises.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +104,5 @@
> >  
> > +#if 0
> > +// XXX  bug 1056652 makes this not very useful for transmit streams
> > +// NOTE: index is != the trackid in the MediaStream
> > +int LocalSourceStreamInfo::HasTrack(DOMMediaStream* aStream, TrackID aTrack)
> 
> Name seems problematic since if(HasTrack(track)) yields true for not found.
> 
> Maybe IndexOfTrack?

Sure.  (It was a bool initially...)

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8476584 [details] [diff] [review]
Backend support for PeerConnection ReplaceTrack()

(In reply to Randell Jesup [:jesup] from comment #20)
> > > +  virtual void AttachToTrack();
> > 
> > virtual void AttachToTrack(TrackID track_id);
> 
> Huh; odd that compiled.

It didn't for me.

> The lock is to avoid TSAN issues.  The value trackid for a transmit track
> will not generally be known when the pipeline is created until the first
> media sample is received.

So if someone tries to replace a track immediately it might fail? Maybe that's OK.

> > > +    int pipeline = info->HasTrackType(&aStream, !!(aThisTrack.AsVideoStreamTrack()));
> > 
> > Excessive parens.
> 
> Habit; I prefer to paren ! to make it clear what is being referred to and
> avoid operation precedence surprises.

I prefer to let the compiler do the lifting. (!!aThisTrack).AsVideoStreamTrack() is unlikely to compile into anything (barring some extreme user-defined boolean type).

Comment 22

[Paul Adenot (:padenot)]

Comment on attachment 8476584 [details] [diff] [review]
Backend support for PeerConnection ReplaceTrack()

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

r+ on the msg parts.

::: media/webrtc/signaling/test/FakeMediaStreams.h
@@ +216,5 @@
>  {
>  public:
>    Fake_MediaStreamTrack(bool aIsVideo) : mIsVideo (aIsVideo) {}
> +  mozilla::TrackID GetTrackID() { return mIsVideo ? 1 : 0; }
> +  Fake_DOMMediaStream *GetStream() { return nullptr; }

nit: * near the type.

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8476584 [details] [diff] [review]
Backend support for PeerConnection ReplaceTrack()

I r+'ed this already in comment 21.

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Is there a way to modify test_peerConnection_replaceTrack.html to verify that it worked?

Comment 25

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

Note that I have a test on http://mozilla.github.com/webrtc-landing/pc_test_swap.html which is meant to prove it works on B2G, which has a limitation of only one camera active at a time.  To handle this, we must switch to disabled (black) fake video, stop the old camera, start the new one, and then swap tracks again.  Note that currently due to a camera issue(?), the last step succeeds, but the camera stops providing frames on b2g.


On desktop if you can open multiple cameras at once, you can direct-swap sources.

Comment 26

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

https://tbpl.mozilla.org/?tree=Try&rev=5222c3da2095

Comment 27

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

Attachment: Update replaceTrack test to check more media

worked even better with bug 1054706, which actually checks media flowing

Comment 28

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

Comment on attachment 8478456 [details] [diff] [review]
Update replaceTrack test to check more media

Checks before and after ReplaceTrack

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8478456 [details] [diff] [review]
Update replaceTrack test to check more media

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

r=me, but why not duplicate the existing flow-test instead of rewriting it?

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html
@@ +28,5 @@
>    runNetworkTest(function () {
>      test = new PeerConnectionTest();
>      test.setMediaConstraints([{video: true}], [{video: true}]);
>      test.chain.removeAfter("PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT");
> +    test.chain.insertAfter("PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT", [[

E.g.

var flowtest = test.chain.remove("PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT");
 test.chain.append(flowtest);
 test.chain.append("PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT", [[/*...*/]]);
 test.chain.append(flowtest);

Less code duplication, and avoids breaking if the flow-test ever changes.

@@ +53,5 @@
>          });
>        }
> +    ],
> +    [
> +      'PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT',

And remove this entry.

Comment 30

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=474e987213bb

https://hg.mozilla.org/integration/mozilla-inbound/rev/71c5e8aa91dd

Comment 31

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

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71c5e8aa91dd
https://hg.mozilla.org/mozilla-central/rev/e09f215b65f8

Comment 33

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 5: update RtpSender.track when replaceTrack succeeds

Comment 34

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

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

Comment 35

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 6: test that addTrack returns sender

Two more tests around senders.

Comment 36

[Nils Ohlmeier [:drno]]

Comment on attachment 8480089 [details] [diff] [review]
Part 6: test that addTrack returns sender

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

The small improvements mentioned below would be nice to have.

But otherwise LGTM.

::: dom/media/tests/mochitest/pc.js
@@ +1538,5 @@
>     * @param {string} side
>     *        The location the stream is coming from ('local' or 'remote')
>     */
>    attachMedia : function PCW_attachMedia(stream, type, side) {
> +    function isSenderOfTrack(sender) {

Doesn't the find callback take three args?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find

@@ +1557,4 @@
>        } else {
>          stream.getTracks().forEach(function(track) {
> +          var sender = this._pc.addTrack(track, stream);
> +          is(sender.track, track, "addTrack returns sender");

The message suggests that we verified that we got a RTCRtpSender object.
I think we should verify typeof() with the above message. And then change the message in this is() to "sender object has expected track" or something like that.

@@ +1561,3 @@
>          }.bind(this));
>        }
>      }

I like how PC.getSenders() gets tests here. But couldn't we also do a verification of PC.getReceivers() here in an else branch here?

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 38

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Nils Ohlmeier [:drno] from comment #36)
> > +    function isSenderOfTrack(sender) {
> 
> Doesn't the find callback take three args?

Yes but isSenderOfTrack doesn't use the index or array argument so they don't need to be specified.

> > +          var sender = this._pc.addTrack(track, stream);
> > +          is(sender.track, track, "addTrack returns sender");
> 
> The message suggests that we verified that we got a RTCRtpSender object.
> I think we should verify typeof() with the above message. And then change
> the message in this is() to "sender object has expected track" or something
> like that.

typeof would just yield "object" which we're implicitly testing by testing sender.track == track. This seems to be best practice for testing the type of a browser-object: http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#801

We could test it twice, first against === undefined and then == track to produce the two more-detailed messages you mention, but that's arguably redundant since the latter ensures both.

> I like how PC.getSenders() gets tests here. But couldn't we also do a
> verification of PC.getReceivers() here in an else branch here?

Good idea once receivers get hooked up (they're not at the moment, maybe they should be).

Green try - https://tbpl.mozilla.org/?tree=Try&rev=f66b44be551e

Comment 39

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry for re-opening. Piggy-backing a patch here (Part 6) seemed more reasonable while it was still open.

Comment 40

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6a176e89ec

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b6a176e89ec

Comment 42

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

We've proposed the API here http://lists.w3.org/Archives/Public/public-webrtc/2014Sep/0001.html

Comment 43

[Eric Shepherd [:sheppy]]

Now documented: https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSender/replaceTrack

It'll get fleshed out a bit more as the surrounding content is completed but this is done in essence.