Implement PeerConnection object

VERIFIED FIXED in mozilla18

Status

()

Core
WebRTC
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: jesup, Assigned: anant)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC], [blocking-webrtc+])

Attachments

(2 attachments, 10 obsolete attachments)

33.50 KB, patch
jst
: review+
Details | Diff | Splinter Review
36.02 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Create initial subset of the W3C WebRTC interfaces to implement.  This is not necessarily the implementation, but a decision on the minimum needed and the rough API.

We can start with the basics needed to create a connection/call, and anticipate what we think the likely API will look like (such as for getUserMedia())

-> (tentatively) Anant, based on WebRTC Eng Planning spreadsheet
(Reporter)

Comment 1

6 years ago
I'm working on an initial peerconnection implementation, partly based on a few bits from ikran, though that will likely get replaced or recoded.  

Initial impl will not include ICE, since that lives in jingle and needs to get factored out somehow.

Most of the media and signaling code will run on a separate thread; one per peerconnection.  (WebRTC media code create a number of additional threads when a call is up)
(Reporter)

Updated

6 years ago
Assignee: anant → rjesup
Status: NEW → ASSIGNED

Updated

5 years ago
Duplicate of this bug: 720089
(Reporter)

Updated

5 years ago
Summary: Create initial subset of the W3C WebRTC interfaces to implement → Implement PeerConnection object
(Reporter)

Updated

5 years ago
Depends on: 711628
(Reporter)

Updated

5 years ago
Depends on: 694808
(Reporter)

Updated

5 years ago
Depends on: 729541
(Reporter)

Updated

5 years ago
Depends on: 729544
(Reporter)

Comment 3

5 years ago
In addition to the dependent bugs, we need to implement the configuration that's eventually decided on in W3C; we need to implement the "do signaling on Stable State" stuff (remove Connect()), make sure all the events get fired properly (and forwarded from the backend), make sure all the states are forwarded from the underlying code, etc.

As we extract the implementation from the libjingle backend, we will need to rewrite portions of the mozillaPeerConnection code I currently have.
(Reporter)

Updated

5 years ago
Blocks: 731429
(Reporter)

Comment 4

5 years ago
Created attachment 601633 [details] [diff] [review]
Base unfinished peerconnection patch
(Reporter)

Comment 5

5 years ago
Created attachment 601634 [details] [diff] [review]
untested mediastream array code
(Reporter)

Comment 6

5 years ago
Created attachment 601636 [details] [diff] [review]
modify peerconnection code to build on top of nsDOMMediaStreams - compiles and works a little
(Reporter)

Comment 7

5 years ago
Created attachment 601757 [details] [diff] [review]
folded unfinished peerconnection patch on top of nsDOMMediaStreams - compiles and works a little
(Reporter)

Updated

5 years ago
Attachment #601633 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #601634 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #601636 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
Replaced the 3 patches with a single 'hg qfold'ed patch, since the distinction between them wasn't useful (and the MediaStream Array patch wasn't cleanly separable without first folding it and then refactoring it out properly, which I may do later).
(Reporter)

Comment 9

5 years ago
Created attachment 602372 [details] [diff] [review]
folded unfinished peerconnection patch on top of nsDOMMediaStreams - compiles and works a little (obsolete, for reference)

Updated for minor change due to conflict with comment fix to roc's patch
(Reporter)

Updated

5 years ago
Attachment #601757 - Attachment is obsolete: true
Assignee: rjesup → nobody
Component: Video/Audio → WebRTC
QA Contact: video.audio → webrtc-general
(Reporter)

Updated

5 years ago
No longer depends on: 729541
(Reporter)

Updated

5 years ago
Depends on: 729541
(Assignee)

Comment 10

5 years ago
Created attachment 645014 [details] [diff] [review]
WIP - v0.0.1

WIP PeerConnection hookup with SIPCC, does not work, uploaded mostly to share with ekr.
(Assignee)

Comment 11

5 years ago
Created attachment 645069 [details] [diff] [review]
WIP v0.0.2

There were some changes on alder, here's the updated patch.
Assignee: nobody → anant
Attachment #645014 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #602372 - Attachment description: folded unfinished peerconnection patch on top of nsDOMMediaStreams - compiles and works a little → folded unfinished peerconnection patch on top of nsDOMMediaStreams - compiles and works a little (obsolete, for reference)
(Assignee)

Comment 12

5 years ago
Created attachment 645387 [details] [diff] [review]
WIP - v0.1

Checkpoint: patch compiles and unit tests pass.
Attachment #645069 - Attachment is obsolete: true
Attachment #645387 - Flags: feedback?(ekr)
Comment on attachment 645387 [details] [diff] [review]
WIP - v0.1

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

Land it with these changes. Then we should merge against TI.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +26,5 @@
>  namespace sipcc {
>  
> +LocalSourceStreamInfo::LocalSourceStreamInfo(nsDOMMediaStream* aMediaStream)
> +  : mMediaStream(aMediaStream) {}
> +LocalSourceStreamInfo:: ~LocalSourceStreamInfo() {}

Inline these into the header.

@@ +79,5 @@
>    }
>  }
>  
> +nsRefPtr<nsDOMMediaStream>
> +LocalSourceStreamInfo::GetMediaStream()

FWIW, I would inline this.

Also, should it be const (the function, not the result).

@@ +272,2 @@
>      localSourceStream->ExpectAudio();
>      mCall->addStream(0, track_id, AUDIO);

the first argument here is the peerconnection MediaStream ID, i.e., the thing named "track_id" above.
The *second* argument is the index within the stream. What's confusing here is that there are two arrays, one for audio and one for video... 

For now, just rename track_id to media_stream_id and pass 0 as the second argument.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +120,2 @@
>    virtual void Close();
> +  */

Can you remove these?

Also, Mozilla convention is to use #if 0

::: media/webrtc/signaling/test/FakeMediaStreams.h
@@ +37,5 @@
>    virtual ~Fake_nsDOMMediaStream() { if (mMediaStream) { delete mMediaStream;} }
>  
> +  // FIXME: Make this thread-safe?
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIDOMMEDIASTREAM

use the threadsafe implementations in Fake_MediaStreamImpl.h

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +32,2 @@
>  
> +NS_IMPL_ISUPPORTS1(Fake_nsDOMMediaStream, nsIDOMMediaStream)

IMPL_THREADSAFE....

::: media/webrtc/signaling/test/signaling_unittests.h
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"
> +
> +namespace test {

hg remove.
Attachment #645387 - Flags: feedback?(ekr) → feedback+
Comment on attachment 645387 [details] [diff] [review]
WIP - v0.1

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

::: dom/media/Makefile.in
@@ +44,5 @@
>  LOCAL_INCLUDES += \
>    -I$(topsrcdir)/media/webrtc/trunk/src \
>    $(NULL)
> +PARALLEL_DIRS += bridge
> +#endif

Should be `endif`

::: dom/media/PeerConnection.js
@@ +15,2 @@
>  function PeerConnection() {
> +  this._pc = Cc["@mozilla.org/peerconnection;1"].

Seems oddly inconsistent with the constant above.

@@ +15,5 @@
>  function PeerConnection() {
> +  this._pc = Cc["@mozilla.org/peerconnection;1"].
> +             createInstance(Ci.IPeerConnection);
> +  this._observer = new PeerConnectionObserver(this._pc);
> +  dump("!!!!!!! mozPeerConnection constructor called " + this._pc + "\n\n");

You'll remove all these dump calls before landing, I trust.

::: dom/media/bridge/IPeerConnection.idl
@@ +16,5 @@
> +  const long kSdpState = 0x3;
> +  const long kSipccState = 0x4;
> +
> +  /* JSEP callbacks */
> +  void onCreateOfferSuccess(in string offer);

Don't use the IDL 'string' type; use 'DOMString'.

::: dom/media/bridge/Makefile.in
@@ +4,5 @@
> +
> +DEPTH   = ../../..
> +topsrcdir = @top_srcdir@
> +srcdir    = @srcdir@
> +VPATH   = @srcdir@

Indentation is weird; just use one space.

::: dom/media/bridge/MediaModule.cpp
@@ +4,5 @@
> +
> +#include "mozilla/ModuleUtils.h"
> +#include "nsIClassInfoImpl.h"
> +
> +#ifdef MOZ_WEBRTC

Why build this file at all if WebRTC is disabled?

@@ +9,5 @@
> +
> +// wat.
> +#include "base/linked_ptr.h"
> +using namespace talk_base;
> +#include "PeerConnectionImpl.h"

Definitely no `using` before #includes.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +57,5 @@
>          break;
>        }
>      }
>  
> +    if (!found) {

if (!mAudioTracks.Contains(aID)) {

@@ +70,5 @@
>          break;
>        }
>      }
>  
> +    if (!found) {

if (!mVideoTracks.Contains(aID)) {

@@ +79,5 @@
>    }
>  }
>  
> +nsRefPtr<nsDOMMediaStream>
> +LocalSourceStreamInfo::GetMediaStream()

Don't return nsRefPtrs by value.

@@ +119,2 @@
>  {
>    PeerConnectionImpl *pc = new PeerConnectionImpl();

* to the left

@@ +142,5 @@
>    PR_DestroyLock(mLocalSourceStreamsLock);
>  }
>  
> +NS_IMETHODIMP
> +PeerConnectionImpl::Initialize(IPeerConnectionObserver* observer) {

{ on the next line

@@ +278,5 @@
>      mCall->addStream(0, track_id, VIDEO);
>    }
>  
>    // Make it the listener for info from the MediaStream and add it to the list
> +  mozilla::MediaStream *plainMediaStream = stream->GetStream();

* to the left

@@ +319,4 @@
>  }
>  
> +NS_IMETHODIMP
> +PeerConnectionImpl::CloseStreams() {

{ on the next line

@@ +341,5 @@
> +  char* tmp = new char[mLocalSDP.size() + 1];
> +  std::copy(mLocalSDP.begin(), mLocalSDP.end(), tmp);
> +  tmp[mLocalSDP.size()] = '\0';
> +
> +  sdp = &tmp;

This doesn't return anything; should have been *sdp = tmp;. But you should use DOMString instead.

@@ +352,5 @@
> +  char* tmp = new char[mRemoteSDP.size() + 1];
> +  std::copy(mRemoteSDP.begin(), mRemoteSDP.end(), tmp);
> +  tmp[mRemoteSDP.size()] = '\0';
> +
> +  sdp = &tmp;

Id.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +120,2 @@
>    virtual void Close();
> +  */

Mozilla convention is to remove the code, actually.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +307,2 @@
>  
> +  void CreateOffer(const char* hints, bool audio, bool video) {

{...
(Assignee)

Comment 15

5 years ago
Thanks for the feedback! I'm just going to land this on alder to get something working quickly, I will revisit this patch later to do more iterations at which point Ms2ger's comments will be addressed.
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/projects/alder/rev/70cf174835ba
(Assignee)

Comment 17

5 years ago
Created attachment 646004 [details] [diff] [review]
Setup JS test shell and a few IDL changes

Added an XPCShell test + a few tweaks to the interface to make sure PeerConnectionImpl will work both from the C++ unit tests as well as xpcshell.
(Assignee)

Comment 18

5 years ago
The other two unit tests were commented out because of a link failure, ekr has already fixed that on the TI branch in alder.

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc+]
(Assignee)

Updated

5 years ago
Attachment #602372 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #645387 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #646004 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 664762 [details] [diff] [review]
PeerConnection DOM Interface - v1

For the review session tomorrow!
Attachment #664762 - Flags: review?(jst)
Attachment #664762 - Flags: feedback?(ekr)
(Assignee)

Comment 20

5 years ago
Comment on attachment 664762 [details] [diff] [review]
PeerConnection DOM Interface - v1

We went through this patch today, and I've captured a bunch of comments from jst. Will fix them and upload a newer patch.
Attachment #664762 - Attachment is obsolete: true
Attachment #664762 - Flags: review?(jst)
Attachment #664762 - Flags: feedback?(ekr)
(Assignee)

Comment 21

5 years ago
Created attachment 666097 [details] [diff] [review]
PeerConnection DOM Interface - v2

With review comments addressed.
Attachment #666097 - Flags: review?(jst)
Comment on attachment 666097 [details] [diff] [review]
PeerConnection DOM Interface - v2

- In dom/media/MediaManager.cpp

+    if (mTrackID == kVideoTrack) {
+      stream = nsDOMMediaStream::CreateInputStream(
+        nsDOMMediaStream::HINT_CONTENTS_VIDEO
+      );
+    } else {
+      stream = nsDOMMediaStream::CreateInputStream(
+        nsDOMMediaStream::HINT_CONTENTS_AUDIO
+      );
+    }

Since mTrackID can only be kVideoTrack or kAudioTrack, maybe only explicitly create an audio stream in that case, and MOZ_ASSERT if it's some other value? Would help working on this code down the road once more types are added etc...

+// Global list of PeerConnection objects, so they can be cleaned up when
+// a page is torn down. (Maps outer window ID to an array of PC objects).
+function GlobalPCList() {
+  this._list = {};
+  Services.obs.addObserver(this, "outer-window-destroyed", true);

After thinking about this a bit more, I realize that this is actually not what you want. You don't want to associate this stuff with an outer window, you want to associate this with an inner window. So you basically want to replace outer-window-destroyed with inner-window-destroyed everywhere, and where you get the outer window ID, you want to get the current inner window ID instead. All else should remain the same. If not, thins will be bound to the lifetime of a tab (or top level window), instead of being bound to the webpage in question.

+  classInfo: XPCOMUtils.generateCI({classID: PC_ICE_CID,
+                                    contractID: PC_ICE_CONTRACT,
+                                    classDescription: "IceCandidate",
+                                    interfaces: [
+                                      Ci.nsIDOMRTCIceCandidate,
+                                      Ci.nsIDOMGlobalObjectConstructor
+                                    ],

I don't think you want nsIDOMGlobalObjectConstructor here, only in the QI implementation. This list controls what interfaces are exposed by default on instances of this class, and nsIDOMGlobalObjectConstructor is an internal implementation detail.

+  constructor: function(win, cand, mid, mline) {
+    this._win = win;

Given that we currently don't have any protection against a webpage calling QI and calling into this function, you should add a guard here to make sure this is only ever called once. I.e. check if _win is already set, and throw an exception if it is. Or something like that.

+  classInfo: XPCOMUtils.generateCI({classID: PC_SESSION_CID,
+                                    contractID: PC_SESSION_CONTRACT,
+                                    classDescription: "SessionDescription",
+                                    interfaces: [
+                                      Ci.nsIDOMRTCSessionDescription,
+                                      Ci.nsIDOMGlobalObjectConstructor
+                                    ],

Same here.

+  constructor: function(win, type, sdp) {
+    this._win = win;

And here.

+  classInfo: XPCOMUtils.generateCI({classID: PC_CID,
+                                    contractID: PC_CONTRACT,
+                                    classDescription: "PeerConnection",
+                                    interfaces: [
+                                      Ci.nsIDOMRTCPeerConnection,
+                                      Ci.nsIDOMGlobalObjectConstructor
+                                    ],

And here.

+  // Constructor is an explicit function, because of nsIDOMGlobalObjectConstructor.
+  constructor: function(win) {
+    this._pc = Cc["@mozilla.org/peerconnection;1"].

And here.

+    this._win = win;
+    this._winID = this._win.QueryInterface(Ci.nsIInterfaceRequestor)
+                           .getInterface(Ci.nsIDOMWindowUtils).outerWindowID;

This is where you should use currentInnerWindowID instead of outerWindowID.

+    this._queueOrRun({
+      func: this._pc.createOffer, args: [constraints], wait: true
+    });

These functions might be a bit more readable if each property in the object argument was on its own line. Up to you tho...

r=jst with that taken care of! If the outer/inner window stuff gets out of hand, I'm happy to help look over a new version of this patch.
Attachment #666097 - Flags: review?(jst) → review+
(Assignee)

Comment 23

5 years ago
Created attachment 668534 [details] [diff] [review]
PeerConnection DOM interface - v3

Thanks jst! This patch addresses all the comments, but is not a valid patch as in it won't apply to mozilla-inbound. I will make a fresh patch once the dependencies for this (mtransport & signaling) land on m-i.
(Reporter)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/86aef70706f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/750fa0811f41
https://hg.mozilla.org/mozilla-central/rev/750fa0811f41
https://hg.mozilla.org/mozilla-central/rev/86aef70706f9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Keywords: verifyme
Going to mark verified given that we've confirmed for months that this is hooked up (confirmed the feature is landed).
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.