Implement Track replace for RTPSender

RESOLVED FIXED in mozilla34

Status

()

Core
WebRTC
P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jesup, Assigned: jib)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla34
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=2], URL)

Attachments

(7 attachments, 9 obsolete attachments)

17.34 KB, patch
jib
: review+
Details | Diff | Splinter Review
30.06 KB, patch
jib
: review+
padenot
: review+
Details | Diff | Splinter Review
1.88 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.11 KB, text/html
Details
2.32 KB, patch
jib
: review+
Details | Diff | Splinter Review
4.14 KB, patch
jesup
: review+
Details | Diff | Splinter Review
1.80 KB, patch
drno
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

This will allow camera-switching without renegotiation
(Reporter)

Updated

4 years ago
Blocks: 1033326
Whiteboard: [p=2]
Blocks: 1017990
Target Milestone: --- → mozilla34
Assignee: nobody → jib
Priority: -- → P1
Created attachment 8471994 [details] [diff] [review]
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
Attachment #8471994 - Flags: review?(rjesup)
Attachment #8471994 - Flags: review?(bugs)
(Reporter)

Comment 2

3 years ago
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
Attachment #8471994 - Flags: review?(rjesup) → review+
(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 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.
Attachment #8471994 - Flags: review?(bugs) → review+
(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.
Created attachment 8472764 [details] [diff] [review]
Part 1: replaceTrack API (2) r=smaug, r=jesup

Incorporated feedback and unbitrotted. Carrying forward r=smaug, jesup.
Attachment #8471994 - Attachment is obsolete: true
Attachment #8472764 - Flags: review+
Created attachment 8472820 [details] [diff] [review]
Part 1: replaceTrack API (3) r=smaug, r=jesup

Unbitrot.
Attachment #8472764 - Attachment is obsolete: true
Attachment #8472820 - Flags: review+
Created attachment 8473513 [details] [diff] [review]
Part 1: replaceTrack API (4) r=smaug, r=jesup

Unbitrot.
Attachment #8472820 - Attachment is obsolete: true
Attachment #8473513 - Flags: review+
(Reporter)

Comment 9

3 years ago
Created attachment 8475474 [details] [diff] [review]
WIP patch to implement ReplaceTrack backend
(Reporter)

Comment 10

3 years ago
Created attachment 8475475 [details]
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).
(Reporter)

Comment 11

3 years ago
Created attachment 8476405 [details]
pc_test.html

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

Comment 12

3 years ago
Created attachment 8476409 [details] [diff] [review]
WIP backend part 2
(Reporter)

Comment 13

3 years ago
Created attachment 8476413 [details] [diff] [review]
WIP patch to implement ReplaceTrack backend

reupload in case I modified it
(Reporter)

Updated

3 years ago
Attachment #8475474 - Attachment is obsolete: true
(Reporter)

Comment 14

3 years ago
Created attachment 8476435 [details]
pc_test.html

Better test..  Change senders1 to senders2 in the replacetracks stuff to swap the other pane
Attachment #8475475 - Attachment is obsolete: true
Attachment #8476405 - Attachment is obsolete: true
(Reporter)

Comment 15

3 years ago
Created attachment 8476584 [details] [diff] [review]
Backend support for PeerConnection ReplaceTrack()

tested and works well, including under ASAN
(Reporter)

Updated

3 years ago
Attachment #8476413 - Attachment is obsolete: true
(Reporter)

Comment 16

3 years ago
Created attachment 8476585 [details] [diff] [review]
Deal with PCImpl's ReplaceStream returning synchronously to PeerConnection.js
(Reporter)

Comment 17

3 years ago
Created attachment 8476588 [details]
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).
Attachment #8476409 - Attachment is obsolete: true
Attachment #8476435 - Attachment is obsolete: true
(Reporter)

Comment 18

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

r? jib for upper-level pieces; r? padenot for MSG bits
Attachment #8476584 - Flags: review?(paul)
Attachment #8476584 - Flags: review?(jib)
(Reporter)

Updated

3 years ago
Attachment #8476585 - Flags: review?(bugs)
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?
Blocks: 1055378
(Reporter)

Comment 20

3 years ago
(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 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).
Attachment #8476584 - Flags: review?(jib) → review+
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.
Attachment #8476584 - Flags: review?(paul)
Attachment #8476584 - Flags: review?(jib)
Attachment #8476584 - Flags: review+
Comment on attachment 8476584 [details] [diff] [review]
Backend support for PeerConnection ReplaceTrack()

I r+'ed this already in comment 21.
Attachment #8476584 - Flags: review?(jib) → review+
Attachment #8476585 - Flags: review?(bugs) → review+
Is there a way to modify test_peerConnection_replaceTrack.html to verify that it worked?
Flags: needinfo?(rjesup)
(Reporter)

Updated

3 years ago
Depends on: 1057955
(Reporter)

Comment 25

3 years ago
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.
Flags: needinfo?(rjesup)
(Reporter)

Comment 26

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=5222c3da2095
(Reporter)

Comment 27

3 years ago
Created attachment 8478456 [details] [diff] [review]
Update replaceTrack test to check more media

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

Comment 28

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

Checks before and after ReplaceTrack
Attachment #8478456 - Flags: review?(jib)
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.
Attachment #8478456 - Flags: review?(jib) → review+
(Reporter)

Comment 30

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

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

Comment 31

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e09f215b65f8
Whiteboard: [p=2] → [p=2][leave-open]
https://hg.mozilla.org/mozilla-central/rev/71c5e8aa91dd
https://hg.mozilla.org/mozilla-central/rev/e09f215b65f8
Flags: in-testsuite+
Created attachment 8479447 [details] [diff] [review]
Part 5: update RtpSender.track when replaceTrack succeeds
Attachment #8479447 - Flags: review?(rjesup)
(Reporter)

Updated

3 years ago
Attachment #8479447 - Flags: review?(rjesup) → review+
Keywords: checkin-needed
(Reporter)

Comment 34

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b682c9e52fee
Keywords: checkin-needed
Whiteboard: [p=2][leave-open] → [p=2]
Created attachment 8480089 [details] [diff] [review]
Part 6: test that addTrack returns sender

Two more tests around senders.
Attachment #8480089 - Flags: review?(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?
Attachment #8480089 - Flags: review?(drno) → review+
https://hg.mozilla.org/mozilla-central/rev/b682c9e52fee
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(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
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Sorry for re-opening. Piggy-backing a patch here (Part 6) seemed more reasonable while it was still open.
(Reporter)

Comment 40

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6a176e89ec
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8b6a176e89ec
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
We've proposed the API here http://lists.w3.org/Archives/Public/public-webrtc/2014Sep/0001.html
Depends on: 1172397
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.