webrtc-pc icon indicating copy to clipboard operation
webrtc-pc copied to clipboard

Queue two tasks upon finishing ICE gathering, and fire gatheringstatechange & icegatheringstatechange in same task

Open jan-ivar opened this issue 2 years ago • 6 comments

Fixes https://github.com/w3c/webrtc-pc/issues/2892. Fixes https://github.com/w3c/webrtc-pc/issues/2893. Fixes https://github.com/w3c/webrtc-pc/issues/2896.

cc @docfaraday


Preview | Diff

jan-ivar avatar Aug 21 '23 21:08 jan-ivar

I think we might be missing the steps that add an a=end-of-candidates to the local description.

docfaraday avatar Aug 23 '23 15:08 docfaraday

Also, it looks like we'll fire duplicate null candidates here. We should only fire the null candidate if the RTCPeerConnection.iceGatheringState just transitioned to "complete".

docfaraday avatar Aug 23 '23 15:08 docfaraday

I think we might be missing the steps that add an a=end-of-candidates to the local description.

Isn't that covered by JSEP? This spec generally doesn't get into that. In any case that seems like a separate issue.

jan-ivar avatar Aug 23 '23 17:08 jan-ivar

I think we might be missing the steps that add an a=end-of-candidates to the local description.

Isn't that covered by JSEP? This spec generally doesn't get into that. In any case that seems like a separate issue.

Hmm. So JSEP handles this when creating a new local description, but does not update the existing local description. Maybe we shouldn't be updating? Anyhow, yeah, separate issue.

docfaraday avatar Aug 23 '23 17:08 docfaraday

duplicate null candidates

Ah, I forgot to remove the now unused "update the ICE gathering state" algorithm. Thanks for catching that!

jan-ivar avatar Aug 23 '23 17:08 jan-ivar

Seems good to merge. Needs test.

alvestrand avatar Aug 24 '23 14:08 alvestrand

WPT tests were merged in https://github.com/web-platform-tests/wpt/pull/44687 (two tests).

https://wpt.fyi/results/webrtc/RTCPeerConnection-iceGatheringState.html image

jan-ivar avatar Mar 28 '24 14:03 jan-ivar

Hi @dontcallmedom I'm getting this error. It says to see developer console, where can I see that? Trying to figure out how to bother you less ;)

  Error: ROR] Function listAmendments threw an error during preProcess.
        Plugin: core/pre-process
          Hint: See developer console.

Also the next step "Check amendment annotations on substantive changes to Rec / check-amendment (pull_reques" seems to have passed, which seems odd if it failed on amendments.

jan-ivar avatar Mar 28 '24 16:03 jan-ivar

I'm getting this error. It says to see developer console, where can I see that? Trying to figure out how to bother you less ;)

The way I check it is by running the ReSpec document in my local browser (served via a local http server since otherwise you'd hit CORS issues). The error in the developer console says:

The container with id rtcicetransport marked as amended cannot embed the other container of amendment rtcicetransportstate-failed-description, see https://github.com/w3c/webrtc-pc/blob/main/amendments.md for amendments management

The problem here is that you're applying the change to the whole "rtcicetransport" section, which contains already a place with another change; I think the solution would probably to add a <div> wrapping the more specific changes; I think you would also need to markup the deletion of the "Update the ICE gathering state" section.

I'm happy to leave it to you, or do it on my end as you prefer.

Also the next step "Check amendment annotations on substantive changes to Rec / check-amendment (pull_reques" seems to have passed, which seems odd if it failed on amendments.

That job's role is only to check that a non-editorial PR is listed in amendments; let me know if you see a better name to use for the check.

dontcallmedom avatar Mar 28 '24 16:03 dontcallmedom