RE: Our simple PeerConnection example with Futures/Promises

If we remove addStream(), what happens when renegotiation is needed?  The streams will have already been added as part of the original offer/answer.  Should the developer pass them in again?  If we say that, then each offer or answer implicitly clears out any Streams that are in the PC.  Alternatively, we could say that the offer/answer uses any streams that are already in the PC, plus any that  are passed to the CreateOffer/Answer call.  

- Jim

-----Original Message-----
From: Adam Bergkvist [mailto:adam.bergkvist@ericsson.com] 
Sent: Monday, June 10, 2013 7:36 AM
To: Jan-Ivar Bruaroey
Cc: public-webrtc@w3.org
Subject: Re: Our simple PeerConnection example with Futures/Promises

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 12:24:24 UTC