Subject: [Wish] Implementation report for draft-ietf-wish-whip-00

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 09 Sep 2021 18:19:06 +0200

Hi,

Lorenzo and I have just shown interoperability between his client and my
server.  Lorenzo's client was almost perfect, the only issue was that he
was sending ICE candidates to the wrong endpoint.  I haven't implemented
ICE restarts yet.

A few preliminary conclusions.

1. Even in its current early form, the draft is good enough to implement.

2. Asymmetric ICE (blocking ICE at the server, trickle at the client) was
   surprisingly easy to implement and works quite well.  The only issue we
   noticed is that since the ICE candidates are sent to the resource
   endpoint, the client must buffer ICE candidates until it receives the
   initial answer.  I think this deserves to be mentioned explicitly.

3. The draft lacks guidance about error handling; for example, it doesn't
   tell me what to do if appling the offer fails (500?), if I cannot parse
   an offer or an iCE candidate (400?), or if I receive an unexpected HTTP
   method or content-type.

   A couple tricky ones:
   
     - what is the error to return when I receive an ICE candidate for
       a PeerConnection that used to exist but has been closed?  I'm
       currently doing 404, which means the client cannot easily
       distinguish between this case and that of a wrong value for the
       resource endpoint.

     - what happens if a candidate is followed by random non-JSON garbage?
       Is that an error?  If so, MUST I return an error or MAY I return an
       error?  I'm currently just ignoring the garbage.

4. Detection of failed clients, as mentioned in a previous message.  It's
   a tricky issue, and needs to be specified precisely or people will get
   it wrong.

5. As somebody already mentioned, the draft lacks guidance about
   authentication.  I've implemented HTTP Basic on the WHIP endpoint, and
   I'm not sure what I should do on the resource endpoint: require HTTP
   Basic in the same realm as on the WHIP endpoint, require HTTP Basic in
   a different realm, or assume that the resource endpoint is a well-kept
   secret with enough entropy to not require authentication.

   Authentication is pretty much application-specific, so I think we'll
   need to be careful to provide precise guidance to the client without
   overly constraining the server.

-- Juliusz

---

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 09 Sep 2021 18:39:12 +0200

>        error?  I'm currently just ignoring the garbage.

Looking at the draft again, it doesn't look like the candidate is JSON,
it's just the raw SDP.  Since we're doing bundle, the sdpMLineIndex is
necessarily 0, so it doesn't need to be sent.

Is that right?

-- Juliusz

---

From: Lorenzo Miniero <[email-masked]>
Date: Thu, 9 Sep 2021 18:52:17 +0200

On Thu, 09 Sep 2021 18:39:12 +0200
Juliusz Chroboczek <[email-masked]> wrote:

> >      - what happens if a candidate is followed by random non-JSON
> > garbage? Is that an error?  If so, MUST I return an error or MAY I
> > return an error?  I'm currently just ignoring the garbage.  
> 
> Looking at the draft again, it doesn't look like the candidate is
> JSON, it's just the raw SDP.  Since we're doing bundle, the
> sdpMLineIndex is necessarily 0, so it doesn't need to be sent.
> 
> Is that right?
> 
> -- Juliusz
> 

Sergio pointed this out too in a private conversation, as the format of
the candidates to send in a PATCH was one of the assumptions I made in
my implementation. I chose to serialize them to a JSON object exactly
because of what you said (sdpMLineIndex), but if bundling is indeed
mandated, then it's unneeded and the "raw" candidate is fine. I'll fix
this in both server and client tomorrow.

What still isn't clear is whether or not grouping multiple candidates in
a single PATCH request (e.g., because we collected a few before we got
the answer back) is doable or not, and how they should be formatted in
case.

Lorenzo

-- 
I'm getting older but, unlike whisky, I'm not getting any better
https://twitter.com/elminiero

---

From: Juliusz Chroboczek <[email-masked]>
Date: Fri, 10 Sep 2021 11:01:44 +0200

> this in both server and client tomorrow.

Even though we don't need the sdpMLineIndex (due to bundling), we do need
the ufrag, so simply sending the candidate line won't do.

Not sure if we need the ice-passwd.  I hope we don't, since it's not part
of the RTCIceCandidate, so it would need parsing of the SDP on the client
side.

-- Juliusz

---

From: Lorenzo Miniero <[email-masked]>
Date: Fri, 10 Sep 2021 11:09:22 +0200

On Fri, 10 Sep 2021 11:01:44 +0200
Juliusz Chroboczek <[email-masked]> wrote:

> > Sergio pointed this out too in a private conversation, as the
> > format of the candidates to send in a PATCH was one of the
> > assumptions I made in my implementation. I chose to serialize them
> > to a JSON object exactly because of what you said (sdpMLineIndex),
> > but if bundling is indeed mandated, then it's unneeded and the
> > "raw" candidate is fine. I'll fix this in both server and client
> > tomorrow.  
> 
> Even though we don't need the sdpMLineIndex (due to bundling), we do
> need the ufrag, so simply sending the candidate line won't do.
> 
> Not sure if we need the ice-passwd.  I hope we don't, since it's not
> part of the RTCIceCandidate, so it would need parsing of the SDP on
> the client side.
> 
> -- Juliusz
> 

Yes, I checked RFC 8840 (my fault for missing it initially) and
ice-ugrag, ice-pwd and the "fake" m-line are all required. I'm updating
the code of both client and server right now to adhere to the spec.

Lorenzo

---

From: Juliusz Chroboczek <[email-masked]>
Date: Fri, 10 Sep 2021 15:35:57 +0200

> the code of both client and server right now to adhere to the spec.

I can easily parse this information in Galene, but won't that preclude
writing WHIP clients in Javascript?  Neither the m-line nor the ice-pwd
are readily available in the onicecandidate callback, so I don't see how
a Javascript client could possibly avoid parsing the local description in
order to get that information.

-- Juliusz

---

From: Lorenzo Miniero <[email-masked]>
Date: Fri, 10 Sep 2021 15:39:18 +0200

On Fri, 10 Sep 2021 15:35:57 +0200
Juliusz Chroboczek <[email-masked]> wrote:

> > ice-ugrag, ice-pwd and the "fake" m-line are all required. I'm
> > updating the code of both client and server right now to adhere to
> > the spec.  
> 
> writing WHIP clients in Javascript?  Neither the m-line nor the
> ice-pwd are readily available in the onicecandidate callback, so I
> don't see how a Javascript client could possibly avoid parsing the
> local description in order to get that information.
> 
> -- Juliusz

That is a bit awkward, yes. That's indeed how I've implemented it in my
client as well, as I'm not aware of a (simpler) way to retrieve the
information from GStreamer's webrtcbin either.

Lorenzo

---

From: Sergio Garcia Murillo <[email-masked]>
Date: Fri, 10 Sep 2021 16:04:31 +0200

> information from GStreamer's webrtcbin either.
I am implementing this in a js client as we speak. It should be possible to
retrieve the local  ice username/pwd from the ice transport object,
although not sure how widely implemented that API is on browsers. A simple
sdp parsing operation shouldn't be so difficult anyway compared to the
amount of ugly things we have to do to mungle it.. 😂

Best regards
Sergio

---

From: Juliusz Chroboczek <[email-masked]>
Date: Sat, 11 Sep 2021 11:56:30 +0200

> A simple sdp parsing operation shouldn't be so difficult anyway

I find it a little disappointing to me that you cannot implement WHIP
without SDP parsing.

My knowledge of ICE is very superficial, but perhaps we could make the
data optional so that only the data available in the onicecandidate
callback is compulsory?  For example, we could say that:

  - ice-pwd is optional;

  - m-line is optional when mLineIndex is 0.

I'm perhaps wrong, but the ufrag should be enough to discriminate between
generations, and the m-line is not required if we're doing bundling.

-- Juliusz

---

From: Lorenzo Miniero <[email-masked]>
Date: Sat, 11 Sep 2021 12:25:06 +0200

On Sat, 11 Sep 2021 11:56:30 +0200
Juliusz Chroboczek <[email-masked]> wrote:

> I'm perhaps wrong, but the ufrag should be enough to discriminate
> between generations, and the m-line is not required if we're doing
> bundling.
> 
> -- Juliusz
> 

I don't see the advantage of only putting ufrag and not pwd, to be
honest: if SDP parsing is the only way to get ufrag/pwd in specific
implementations, then once you've got one you may as well get the other
too. Same for the m-line: once you've done parsing, even in the case of
bundling it's easy to figure out what media the first m-line contains.

Lorenzo

---

From: Juliusz Chroboczek <[email-masked]>
Date: Sat, 11 Sep 2021 13:13:40 +0200

> I don't see the advantage of only putting ufrag and not pwd,

The ufrag is available in event.candidate.usernameFragment.  Unless I'm
mistaken, the pwd is not, it requires parsing the local description.

> Same for the m-line: once you've done parsing, even in the case of

The m-line is available in event.candidate.sdpMid, so no objection to
making it compulsory, even though I don't see how it is useful since we
mandate bundling.

-- Juliusz

---

From: Sergio Garcia Murillo <[email-masked]>
Date: Sat, 11 Sep 2021 14:35:24 +0200

You can use the ice transport apis to get username and password and only
fallback to sdp parsing in case browser don't support the apis

try {
//Get local ice properties
const local =
this.pc.getTransceivers()[0].sender.transport.iceTransport.getLocalParameters();
//Get them for transport
this.iceUsername = local.usernameFragment;
this.icePassword = local.password;
} catch (e) {
//Fallback for browsers not supporting ice transport
this.iceUsername = offer.sdp.match(/a=ice-ufrag:(.*)\r\n/)[1];
this.icePassword = offer.sdp.match(/a=ice-pwd:(.*)\r\n/)[1];
}

I am in favor of extending the abnf of the sdp fragment mime type [it is
supported by the rfc] to allo0w sending candidates at session level.
Mandating using RTP/,AVP at the m line seems really incorrect.

Not sure about changing the logic for not requiring username and password.
It's an edge case but the request for candidates and ice restart may be
received out of order and the username and password may be used for
discarding obsolete candidates, or worse end-of-candisates signaling.

Best regards
Sergio

---

From: Juliusz Chroboczek <[email-masked]>
Date: Sat, 11 Sep 2021 14:40:38 +0200

> received out of order and the username and password may be used for discarding
> obsolete candidates, or worse end-of-candisates signaling.

Just the password, not the ufrag.  The ufrag should be enough to
discriminate generations, no?

---

From: Sergio Garcia Murillo <[email-masked]>
Date: Sat, 11 Sep 2021 14:47:44 +0200

El sáb., 11 sept. 2021 14:40, Juliusz Chroboczek <[email-masked]> escribió:

> discarding

Yes, ufrag would be enough for this case.

Best regards
Sergio

---

From: Juliusz Chroboczek <[email-masked]>
Date: Sat, 11 Sep 2021 15:01:04 +0200

> Yes, ufrag would be enough for this case. 

Ok.  I've just pushed the new version of WHIP-aware Gelene.  It parses
RFC 8840 format, but ignores everything except ufrag and candidate.

Thanks again to Lorenzo for the client.

-- Juliusz

---

From: Adam Roach <[email-masked]>
Date: Sat, 11 Sep 2021 14:26:36 -0500

A couple of thoughts about some pints Juliusz raised:

>         distinguish between this case and that of a wrong value for the

The response code with the semantic you describe is 410, but I don't 
think we can require that servers make a distinction. Doing so requires 
maintaining historical records of all PeerConnections ever created. We 
could specify a window of time that servers need to remember this 
information, but I think we need to understand the value of the client 
knowing the difference before we imposed this kind of requirement on 
servers.

>         error?  I'm currently just ignoring the garbage.

Based on a lot of real-world experience with SIP, HTTP, and HTML, I've 
come around to the position that draft-iab-protocol-maintenance makes 
some good points, and that section 7 is particularly compelling. 
https://www.ietf.org/archive/id/draft-iab-protocol-maintenance-05.html#name-virtuous-intolerance

/a