Webrtc-pc: pc.createOffer(); pc.addTrack(track); should not include track

[jan-ivar]

Now that createOffer has procedural steps thanks to @adam-be, we should add language to capture some state synchronously, so that in the infamous case of:

    pc.addTrack(trackX, stream);
    pc.createOffer().then(offer => { ... })
    pc.addTrack(trackY, stream);

the offer does not include trackY in the typical (nothing on the queue) case, as we agreed over a year ago.

[jesup]

And the same should apply to:
...
pc.createOffer().then(....);
pc.createDataChannel(...);
(assuming no createDataChannel before createOffer())

[stefhak]

I agree. Willing to provide a PR?

[alvestrand]

Does this mean that addTrack has a queued step (which doesn't return a promise, so it's unobservable when it finishes)?

[stefhak]

@alvestrand I guess you are right in that it is not that easy to detect when a track added will start to result in RTP being sent (and received by the peer). In this case there would have to be another offer/answer exchange to make that happen. And observability is in stats!?

But I don't think addTrack in itself has a queued step, it's rather that createOffer samples the current tracks and if one is added after createOffer it won't be part of the produced offer.

[jan-ivar]

Given that addTrack returns a sender, it would be hard to queue. I.e. I think one would expect pc.getSenders() to include this new sender immediately.

[alvestrand]

So if createOffer has "gather info on all the tracks to be sent" in its synchronoous part, that works too. I guess that was what @jan-ivar said initially, I just didn't read it properly.

[pthatcherg]

addTrack creates an RtpTransceiver (usually) and then returns the RtpSender for that transceiver. So what we're talking about here is delaying when the transceiver shows up in createOffer. Something like have a flag on each transceiver ready to go into createOffer and that doesn't happen immediately after creation.

[taylor-b]

Apologies in advance if I have some fundamental misunderstanding about how PR #820 works, but to collect my thoughts on this while they're fresh:

I think the problem with this approach in general (taking a snapshot of the PC state) is that we're trying to fulfill two rules:

1. The snapshot is taken _after_ any previously queued operation (like SRD) modifies the PC state.
2. The snapshot is taken _before_ any subsequent synchronous method call (like anything to do with transceivers) modifies the PC state.

And in the following case (for example) it's impossible to satisfy those two rules with a single unchanging snapshot:

pc.setRemoteDescription(answer_that_stops_transceiver_a);
pc.createOffer().then(...);
// Some other stuff happens, but SRD is still in progress...
pc.addTrack(track_that_adds_transceiver_b, streams);

With the snapshot model, how can createOffer reflect the changes from SRD but not addTrack?

We could extend the snapshot logic such that a snapshot is taken immediately when createOffer/Answer is called (like in the first version of the PR), but queued setRemote/LocalDescription calls, when finished, apply a diff to the snapshots in the queue. So in the previous example we'd have:

1. SRD called and queued.
2. createOffer called, creating offerBundle containing an unstopped transceiver A, with remaining operations queued.
3. addTrack called, creating transceiver B.
4. SRD finishes, stopping transceiver A. The diff ("stopped transceiver A") is applied to answerBundle.
5. createOffer creates an offer with a stopped transceiver A and no transceiver B.

However, this seems like a crazy amount of complexity to add to the spec. This is what Peter was describing, where we have this "asynchronous" set of state as seen by JSEP procedures, and a "synchronous" set of state as seen by the application.

Or, maybe people are happy with PR #820 as is, because we don't care about the "something's on the queue" case? But that distinction seems likely to trip up developers and cause crazy unexpected behavior that only the people in this working group understand, in my opinion. For example, an innocuous addIceCandidate process sitting on the queue could sporadically cause tracks to appear in offers they wouldn't otherwise appear in.

[jan-ivar]

@taylor-b That could work but I agree it is crazy, and that we don't want that.

Or, maybe people are happy with PR #820 as is, because we don't care about the "something's on the queue" case?

We said a year ago that we don't care about the "something's on the queue" case.

The logic is:

1. Using the queue is an edge-case, and frowned on even.
2. Relying on the better-defined addTrack-behavior, is an edge-case.
3. Using both is an edge-case of an edge-case.

I like PR #820 because it fixes createOffer and createAnswer to not race with JS:

    var wait = ms => new Promise(resolve => setTimeout(resolve, ms));

    pc.createOffer().then(offer => { ... })
    Promise.resolve().then(() => pc.addTrack(trackX, stream)); // in or out?
    wait(0).then(() => pc.addTrack(trackY, stream));           // in or out?
    wait(7).then(() => pc.addTrack(trackZ, stream));           // in or out?

Which has nothing to do with the queue. I'm happy to ignore the queue on this.

[jan-ivar]

For example, an innocuous addIceCandidate process sitting on the queue could sporadically cause tracks to appear in offers they wouldn't otherwise appear in.

Great example, though I think this would only be surprising to people relying on the second edge-case to not have things added.

Compare to if we made methods fail - which was suggested - then this would cause things to sporadically fail, which I think would surprise everyone.

As it is, a properly configured onnegotiationneeded is the catch-all for these things.

[adam-be]

@taylor-b, I believe your interpretation of #820 is correct.

In Lisbon, we briefly talked about #820's problem with solving the original problem in this issue. I.e. trackY can either be in or out of the answer depending on the state of the queue. One way bring consistency to that, and similar cases, could be to not run an operation's queued steps synchronously if the queue is empty. The effect would be that all sync changes in a JS event loop iteration are always visible to the queued steps of the next async operation, like there was something on the queue. I.e. trackYis always in the answer in the above example. If I understood @jan-ivar correctly, this is how Firefox is doing it currently. This change should not affect the case when promises are used properly.

[jan-ivar]

@adam-be Yes it's how Firefox currently works, as the new behavior is blocked on some bugs.

Your suggestion seems to mirror what I suggested a year ago, which was rejected. What new information do you have to reopen that discussion?

Btw, I think we need https://github.com/w3c/webrtc-pc/pull/820 regardless to fix the JS racing which exists even without a queue.

[dontcallmedom]

can someone remind me of the reason why we run synchronously if the queue has length one? (in terms of new information, I think it's clear that we underestimated the cost of keeping this mix of async/sync states)

[jan-ivar]

@dontcallmedom I think the main argument was that it was unintuitive, given that "run synchronously" is how things normally work without a queue invention (that is: the synchronous part of an asynchronous method's processing steps run immediately), though bear in mind I was arguing the other side at the time.

But we keep confusing two things here:

1. The queue, where a year ago we agreed to ignore the queued case. Where's the new information to care about the queued case?
2. Any "cost" in https://github.com/w3c/webrtc-pc/pull/820 seems to come from the fact that the individual (under-specified until now) steps in _createOffer_, _createAnswer_, and _setLocalDescription_ are racy.

How will always queueing or removing the queue let us ignore (2)?

[taylor-b]

I like @adam-be's idea of always running the queued steps asynchronously. In fact I'd propose doing the opposite of what this issue describes; pc.createOffer(); pc.addTrack(t, s); will _always_ include the track in the offer.

It may seem unintuitive, but really it provides more flexibility. If a createOffer is on the queue, this gives the application more time to update the PeerConnection state before the offer is actually generated, so that an additional offer/answer cycle may not be necessary. And if you really _don't_ want the offer to contain those changes, you can just delay them until after the offer is generated.

@jan-ivar:

Where's the new information to care about the queued case?

I wasn't around for the decision to ignore the queued case, but I would have argued against it. I don't like APIs that behave differently based on some internal state that the application should (in theory) be able to ignore.

How will always queueing or removing the queue let us ignore (2)?

It won't, but it will allow the solution to be simpler and not result in weird side-effects (like "addIceCandidate on the queue makes things work differently").

[jan-ivar]

@taylor-b You make a compelling case that addIceCandidate throws a wrench in things, because it's not a method that's normally chained with any of the others, even when written properly with promises.

So we'd always queue and keep https://github.com/w3c/webrtc-pc/pull/820 ? This shifts the problem only a little bit:

var wait = ms => new Promise(resolve => setTimeout(resolve, ms));

pc.createOffer().then(offer => { ... })
pc.addTrack(a, s);                               // always in
Promise.resolve().then(() => pc.addTrack(b, s)); // in if addIceCandidated, out otherwise
wait(0).then(() => pc.addTrack(c, s));           // in if addIceCandidated, out otherwise
wait(7).then(() => pc.addTrack(d, s));           // who knows

We have to grab the values sometime, so I see no way to ignore the internal state in general.

Where you lose me is I think I've stopped caring about the person who expects:

pc.createOffer().then(offer => { ... })
pc.addTrack(a, s);

to be more deterministic than, say:

button1.onclick = e => pc.createOffer().then(offer => { ... })
button2.onclick = e => pc.addTrack(a, s);

I don't think either should fail, but I don't think I care which train they catch.

[taylor-b]

But I think it's actually preferable to have the option to catch the earlier train. In the example above with button clicks, if the added track makes it into the offer, that may be one less offer/answer cycle. And if you want to catch the later train, it's easy to do that too.

Anyway, I agree that "always queue + PR #820" only shifts the problem. But I had an idea for that. How about a createOffer algorithm like:

1. In parallel:
   
   
   
   1. Perform I/O-bound and/or CPU-intensive tasks still needed for generating an offer. Such as generating a certificate, querying hardware encode/decode capabilities, obtaining an identity assertion, etc. Can define these in more detail later.
   
   
   2. If this failed for any reason, queue task to reject p with an error (same text we already have).
   
   
   3. If successful, queue a task that performs the following steps:
      
      
      
      
      
      1. If more work needs to be done, due to changes made since createOffer was called, return to #1. For example, createOffer may have been called with only an audio transceiver, and now there's a video transceiver, so we need to determine supported video codecs.
      
      
      
      2. Generate the blob of SDP according to JSEP.
      
      
      
      3. (boilerplate stuff)
      
      
      
      4. Resolve promise with offer.
      
      
      
   
   

Now the behavior is pretty easy to understand. If the createOffer promise is resolved, it's too late to stick things in the offer. If it isn't resolved, it's not too late. As opposed to "if createOffer hasn't yet been picked up from the operations queue, it's not too late," which is much less intuitive. Basically it's moving the sequence point of createOffer from the start to the end. And I think the same kind of thing could be done for setLocalDescription/setRemoteDescription.

[adam-be]

Your suggestion seems to mirror what I suggested a year ago, which was rejected. What new information do you have to reopen that discussion?

Yes. It seems to be pretty much option A.

Btw, I think we need #820 regardless to fix the JS racing which exists even without a queue.

Yes. I was perhaps a bit unclear about that.

[adam-be]

I think @taylor-b's algorithm is interesting with the late SDP generation on the main thread and the heavy lifting done in parallel earlier.

I suspect that there's need for a step 0 (before the parallel steps) that takes a snapshot of the media configuration to determine what work that needs to be done in 1 (similar to what's done in 1.iii.a).

[adam-be]

@jan-ivar, what do you think of @taylor-b's alternative algorithm?

[taylor-b]

I thought of a slight issue with my idea. "Resolve promise with offer" still enqueues a job to call the promise fulfill functions, if I understand the ECMAScript spec correctly. So my assumption is incorrect that the script could predict the outcome of addTrack or other methods by whether or not the promise fulfill function has been called; code can execute _in between_ the "resolve promise" step and the fulfill function being called. Argh.

We could expose a new piece of information to address this, like a "number of created offers" count which is incremented before resolving the promise. And setLocalDescription/setRemoteDescription would be ok, because they modify the signaling state before resolving the promise. But it's a little hard to justify adding a new API just to address a corner-case race condition.

[adam-be]

@taylor-b, I actually think we're fine since "Resolve promise with offer" creates a microtask and the microtask queue is emptied before any other queued task is picked up.

[adam-be]

I'm trying to think of anything in our 'enqueue operation' steps that could mess this up, but I think we're fine there as well.

[jan-ivar]

@taylor-b You should be fine. Most (I think all) browsers run promise callbacks on a micro-task queue that empties completely at the tail of the current run of the main task queue, so there's no way for JS to execute "in between".

@adam-be I think it could work.

[taylor-b]

There's a micro-task queue? Shows how much I know. Is that just how browsers happen to work today, or is it specified anywhere? I was going off ECMAScript's "This specification does not define the order in which multiple Job Queues are serviced."

And regardless of that, couldn't something still sneak ahead of the "fulfill promise reactions" job on the microtask queue? Another promise could be resolved on a background thread at the same time as the offer SDP is being generated, right?

[jan-ivar]

To sneak in I htink it would have to be a callback attached to a promise mentioned in the same queued task in our spec, which I don't think is possible.

I doubt much would work if the microtask queue didn't work as it does today.

@bzbarsky can you comment on whether the guarantees of the micro-task queue behavior (being emptied at the tail of the current run of the main task) is covered in any spec?

[bzbarsky]

There are various spec bits that address this. For example, https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script step 5 and https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model step 6

There is also https://html.spec.whatwg.org/multipage/webappapis.html#integration-with-the-javascript-job-queue which basically overrides the ES EnqueueJob to put the thing into the microtask queue instead of an ES Job Queue.

Note that I did not read the rest of this issue carefully and so I don't know whether the fact that promises run off microtasks address the concerns raised above in any way.

[bzbarsky]

Another promise could be resolved on a background thread at the same time

No. Promise resolution has to happen on the right thread, via a task queued to that thread. You can't magically resolve promises from background threads.

[taylor-b]

I still don't understand what prevents some other "in parallel" algorithm from queuing a microtask while the browser is executing the "create offer" task.

Example:

1. createOffer setup happening in parallel, along with some other parallel action initiated by the application.
2. createOffer setup finishes, task queued to generate SDP.
3. SDP generation task starts to execute.
4. Other parallel action finishes, it queues a microtask to call promise fulfill callbacks.
5. SDP generation finishes, its task queues a microtask to call promise fulfill callbacks.

[taylor-b]

Oh, I missed your last comment. I guess we're safe after all then.

I missed the sentence at the bottom when reading "Writing Promise-Using Specifications":

"Resolve p with x" is shorthand for calling a previously-stored resolve function from creating p, with argument x.
...
If the algorithm using these shorthands is running in parallel, the shorthands queue a task on p’s relevant settings object’s responsible event loop to call the stored function.

[adam-be]

Any objections to proceeding with @taylor-b's approach and drafting a PR? What do you say @jan-ivar?

[adam-be]

Great, @taylor-b, do you want to take a stab at it and @jan-ivar and I can review?

[taylor-b]

Sure. Feel free to assign me to the issue.

[alvestrand]

Closing this one - #806 covers the outstanding work.