content icon indicating copy to clipboard operation
content copied to clipboard

Issue with "Establishing a connection: The WebRTC perfect nego…": …

Open cdgru opened this issue 3 years ago • 5 comments

MDN URL: https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/Perfect_negotiation

What information was incorrect, unhelpful, or incomplete?

Specific section or headline?

What did you expect to see?

Did you test this? If so, how?

MDN Content page report details
  • Folder: en-us/web/api/webrtc_api/perfect_negotiation
  • MDN URL: https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/Perfect_negotiation
  • GitHub URL: https://github.com/mdn/content/blob/master/files/en-us/web/api/webrtc_api/perfect_negotiation/index.html
  • Document last modified: 2021-01-02T19:18:52.000Z

cdgru avatar Jan 27 '21 12:01 cdgru

I am concerned about makingOffer. It is used in A), no more used/needed in B), but still needed in C) for offerCollision.

A) pc.onnegotiationneeded

let makingOffer = false;
pc.onnegotiationneeded = async () => {
  try {
    makingOffer = true;
    await pc.setLocalDescription();
    signaler.send({ description: pc.localDescription });
  } catch(err) {
    console.error(err);
  } finally {
    makingOffer = false;
  }
};

B) in the last section "restartice" where the focus in oniceconnectionstatechange, the (new?) pc.onnegotiationneeded handler doen not use makingOffer anymore:

pc.onnegotiationneeded = async options => {
  await pc.setLocalDescription(await pc.createOffer(options));
  signaler.send({ description: pc.localDescription });
};
pc.oniceconnectionstatechange ...

C) What I am concerned about, is the signaler.onmessage handler, which still using makingOffer for offerCollision:

let ignoreOffer = false;
signaler.onmessage = async ({ data: { description, candidate } }) => {
  try {
    if (description) {
      const offerCollision = (description.type == "offer") &&
                             (makingOffer || pc.signalingState != "stable");
      ignoreOffer = !polite && offerCollision;
      if (ignoreOffer) {
        return;
      }  ...

What You are recommending for makingOffer? Keep „let makingOffer = false;“ from A) but looks like that it'll never be changed or am I missing something else?

cdgru avatar Jan 27 '21 12:01 cdgru

@jan-ivar @henbos can you take a look?

fippo avatar Jul 12 '22 13:07 fippo

The code quoted in B) is under the section "The old way" but just after this section in the article there is the section "Using restartIce()" which does not explicitly call createOffer(), it just calls restartIce() and relies on the Perfect Negotiation logic doing everything correctly for you.

henbos avatar Jul 12 '22 13:07 henbos

Calling restartIce() triggers onnegotiationneeded and that event is still using makingOffer so you're safe.

We could update the article not to talk about the old versus the new way of doing things, it sounds a bit like a sales pitch for Perfect Negotiation or an interesting fact about how things were supposed to work before, but it's not really helpful in a guide to show how not to do it if you're only interested in how to do it

henbos avatar Jul 12 '22 14:07 henbos

We could update the article not to talk about the old versus the new way of doing things

That sounds great to me

but it's not really helpful in a guide to show how not to do it if you're only interested in how to do it

100% agreed

sideshowbarker avatar Jul 13 '22 11:07 sideshowbarker