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

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Tue, 28 May 2019 21:33:59 -0700

Benjamin Kaduk has entered the following ballot position for
draft-ietf-tsvwg-rlc-fec-scheme-14: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-tsvwg-rlc-fec-scheme/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

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!

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.

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.

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.

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.)

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.

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 [])?

                                      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)

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.

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?

Appendix A

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

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

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.

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.