Last Call Review of draft-ietf-wish-whip-09
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.