Re: [straw] Review of draft-ietf-straw-b2bua-rtcp-10

[Lorenzo Miniero <[e-mail]>]

On Fri, 27 May 2016 17:17:35 -0500
"Ben Campbell" <[e-mail]> wrote:

> [Oops, I sent that from the wrong address. Please send any discussion to 
> [e-mail]]
> 
> Hi,
> 
> Here is my review of draft-ietf-straw-b2bua-rtcp-10. I mentioned in my 
> AD evaluation of a previous version that the draft needed editing for 
> clarity and readability. In my opinion, this version is improved, but 
> still a bit rough in places. I have a few substantive comments and quite 
> a few editorial comments.
> 
> This is not a formal AD evaluation. That will occur after the chairs 
> submit a new publication request. But I think we need to work through 
> this before it will be ready for IETF last call.
> 


Hello Ben,

thanks for this detailed review! I'll address this soon and come back with an updated version in the next few days.

Just a quick comment on the intended status. I believe it was defined a Proposed Standard as the idea was to discipline the behaviour of B2BUAs, as other documents in this WG do on other aspects (e.g., DTLS-SRTP). Considering the document is called "Guidelines", I personally wouldn't object to turn it into a BCP, although I think keeping it a Proposed Standard would strenghten the importance of doing it right. That said, would a BCP document still need the MUSTs in some places where we have SHOULDs?

Thanks,
Lorenzo


> Thanks,
> 
> Ben.
> 
> ------------
> 
> 
> ### Substantive Comments:
> 
> * The intended status is proposed standard. Do people really expect 
> b2bua vendors to implement the rules in this draft? Do they expect 
> existing b2bua implementations to be updated?
> 
> * Abstract - Is it really true that media passthrough b2buas need
>    to implement the full RTP stack?
> 
> * Section 1:
> 
>      * 2nd paragraph - The following has been overtaken by events:
> 
>       >"These topologies are currently being updated to address new   
> commonly
>       >encountered scenarios as well [RFC7667]."  
> 
>      * paragraph after figure 1, last sentence - Why does it matter that 
> the RTP packet is audio?
> 
> * Section 3, last paragraph - This paragraph seems to argue that the 
> fact that
>    existing b2buas do not follow specs is a reason to create more specs. 
> That seems
>    questionable.
> 
> * Section 3.1:
> 
>      * Second Paragraph
>           * 2nd sentence: Do you mean if the endpoints sent
>             bad SDP, or if the b2bua messes up the SDP?
>           * "but having them synced during setup would
>             help nonetheless" - Help how? Please elaborate, or consider
>             dropping the text in the parentheses.
> 
> * Section 3.2
> 
>    * third paragraph from end: "...it SHOULD update the related 
> ’ssrc’
>      attributes...": Why not MUST? Can you envision reasonable 
> circumstances in which
>      one might do follow this?
> 
> 
>    * last paragraph, 2nd to last sentence - Wouldn’t the primary 
> reason to do this
>     be because the endpoint on the non-supporting side didn’t support 
> reduced-size? Wouldn’t
>     the b2bua need to advertise the feature to discover that?
> 
> * Section 4
>     * Last paragraph, 2nd to last sentence - If you think people will 
> really pay attention,
>       then this might be a good place for a MUST or SHOULD
> 
> * Section 6
>      * The first paragraph describes this draft as a "summary" and "best 
> common practice overview"
>        That seems to argue against the proposed standard status (see 
> previous comment).
> 
> 
> ### Editorial Comments and Nits:
> 
> * IDNits reports unused references for RFCs 4568, 5764 and 4588
> 
> * Please consider proofreading for unnecessary filler words, such as
>   "besides", "basically", "just", "also", "as well", "instead,", "as 
> such", etc.
>    To some degree, this is a matter of style, but when used heavily
>    they can impact readability.
> 
> * Section 1, First Paragraph:
> 
>      * s/completelely/completely
>      * I'm not sure what "... can lead to unexpected situations the
>        IETF is trying to address" means.
>      * does "most deployed" mean "most commonly deployed"?
>      * 2nd paragraph - The following is convoluted and hard to parse.
>        Can you simplify it?
> 
>         >"...manipulate the session description [RFC4566], in order to
>         >have all RTP and RTCP [RFC3550] pass through it as well within
>         >the context of an SDP offer/answer [RFC3264]."  
> 
>      * Third paragraph - I'm not sure what this means:
> 
>         >"... their effectiveness may be affected as a consequence"  
> 
>      * Paragraph after Figure 1:
> 
>       >"As part of this process, though, it is also rewriting some of   
> the
>       >RTP header information on the way, for instance because that’s   
> how
>       >its RTP relaying stack works:"  
> 
>          * That basically says it rewrites RTP headers because it does.
>            That’s not a reason. I suggest either offering a real 
> reason,
>            or don’t try to offer a reason at all.
>          * The antecedent for "it" is ambiguous
> 
>      * 2nd paragraph from end of paragraph
>          * Sentence starting with "Since the B2BUA" is very hard to 
> parse.
>          * Sentence starting with "As a consequence" is also hard to 
> parse.
> 
>      * Last paragraph in section, last sentence
>          * Should "such feedback" be "RTCP"?
>          * Are there things they _are_ supposed to break?
> 
> * Section 3:
> 
>      * First paragraph:
>          * s/anticipated/described ; (or mentioned).
>          * 2 of the 3 categories have numbers, but the 3rd does not.
> 
> * Section 3.1
> 
>      * 2nd paragraph:
>          * s/"in case"/"when"  (or "if"). (This pattern occurs 
> repeatedly)
>          * s/"is generating"/"generates"
>          * What does "taking into account the original session 
> description"
>            mean? Maybe s/"taking into account"/"using"
> 
>      * Third paragraph:
>          * Are there uninterested participants?
>          * "It SHOULD NOT, though, blindly forward all SDP attributes," 
> - Isn’t
>            the idea that it should forward any attributes that it 
> doesn’t
>            have a specific reason not to forward? (i.e. default to 
> forwarding?)
> 
>      * Fourth paragraph, first sentence: Hard to parse. Does this mean 
> the
>        same as: “Aside from RTCP, the relay may need to hide 
> information due
>        to local policy.”?
> 
> * Section 3.2:
> 
>       * First paragraph - Would it make sense to s/"is able to 
> inspect"/"inspects" and
>         s/"be able to modify"/"modify"?
> 
>       * Third paragraph: Consider s/"is receiving"/"receives".
> 
>       * Fourth paragraph: I can't parse the sentence.
> 
>       * Feedback messages - It’s a little confusing having the 
> following FCI
>         entries at the same section outline level as the feedback 
> message entry.
>         Consider making “Feedback messages” a separate section.
> 
>       * ECN Section - The section is hard to parse.
> 
>       * Third paragraph from end:
>           * consider s/"is going to change"/"changes"
>           * 2nd to last sentence: Does this mean “prevent a UAC from 
> _sending_
>             NACK messages that would never reach the intended 
> recipients”?
>           * last sentence - There’s something wrong with this 
> sentence. Do you mean
>              all three instances of "RTCP message" to really be be "RTCP 
> message"?
> 
>       * Second to last paragraph:
>           * First sentence is hard to parse
>           * "not use it on the other side, if it’s not supported." - 
> If what doesn't support it?
>             The b2bua? The endpoint?
>           * "any of the parties in the communication does not support" - 
> s/does/do
>           * s/involve/use
>           * "no conflicting RTP payload type numbers on both sides" - 
> should this say "either side"?
>           * "ensure no conflict" - I suggest "prevent conflict" or 
> "ensure that there is no conflict"
>           * What do you mean by "domain" in this context?
> 
> * Section 3.3, last paragraph, last sentence - Sentence hard to parse.
> 
> * Section 4 - Seems like this belongs in security considerations
> 
>       * Third paragraph - s/Terminarion/Termination
>