Re: [tsvwg] Benjamin Kaduk's No Objection on draft-ietf-tsvwg-rlc-fec-scheme-14: (with COMMENT)

[Vincent Roca <[e-mail]>]

Hi Roman, Benjamin, all,

Back to the RLC I-D.
I have copied below the Discuss/Comments made for the -14 version of the document.
 
Many thanks, the document quality has been further improved.
Cheers,

   Vincent and Belkacem

===

> Roman Danyliw Discuss
> Discuss (2019-05-28)
>
> A few code nits for Section 3.6 so that the code compiles:
> ** To make the combination of this source code and that in draft-ietf-tsvwg-tinymt32 compile requires that the directive “#include <string.h>” be added (for the memset).
>
> ** The final return in Section 3.6 is missing a semicolon:
> s/return 0/return 0;/

[VR] Done. I usually don't add system headers to focus on key aspects, but it does not hurt. And thanks for the ";".

===

> Comment (2019-05-28)
>
> (1) Section 3.5  Is there any guidance that needs to be provided on the value of the seed to tinymt32_init?

[VR] Good point. Anything is accepted, which is now clearly stated. Since we already discuss Repair_Key management (this repair_key is used as seed), we refer to section 6.1.

NEW:
   With the FEC Schemes defined in this document, the seed is in
   practice restricted to a value between 0 and 0xFFFF inclusive (note
   that this PRNG accepts a seed value equal to 0), since this is the
   Repair_Key 16-bit field value of the Repair FEC Payload ID
   (Section 4.1.3).  In practice, how to manage the seed and Repair_Key
   values (both are equal) is left to the implementer, using a
   monotonically increasing counter being one possibility (Section 6.1).


> (2) Section 3.6.  In the code comments for cc_nb, I recommend explicitly stating:
> s/number of entries in the table/
> number of entries in the cc_tab[] table/

[VR] Done.


> (3) Section 3.6.  Multiple C code fragments are used in the text.  Somewhere the text should state that the examples are C code and made a reference to what version -- consider C99 (ISO/IEC 9899:1999), C11 (ISO/IEC 9899:2011), C18 (ISO/IEC 9899:2018).

[VR] Yes. I added the same C99 normative reference as in the TinyMT32 I-D
and included some text to clearly state it. I did it for both Sections, 3.5
and 3.6.

NEW (3.6):
   Figure 3 shows the reference generate_coding_coefficients()
   implementation.  This is a C language implementation, written for C99
   [C99].


> (4) Other References Nits
> Section 1.  Please include a reference for FECFRAME on first introduction.

[VR] Done.

> Section 1.1.  What is the reference for Raptor/RaptorQ?

[VR] Added RFC6681.


> (6) Editorial Nits
> Abstract.  No references are permitted in the abstract

[VR] Removed.

> Section 1.1.  Multiple Typos.  s/either a end-/either an end-/g
> Section 1.2.  Extra space. s/constraints) ./constraints)./
> Section 1.4.  Multiple Typos.  s/signalling/signaling/g
> Section 6.2.  Typo.  s/Section Section 3.6/Section 3.6/

[VR] Done. Thanks.

===

>  Benjamin Kaduk (was Discuss) No Objection
> Comment (2019-05-28)
>
> Thank you for resolving my Discuss points!  Additionally,
> I really like the reorganization of the document(s) -- it seems to do a
> great job at putting the core protocol/algorithm elements in the
> spotlight, while retaining the interesting discussion about use cases
> and parameter derivation in a less-prominent location.  The introductory
> material for Appendix C also does a great job to frame the subsequent
> discussion.  Thank you!

[VR] You're welcome.
> I agree with Roman that the code snippets should be more clearly marked
> as (a specific version of) C and in the case of
> generate_coding_coefficeints() as a normative part of the protocol
> specification.

[VR] Done.


> Section 3.1
>
>    Appendix C proposes non normative technics to derive those
>    parameters, depending on the use-case specificities.
>
> nit: I think "techniques" would be more conventional usage than
> "technics" here.

[VR] Changed.


> Section 3.5
>
>    Any implementation of this PRNG MUST fulfill three validation
>    criteria: the one described in [tinymt32] (for the TinyMT32 32-bit
>    unsigned integer generator), and the two others detailed in
>    Appendix A (for the mapping to 4-bit and 8-bit intervals).  [...]
>
> As I noted on the tinymt32 document, this phrasing is a bit odd, though
> it feels less out of place here than in the tinymt32 document.

[VR] Yes, and I updated this I-D in the same way.

NEW:
   Any implementation of this PRNG MUST have the same output as that
   provided by the reference implementation of [tinymt32].  In order to
   increase the compliancy confidence, three criteria are proposed: the
   one described in [tinymt32] (for the TinyMT32 32-bit unsigned integer
   generator), and the two others detailed in Appendix A (for the
   mapping to 4-bit and 8-bit intervals). [...]


> Section 3.6
>
>     * (in/out) cc_tab[]  pointer to a table of the right size to store
>     *                    coding coefficients. All coefficients are
>     *                    stored as bytes, regardless of the m parameter,
>     *                    upon return of this function.
>
> Assuming this is C, it's perhaps a bit jarring to spell the function
> parameter as "cc_tab[]" but describe it as a pointer.  (In either case,
> the encoding for the function call will be that of a pointer, of course,
> so the array syntax seems needlessly confusing.)

[VR] "uint8_t* cc_tab" and "uint8_t cc_tab[]" declarations are equivalent.
        Both are pointers to uint8_t elements so there's nothing wrong.
        However I see your point and changed for the explicit pointer
        syntax.


> Section 3.7.1
>
>    The two RLC FEC Schemes specified in this document reuse the Finite
>    Fields defined in [RFC5510], section 8.1.  More specifically, the
>    [...]
>    The following irreducible polynomial MUST be used for GF(2^^8):
>
>       x^^8 + x^^4 + x^^3 + x^^2 + 1
>
> Given the former chunk, the normative "MUST" in the second chunk is
> redundant, since the authoritative source for this polynomial is the
> cited portion of RFC 5510.

[VR] Removed the second upper case MUST.

NEW:
   The following irreducible polynomial is used for GF(2^^8):
      x^^8 + x^^4 + x^^3 + x^^2 + 1


> Section 3.7.2
>
>              For each byte of position i in each source and the repair
>    symbol, where i belongs to {0; E-1}, compute:
>
> nit: do we expect the usage of the curly braces {} to indicate a closed
> interval to be well-understood by the readership (as opposed to square
> brackets [])?

[VR] Well it depends. In France, it seems preferable to use {...} for an
enumeration, and [] for a span, but conventions may differ in other
countries. Given you comment, I've changed for [].


>                                       The XOR sum of the byte of
>    position i in each source is computed and stored in the corresponding
>    byte of the repair symbol, where i belongs to {0; E-1}.  […]
>
> (ditto)

[VR] Done.


> Section 6.1
>
> I might add a note that the scheme for chosing the repair key is
> entirely at the discretion of the sender, since it is communicated in
> the wire format to the receiver(s), and that the sequential counter is
> presented as the simplest method that ensures new coefficients for each
> repair packet.  As already mentioned, other schemes are possible, and we
> definitely don't want any receivers hardcoding the assumption of
> sequentially incrementing counter.

[VR] Yes, definitively. It even didn't come to my mind that a receiver
may hardcode the way the repair key is managed. I've further reinforced
this.

NEW:
   In any case, chosing the repair key
   is entirely at the discretion of the sender, since it is communicated
   to the receiver(s) in each Repair FEC Payload ID.  A receiver should
   not make any assumption on the way the repair key is managed.


> Section 9.1
>
>                                               In case of small encoding
>    windows, the associated processing overhead is not an issue (e.g., we
>    measured decoding speeds between 745 Mbps and 2.8 Gbps on an ARM
>    Cortex-A15 embedded board in [Roca17] for an encoding window of size
>    18 or 23 symbols).  [...]
>
> Just to double-check: does the 2.8 Gbps correspond to the window of 18
> symbols or 23?

[VR] Not exactly, it essentially depends on the code rate (the higher,
the faster) and channel conditions, in addition to the ew_size that is
determined by the code rate.
It's well described in the paper and I don't want to put all details here,
however this is misleading and I've changed the wording.

NEW:
   In case of small encoding
   windows, the associated processing overhead is not an issue (e.g., we
   measured decoding speeds between 745 Mbps and 2.8 Gbps on an ARM
   Cortex-A15 embedded board in [Roca17] depending on the code rate and
   the channel conditions, using an encoding window of size 18 or 23
   symbols; see the above article for the details).

> Appendix A
>
> Please note whether the numbers should be read out column-first or
> row-first.

[VR] Done.


> I refer again to my comment on the tinymt32 document regarding the
> normative "MUST" language here vs. stating that these are test vectors.

[VR] Done.


> Appendix B
>
> side note: I don't propose to have you redo the analysis, but looking at
> the small sequences generated using seed values from 0 to 1,000,000 - 1
> is not terribly representative of the FEC usage, since the seed values
> there are limited to the range from 0 to 16k.  Comprehensive statistics
> on the 16k possible sequences (corresponding to the 16k possible repair
> keys) of 20 pseudo-random numbers would be more compelling.
>
> Also, for the scenario presented in Figure 12, I'm not sure I agree with
> the claim that "although the distribution is not perfect, there is no
> major bias".  Not in that I think there is bias, but in that I think
> that we should not expect the average occurrences column to be a
> constant -- the numbers here seem largely within the expected sort of
> variances for this type of sampling.

[VR] I need to give it a second thought... I've submitted -15 without
considering this comment and will see how to adress it in -16.


> Appendix D
>
>    Finally, it is worth noting that a receiver that benefits from an FEC
>    protection significantly higher than what is required to recover from
>    packet losses, can choose to reduce the ls_max_size.  In that case
>    lost ADUs will be recovered without relying on this optimization.
>
> nit(?): It's not entirely clear to me that this statement is well-placed
> in its current location.

[VR] I think it's well placed. We talk about ls_max_size and here we
say that using a too large value is not necessarily meaningful.