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

From: "Ben Campbell" <[email-masked]>
Date: Fri, 27 May 2016 17:17:35 -0500

[Oops, I sent that from the wrong address. Please send any discussion to 
[email-masked]]

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.

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

---

From: Lorenzo Miniero <[email-masked]>
Date: Sat, 28 May 2016 10:17:07 +0200

On Fri, 27 May 2016 17:17:35 -0500

> 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

---

From: Lorenzo Miniero <[email-masked]>
Date: Tue, 31 May 2016 13:12:23 +0200

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

> [Oops, I sent that from the wrong address. Please send any discussion
> to [email-masked]]
> 
> 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.
> 

Hello again, Ben,

I just posted a new version of the document (an announcement should get
to the ML soon) that tries to address most, if not all, of your points.
You can find some additional comments inline on the parts I haven't
addressed yet (or I only partly addressed), for one reason or another.
To make this more readable, I snipped the parts that I already
integrated.

> existing b2bua implementations to be updated?

I already answered to this in my previous mail, but just to reinstate,
I'll leave the decision to WG and chairs/ADs as to what the intended
status should be. I personally would keep it a proposed standard, but
again wouldn't object to a BCP, if needed.

>    existing b2buas do not follow specs is a reason to create more
> specs. That seems
>    questionable.
> 

Fair point. I haven't changed that part in the doc yet, as I wanted to
be sure I'd get it right, first. Do you think it would be ok to say
something along the lines of "due to the mixed nature of B2BUAs, they
sometimes can't or won't implement all specifications, so they should
at least do this"?

>             help nonetheless" - Help how? Please elaborate, or
> consider dropping the text in the parentheses.
> 

This came up in a STRAW meeting some time ago. The main scenario for
that is WebRTC usage, where browsers typically advertize the SSRCs
they're going to use in advance. The RTCWEB RTP usage drafts,
specifically, mandates support for signalled SSRC identifiers (see
Section 4.8), although it also clarifies that endpoints should be
prepared to handle SSRCs that were not advertized. The text in the
parenthesis wanted to refer to that kind of usage, but I realize the
wording was pretty awful, so I dropped it as you suggested.

>       then this might be a good place for a MUST or SHOULD

Do you mean where we say "behaviour is unspecified and discouraged"? Or
more in general about the fact that a B2BUA can only follow the
guidelines when it has access to editing fields without breaking
hashes? Not sure which MUST or SHOULD we might add there, considering
that, if the media the B2BUA is encrypted, it can't possibly edit RTP
related information without disrupting the communication, which in
turn means no RTCP editing will be needed/possible either.

Do you think a "B2BUAs MUST NOT modify RTP/RTCP data if media is
encrypted" to be enough, in case?

>        IETF is trying to address" means.

It was an awkward attempt at referring to the scope of the STRAW WG
itself. I removed the text after "situations".

>            have a specific reason not to forward? (i.e. default to 

The issue here is the reason not to forward itself. In order to figure
out whether an attribute can be forwarded or not, the B2BUA must have
some knowledge of it and of its impact. What I wanted to highlight in
that sentence is that there are attributes that, if left where they are
by a B2BUA unaware of their purpose, can actually cause issues. Just
think, for instance, of 'rtcp' attribute that is explained in the text,
or other attributes supporting features a B2BUA may not support itself.

Of course, as you point out there could be reasons to actually keep
attributes you don't know about in there, e.g., because they advertize
support for an end-to-end feature the B2BUA doesn't need to worry
about. Besides, the attributes that can cause issues are probably not
that many anyway.

I'll think about a way to make this clearer in the next for next
version.

Thanks!
Lorenzo