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

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)

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.

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

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

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

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

Checkpoint: patch compiles and unit tests pass.

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

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.

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.

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

For the review session tomorrow!

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.

With review comments addressed.

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.

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.

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

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