Implement basic RTPSender and RTPReceiver with RTCPeerConnection

RESOLVED FIXED in mozilla34

Status

()

Core
WebRTC
P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jesup, Assigned: jib)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=1], URL)

Attachments

(4 attachments, 8 obsolete attachments)

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
(Reporter)

Description

3 years ago
This is for the basic framework of RTPSender/RTPReceiver

In particular, enough to support dependent bugs in order to implement camera switching (without renegotiation).
(Reporter)

Updated

3 years ago
Blocks: 1032839
(Reporter)

Updated

3 years ago
Blocks: 1032840
(Reporter)

Updated

3 years ago
Blocks: 1033326
(Assignee)

Updated

3 years ago
(Assignee)

Updated

3 years ago
Assignee: nobody → jib
Depends on: 1033833
(Assignee)

Comment 1

3 years ago
Created attachment 8459017 [details] [diff] [review]
Part 1: RTCRtpSender|Receiver webidl-part
Attachment #8459017 - Flags: review?(rjesup)
Attachment #8459017 - Flags: review?(bugs)
(Reporter)

Updated

3 years ago
Attachment #8459017 - Flags: review?(rjesup) → review+
(Assignee)

Comment 2

3 years ago
Created attachment 8459253 [details] [diff] [review]
Part 1: RTCRtpSender|Receiver webidl-part (2)

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 3

3 years ago
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+
Blocks: 1017990
Target Milestone: --- → mozilla34
Priority: -- → P1
(Assignee)

Comment 4

3 years ago
Created attachment 8469864 [details] [diff] [review]
Part 2: addTrack/removeTrack on-top of existing implementation

Try - https://tbpl.mozilla.org/?tree=Try&rev=8802363535e3
Attachment #8469864 - Flags: review?(rjesup)
Attachment #8469864 - Flags: review?(bugs)

Comment 5

3 years ago
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+
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
Created attachment 8470287 [details] [diff] [review]
Part 3: rubberstamp RTCRtpSender, RTCRtpReceiver + MediaStreamTrackEvent

Try - https://tbpl.mozilla.org/?tree=Try&rev=aaf7cc96e298
Attachment #8470287 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8470287 - Flags: review?(bugs) → review+
(Reporter)

Comment 8

3 years ago
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-
(Assignee)

Comment 9

3 years ago
(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.
(Reporter)

Comment 10

3 years ago
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+
(Assignee)

Comment 11

3 years ago
(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.
(Assignee)

Comment 12

3 years ago
Created attachment 8472757 [details] [diff] [review]
Part 1: RTCRtpSender|Receiver webidl-part (3) r=smaug

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+
(Assignee)

Comment 13

3 years ago
Created attachment 8472759 [details] [diff] [review]
Part 2: addTrack/removeTrack on-top of existing implementation (2) r=smaug, jesup

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+
(Assignee)

Comment 14

3 years ago
Created attachment 8472760 [details] [diff] [review]
Part 3: rubberstamp RTCRtpSender, RTCRtpReceiver + MediaStreamTrackEvent (2) r=smaug

Updated commit msg.
Attachment #8470287 - Attachment is obsolete: true
Attachment #8472760 - Flags: review+
(Assignee)

Comment 15

3 years ago
Created attachment 8472762 [details] [diff] [review]
Part 4: Add/RemoveStream now call AddTrack/RemoveTrack internally

- 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)
(Assignee)

Comment 16

3 years ago
Created attachment 8472797 [details] [diff] [review]
Part 4: Add/RemoveStream now call AddTrack/RemoveTrack internally (2)

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)
(Assignee)

Comment 17

3 years ago
Created attachment 8472819 [details] [diff] [review]
Part 4: Add/RemoveStream now call AddTrack/RemoveTrack internally (3)

- 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)
(Assignee)

Comment 18

3 years ago
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)
(Assignee)

Comment 19

3 years ago
Created attachment 8473508 [details] [diff] [review]
Part 4: add/removeStream now implemented with addTrack/removeTrack (4)

- 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)
(Reporter)

Comment 20

3 years ago
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+
(Assignee)

Comment 21

3 years ago
(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.
(Assignee)

Comment 22

3 years ago
Created attachment 8473821 [details] [diff] [review]
Part 4: add/removeStream now implemented with addTrack/removeTrack (5) r=jesup

Updated commit msg. Green try in comment 19.
Attachment #8473508 - Attachment is obsolete: true
Attachment #8473821 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/880ae230025d
https://hg.mozilla.org/integration/mozilla-inbound/rev/79525105ef9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1edead1c5b2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/50e64b3d2d88
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/880ae230025d
https://hg.mozilla.org/mozilla-central/rev/79525105ef9a
https://hg.mozilla.org/mozilla-central/rev/1edead1c5b2d
https://hg.mozilla.org/mozilla-central/rev/50e64b3d2d88
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
(Assignee)

Comment 25

3 years ago
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.