W3C home > Mailing lists > Public > public-webrtc@w3.org > June 2013

Re: Our simple PeerConnection example with Futures/Promises

From: Adam Bergkvist <adam.bergkvist@ericsson.com>
Date: Mon, 10 Jun 2013 13:36:08 +0200
Message-ID: <51B5BA28.1000900@ericsson.com>
To: Jan-Ivar Bruaroey <jib@mozilla.com>
CC: public-webrtc@w3.org
Hi

Thanks for very good feedback. Comments inline.

On 2013-06-07 03:10, Jan-Ivar Bruaroey wrote:
> On 6/6/13 7:54 AM, Adam Bergkvist wrote:
>> Hi
>>
>> I played around a bit with slightlyoff's Future/Promise polyfill,
>> redefined some of the methods on PeerConnection and tested it in
>> chrome. Here's how our current simple example from the spec could look
>> with futures. It's not the simplest re-write (method(success, error)
>> -> method().then(success, error)); it uses some chaining.
>>
>> An alternative would be to not listen for the negotiationneeded event
>> and chain offer generation with the call to getUserMedia().
>
> I was going to say, at the Boston interim I thought we agreed that
> keying off onnegotiationneeded was unusual. Didn't someone get an action
> item to "align" the example to not steer people this way? - If this has
> changed, please let me know, as this doesn't work in Firefox currently.
>

I remember we talked about it, but I don't remember a clear resolution. 
It's a small task to convert the example to not use the 
negotiationneeded event though.

As I understood it, people see a need for something like the 
negotiationneeded event, but the question is if it should trigger when 
you do addStream() since in that case, the developer knows that 
negotiation is needed.

If we remove the triggering of the negotiationneeded event as a result 
of addStream(), then I think we remove the last motivation of having 
addStream()/removeStream(). Their original purpose, in the pre-JSEP API, 
was to queue streams that were automatically processed when stable state 
was reached and the result was a callback with a new description (to be 
sent to the other side). With JSEP (and no negotiationneeded event) it 
becomes a strange part of the API. IMO, it would be cleaner to provide 
the streams that should affect the offer/answer in the actual call to 
createOffer()/createAnswer(). Then we don't have to argue if 
addStream()/removeStream() needs success and error callbacks either. :)

pc.createOffer({ "streamsToAdd": [stream] }, successCb, errorCb);

A nice effect is that the stream wouldn't be added to the "local streams 
set" (or the sending set) before it's actually negotiated and being sent.

>> Here's the code if someone is interested.
>
> Great! Comments below.
>
>> /Adam
>>
>> var remotePeer = new SignalingChannel();
>> var configuration = { "iceServers":
>>         [{ "url": "stun:stun.example.org" }] };
>> var pc;
>>
>> // call start() to initiate
>> function start() {
>>     pc = new RTCPeerConnection(configuration);
>>
>>     // send any ice candidates to the other peer
>>     pc.onicecandidate = function (evt) {
>>         if (evt.candidate)
>>             remotePeer.send(JSON.stringify(
>>                     { "candidate": evt.candidate }));
>>     };
>>
>>     // let the "negotiationneeded" event trigger offer generation
>>     pc.onnegotiationneeded = function () {
>>         pc.createOffer().then(function (offer) {
>>             // set the newly created offer
>>             return pc.setLocalDescription(offer);
>>         }, logError)
>>         .then(function () {
>>             // send the offer to the other peer
>>             remotePeer.send(JSON.stringify(
>>                     { "sdp": pc.localDescription }));
>>         }, logError);
>>     };
>
> I believe Futures is optimized for chains that take the single result of
> the previous step (the single success-callback arg), so you should be
> able to collapse:
>
>      // let the "negotiationneeded" event trigger offer generation
>      pc.onnegotiationneeded = function () {
>          pc.createOffer()
> .then(pc.setLocalDescription)
> .then(function () {
>              // send the offer to the other peer
>              remotePeer.send(JSON.stringify({ "sdp": pc.localDescription
> }));
>          }, logError);
>      };
>

I tried to run the example with this change but got "TypeError: Illegal 
invocation". My guess is that pc.setLocalDescription is just a function 
and it doesn't have an associated "this" (to call on) with this syntax.

The syntax is nice and brief, and I can see it being used in other 
scenarios.

> Errors bubble, so logError isn't needed on every step and still gets
> called.
>

True. When you mention it, I remember reading that in the DOM spec.

>> // once remote stream arrives, show it in the remote video element
>>     pc.onaddstream = function (evt) {
>>         remoteVideo.src = URL.createObjectURL(evt.stream);
>>     };
>>
>>     // get a local stream, show it in a self-view and add it to be sent
>>     navigator.getUserMedia({ "audio": true, "video": true })
>>             .then(function (stream) {
>>         selfVideo.src = URL.createObjectURL(stream);
>>         pc.addStream(stream);
>>     });
>> }
>>
>> remotePeer.onmessage = function (evt) {
>>     // got signaling message from other peer
>>     if (!pc)
>>         start();
>>
>>     var message = JSON.parse(evt.data);
>>     if (message.sdp) {
>>         var desc = new RTCSessionDescription(message.sdp);
>>
>>         pc.setRemoteDescription(desc).then(function () {
>>             // if we received an offer, we need to create an answer
>>             if (pc.remoteDescription.type == "offer") {
>>                 pc.createAnswer().then(function (answer) {
>>                     // set answer locally
>>                     return pc.setLocalDescription(answer);
>>                 }, logError)
>>                 .then(function () {
>>                     // send our updated local description to ther peer
>>                     remotePeer.send(JSON.stringify(
>>                             { "sdp": pc.localDescription }));
>>                 }, logError);
>>             }
>>         }, logError);
>
> Nice! You're leaning right a little though. How about:
>
>      var message = JSON.parse(evt.data);
>      if (message.sdp) {
>          var desc = new RTCSessionDescription(message.sdp);
>          if (desc.type == "offer") {
>              pc.setRemoteDescription(desc)
>              .then(function () { return pc.createAnswer(); })
>              .then(pc.setLocalDescription)
>              .then(function () {
>                  // send our updated local description to ther peer
>                  remotePeer.send(JSON.stringify(
>                          { "sdp": pc.localDescription }));
>              }, logError);
>          } else
> pc.setRemoteDescription(desc).then(function(){}, logError);
>
> Different pattern, different pain-points, but I find this easier to read
> as it sets up distinct chains for offer and answer.

Very nice. It's indeed cleaner with distinct chains and take care of the 
condition before.

I tweaked a bit and used catch (instead of then) at the end to avoid the 
empty accept/fulfill function.

/Adam

==========================

The updated version (haven't done anything about the onnegotiation event 
yet):

// call start() to initiate
function start() {
     pc = new RTCPeerConnection(configuration);

     // send any ice candidates to the other peer
     pc.onicecandidate = function (evt) {
         if (evt.candidate)
             remotePeer.send(JSON.stringify(
                 { "candidate": evt.candidate }));
     };

     // let the "negotiationneeded" event trigger offer generation
     pc.onnegotiationneeded = function () {
         pc.createOffer().then(function (offer) {
             // set the newly created offer
             return pc.setLocalDescription(offer);
         })
         .then(function () {
             // send the offer to the other peer
             remotePeer.send(JSON.stringify(
                 { "sdp": pc.localDescription }));
         }, logError);
     };

     // once remote stream arrives, show it in the remote video element
     pc.onaddstream = function (evt) {
         remoteVideo.src = URL.createObjectURL(evt.stream);
     };

     // get a local stream, show it in a self-view and add it to be sent
     navigator.getUserMedia({ "audio": true, "video": true })
             .then(function (stream) {
         selfVideo.src = URL.createObjectURL(stream);
         pc.addStream(stream);
     });
}

remotePeer.onmessage = function (evt) {
     // got signaling message from other peer
     if (!pc)
         start();

     var message = JSON.parse(evt.data);
     if (message.sdp) {
         var desc = new RTCSessionDescription(message.sdp);

         // if we get an offer, we need to reply with an answer
         if (desc.type == "offer") {
             pc.setRemoteDescription(desc).then(function () {
                 return pc.createAnswer();
             })
             .then(function (answer) {
                 return pc.setLocalDescription(answer);
             })
             .then(function () {
                 remotePeer.send(JSON.stringify(
                     { "sdp": pc.localDescription }));
             }, logError);
         } else
             pc.setRemoteDescription(desc).catch(logError);
     } else
         pc.addIceCandidate(new RTCIceCandidate(message.candidate));
};

function logError(error) {
     log(error.name + ": " + error.message);
}
Received on Monday, 10 June 2013 11:36:34 UTC

This archive was generated by hypermail 2.3.1 : Monday, 23 October 2017 15:19:33 UTC