Re: [payload] AD Evaluation of draft-ietf-payload-flexible-fec-scheme-13

[Giridhar Mandyam <[e-mail]>]

Thank you for the careful review.  Before submitting rev 14, I would like to get agreement on the following proposed changes.
-Giri

> General: Do I read correctly that this document can completely stand alone from RFC 6363? That is, the calculation of the protection stream and reconstruction of lost source packets is completely specified in this document? I don’t see that as a problem, but if it’s correct some introduction text to that effect would be helpful. Is a normative reference to 6363 even needed?

FLEX FEC certainly was inspired by FEC FRAME, but the document can stand on its own.    Since several definitions are leveraged from RFC 6363, I believe a normative reference is still appropriate.  My suggestion is to add one sentence to the last paragraph of Section 1.

Proposed change:  Add "This document also leverages several definitions along with the basic source/repair header description from RFC 6363 in their application to the parity codes defined here."

>§2: Unless there’s a strong reason otherwise, please use the updated key word boilerplate from RFC 8174.

Proposed change:  will replace with text in Sec. 2 of RFC 8174.

>§4.2: "The FEC repair packets MUST contain information that identifies the source block...”: The MUST seems more like a statement of fact than a normative requirement.

Proposed change:  " The FEC repair packets will contain information that identifies the source block..."

>§4.2.1:
>- There’s a number of 2119/8174 keywords in this section that seem more natural consequences of using RTP than new normative requirements. If that is the case, they shouldn’t use normative keywords unless in the form of direct quotes.

Apologies - I had trouble identifying such instances.  All instances of such keywords are related to requirements with respect to RFC 3550.  Do you have specific examples?

>- 10th paragraph: "The repair streams’ SSRC’s CNAME SHOULD be identical to the CNAME of the source RTP stream(s) that this repair stream protects.” Why is the SHOULD not a MUST? What happens if it is violated?

Proposed change:  Agreed that the requirement makes more sense as a MUST.  Will change accordingly.

>- 11th paragraph: "such scenarios, using the same CNAME for the source and repair RTP streams means that the RTP Source and the FEC Source MUST share the same CNAME (for this specific source-repair stream association).” That MUST seems like a statement of fact.

Proposed change:  " such scenarios, using the same CNAME for the source and repair RTP streams means that the RTP Source and the FEC Source will share the same CNAME (for this specific source-repair stream association).”

>§4.2.2: Please add a short explanation of why we need two different FEC packet formats. (Not counting the retransmission packet; the reasoning there is pretty obvious.)

Proposed change:  Add additional sentence to first paragraph of Sec. 4.2.2:  "Two of these variants are meant to describe different methods for deriving the source data from a source packet for a repair packet.  The third variant is for a straight retransmission of the source packet."

>§5.1.1 (and subsequent media type definitions) - Please consider using the IESG ([e-mail]) as the contact for further information. Working groups come and go, and authors change jobs. But someone on the IESG should (hopefully) always be able to find the right person,

TENTATIVE proposed change:  I am happy to swap out Varun's email for IESG, but this does not seem to be a widespread practice even for recently-published RFC's.  Can you confirm that this is now becoming a best practice for contact info regarding registries?

>- Change control:  In 5.1.1, shouldn’t that be the PAYLOAD working group (or it’s successor as delegated by the IESG...)

Proposed change:  Change to "IETF Audio/Video Payloads Transport Payloads Working Group"

>- Are these registrations really intended to be provisional?

At this point, no.

Proposed change:  Eliminate mention of provisional registration for all media types.

>§5.2.1, last bullet: "Any unknown option in the offer MUST be ignored and deleted from the answer.”: Is that a a new requirement specific to flex-fec, or just a normal offer/answer requirement?

RFC 3264 Section 6 states
'For each "m=" line in the offer, there MUST be a corresponding "m=" line in the answer.  The answer MUST contain exactly the same number of "m=" lines as the offer.  This allows for streams to be matched up based on their order.  This implies that if the offer contained zero "m=" lines, the answer MUST contain zero "m=" lines.'

I don't believe this is normal offer/answer.  It is specific to FLEX FEC.

>§6.3.1.1, last paragraph: This is the only mention of “staircase” in the draft. Please add or cite a definition.

This is leftover from an early version of the spec where additional (non-parity) codes were mentioned in the spec.  

Proposed change:  Eliminate mention of staircase.  Any derivative specifications requiring this definition (e.g. LDPC) can produce the definition in their context.

>§8: - "the potential impacts of using FEC MUST be considered” is vague for a MUST. Who/what does this apply to? Is the implementor expected to know in advance whether their implementation will be used in a congestion-prone network? (If that is the intent, I suggest non-normative guidance.)

This is dependent on many considerations (e.g. is this being used on IP multicast network, is rate control possible, etc.).  Normative guidance is not applicable.

Proposed change:  " the potential impacts of using FEC should be considered "

>- “In a network-friendly implementation”: Do you expect to see network-unfriendly implementations? Is that okay?

I don't view "network-friendly" as being a static condition.  There are scenarios where an FEC transmitter/receiver pair have negotiated a connection which is network-friendly for one application, but may not be for a different application.  There was a discussion about when FEC is appropriate and when it is not on the WebRTC mailing lists - see https://lists.w3.org/Archives/Public/public-webrtc/2018Nov/0156.html.   "Network-unfriendly" implementations could conceivably occur when an FEC session has been negotiated between endpoints, but neither endpoint has assessed transmission conditions. 

In short, I think the text is fine as is.  

>§9, last paragraph: What are the security implications of unused source-buffers? Is this a potential DoS vector?

It could be.  Unused source buffers combined with potentially large packet size (e.g. for bundled protection) could eventually result in a lack of resources (e.g. memory) in an endpoint.

Proposed additional clarifying text: Following  "Given that FLEX FEC enables the protection of multiple source streams, there exists the possibility that multiple source buffers may be created that may not be used" add  "An attacker could leverage unused source buffers to as a means of occupying memory in a FLEX FEC endpoint."

>- Abstract: I’m not sure how to interpret the phrase “decent complexity”. How much complexity qualifies as “decent”?

Proposed change:  remove the word "decent."

>§1, - first paragraph: Please add  or cite a definition for FEC. I’m not sure all readers will understand it the same way the authors do. (I was personally surprised to find discussion about reacting to feedback, since I thought of FEC as something used when no feedback channel was available.)

Proposed change:  Add after the first sentence in this paragraph "For a definition of FEC, please refer to Section 1 of [RFC6363]/"

>- 2nd paragraph: s/Redunadncy/Redundancy”

Proposed change:   "Redunadncy" changed to "Redundancy".  Thank you for catching this.

>§1.1.1: - It would be helpful to describe what L and D represent here, or at least give a forward reference to the definition. (I note that a lot of text goes by before you get to the  definitions or the 2119 boilerplate.)

Current text states:  " Consider a group of D x L source packets that have sequence numbers starting from 1 running to D x L, and a repair packet is generated by applying the XOR operation to every L consecutive packets as sketched in Figure 3."

I don't think that there is a further definition required.  However, an addition sentence could follow the above description to clarify. 

Proposed text:  "In general, D and L represent values that describe how packets are grouped together for parity code generation when interleaving all D x L source packets."

>- Does 1-D and later 2-D stand for “one dimensional” and “two dimensional”? If so, please expand  both first mention. (As it was, I got a bit confused by trying to conflate the “D” in “1-D” with the “D” in “LxD”.)

Yes it does.  

Proposed change:  Sec. 1.1 title changes to "One-Dimensional (1-D) Non-interleaved (Row) FEC Protection"
Proposed change:  Sec. 1.1.4 title changes to " Two-Dimensional (2-D) (Row and Column) FEC Protection"

>§1.1.5:  "assuming that each repair packet carries an equal number of bytes carried by a source packet,”
>s/ “bytes carried”/“bytes as carried”

Agreed.  Change made.

>§4.2, last paragraph: Missing article before ‘Repair “Payload” ‘

Proposed change:  "The repair "Payload" ..."

>§4.2.2, paragraph after Figure 10: The text seems to be suggest there is one source packet protected by a given repair packet. IIUC, that’s not the case; a given FEC packet protects several source packets, doesn’t it?

The text reads in part "... includes repair of everything following the fixed 12-byte RTP header of the source packet ...".  

"... everything following ..." implies more than just a single source RTP packet.  

> §5.2, first paragraph: s/ "Applications that are using RTP transport” / “Applications that use the RTP transport”

Agreed. Change made.

>§5.2.1, first bullet: I don’t know how to interpret “SHOULD normally”.

Proposed change"  Remove the word "normally"

>§5.2.2, first paragraph: Is there a reason to cite the obsolete RFC 2326?

This also came up as part of the I.-D. nits.  I decided to leave the 2326 reference in because I did not want to imply that FLEX FEC cannot be used for RTSP 1.0, and RTSP 2.0 is not backwards-compatible.

>§6.3.3: - list item 2: "The padding of octet 0 MUST be added at the end of the bit string.”: I assume that means “padded with zeros”, but it could be interpreted to mean “pad the zeroth byte”.

Proposed change:  "The zero-padding octets MUST ..."

>- list item 5: Should “num_recovered_until_this_iteration” say “before_this_iteration”?

The current Sec. 6.3.3 list item 5 reads " Append the recovered bit string (Y octets) to the new packet generated in Section 6.3.2."

If you are referring to the ensuing sections, I think the name of the variable is fine as is.

>§9, last paragraph: I don’t understand the intent of the last sentence.

Originally suggested in https://mailarchive.ietf.org/arch/msg/payload/AIHw5DSABEJLa6lSOLryLdI0eBk.  The application source data may not be perfectly matched with FLEX FEC source partitioning.  If this is the case, there is a possibility for unprotected source data if, for instance, the FLEX FEC implementation discards data that does not fit perfectly into its source processing requirements.

Additional changes to be made in next version:

Sec. 4.2.1:  "repaire" changed to "repair"

Figure 2 is incorrectly formatted (top of drawing).

Change IP address in example in Section 7.1.2 to a unicast address (192.0.2.0/24) as per discussion in https://mailarchive.ietf.org/arch/msg/payload/uDMO5bp2edlKX8ZPAPZiKBF4nIk. 


-----Original Message-----
From: Ben Campbell <[e-mail]> 
Sent: Tuesday, December 18, 2018 9:01 PM
To: [e-mail]
Cc: [e-mail]
Subject: AD Evaluation of draft-ietf-payload-flexible-fec-scheme-13

Hi,

This is my AD Evaluation of draft-ietf-payload-flexible-fec-scheme-13.

The document is overall in pretty good shape. It’s more readable than I expected, given the subject matter. I would like to discuss my substantive comments/questions prior to IETF LC.

Thanks!

Ben.

*** Substantive***

General: Do I read correctly that this document can completely stand alone from RFC 6363? That is, the calculation of the protection stream and reconstruction of lost source packets is completely specified in this document? I don’t see that as a problem, but if it’s correct some introduction text to that effect would be helpful. Is a normative reference to 6363 even needed?

 If that’s _not_ the intent, then we need some discussion about where I went wrong in reading it.

§2: Unless there’s a strong reason otherwise, please use the updated key word boilerplate from RFC 8174.

§4.2: "The FEC repair packets MUST contain information that identifies the source block...”: The MUST seems more like a statement of fact than a normative requirement.

§4.2.1:

- There’s a number of 2119/8174 keywords in this section that seem more natural consequences of using RTP than new normative requirements. If that is the case, they shouldn’t use normative keywords unless in the form of direct quotes.

- 10th paragraph: "The repair streams’ SSRC’s CNAME SHOULD be identical to the CNAME of the source RTP stream(s) that this repair stream protects.”
Why is the SHOULD not a MUST? What happens if it is violated?


- 11th paragraph: "such
scenarios, using the same CNAME for the source and repair RTP streams means that the RTP Source and the FEC Source MUST share the same CNAME (for this specific source-repair stream association).”

That MUST seems like a statement of fact.

§4.2.2: Please add a short explanation of why we need two different FEC packet formats. (Not counting the retransmission packet; the reasoning there is pretty obvious.)

§5.1.1 (and subsequent media type definitions)

- Please consider using the IESG ([e-mail]) as the contact for further information. Working groups come and go, and authors change jobs. But someone on the IESG should (hopefully) always be able to find the right person,

- Change control:  In 5.1.1, shouldn’t that be the PAYLOAD working group (or it’s successor as delegated by the IESG...)

- Are these registrations really intended to be provisional?

§5.2.1, last bullet: "Any unknown option in the offer MUST be ignored and deleted from the answer.”: Is that a a new requirement specific to flex-fec, or just a normal offer/answer requirement?

§6.3.1.1, last paragraph: This is the only mention of “staircase” in the draft. Please add or cite a definition.

§8:
- "the potential impacts of using FEC MUST be considered” is vague for a MUST. Who/what does this apply to? Is the implementor expected to know in advance whether their implementation will be used in a congestion-prone network? (If that is the intent, I suggest non-normative guidance.)
- “In a network-friendly implementation”: Do you expect to see network-unfriendly implementations? Is that okay?

§9, last paragraph: What are the security implications of unused source-buffers? Is this a potential DoS vector?


*** Editorial ***

- Abstract: I’m not sure how to interpret the phrase “decent complexity”. How much complexity qualifies as “decent”?

§1,

- first paragraph: Please add  or cite a definition for FEC. I’m not sure all readers will understand it the same way the authors do. (I was personally surprised to find discussion about reacting to feedback, since I thought of FEC as something used when no feedback channel was available.)

- 2nd paragraph: s/Redunadncy/Redundancy”

§1.1.1:

- It would be helpful to describe what L and D represent here, or at least give a forward reference to the definition. (I note that a lot of text goes by before you get to the  definitions or the 2119 boilerplate.)

- Does 1-D and later 2-D stand for “one dimensional” and “two dimensional”? If so, please expand  both first mention. (As it was, I got a bit confused by trying to conflate the “D” in “1-D” with the “D” in “LxD”.)

§1.1.5:
- "assuming that each repair packet carries an equal number of bytes carried by a source packet,”
s/ “bytes carried”/“bytes as carried”

§4.2, last paragraph: Missing article before ‘Repair “Payload” ‘

§4.2.2, paragraph after Figure 10:
The text seems to be suggest there is one source packet protected by a given repair packet. IIUC, that’s not the case; a given FEC packet protects several source packets, doesn’t it?

§5.2, first paragraph: s/ "Applications that are using RTP transport” / “Applications that use the RTP transport”

§5.2.1, first bullet: I don’t know how to interpret “SHOULD normally”.

§5.2.2, first paragraph: Is there a reason to cite the obsolete RFC 2326?

§6.3.3:
- list item 2: "The padding of octet 0 MUST be added at the end of the bit string.”: I assume that means “padded with zeros”, but it could be interpreted to mean “pad the zeroth byte”.

- list item 5: Should “num_recovered_until_this_iteration” say “before_this_iteration”?

§9, last paragraph: I don’t understand the intent of the last sentence.