Closed Bug 1032835 Opened 5 years ago Closed 5 years ago

Implement basic RTPSender and RTPReceiver with RTCPeerConnection

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jesup, Assigned: jib)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-needed, Whiteboard: [p=1])

Attachments

(4 files, 8 obsolete files)

10.62 KB, patch
jib
: review+
Details | Diff | Splinter Review
30.95 KB, patch
jib
: review+
Details | Diff | Splinter Review
4.52 KB, patch
jib
: review+
Details | Diff | Splinter Review
18.92 KB, patch
jib
: review+
Details | Diff | Splinter Review
This is for the basic framework of RTPSender/RTPReceiver

In particular, enough to support dependent bugs in order to implement camera switching (without renegotiation).
Blocks: 1032839
Blocks: 1032840
Blocks: 1033326
Assignee: nobody → jib
Depends on: 1033833
Attachment #8459017 - Flags: review?(rjesup)
Attachment #8459017 - Flags: review?(bugs)
Attachment #8459017 - Flags: review?(rjesup) → review+
A few more things to make the auto-generated bits work.
- JSImplemented skeleton.
- Added defaults to MediaStreamTrackEvent's dictionary per Bug 900904 comment 9.

Try - https://tbpl.mozilla.org/?tree=Try&rev=098658f85f8d
Attachment #8459017 - Attachment is obsolete: true
Attachment #8459017 - Flags: review?(bugs)
Attachment #8459253 - Flags: review?(bugs)
Comment on attachment 8459253 [details] [diff] [review]
Part 1: RTCRtpSender|Receiver webidl-part (2)

>+  RTCRtpSender addTrack(MediaStreamTrack track, DOMString streamId);
I assume streamId isn't anything global (so that pages can't refer to IDs in different pages)
Attachment #8459253 - Flags: review?(bugs) → review+
Target Milestone: --- → mozilla34
Priority: -- → P1
Comment on attachment 8469864 [details] [diff] [review]
Part 2: addTrack/removeTrack on-top of existing implementation

>+DOMMediaStream::HasTrack(const MediaStreamTrack& aTrack) const
>+{
>+  for (uint32_t i = 0; i < mTracks.Length(); ++i) {
>+    if (&*mTracks[i] == &aTrack) {
A bit odd looking.
Why not mTracks[i].get() == &aTrack

>+  addTrack: function(track) {
>+    if (arguments.length < 2) {
>+      throw new this._win.DOMError("", "addTrack needs at least 1 stream arg for now.");
>+    }
So couldn't you use webidl magic for this?
Something like
void addTrack(MediaStreamTrack track, MediaStream firstStream, MediaStream... otherStreams);


>+    if (arguments[1].currentTime === undefined) {
>+      throw new this._win.DOMError("", "invalid stream.");
>+    }
and then this could use firstStream.currentTime === ... and so on


I assume jesup will look more carefully media/*
Attachment #8469864 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #5)
> >+DOMMediaStream::HasTrack(const MediaStreamTrack& aTrack) const
> >+{
> >+  for (uint32_t i = 0; i < mTracks.Length(); ++i) {
> >+    if (&*mTracks[i] == &aTrack) {
> A bit odd looking.
> Why not mTracks[i].get() == &aTrack

Turns out this worked:

bool
DOMMediaStream::HasTrack(const MediaStreamTrack& aTrack) const
{
  return mTracks.Contains(&aTrack);
}

> So couldn't you use webidl magic for this?
> Something like
> void addTrack(MediaStreamTrack track, MediaStream firstStream,
> MediaStream... otherStreams);

Good idea, will do. Thanks.
Attachment #8470287 - Flags: review?(bugs) → review+
Comment on attachment 8469864 [details] [diff] [review]
Part 2: addTrack/removeTrack on-top of existing implementation

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

One overarching issue is that AddTrack() is only valid (for us) until CreateOffer, so at least we should throw warnings to the console if it's used after that.  (Similar for RemoveTrack, though we appear to not support that at all - could we support it until CreateOffer?)

I want to see it one more time

::: dom/media/PeerConnection.js
@@ -213,5 @@
> -  function appendStats(stats, report) {
> -    stats.forEach(function(stat) {
> -        report[stat.id] = stat;
> -      });
> -  }

Why is this being removed?

::: dom/media/tests/mochitest/pc.js
@@ +1544,5 @@
>  
>      if (side === 'local') {
> +      stream.getTracks().forEach(function(track) {
> +        this._pc.addTrack(track, stream);
> +      }.bind(this));

With this change, will anything be testing AddStream()?

Also, is this green on Try?
Attachment #8469864 - Flags: review?(rjesup) → review-
(In reply to Randell Jesup [:jesup] from comment #8)
> One overarching issue is that AddTrack() is only valid (for us) until
> CreateOffer, so at least we should throw warnings to the console if it's
> used after that.  (Similar for RemoveTrack, though we appear to not support
> that at all - could we support it until CreateOffer?)

These issues already existed with AddStream and RemoveStream, so I tried to avoid touching that by basically mirroring what AddStream and RemoveStream currently do (and as you note, we don't support RemoveStream at all at the moment).

Would it be cleaner to open a new bug for that? RemoveTrack would presumably need tests etc.

> ::: dom/media/PeerConnection.js
> @@ -213,5 @@
> > -  function appendStats(stats, report) {
> > -    stats.forEach(function(stat) {
> > -        report[stat.id] = stat;
> > -      });
> > -  }
> 
> Why is this being removed?

It's some unrelated unused code I ran across that should have been removed in Bug 998989 http://hg.mozilla.org/mozilla-central/rev/b336c6598bf1 

> ::: dom/media/tests/mochitest/pc.js
> @@ +1544,5 @@
> >  
> >      if (side === 'local') {
> > +      stream.getTracks().forEach(function(track) {
> > +        this._pc.addTrack(track, stream);
> > +      }.bind(this));
> 
> With this change, will anything be testing AddStream()?

No. I have an additional patch locally that implements AddStream using AddTrack, which would address this. Unfortunately it doesn't work with the signaling_unittests which use an opaque FakeMediaStream unaware of tracks. I could add a FakeMediaStreamTrack to address that. I'll look at that.

> Also, is this green on Try?

Yes, see comment 7.
Comment on attachment 8469864 [details] [diff] [review]
Part 2: addTrack/removeTrack on-top of existing implementation

r+ with followup filing for warnings on use-after-CreateOffer, and to land with a patch that restores testing for AddStream
Attachment #8469864 - Flags: review- → review+
(In reply to Randell Jesup [:jesup] from comment #10)
> r+ with followup filing for warnings on use-after-CreateOffer, and to land
> with a patch that restores testing for AddStream

Thanks, filed Bug 1053415, and patch is forthcoming.
Updated commit msg.

(In reply to Olli Pettay [:smaug] from comment #3)
> >+  RTCRtpSender addTrack(MediaStreamTrack track, DOMString streamId);
> I assume streamId isn't anything global (so that pages can't refer to IDs in
> different pages)

Turns out that was an early spec draft. I changed DOMString streamId to a MediaStream stream instance in Part 2 so we should be good.
Attachment #8459253 - Attachment is obsolete: true
Attachment #8472757 - Flags: review+
Incorporated feedback. Carrying forward r=smaug, jesup.

addStream testing will be restored in Part 4.
Attachment #8469864 - Attachment is obsolete: true
Attachment #8472759 - Flags: review+
- Removes near-double-code and instead implements AddStream using AddTrack and
  RemoveStream using RemoveTrack.
- Had to add Fake_MediaStreamTrack to make this work with unittest compile.
- Now tests both addStream and addTrack JS APIs.
Attachment #8472762 - Flags: review?(rjesup)
Unbreaks signaling_unittest.

Try - https://tbpl.mozilla.org/?tree=Try&rev=2fa83ca43638
Attachment #8472762 - Attachment is obsolete: true
Attachment #8472762 - Flags: review?(rjesup)
Attachment #8472797 - Flags: review?(rjesup)
- Compiles on Windows.
- Unbreaks pc.js test.

Try again - https://tbpl.mozilla.org/?tree=Try&rev=15ea76e99591
Attachment #8472797 - Attachment is obsolete: true
Attachment #8472797 - Flags: review?(rjesup)
Attachment #8472819 - Flags: review?(rjesup)
Comment on attachment 8472819 [details] [diff] [review]
Part 4: Add/RemoveStream now call AddTrack/RemoveTrack internally (3)

Hmm, I think I'm going to do this a bit differently and have addStream use addTrack as early as in the JS.

Reason: my tests are failing now with pc.js calling addStream for video, because calling addStream doesn't produce senders in getSenders, which it arguably should, even though it doesn't return them. Patch coming up.
Attachment #8472819 - Flags: review?(rjesup)
- Removed more code.
- JS addStream now calls JS addTrack, so getSenders() is populated.
- Mochitests and signaling_unittests work.

Try - https://tbpl.mozilla.org/?tree=Try&rev=6b05ca008e85
Attachment #8472819 - Attachment is obsolete: true
Attachment #8473508 - Flags: review?(rjesup)
Comment on attachment 8473508 [details] [diff] [review]
Part 4: add/removeStream now implemented with addTrack/removeTrack (4)

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -1546,5 @@
>        ((aTrack.AsAudioStreamTrack()? DOMMediaStream::HINT_CONTENTS_AUDIO : 0) |
>         (aTrack.AsVideoStreamTrack()? DOMMediaStream::HINT_CONTENTS_VIDEO : 0));
> -#else
> -  uint32_t hints = aMediaStream.GetHintContents();
> -#endif

This is no longer needed?  (Perhaps not...)

@@ -1622,5 @@
>         (aTrack.AsVideoStreamTrack()? DOMMediaStream::HINT_CONTENTS_VIDEO : 0));
> -#else
> -  uint32_t hints = aMediaStream.GetHintContents();
> -#endif
> -

I love getting rid of lots of MOZILLA_INTERNAL_API stuff
Attachment #8473508 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #20)
> > -#else
> > -  uint32_t hints = aMediaStream.GetHintContents();
> > -#endif
> 
> This is no longer needed?  (Perhaps not...)

Thankfully no, as this Patch 3 code was actually wrong: It would remove all tracks in the stream rather than the provided track, in the unit-tests only.

> I love getting rid of lots of MOZILLA_INTERNAL_API stuff

Me too. Unit-tests testing non-shipping code defeats the purpose.
Updated commit msg. Green try in comment 19.
Attachment #8473508 - Attachment is obsolete: true
Attachment #8473821 - Flags: review+
Until it lands in the spec, here's another source on the API https://www.w3.org/2011/04/webrtc/wiki/images/6/63/WebRTC_RTCSender-Receiver.pdf

Note that our RtpReceiver isn't really hooked up yet, as our main aim here was to support Bug 1032839.
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.