Closed Bug 797534 Opened 7 years ago Closed 7 years ago

Add JSEP APIs getLocalDescription and getRemoteDescription

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla20

People

(Reporter: emannion, Assigned: abr)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [blocking-webrtc+]
Whiteboard: [blocking-webrtc+] → [WebRTC] [blocking-webrtc+]
Assignee: nobody → adam
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.
The cleanup task has been filed as Bug 821066.
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.
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.
Attachment #693405 - Flags: review?(ethanhugg)
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 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+
Note from :jesup via IRC -- DOM code changes suggested by Ethan above need review by :smaug or :jst
Attachment #693405 - Attachment is obsolete: true
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 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+
(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.
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)
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad96e42102e
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla20
Attachment #693876 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/4ad96e42102e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Keywords: verifyme
Already verified through existing PC basic smoke test automation.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Duplicate of this bug: 786158
You need to log in before you can comment on or make changes to this bug.