Last Call Review of draft-ietf-wish-whip-09
review-ietf-wish-whip-09-artart-lc-leiba-2023-07-29-00
review-ietf-wish-whip-09-artart-lc-leiba-2023-07-29-00
Thanks very much for addressing the issues I raised in my early review on the IANA Considerations. Those are all satisfactory now. This is a Last Call review where I look at the rest of the document, and I have a few comments. None of the Figures are mentioned in the text. I found that jarring because as I read I just suddenly encountered something that didn’t seem to fit. When I got to the bottom of it and found the “Figure” caption, I then looked for a reference to it to see why it was there, and found nothing. I think it would be useful to have something in the text before each figure appears, which would tell the reader that the figure is an example showing <whatever> so that the reader knows what’s going on. — Section 4 — The SDP offer SHOULD use the "sendonly" attribute and the SDP answer MUST use the "recvonly" attribute in any case. My usual thing with SHOULD: what happens if the offer does not do this? What is the interoperability issue that prompts the “SHOULD”, so that an implementer knows what the tradeoffs are? — Section 4.1 — The initial offer by the WHIP client MAY be sent after the full ICE gathering is complete with the full list of ICE candidates, or it MAY only contain local candidates (or even an empty list of candidates) as per [RFC8863]. I found this sentence a bit hard to read for a couple of reasons: the word ordering makes its meaning unclear and the two MAY key words are awkward together. Because both MAYs are describing something optional, I don’t know what happens if the client does neither. I THINK you mean the following, but please adjust this suggestion if I don’t quite have it right: NEW The WHIP client MAY take advantage of ICE PAC [RFC8863], so it might not wait to gather the full list of ICE candidates and might instead send its initial offer with only the local candidates or even with an empty candidate list. END Because the WHIP client needs to know the entity-tag associated with the ICE session in order to send new ICE candidates, it MUST buffer any gathered candidates before it receives the HTTP response to the initial POST request or the PATCH request with the new entity-tag value. I initially read this as saying that the WHIP client can’t receive the HTTP response until after it buffers gathered candidates. Again, the word order here makes this unclear. I suggest this: NEW Because the WHIP client needs to know the entity-tag associated with the ICE session in order to send new ICE candidates, it cannot send them until after it receives the HTTP response that contains the entity-tag value. The client MUST therefore buffer any gathered candidates until it receives that response. END — Section 4.2 — This version of the specification only supports, at most, a single audio and video MediaStreamTrack in a single MediaStream It feels odd to say “at most a single one”. Does it make sense for there to be zero? I suggest removing “at most” and the commas around it. When a WHIP client sends an SDP offer, it SHOULD insert an SDP "setup" attribute with an "actpass" attribute value, as defined in [RFC8842]. However, if the WHIP client only implements the DTLS client role, it MAY use an SDP "setup" attribute with an "active" attribute value. If the WHIP endpoint does not support an SDP offer with an SDP "setup" attribute with an "active" attribute value, it SHOULD reject the request with a "422 Unprocessable Entity" response. NOTE: [RFC8842] defines that the offerer must insert an SDP "setup" attribute with an "actpass" attribute value. However, the WHIP client will always communicate with a Media Server that is expected to support the DTLS server role, in which case the client might choose to only implement support for the DTLS client role. I found those two paragraphs to be very confusing and I’m still not sure I follow what they’re trying to say. They appear to go together, such that I think they could be merged and reworded to make the point more clearly and concisely. Can you try, please? — Section 4.4 — In order to support these clients, the WHIP endpoint MAY also include the STUN/ TURN server configuration on the responses to OPTIONS request sent to the WHIP endpoint URL before the POST request is sent. However, this method is not NOT RECOMMENDED and if supported by the underlying WHIP Client's webrtc implementation, the WHIP Client SHOULD wait for the information to be returned by the WHIP Endpoint on the response of the HTTP POST request instead. This combination of MAY and NOT RECOMMENDED (equivalent to “SHOULD NOT”) is contradictory: it doesn’t make sense to say “you MAY do X, but you SHOULD NOT.” It also seems odd to talk about how the endpoint can accommodate a client limitation and then to talk about what the client should do instead. It seems like you’re telling the client how to work around its own limitation, and I don’t understand the point. In any case, the contradiction definitely needs to be fixed. — Section 5 — On top of that, the WHIP protocol exposes a thin new attack surface expecific of the REST API methods used within it: What is “expecific”? All three bullets after that contain numerous typos and usage errors (“setup” instead of “set up”, “enought”, “legit” instead of “legitimate”, “URsL”, “abalanche”, and several more); please look at the text closely and correct them.