Closed
Bug 797534
Opened 12 years ago
Closed 11 years ago
Add JSEP APIs getLocalDescription and getRemoteDescription
Categories
(Core :: WebRTC: Signaling, defect)
Core
WebRTC: Signaling
Tracking
()
VERIFIED
FIXED
mozilla20
People
(Reporter: emannion, Assigned: abr)
References
Details
(Whiteboard: [WebRTC] [blocking-webrtc+])
Attachments
(1 file, 1 obsolete file)
68.43 KB,
patch
|
smaug
:
review+
jesup
:
checkin+
|
Details | Diff | Splinter Review |
These need to be completed in gsm in fsmdef.c I have the plumbing into SIPCC and the functions stubs created they need to be filled in.
Updated•12 years ago
|
Whiteboard: [blocking-webrtc+]
Updated•12 years ago
|
Whiteboard: [blocking-webrtc+] → [WebRTC] [blocking-webrtc+]
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → adam
Assignee | ||
Comment 1•11 years ago
|
||
The WebRTC spec (W3C Editor's Draft 12 December 2012) stipulates that localDescription and remoteDescription are read-only attributes on RTCPeerConnection. This means that they are accessed synchronously. Any calls into the SIPCC code would go across two thread boundaries. This means that the fsmdef_ev_localdesc() and fsmdef_ev_remotedesc() functions (and their corresponding events) don't really make sense. In practice, maintaining these read-only attributes needs to be taken care of inside PeerConnectionImpl. A quick check of PeerConnectionImpl::GetRemoteDescription and PeerConnectionImpl::GetLocalDescription shows that they are presently copying out of the PeerConnection's own local idea of what the SDP state for each side is. Looking at fsmdef_ev_setlocaldesc() and fsmdef_ev_setremotedesc() in fsmdef.c, both functions end with a ui_set_remote_description() that sends the appropriate event (SETLOCALDESC or SETREMOTEDESC) back to the PeerConnectionImpl. There is no similar call at the end of fsmdef_ev_addcandidate(), which will cause the remoteDescription to get out of sync when they use trickle ice. Inside PeerConnectionImpl::onCallEvent, these events currently result in copying the requested SDP into the current SDP. While this is obviously just a stub to get things working for the moment, this behavior is masked by Bug 784514. At this point, then, I see two things that need to be fixed to finalize the localDescription and remoteDescription handling: 1) PeerConnectionImpl::onCallEvent() needs to be updated to grab the correct SDP out of the SETLOCALDESC and SETREMOTEDESC events. (There may be complementary code in fsmdef_ev_setlocaldesc() and fsmdef_ev_setremotedesc() that ensures that a serialized version of the SDP is available in those events). 2) fsmdef_ev_addcandidate() needs to be updated to fire off a UI event of SETREMOTEDESC with the new SDP any time we get an additional ICE candidate. In the future, when we support trickle ICE, we also need a means for SIPCC to find out about new candidates as nICEr finds out about them. At that point, we need to update our local SDP and fire off a SETLOCALDESC UI event. My understanding is that we do not currently employ trickle ICE for local candidates. I would consider that behavior outside the scope of the present bug, and propose that it be created as a new bug at the same time as the trickle ICE enhancement bug is filed.. There is a related cleanup task to remove the CC_MSG_LOCALDESC and CC_MSG_REMOTEDESC messages, as well as the fsmdef_ev_localdesc() and fsmdef_ev_remotedesc() functions, as these events are not compatible with the notion of synchronous access to the remote and local SDP. I would also consider such behavior outside the scope of the present bug, and plan to file a new low-priority bug for such clean-up.
Assignee | ||
Comment 2•11 years ago
|
||
The cleanup task has been filed as Bug 821066.
Assignee | ||
Comment 3•11 years ago
|
||
As an additional note: once we support adding and removing streams to and from active sessions, we'll need to send messages to the PeerConnection to update the SDP also.
Assignee | ||
Comment 4•11 years ago
|
||
Scratch comment 3 -- the current model appears to be that adding and removing streams triggers the javascript to renegotiate the session. During that renegotiation, the local and remote descriptions will be set properly.
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #693405 -
Flags: review?(ethanhugg)
Comment 6•11 years ago
|
||
Still reviewing, but I thought I'd add this. I think it's important for pc.localDescription to work at the JS level along with remoteDescription so I hacked this to my PeerConnection.js to get that to work (copied stuff from remoteDescription). Not sure if this is what we want exactly, but it will allow this patch to be looked at by testers. I can now view the SDP from JS. diff --git a/dom/media/PeerConnection.js b/dom/media/PeerConnection.js --- a/dom/media/PeerConnection.js +++ b/dom/media/PeerConnection.js @@ -204,16 +204,17 @@ function PeerConnection() { // Public attributes. this.onaddstream = null; this.onopen = null; this.onremovestream = null; this.onicecandidate = null; this.onstatechange = null; this.ongatheringchange = null; this.onicechange = null; + this.localDescription = null; this.remoteDescription = null; // Data channel. this.ondatachannel = null; this.onconnection = null; this.onclosedconnection = null; } PeerConnection.prototype = { @@ -435,16 +436,21 @@ PeerConnection.prototype = { break; default: throw new Error( "Invalid type " + desc.type + " provided to setRemoteDescription" ); break; } + this.localDescription = { + type: desc.type, sdp: desc.sdp, + __exposedProps__: { type: "rw", sdp: "rw"} + }; + this.remoteDescription = { type: desc.type, sdp: desc.sdp, __exposedProps__: { type: "rw", sdp: "rw" } }; this._queueOrRun({ func: this._pc.setRemoteDescription, args: [type, desc.sdp],
Comment 7•11 years ago
|
||
Comment on attachment 693405 [details] [diff] [review] Update PeerConnectionImpl to use SDP from SIPCC Review of attachment 693405 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I built/tested on Ubuntu64 and saw no new warnings. I think it should be testable from JS before we push it. ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +533,5 @@ > char* offer() const { return offer_; } > char* answer() const { return answer_; } > > + std::string getLocalDescription() const { > + char *sdp = 0; I think the style guide says to use nullptr in this case for new code - https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
Attachment #693405 -
Flags: review?(ethanhugg) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Note from :jesup via IRC -- DOM code changes suggested by Ethan above need review by :smaug or :jst
Assignee | ||
Comment 9•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #693405 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 693876 [details] [diff] [review] Update PeerConnectionImpl to use SDP from SIPCC, smaug -- I'm asking for your review on the changes in PeerConnection.js -- ehugg has already done a review+ for the remaining files. Thanks!
Attachment #693876 -
Flags: review?(bugs)
Comment 11•11 years ago
|
||
Comment on attachment 693876 [details] [diff] [review] Update PeerConnectionImpl to use SDP from SIPCC, Would be so nice to see Peerconnection to be rewritten in C++ using WebIDL bindings at some point. Reviewing would be easier and behavior would be more correct.
Attachment #693876 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #11) > Would be so nice to see Peerconnection to be rewritten in C++ using WebIDL > bindings at some point. Reviewing would be easier and behavior would be more > correct. I've opened Bug 823512 to track this suggestion.
Assignee | ||
Comment 13•11 years ago
|
||
Comment on attachment 693876 [details] [diff] [review] Update PeerConnectionImpl to use SDP from SIPCC, Okay, I've double checked that this patches against m-i cleanly, so I think we're ready to land.
Attachment #693876 -
Flags: checkin?(rjesup)
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad96e42102e
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla20
Updated•11 years ago
|
Attachment #693876 -
Flags: checkin?(rjesup) → checkin+
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4ad96e42102e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 16•11 years ago
|
||
Already verified through existing PC basic smoke test automation.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•