wish-wg / webrtc-http-ingest-protocol

WHIP - WebRTC HTTP ingest protocol draft
50 stars 11 forks source link

HTTP PATCH response clarification #163

Closed cdh4u closed 10 months ago

cdh4u commented 11 months ago

Section 4.1.3:

"If the HTTP PATCH request results in an ICE restart, the WHIP session SHALL return a "200 OK" with an "application/trickle-ice-sdpfrag" body containing the new ICE username fragment and password and OPTIONALLY a new set of ICE candidates for the WHIP client."

Q1: What if the 200 OK does not contain a new set of ICE candidates. Do the earlier ones still apply? Please clarify.

Q2: It is fist said that a new set of ICE candidates is OPTIONALLY sent, and later that that the response MAY contain ICE candidates. I suggest removing the duplication.

murillo128 commented 11 months ago

Not sure what would be best, if letting the new set of ICE candidates override the previous set, or just add them incrementally and just get the previous invalid ones pruned when they timeout/are rejected by the server. What do you think?

If we go for the override way, there would be no way of returning an empty set of candidates from the server side, although not sure if this would be an interesting scenario.

cdh4u commented 11 months ago

As an ICE restart by definition resets the previous state (excluding the roles), I think we have to assume that the previous candidate set is gone.

I think the main reason for the confusion is because you are using sdpfrag (which is defined for incrementing candidates) with ICE restart (which is defined for reseting/renegotiating candidates). Note that ICE only specifies usage of sdpfrag for trickle - not for ICE restart.

murillo128 commented 11 months ago

I don't think we should specify what happens with the current ICE candidates when the ICE restart happens, we should rely on whatever jsep has specified. I have been trying to find what is that behavior, but I am not able to find were it is defined.

cdh4u commented 11 months ago

I haven't checked JSEP, but Section 9 of RFC 8445 says:

"An ICE restart causes all previous states of the data streams, excluding the roles of the agents, to be flushed."

But, simply referring to JSEP and/or 8445 does not solve the issue. The problem is that WHIP uses sdpfrag with ICE restarts, and sdpfrag is defined for providing additional candidates, assuming that the previous still apply.

murillo128 commented 11 months ago

Ok, I see what you mean, you are referring about how to modify the SDP based on the received sdpfrag, and not about what does the client ICE agent do with the candidates.

I am fine with discarding all previous ICE candidates when doing the restart.

murillo128 commented 11 months ago

please let me know if https://github.com/wish-wg/webrtc-http-ingest-protocol/commit/b6ee4d50af0e7556c4db9dade397b1624db68656 would close this issue

cdh4u commented 11 months ago

please let me know if b6ee4d5 would close this issue

Q1: The text says that the WHIP session OPTIONALLY resturns a new set of candidates, and then you add text that the WHIP client MUST discard the previous candidates. What happens if the WHIP session does not return any candidates? I think the WHIP session MUST return candidates.

Q2: There is still the issue that sdpfrag is used for something it was not defined for. Is there a reason why applicatoin/sdp cannot be used when doing ICE restart?

murillo128 commented 11 months ago

Q1: Yes, not sure what I was thinking that providing an empty set of candidates was useful at all. I will change that.

Q2: I don't want to send a full sdp and then have to ensure that there has been no changes in anything except the ice stuff. Regarding RFC 8840 not covering ICE restarts, it is a problem about how to technically define it or about marking this draft updating rfc 8840?

murillo128 commented 11 months ago

@cdh4u I have created https://github.com/wish-wg/webrtc-http-ingest-protocol/pull/169/files in an attempt to clarify the extension of ICE restarts for sdp-frag over RFC 8840