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

From: "Mo Zanaty (mzanaty)" <[email-masked]>
Date: Thu, 7 Dec 2017 21:45:15 +0000

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 <[email-masked]<mailto:[email-masked]>> on behalf of Stephen Botzko <[email-masked]<mailto:[email-masked]>>
Date: Tuesday, November 21, 2017 at 4:22 PM
To: "Ali C. Begen" <[email-masked]<mailto:[email-masked]>>
Cc: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
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 <[email-masked]<mailto:[email-masked]>> wrote:
Obviously the deadline for the WGLC is Dec. 7th.

-acbegen

> On Nov 17, 2017, at 5:35 AM, Roni Even <[email-masked]<mailto:[email-masked]>> 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
> [email-masked]<mailto:[email-masked]>
> https://www.ietf.org/mailman/listinfo/payload

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

---

From: Magnus Westerlund <[email-masked]>
Date: Fri, 8 Dec 2017 13:50:50 +0100

Hi,

Sorry for being a day late, but here are my WGLC comments.

I thought this was in better shape, but clearly not ready. I actually 
have not reviewed all in detail. I hope I will have time to do that when 
you have addressed these issues.

1. Section 4.2:

             +------------------------------+
             |          IP Header           |
             +------------------------------+
             |       Transport Header       |
             +------------------------------+
             |          RTP Header          | __
             +------------------------------+   |
             |          FEC Header          |    \
             +------------------------------+     > RTP Payload
             |        Repair Symbols        |    /
             +------------------------------+ __|

                     Figure 9: Format of repair packets

The bracket appears to big as it indicates that parts of the RTP header 
is payload. I would prefer drawing this like this:

             +------------------------------+
             |          IP Header           |
             +------------------------------+
             |       Transport Header       |
             +------------------------------+
             |          RTP Header          |
             +------------------------------+ --+
             |          FEC Header          |    \
             +------------------------------+     > RTP Payload
             |        Repair Symbols        |    /
             +------------------------------+ --+

                     Figure 9: Format of repair packets

2. Section 4.2:

    o  Synchronization Source (SSRC): The SSRC value SHALL be randomly
       assigned as suggested by [RFC3550].

I would suggest that you make it clear that the SSRC value should be set 
randomly for a particular Repair flow, to avoid the misinterpretation 
that each packet should have a random SSRC value.

3. Section 4.2:

This allows the sender to
       multiplex the source and repair flows on the same port, or
       multiplex multiple repair flows on a single port.

I would suggest that you rewrite this like this:

This allows the sender to
       multiplex the source and repair flows in the same RTP session, or
       multiplex multiple repair flows in a single RTP session.

4. Section 4.2:

The repair
       flows SHOULD use the RTCP CNAME field to associate themselves with
       the source flow.

I think you should reformulate this to be more precise to say:

The repair
       flows SHOULD use the same RTCP SDES CNAME value as the source 
flows to associate them when
source and repair flows originate from the same endpoint.

5. Section 4.2:

       In some networks, the RTP Source, which produces the source
       packets and the FEC Source, which generates the repair packets
       from the source packets may not be the same host.  In such
       scenarios, using the same CNAME for the source and repair flows
       means that the RTP Source and the FEC Source MUST share the same
       CNAME (for this specific source-repair flow association).  A
       common CNAME may be produced based on an algorithm that is known
       both to the RTP and FEC Source [RFC7022].  This usage is compliant
       with [RFC3550].

I think this may actually be the wrong way of handling this. Is it 
really important that the CNAME can be used to determine which SSRC are 
plausible as source flows for a given repair flow. If we skip that 
feature, one can use CNAMES as usual where they are primarily depending 
on the endpoint that originates the RTP flow with a given SSRC and the 
synchronization context. As the repair packet is indicating explicitly 
which source packets, this should not be a real issue.

Thus, I suggest that the above text removes sentence with MUST. Instead 
one could add the following sentence directly after the propsed sentence 
in issue 4:

However, repair flows that protects source flows from multiple different 
endpoints SHOULD instead use a CNAME value associated with the Repair 
Flow sending endpoint.

And the above text could be deleted.

6. Section 4.2:

Figure 9 should include the CSRC list and the FEC payload formats use of 
the CSRC list should be listed prior to Figure 10. This as the CSRC list 
is part of the RTP header.

7. Section 4.2:

    o  The SN base_i (16 bits) field indicates the lowest sequence
       number, taking wrap around into account, of the source packets for
       a particular SSSRC (indicated in CSRC_i) protected by this repair
       packet.

Note spelling SSSRC / SSRC.

8. Label on Figures 12, 13, 15:

They all says "Protocol format for ..."

While I think for clarity they should say: "Payload FEC header format 
for ..."

9. Section 4.2:

    Note that the parsing of this packet is different.  The sequence
    number (SN base_i) replaces the length recovery in the FEC packet.
    The CSRC Count (CC) which would be 1, M and N would be set to 0, and
    the reserved bits from the FEC header are removed.  By doing this, we
    save 64 bits.

I actually don't fully understand this paragraph:

In the third sentence: Why will the CC field be 1? This is the CC field 
value of the reconstructed (retransmitted) packet, isn't it? Thus, you 
can't say anything about its original value here? The CC field value in 
the RTP header of the FEC packet will have CC = 1, but not this field.

Then you have a confusion around what M and N is. Actually, I think the 
M (Columns) needs to change letter to something that isn't used in the 
packet already. M is colliding with M (marker bit) recovery field, thus 
please change the letter.

10. Figure 15:

When retransmitting an packet, where is the source packets CSRC list 
going? That should be indicated in this figure for clarity. Same is true 
for RTP header Extensions present in the source packet.

11. Section 6.2:

    The FEC header includes 12 octets (or upto 28 octets when the longer
    optional masks are used).

This is not strictly true, it is dependent on the number of SSRCs that 
the packet repairs.

12. Section 6.2:

Missing discussion of what to do with source packet CSRC list and RTP 
header extension.

13. Section 6.3.2:

Missing text for recovery of source packets CSRC list and RTP header 
extensions.

14. Section 6.2:

    Due to this possible padding and mandatory FEC header, a repair
    packet has a larger size than the source packets it protects. This
    may cause problems if the resulting repair packet size exceeds the
    Maximum Transmission Unit (MTU) size of the path over which the
    repair flow is sent.

I think this should add a recommendation that the media encoding and RTP 
payload packetization for source packets should be adjusted to a lower 
maximum packet size to allow space for the FEC headers to avoid forcing 
them into being fragmented. This of course only is possible in cases 
where the source packet generating endpoint is aware of the addition of 
the FEC packets and its strategy of combining them.

Cheers

Magnus

Den 2017-11-17 kl. 03:35, skrev Roni Even:
>
> 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 
> <https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05>.
>
> The WGLC will end on November 7^th
>
> <https://mailarchive.ietf.org/arch/search/?email_list=payload>
>
> rtcweb mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/rtcweb

-- 

Magnus Westerlund

----------------------------------------------------------------------
Media Technologies, Ericsson Research
----------------------------------------------------------------------
Ericsson AB                 | Phone  +46 10 7148287
Torshamnsgatan 23           | Mobile +46 73 0949079
SE-164 80 Stockholm, Sweden | mailto: [email-masked]
----------------------------------------------------------------------

---

From: "Mo Zanaty (mzanaty)" <[email-masked]>
Date: Fri, 8 Dec 2017 18:52:32 +0000

Hi Magnus,

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

Thanks,
Mo

From: payload <[email-masked]<mailto:[email-masked]>> on behalf of 'Magnus Westerlund' <[email-masked]<mailto:[email-masked]>>
Date: Friday, December 8, 2017 at 7:50 AM
To: Roni Even <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Cc: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Subject: Re: [payload] [rtcweb] WGLC on RTP Payload Format for Flexible Forward Error Correction

Hi,

I thought this was in better shape, but clearly not ready. I actually have not reviewed all in detail. I hope I will have time to do that when you have addressed these issues.

1. Section 4.2:

The bracket appears to big as it indicates that parts of the RTP header is payload. I would prefer drawing this like this:

Mo: Agreed, we will update the ASCII art.

2. Section 4.2:

I would suggest that you make it clear that the SSRC value should be set randomly for a particular Repair flow, to avoid the misinterpretation that each packet should have a random SSRC value.

Mo: Agreed, will do.

3. Section 4.2:

Mo: Agreed, we will reword as suggested.

4. Section 4.2:

The repair

The repair
      flows SHOULD use the same RTCP SDES CNAME value as the source flows to associate them when
source and repair flows originate from the same endpoint.

Mo: Per the point below, I am inclined to remove CNAME guidance or restrictions entirely, since they seem irrelevant.

5. Section 4.2:

I think this may actually be the wrong way of handling this. Is it really important that the CNAME can be used to determine which SSRC are plausible as source flows for a given repair flow. If we skip that feature, one can use CNAMES as usual where they are primarily depending on the endpoint that originates the RTP flow with a given SSRC and the synchronization context. As the repair packet is indicating explicitly which source packets, this should not be a real issue.

Thus, I suggest that the above text removes sentence with MUST. Instead one could add the following sentence directly after the propsed sentence in issue 4:

However, repair flows that protects source flows from multiple different endpoints SHOULD instead use a CNAME value associated with the Repair Flow sending endpoint.

Mo: Since all source and repair streams must be in the same RTP session, I agree CNAME is mostly irrelevant, and I am inclined to remove all mention of CNAME, except maybe to say it should follow RFC 7022.

6. Section 4.2:

Figure 9 should include the CSRC list and the FEC payload formats use of the CSRC list should be listed prior to Figure 10. This as the CSRC list is part of the RTP header.

Mo: Agreed, the text on CSRC_i / list will move up before Figure 10.

7. Section 4.2:

Mo: Noted.

While I think for clarity they should say: "Payload FEC header format for ..."

Mo: Ok, we will rename the labels.

9. Section 4.2:

In the third sentence: Why will the CC field be 1? This is the CC field value of the reconstructed (retransmitted) packet, isn't it? Thus, you can't say anything about its original value here? The CC field value in the RTP header of the FEC packet will have CC = 1, but not this field.

Then you have a confusion around what M and N is. Actually, I think the M (Columns) needs to change letter to something that isn't used in the packet already. M is colliding with M (marker bit) recovery field, thus please change the letter.

Mo: We will clarify this by splitting into separate sections for each repair format variant, and perhaps use L/D instead of M/N (to match the SDP parameters L/D).

10. Figure 15:

When retransmitting an packet, where is the source packets CSRC list going? That should be indicated in this figure for clarity. Same is true for RTP header Extensions present in the source packet.

Mo: We will clarify this by splitting into separate sections for each repair format variant. For all variants, all bytes after 12 (including any CSRC list or extension headers) are considered "payload" (we will find a better word for this since payload is obviously very overloaded).

11. Section 6.2:

This is not strictly true, it is dependent on the number of SSRCs that the packet repairs.

Mo: Yes, we will clarify this.

12. Section 6.2:

Missing discussion of what to do with source packet CSRC list and RTP header extension.

Mo: Yes, the updates to section 4.2 will be echoed in 6.2.

13. Section 6.3.2:

Missing text for recovery of source packets CSRC list and RTP header extensions.

Mo: Yes, we will add this.

14. Section 6.2:

I think this should add a recommendation that the media encoding and RTP payload packetization for source packets should be adjusted to a lower maximum packet size to allow space for the FEC headers to avoid forcing them into being fragmented. This of course only is possible in cases where the source packet generating endpoint is aware of the addition of the FEC packets and its strategy of combining them.

Mo: Agreed, this seems like an appropriate recommendation, so we will add it.

Cheers

Magnus

Den 2017-11-17 kl. 03:35, skrev Roni Even:
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<https://tools.ietf.org/html/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<https://mailarchive.ietf.org/arch/search/?email_list=payload>

Roni Even
Payload WG co-chair

_______________________________________________
rtcweb mailing list
[email-masked]<mailto:[email-masked]>https://www.ietf.org/mailman/listinfo/rtcweb

--

Magnus Westerlund

SE-164 80 Stockholm, Sweden | mailto: [email-masked]<mailto:[email-masked]>