- From: Adam Bergkvist <adam.bergkvist@ericsson.com>
- Date: Mon, 10 Jun 2013 13:36:08 +0200
- 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