Re: [rtcweb] [payload] WGLC on RTP Payload Format for Flexible Forward Error Correction

["Mo Zanaty (mzanaty)" <[e-mail]>]

Hi Stephen,

Thank you for the detailed review and comments. We are updating the draft to address your comments as noted below (see Mo: inline).

Thanks,
Mo

From: payload <[e-mail]<mailto:[e-mail]>> on behalf of Stephen Botzko <[e-mail]<mailto:[e-mail]>>
Date: Tuesday, November 21, 2017 at 4:22 PM
To: "Ali C. Begen" <[e-mail]<mailto:[e-mail]>>
Cc: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>
Subject: Re: [payload] WGLC on RTP Payload Format for Flexible Forward Error Correction

I have reviewed it, but I don't think it is ready for publication yet.

My commends follow.

BR
Stephen



>>>

Abstract

>>>

Overall, the purpose is to reconstruct RTP source packets that were lost in transmission, using the source and repair packets that were received.  I expected that to be stated somewhere in the abstract and introduction.

Mo: Agreed, some readers unfamiliar with FEC may benefit from a brief statement of the purpose of FEC, which we will add.



>>>

These parity codes are systematic codes, where a number of repair

   symbols are generated from a set of source symbols.

>>>

This is of course the usual FEC jargon.  But it isn’t clear if the symbols in this draft are bits, bytes, or the full source packets payloads.

Overall, the draft generally uses the phrase “source packet” and “repair packet”.  “repair symbols” shows up in section 4.2, and there it seems to mean bits or bytes.  The draft needs to clarify this, as the many implementers won’t have any real knowledge of FEC.



I suggest removing references to “symbols”, as the XOR construction and repair mechanisms really don’t need to talk about “symbols”.

Mo: Agreed, we will use packets not symbols throughout.

Also, “FEC packets” appears to be the same thing as “repair packets”.  It would be better to use one term for both.  Repair packets might be clearer (though they reconstruct lost packets, and don't actually repair them).

Mo: We will clarify that these are FEC repair packets, and any shorter name will be consistent throughout.


>>>

These repair symbols are sent in a repair flow separate from the source flow

>>>
The word “flow” appears 19 times in the document with no explanation on what exactly is meant.  I believe it simply means that the source and repair packets use different SSRCs (or different payload types?), but this is not as clear as it might be.

FWIW, I realize that “flow” is also used extensively in RFC6363.   But RFC6363 defines flows in terms of transport identifier (such as standard 5 tuple), and that definition doesn’t apply here, since source and repair flows can use the same 5 tuple.

Mo: We will avoid 'flow', which is also used extensively in RFC7656 RTP Taxonomy with no definition. The taxonomy terms relevant for this draft are Redundancy RTP Stream and Source RTP Stream. If this begins to sound awkward repeated many times, we will choose a consistent shorthand and avoid 'flow' (replace with stream).
>>>

Moreover, alternate FEC codes may be used with the payload formats presented.
>>>
Does this mean that the FEC codes might not be XOR?  If so, the document doesn’t say anything about how that is done (and the repair generation and recovery methods in the draft are specific to XOR).  So if means what is meant, the sentence should probably be removed.
If it doesn’t mean that, then what does it mean?
Mo: This will be removed. The original draft allowed other codes (e.g. Reed-Solomon or Raptor) to be negotiated, but the workgroup decided to remove this flexibility.
Section 1.1
>>>

   Note that the source and repair packets belong to different source

   and repair flows, and the sender must provide a way for the receivers

   to demultiplex them, even in the case they are sent in the same

   5-tuple (i.e., same source/destination address/port with UDP).
>>>
The “different source and repair flows” aspect of the note is repetitive, since it is also stated in step 3 immediately above this paragraph.  “Even in the case” is understated, “especially in the case” is closer to the truth of it.  It might be simpler to just say that source and repair packets MUST use different SSRCs  (or different payload types?), and delete this sentence.
Mo: This will be clarified when 'flow' is removed throughout.
>>>

The repair packets may be sent proactively or on-demand.
>>>
How are they sent on demand?  I don’t see any methods for requesting repair packets.  Either procedures should be defined/referenced or the sentence deleted.
Mo: We will clarify that on-demand means in response to RTCP NACK, or the new CC (ACK) feedback under design for RMCAT (draft-avtcore-cc-feedback-message). Or we can word this more generically for any current or future NACK or ACK based feedback messages.

>>>

In this document, we refer to the time that

   spans a FEC block, which consists of the source packets and the

   corresponding repair packets, as the repair window.  At the receiver

   side, the FEC decoder should wait at least for the duration of the

   repair window after getting the first packet in a FEC block, to allow

   all the repair packets to arrive.
>>>
Not sure if the intent is SHOULD wait or should wait.
Mo: We will clarify this as SHOULD wait, and more specifically, SHOULD buffer source and repair packets this long to allow for recovery upon packet loss.
More importantly, the FEC patterns in the draft are all defined to have a fixed number of packets.  The “time that spans an FEC block” is not constant, especially for variable-rate video.  The receiver has no idea how long that repair window time might be for a specific FEC block.
At some point the receiver decides it’s time to deliver the source packets – either because

(a)    It is receiving source packets with no break in sequence number, so there is no need to wait for repair

(b)    It has received enough repair packets to recover the missing source packets

(c)     It has received all the repair packets, and they aren’t sufficient to recover the missing source packets

(d)    It is choosing not to wait any longer.



Case (d) might be reached because it is starting to receive packets in the next FEC block, or it might be reached because of a real-time constraint


The wording in the above paragraph doesn’t cover these cases very well.  And the FEC decoder doesn’t always need to wait for all the repair packets to be received before it can begin the repair process.
It’s probably more useful to point out that using flexflec does add delay, and it needs to be clear that the repair window (in time units) is nominal.  Or maybe specify the repair window in packets?
Mo: repair-window (in microseconds) is a required parameter when negotiating this payload format. Fixed block size in packets is an optional parameter (L=columns, D=rows).
>>>

Suppose that we have 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.  This process is

   referred to as 1-D non-interleaved FEC protection
>>>
I think the document would be clearer if this was in new section – preceded by a short into saying how flexspec provides non-interleaved (row), interleaved (column), and hybrid row+column (2-D) FEC patterns.  This is a key concept in the draft, and should be more prominent.
I think there should also be four use-case sections.  Separate sections for row and column, and the retransmission mode (“R” bit) should probably have a short section too.
Mo: Agreed, we will separate into more sections.
>>>
1.1.3<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-1.1.3>.  Overhead Computation
>>>

Overhead isn’t used much in the document, and I am not sure this particular definition adds much value.  It might be better phrased as “FEC Overhead Considerations”, since the overhead fractions really don’t have much practical use, especially when the source packets are of unequal size.
Mo: Ok, we will rename this.
>>>
3.1<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-3.1>.  Definitions
>>>
FEC Block, and repair window should be defined here (neither are defined in RFC6363).  Parity Codes are not defined in RFC6363 either, and probably should be defined here.

1-D non-interleaved, 1-D interleaved, 2-D protection perhaps should be defined.

It might be cleaner to define source block here, since the RFC6363 definition doesn’t precisely apply (because its definition of ADU flow depends on 5-tuple).

In general there are very few definitions in RFC6363 that are used in flexfec, so if it were my draft I’d just redefine them explicitly here.
Mo: Ok, we will define what is used.
>>>
3.2<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-3.2>.  Notations
>>>
I’m not sure that bitmask needs to be here, though the text is ok.
Mo: We will keep it for clarity.

4.1<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-4.1>.  Source Packets
>>>

   The source packets MUST contain the information that identifies the

   source block and the position within the source block occupied by the

   packet.
>>>
This is a meaningless MUST, since the source RTP packet is simply sent without modification.  The sequence number and SSRC are used to map the packets onto the repair packets.
Overall, paragraph 4.1 is confusing, because it gives the impression that something else needs to be done, and then says that there isn’t.  Plus, it is supposed “define the format of the source packet” -  which it doesn’t do.

I think it’s better just to say earlier in the document that the source packets can be any RTP packets, and leave it at that.  Perhaps tie that statement to the use of systematic codes.  Then focus on defining the repair packet format, which is the heart of the draft.

The advantages of not modifying the source packets is better placed somewhere else in the document – perhaps in section 1.1, when the FEC encoder and Decoders are introduced.
Mo: Agreed, nothing normative to do here, so we will remove this MUST and describe what systematic codes are.
4.2<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-4.2>.  Repair Packets


>>>

   o  Marker (M) Bit: This bit is not used for this payload type, and

      SHALL be set to 0
>>>
And ignored by receivers?
Mo: Agreed, "SHALL be set to 0 by senders, and SHALL be ignored by receivers".

>>>

Note that this document

      registers new payload formats for the repair packets (Refer to

      Section 5<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-5> for details).  According to [RFC3550<https://tools.ietf.org/html/rfc3550>], an RTP receiver

      that cannot recognize a payload type must discard it.  This

      provides backward compatibility.  If a non-FEC-capable receiver

      receives a repair packet, it will not recognize the payload type,

      and hence, will discard the repair packet.
>>>
This probably should be said somewhere, but I think it’s better moved somewhere other than the repair packet format description.
Mo: Agreed, will move to a better section.
I believe this also requires that the CSRCs in the repair RTP header be set to the SSRCs of the source packets.  That isn’t stated, but is implied by the structure in table 10.
Mo: It is stated below Figure 10 in FEC Header (see CSRC_i), but should be moved up before Figure 10 in RTP Header.
>>>

The format of the FEC header is shown in Figure 10.

The FEC header consists of the following fields:
>>>
No.  This is the first of four? possible formats, depending on R,F,M, and N, shown in figures 10-15.  M is ambiguous - appears as M and M (columns).  Some fields are really repair symbols (P,X,C,M, PT Recovery, Length Recovery, recovery), and are meaningless on their own.  This rather important detail doesn’t show up until you get to 6.2.

Figure 15 also affects figure 9, since the unspecified “Repair symbols” in figure 9 are replaced by the “retransmission payload” In figure 15.  The contents of that “retransmission payload” are not specified anywhere in the draft.

This whole section is a confusing mess, and needs to be restructured.  I guess one approach is to change 6.2 to “Repair symbol construction”, and explicitly refer to it for all fields in the FEC header that are XORed.  One way or another, you need to separate out the fields that specify the FEC parameters from the embedded repair symbols mixed into the structure.
Mo: Agreed, implementors have also asked for clearer separation of the different repair formats, especially for FEC vs Retransmission, so we will separate this into clearer sections.
>>>

   o  The CSRC_i (32 bits) field describes the SSRC of the packets

      protected by this particular FEC packet.  If a FEC packet contains

      protects multiple SSRCs (indicated by the CSRC Count > 1), there

      will be multiple blocks of data containing the SN base and Mask

      fields.
>>>
Does this mean that a repair flow can protect multiple RTP sources?  If so, this should be clearly stated earlier.  There might be other implications of supporting this (do all the sources need to have the same CNAME???)
Mo: Yes, a repair stream can span multiple source streams. We will clarify this earlier. The same CNAME is noted earlier in this section under the SSRC bullet.
>>>

If M>0, N=0,  is Row FEC, and no column FEC will follow

               Hence, FEC = SN, SN+1, SN+2, ... , SN+(M-1), SN+M.



   If M>0, N=1,  is Row FEC, and column FEC will follow.

                 Hence, FEC = SN, SN+1, SN+2, ... , SN+(M-1), SN+M.

            and more to come



   If M>0, N>1,  indicates column FEC of every M packet

                    in a group of N packets starting at SN base.

                 Hence, FEC = SN+(Mx0), SN+(Mx1), ... , SN+(MxN).





             Figure 14: Interpreting the M and N field values
>>>
First, this is not a figure. Second, the names are not consistent with the 1-d non-interleaved, 1-d interleaved, and 2-d described above.  The names need to be consistent.  FWIW, these names are a bit more intuitive to me.
Mo: Agreed, we will add the 1-d non-interleaved, 1-d interleaved and 2-d labels to the row/column FEC shorthand names here.
>>>

Note that the parsing of this packet is different.
>>>
“this” is indefinite.  In general you should name the four(?) header types, I think that will make things simpler.
Mo: Agreed, the different repair formats will be separated into cleaer sections.
>>>

Figure 15: Protocol format for Retransmission
>>>
This is mis-titled.  It also isn’t clear.  The figure shows the alternative FEC header format, but I believe the intent is that the retransmission payload replaces the “repair symbols” shown in figure 9.  That isn’t obvious from the organization.
Mo: Agreed, the different repair formats will be separated into cleaer sections.

>>>
   It should be noted that a mask-based approach (similar to the ones
   specified in [RFC2733<https://tools.ietf.org/html/rfc2733>] and [RFC5109<https://tools.ietf.org/html/rfc5109>]) may not be very efficient to
   indicate which source packets in the current source block are
   associated with a given repair packet.  In particular, for the
   applications that would like to use large source block sizes, the
   size of the mask that is required to describe the source-repair
   packet associations may be prohibitively large.  The 8-bit fields
   proposed in [SMPTE2022-1<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#ref-SMPTE2022-1>] indicate a systematized approach.  Instead
   the approach in this document uses the 8-bit fields to indicate
   packet offsets protected by the FEC packet.  The approach in
   [SMPTE2022-1<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#ref-SMPTE2022-1>] is inherently more efficient for regular patterns, it
   does not provide flexibility to represent other protection patterns
   (e.g., staircase).
>>>
I have no idea why this note is here.  I’d delete it.  There's no point in talking about all the roads not taken.
Mo: Agreed, we will remove it.

>>>
5<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-5>.  Payload Format Parameters
>>>

If no specific FEC code is specified

   in the subtype, then the FEC code defaults to the parity code defined

   in this specification.
>>>

Since there is no syntax to specify the FEC code, a receiver has no way to know for certain that the parity code is being used.  I guess that the statement in 5.2.1 might cover this

Mo: This will be removed. The original draft allowed other codes (e.g. Reed-Solomon or Raptor) to be negotiated, but the workgroup decided to remove this flexibility.

>>>

“There are no optional format parameters defined for this payload.

      Any unknown option in the offer MUST be ignored and deleted from

      the answer.  If FEC is not desired by the receiver, it can be

      deleted from the answer.

>>>

For SDP at least, any FEC code in the offer would be deleted by an existing receiver, so if parity is mandatory-to-implement you’d get interoperability at least.  But why not just specify the FEC code parameter (with Parity as a choice) now?

Mo: The original draft allowed other codes (e.g. Reed-Solomon or Raptor) to be negotiated as a parameter, but the workgroup decided to remove this flexibility and require other codes to use other payload format names.

>>>


6.2<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-6.2>.  Repair Packet Construction



   The RTP header of a repair packet is formed based on the guidelines

   given in Section 4.2<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-4.2>.

>>>
Since 4.2 is a mess, it’s not surprising that 6.2 is also.
The FEC header length isn’t limited to 28 octets in the retransmission case (which is conveniently skipped in 6.2)
It would be good to specify what’s in the skipped bits in the header, rather than making people figure it out. (“V-2” and Sequence Number)

Also phrases like “The next 4 bits of the FEC bit string are written into the CC recovery field in the FEC header” should be written as like “The next 4 bits of the FEC bit string are XORed into the CC recovery field in the FEC header”

BTW, I don’t think the “FEC bit string” nomenclature is all that useful, given that you end up specifying the XOR field by field anyway.
Mo: Agreed, we will clarify this.
>>>

   The repair packet payload consists of the bits that are generated by

   applying the XOR operation on the payloads of the source RTP packets.

   If the payload lengths of the source packets are not equal, each

   shorter packet MUST be padded to the length of the longest packet by

   adding octet 0's at the end.
>>>
This presumably defines what is placed into the “repair symbols” back in figure 9.  But it doesn’t say that.
I believe this is incomplete.  It doesn’t say how to handle extension headers.  It isn’t clear whether padding octets are XORed (RFC 3550 says the padding octets “are not part of the payload”). SRTP MKI and authentication tag would also be recovered (which is not possible in the current draft).
Mo: We will clarify this. We need a good name for the part of the RTP packet after the fixed 12 byte header (CSRC list, extension header, RTP payload and padding, as described in the second bullet of this section).
>>>
6.3.2<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-6.3.2>.  Recovering the RTP Header

This procedure recovers the header of an RTP packet up to (and

   including) the SSRC field.

>>>
But it doesn’t recover the full header, and that should be more clearly stated,
Mo: Do you mean it can't recover RTP version (assumes 2)? We can clarify that. The rest of the header can be recovered.
FWIW, “if the repair symbols” were defined as running from the first byte after the SSRC to the end of the source RTP packet, then the full RTP packet would be recovered – including SRTP MKI and authentication tag, which cannot be recovered now.
Mo: Yes, that is the design, we will clarify this. Again, a word for that (after SSRC to end, after fixed 12 byte header) would greatly clarify this.
>>>
6.3.3<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-6.3.3>.  Recovering the RTP Payload


   2.  For each of the source packets that are successfully received in

       T, compute the bit string from the Y octets of data starting with

       the 13th octet of the packet.  If any of the bit strings

       generated from the source packets has a length shorter than Y,

       pad them to that length.  The padding of octet 0 MUST be added at

       the end of the bit string.  Note that the information of the

       first 8 octets are protected by the FEC header
>>>
As is the SSRC, though in a different way.
Mo: Yes, we will note this to avoid any confusion.
>>>
   2.  For each of the source packets that are successfully received in
       T, compute the bit string from the Y octets of data starting with
       the 13th octet of the packet.  If any of the bit strings
       generated from the source packets has a length shorter than Y,
       pad them to that length.  The padding of octet 0 MUST be added at
       the end of the bit string.  Note that the information of the
       first 8 octets are protected by the FEC header.

   3.  For the repair packet in T, compute the FEC bit string from the
       repair packet payload, i.e., the Y octets of data following the
       FEC header.  Note that the FEC header may be 12, 16, 32 octets
       depending on the length of the bitmask.


   4.  Calculate the recovered bit string as the XOR of the bit strings
       generated from all source packets in T and the FEC bit string
       generated from the repair packet in T.

   5.  Append the recovered bit string (Y octets) to the new packet
       generated in Section 6.3.2<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-6.3.2>.
>>>

This is close to the algorithm I suggested above (starting the “Repair symbols” in figure 9 from the first octet after the SSRC).

That’s good, but it isn’t consistent with the text in 6.2 (“The repair packet payload consists of the bits that are generated by applying the XOR operation on the payloads of the source RTP packets).

So the text as written fails to recover (or fails to generate the repair packet correctly, depending on how you look at it).

Also, any RTP padding, SRTP MKI + authentication tag in the repair packets themselves shouldn’t be included in the XOR.

RTP Padding, RTP MKI and the authentication tag lengths that are part of the source packets do need to be taken into account when computing Y.  I'm not sure the current FEC header handles that well.


This section uses "FEC Bit String" to mean something different than it meant earlier in the document.


Note the repair algorithms could include authentication of input packets and recovered source packets.

Mo: Yes, "payloads" will be removed, it is overloaded and incorrect. Again, we need a good word for all bytes after 12.
>>>

8<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-8>.  Congestion Control Considerations


>>>

I’d remove the reference to[I-D.singh-rmcat-adaptive-fec<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#ref-I-D.singh-rmcat-adaptive-fec>].
Mo: Agreed, we will remove it.
>>>

However, it MAY still continue to use FEC if considered for bandwidth

   estimation instead of speculatively probe for additional capacity

   [Holmer13<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#ref-Holmer13>][Nagy14].
>>>
What does this sentence mean?  The draft doesn’t talk about using FEC for either bandwidth estimation or capacity probing.  I suggest deleting it.
Mo: Agreed, we will remove it.
>>>
9<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-9>.  Security Considerations
>>>

Since the sender of the repair packets might not be the sender of the source packets, there is an obvious threat of injection of bogus repair packets.  Authentication can be helpful in mitigating that threat.
Such authentication (if possible) should be done an any packets that are used in the recovery process, and also on the reconstructed output packets.

One could strengthen this into "MUST not" normative language if one wished.
Mo: Agreed, we will make it a MUST if the recovered packet can be authenticated (i.e. SRTP source packets).

NITS
Overall, the document frequently uses first person plural (Suppose we have, If we apply, We generate,  We refer, We assume, …).  I think the authors should consider rephrasing those sentences.
Mo: We will rephrase ;)

Section 1
>>>
Situations where adaptivitiy
   of FEC parameters is desired, the endpoint can use the in-band
   mechanism, whereas when the FEC parameters are fixed, the endpoint
   may prefer to negotiate them out-of-band.
>>>
The sentence uses incorrect grammar.  I suggest
The in-band mechanism is advantageous when the endpoint is adapting the FEC parameters; the out-of-band mechanism may be preferable when the FEC parameters are fixed.
Mo: Agreed, we will reword.
Section 1.1
>>>

In a nutshell,
>>>
Informal English – I suggest rephrasing.
Mo: Agreed, we will remove.
>>>
Section 8

However, it MAY still continue to use FEC if considered for bandwidth

   estimation instead of speculatively probe for additional capacity

   [Holmer13<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#ref-Holmer13>][Nagy14].
>>>
Probing?
Mo: We will remove this anyway.


On Sun, Nov 19, 2017 at 4:07 AM, Ali C. Begen <[e-mail]<mailto:[e-mail]>> wrote:
Obviously the deadline for the WGLC is Dec. 7th.

-acbegen

> On Nov 17, 2017, at 5:35 AM, Roni Even <[e-mail]<mailto:[e-mail]>> wrote:
>
> Hi,
> I would like to start a three week WGLC on RTP Payload Format for Flexible Forward Error Correction in draft-ietf-payload-flexible-fec-scheme-05.
> The WGLC will end on November 7th
>
> Mo has posted some clarifications to the payload WG mailing list  payload mailing list archives
>
> Please send comments to the payload mailing list.
> THe double posting is to  notify RTCweb WG that the WGLC has started since this document is needed for RTCweb
>
>
> Roni Even
> Payload WG co-chair
>
>
> _______________________________________________
> payload mailing list
> [e-mail]<mailto:[e-mail]>
> https://www.ietf.org/mailman/listinfo/payload

_______________________________________________
payload mailing list
[e-mail]<mailto:[e-mail]>
https://www.ietf.org/mailman/listinfo/payload