Re: [rtcweb] [Gen-art] Genart last call review of draft-ietf-rtcweb-jsep-21

[Alissa Cooper <[e-mail]>]

Robert, thanks for your review. Justin, thanks for your response. I have entered a Yes ballot.

Alissa


> On Aug 26, 2017, at 2:10 PM, Justin Uberti <[e-mail]> wrote:
> 
> Thanks for this careful review. A new version of the document (-22) has been posted which addresses almost all of these comments, with a few exceptions noted below.
> 
> Diff: https://www.ietf.org/rfcdiff?url1=draft-ietf-rtcweb-jsep-21&url2=draft-ietf-rtcweb-jsep-22&difftype=--html <https://www.ietf.org/rfcdiff?url1=draft-ietf-rtcweb-jsep-21&url2=draft-ietf-rtcweb-jsep-22&difftype=--html>
> 
> On Tue, Aug 8, 2017 at 12:31 PM, Robert Sparks <[e-mail] <mailto:[e-mail]>> wrote:
> Reviewer: Robert Sparks
> Review result: Almost Ready
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>>.
> 
> Document: draft-ietf-rtcweb-jsep-21
> Reviewer: Robert Sparks
> Review Date: 2017-08-08
> IETF LC End Date: 2017-08-11
> IESG Telechat date: Not scheduled for a telechat
> 
> Summary: Almost ready for publication as a Proposed Standard RFC, but with
> issues that need to be addressed before publishing
> 
> Major issues:
> 
> The last paragraph of 5.1 needs more context. Why woudn't you expect the
> re-offer to fail, given that you already know the peer is using the older
> profile strings? It's not clear from the email list and proceedings how the
> document ended up with this particular text.
> 
> Minor issues:
> 
> Second paragraph of 3.2, last sentence: at "these parameters or reject
> them" - "these" and "them" have ambiguous antecedents. It's not clear if you
> are trying to point to the parameters that don't follow the earlier stated
> rule, or all parameters. The sentence is complex. Splitting it up and
> finding a way to avoid the need for the semicolon will likely make it
> easier to say what you mean.
> 
> Last paragraph on page 5 - first sentence: at "One way to mitigate this" -
> "this" has an unclear antecedent. Did you mean "One way to reduce the
> complexity of the application's responsibilities"?
> 
> Last paragraph of 3.5.2.1 - "all of these fields" is ambiguous. Please
> state precisely which fields you mean.
> 
> Odd edge: The description createAnswer (in 4.1.7) leaves open the
> possibility that the application could call createAnswer twice
> (back-to-back with no intervening calls to other APIs) with different
> values in the options parameter. Is this ok? If not, should there be
> language that says not to do that somewhere?
> 
> Slightly more than minor maybe: The first paragraph of 5.1.2 has at least a
> reference error. UDP/TLS/RTP/SAVPF is not specified in RFC7850. It's in
> RFC5764. Assuming that this is only a reference error, then the last
> sentence seems like a stray. It doesn't seem to be in the right context
> ("this advertisement" doesn't cast back to the discussion of the two
> profiles above it, which are both UDP only). Please clarify the sentence,
> giving it the right context, or remove it.
> 
> The fourth sentence of the second bullet of section 5.2.1 is confusing. (It
> starts "It is RECOMMENDED that the <sess-id>".) RFC3265 requires the
> initial sess-id high-order bit to be zero. As written, this can be read to
> say that setting that bit to zero is only RECOMMENDED, and that it's
> possible to have sess-ids that are something other than 64 bits. You mean
> to say "Do what 3265 says to do and make the lower 63 bits random". Here's
> one possible replacement for the sentence: "RFC 3265 requires that the
> high-order bit of the initial <sess-id> be zero. It is RECOMMENDED that the
> remaining 63 bits be initially set to a cryptographically random value."
> 
> The last paragraph of 5.3.1 prohibits the same attributes from generated
> answers that are prohibited in generated offers. Consider adding text for
> what to do if an offer shows up from a peer with some of those prohibited
> attributes.
> 
> This is covered by the last paragraph on page 43:
> 
>    Note that these requirements are in some cases stricter than those of
>    SDP.  Implementations MUST be prepared to accept compliant SDP even
>    if it would not conform to the requirements for generating SDP in 
>    this specification. 
> 
> 
> The second paragraph of section 5.4 is confusing. A slight restructuring of
> the sentences would remove much of the potential confusion, I think. I
> suggest replacing the first sentence (which is very complex) with this:
> "After calling setLocalDescription, the application MAY modify the SDP to
> reduce the capabilities in the offer before sending it to the far side, as
> long as it remains a valid SDP offer and specifies a subset of what was in
> the original offer. Likewise, an application that has received an offer
> from a peer MAY modify the received SDP, subject to those same constraints,
> before calling setRemoteDescription."
> 
> The first paragraph of section 5.7.1 is simply restating requirements from
> RFC4566. Restating normative requirements like that has led to interop
> problems through unintended introduced ambiguity and spec maintenance
> problems. Please consider rewriting the paragraph to say "RFC4566 says" and
> avoid the 2119 keywords.
> 
> We considered this, but found that a) RFC4566 is clearly cited, and b) the actual restatement was very minor, really just affecting the discussion of the v= and o= lines. As a result we concluded that pointing the reader elsewhere just for those two lines did not seem like an improvement.
>  
> 
> On page 69, at "If the application attempt to transmit DTMF when using a
> media format that does not have a corresponding telephone-event format,
> this MUST result in an error". This sentence is out-of-place. The section
> is about applying a remote description. If the document needs to retain the
> sentence, it belongs off in some "processing media" section. In particular,
> the current placement of text is confusing about what and when an error
> could be returned, and it could be misread to say that an error should be
> returned to the setRemoteDescription call.
> 
> Nits:
> 
> Section 1.2 second paragraph - "This was rejected based on a feeling". Can
> you write something that speaks to consensus here? "Based on a feeling"
> isn't quite right. Maybe "using the argument"?
> 
> PeerConnection is used first at the next to last paragraph of section 3.
> Please add a reference at that point.
> 
> "LS" (lipsync) is first used at 4.1.2, with no context for what it is.
> Please add a reference.
> 
> The concept of a "pending local description" is used in sections 3.5.1,
> 4.1.6, and 4.1.7  but isn't really explained until much later in the
> document. Please provide forward references, or move a description of what
> a pending description is earlier in the document.
> 
> In 4.1.7 (create Answer) why would calling setLocalDescription cause the
> answer to be a _pending_ local description. I think it's the full on local
> description at that point, not a pending one?
> 
> At 4.1.17's first paragraph: Why is "if parsed successfully" necessary to
> call out? I suggest removing the phrase.
> 
> 4.2.3 talks about setting direction properties, but the values that can be
> set aren't called out (and even then only by inference from a list in an
> example) in section 4.2.5. Should there be explicit pointers to the api doc
> here? (And in section 4.2.6?)
> 
> There are two paragraphs on page 50 that start "The next step is to". They
> are both referring to the same step. I suggest combining the paragraphs.
> 
> The 4th bullet on page 64 is ambiguous at "this m= section". It looks like
> this bullet should just be merged with the previous bullet.
> 
> The second paragraph of 6.8 starts "Next, ". I think it should start
> "First, ".
> 
> It's not clear what the "Or," in the first bullet on page 65 is referring
> to. Are you trying to say "If the m= section is not new, and the ICE ufrag
> and password values have changed" or something else?
> 
> The intent here is as stated, "if the m= section is not new and the values have changed". We discussed this but concluded that the meaning of the current text was clear.
>  
> 
> In the last paragraph of section 8 at "programmer MUST recognize". That is
> not an appropriate use of 2119. I suggest "needs to be aware" instead of
> "MUST recognize".
> 
> Micro Nits:
> 
> Suggested change to the first sentence of 1.1
> 
>   OLD:
> 
>     The thinking behind WebRTC call setup has been to fully specify and
> control the media plane, but to leave the signaling plane up to the
> application as much as possible.
> 
>   New:
> 
>     WebRTC call setup has been designed to focus on controlling the media
> plane, leaving signaling plane behavior up to the application as much as
> possible.
> 
> The first full bullet on page 18 has a sentence that starts "Note that when
> considering". Please change that to simply "When considering". As written,
> there is an implication that this is a consequence of something specified
> somewhere else.
> 
> The verb "surfaced" appears three times in the document without support or
> definition. Can you replace them with simpler language?
> 
> At the second to last sub-bullet at the end of page 66, please remove "note
> that". The sentence is stronger without it.
> 
> I noted these sections as particularly hard to read when going through the
> document linearly. I don't have concrete suggestions for improvement, but
> perhaps other eyes (if they agree the sections could use improvement) can
> come up with something:
>  - First sentence of the first paragraph on page 19.
>  - 2nd sentence of the 4th paragraph on page 19.
> 
> 
> _______________________________________________
> rtcweb mailing list
> [e-mail] <mailto:[e-mail]>
> https://www.ietf.org/mailman/listinfo/rtcweb <https://www.ietf.org/mailman/listinfo/rtcweb>
> 
> _______________________________________________
> Gen-art mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/gen-art