694807 - Implement PeerConnection object

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

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

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

Comment 1

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

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)

Comment 3

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

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.

Comment 4

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

Attachment: Base unfinished peerconnection patch

Comment 5

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

Attachment: untested mediastream array code

Comment 6

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

Attachment: modify peerconnection code to build on top of nsDOMMediaStreams - compiles and works a little

Comment 7

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

Attachment: folded unfinished peerconnection patch on top of nsDOMMediaStreams - compiles and works a little

Comment 8

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

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).

Comment 9

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

Attachment: 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

Comment 10

[Anant Narayanan [:anant]]

Attachment: WIP - v0.0.1

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

Comment 11

[Anant Narayanan [:anant]]

Attachment: WIP v0.0.2

There were some changes on alder, here's the updated patch.

Comment 12

[Anant Narayanan [:anant]]

Attachment: WIP - v0.1

Checkpoint: patch compiles and unit tests pass.

Comment 13

[Eric Rescorla (: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.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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) {

{...

Comment 15

[Anant Narayanan [:anant]]

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.

Comment 16

[Anant Narayanan [:anant]]

https://hg.mozilla.org/projects/alder/rev/70cf174835ba

Comment 17

[Anant Narayanan [:anant]]

Attachment: 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.

Comment 18

[Anant Narayanan [:anant]]

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

Comment 19

[Anant Narayanan [:anant]]

Attachment: PeerConnection DOM Interface - v1

For the review session tomorrow!

Comment 20

[Anant Narayanan [:anant]]

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.

Comment 21

[Anant Narayanan [:anant]]

Attachment: PeerConnection DOM Interface - v2

With review comments addressed.

Comment 22

[Johnny Stenback (: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.

Comment 23

[Anant Narayanan [:anant]]

Attachment: 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.

Comment 24

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/86aef70706f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/750fa0811f41

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/750fa0811f41
https://hg.mozilla.org/mozilla-central/rev/86aef70706f9

Comment 26

[Jason Smith [:jsmith]]

Going to mark verified given that we've confirmed for months that this is hooked up (confirmed the feature is landed).