Make stop() safe and affect createOffer only. Replace attributes with "stopped" direction.

[jan-ivar]

Fixes #2150 and #2176. For consideration.

---

Preview | Diff

[jan-ivar]

@henbos This is based on what we discussed. PTAL.

In short, the PR creates a new tc.stopping sibling to tc.stopped. The former only affects createOffer, not createAnswer, sidestepping #2150. The stop() method now sets stopping instead of stopped, but otherwise works as before locally, and setRemoteDescription(answer) transitions it to stopped, basically.

Renamed old dangerous stop to reject(); made it work only in "have-remote-offer", solving #2176.

I had to audit existing places the spec reads [[Stopped]], to see which should be [[Stopping]] instead. Very few it turned out, mostly high-level modification APIs like addTrack.

Stop reading now if you don't care about edge cases...

The good news is that the setting an RTCSessionDescription algorithm did not look at [[Stopped]] much. I found two places, but they seem like optimizations, and one may even be a bug. I'll file a separate issue.

This means most of our negotiation event firing code already runs on stopped transceivers today, something I verified in Firefox with this fiddle. That may sound bad, but is actually quite orthogonal and seems benign to me, since it's basically shuffling around ended tracks at that point. This was informative to figure out how a peer should react to incoming offers/answers in the new stopping state:

For example: an edge-case is where a fixed-role answerer may end up staying in stopping state for a while, because things like tc.stop() and pc.addTrack won't automatically be negotiated (requires out-of-band-signaling to the fixed-role offerer). So it sits until negotiation comes from the other end, because it's never the offerer. In this case, events may still fire from incoming non-stopped offers, except on the transceiver.receiver.track itself which has been ended synchronously already by stop(), like today.

[henbos]

Stop reading now if you don't care about edge cases...

Of course I care ;) I'll review carefully tomorrow

[jan-ivar]

Slides

[aboba]

Need to remove the reject() portion.

[jan-ivar]

Removes reject(), and incorporates @henbos's changes proposed at the tail of our last VI, which simplifies by removing the stopped and stopping attributes in favor of a single "stopped" direction.

So instead of reading a stopping attribute, use:

if (tc.direction == "stopped") {

Instead of reading the old stopped attribute (which now only happens from SRD) use:

if (tc.currentDirection == "stopped") {

Keeps the old stop() method, unless there's appetite for direction = "stopped"? (throws now)

tc.stop();
console.log(tc.direction);        // stopped
console.log(tc.currentDirection); // sendrecv
doSignalingHandshake();
console.log(tc.direction);        // stopped
console.log(tc.currentDirection); // stopped

Note how the final value of currentDirection is now "stopped" rather than null. 🤘

[jan-ivar]

Travis seems unhappy. Some kind of respec issue? I can't make out the problem here.

[caribouW3]

Travis seems unhappy. Some kind of respec issue? I can't make out the problem here.

Apparently a transient problem, it works OK now.

[henbos]

Great to see this PR, I think it's good. Just one comment.