Re: [Cellar] AD Review: draft-ietf-cellar-ffv1

[Dave Rice <[e-mail]>]

Hi Adam,
Thanks for your review. I have addressed some of these in a pull request at https://github.com/FFmpeg/FFV1/pull/185 and left questions or nudges to other authors below.

> On Jan 2, 2020, at 6:26 PM, Adam Roach <[e-mail]> wrote:
> 
> This is my AD review for draft-ietf-cellar-ffv1, which I am processing
> at Alexey Melinikov's request. Thanks to everyone who has taken the time
> to document these formats for publication as an RFC.
> 
> I have identified a couple of issues that will need to be addressed prior to
> IETF last call (which are clearly identified below), as well as a number of
> smaller issues that can be addressed as normal IETF last call comments.
> 
> ---------------------------------------------------------------------------
> 
> §1:
> 
> >  The latest version of this document is available at
> >  https://raw.github.com/FFmpeg/FFV1/master/ffv1.md
> >  (https://raw.github.com/FFmpeg/FFV1/master/ffv1.md)
> 
> Please add a note to the RFC editor to remove this paragraph prior to
> publication (or remove the paragraph in the draft at this time).

In the pull request, I removed this line.

> ---------------------------------------------------------------------------
> 
> §2:
> 
> >  The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
> >  "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
> >  document are to be interpreted as described in [RFC2119].
> 
> Please update this to match the updated boilerplate given in RFC 8174.

Updated in the pull request.

> ---------------------------------------------------------------------------
> 
> §2.2.1:
> 
> >  The pseudo-code used is based upon the C programming
> >  language [ISO.9899.1990] and uses its "if/else", "while" and "for"
> >  functions as well as functions defined within this document.
> 
> It's a minor nit, but these are keywords, not functions.

Should both references to the term functions be keywords or just the first? Note there are also two section headers that use the term Functions.

> ---------------------------------------------------------------------------
> 
> §2.2.2:
> 
> >  Note: the operators and the order of precedence are the same as used
> >  in the C programming language [ISO.9899.1990].
> 
> Is there a reason to cite C90 here instead of any of the four newer versions?

Updated in the pull request. Updated to the 2018 version.

> ---------------------------------------------------------------------------
> 
> §2.2.2:
> 
> >  Note: the operators and the order of precedence are the same as used
> >  in the C programming language [ISO.9899.1990].
> ...
> >  "a ^ b" means a raised to the b-th power.
> 
> One of these two statements is incorrect: the "^" operator in C is a bitwise
> XOR, not a power operator. C has no built-in exponentiation operator, and
> relies on a library-provided `pow(a,b)` function.
> 
> Because this introduces an ambiguity into the specification, it will need
> to be corrected before entering IETF last call.

This impacts a lot of the math. Is there consensus to replace `a^b` with `pow(a,b)` or should `a^b` be defined locally as an exception?

> ---------------------------------------------------------------------------
> 
> §2.2.2:
> 
> >  "a >> b" means arithmetic right shift of two's complement integer
> >  representation of a by b binary digits.
> 
> This needs to indicate whether the resulting value is sign-extended
> or zero-padded. We can't rely on C's definition here, as right
> shifting a signed value is implementation-defined by the standard:
> 
>     The result of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an
>     unsigned type or if E1 has a signed type and a nonnegative value, the
>     value of the result is the integral part of the quotient of E1 / 2E2. If
>     E1 has a signed type and a negative value, the resulting value is
>     implementation-defined.
> 
> Because this introduces an ambiguity into the specification, it will need
> to be corrected before entering IETF last call.

I’m uncertain, nudging the other authors.

> ---------------------------------------------------------------------------
> 
> §2.2.5:
> 
> >  a~b,c.  the 'b,c'-th value of a sequence of a
> 
> Please explain this with more words. I can't figure out what it means.

Same. I’m uncertain, nudging the other authors.

> ---------------------------------------------------------------------------
> 
> §2.2.8:
> 
> >  relies on its "Container" to store the "NumBytes" values, see the
> >  section on the Mapping FFV1 into Containers (#mapping-ffv1-into-
> >  containers).
> 
> Nit: "...values; see the section..."
> 
> Also, please replace the MD-style anchor with a section number.

Updated in the pull request.

> ---------------------------------------------------------------------------
> 
> §3:
> 
> >  For each "Slice" (as described in the section on Slices (#slice)) of
> >  a "Frame", the "Planes", "Lines", and "Samples" are coded in an order
> >  determined by the "Color Space" (see the section on Color Space
> >  (#color-spaces)).  Each "Sample" is predicted by the median predictor
> >  as described in the section of the Median Predictor (#median-
> >  predictor) from other "Samples" within the same "Plane" and the
> >  difference is stored using the method described in Coding of the
> >  Sample Difference (#coding-of-the-sample-difference).
> 
> Please replace the MD-style anchors with section numbers.

Updated all such anchors in the pull request.

> ---------------------------------------------------------------------------
> 
> §3.4:
> 
> >  context = Q_{0}[l - tl] +
> >            Q_{1}[tl - t] +
> >            Q_{2}[t - tr] +
> >            Q_{3}[L - l]  +
> >            Q_{4}[T - t]
> 
> This is the first time in the document we're encountering Q. Please add a
> forward reference to its definition.

To do, as the language of the Quantization Table Sets section never says that the table is referred to as Q.

> ---------------------------------------------------------------------------
> 
> §3.7.2:
> 
> >  As an example, a "Frame" that is two "Pixels" wide and two "Pixels"
> >  high, could be comprised of the following structure:
> 
> Nit: "...could be composed of the following structure:" OR "...could comprise
> the following structure:”

Updated to the latter suggestion in the pull request.

> ---------------------------------------------------------------------------
> 
> §3.8.1.1:
> 
> >  S_{i+1,C_{i}} =  zero_state_{S_{i,C_{i}}} XOR
> >            l_i =  L_i                      XOR
> >            t_i =  R_i - r_i                <==
> >            b_i =  0                        <==>
> >            L_i <  R_i - r_i
> 
> Substantial parts of this syntax are undefined at this point in the document.
> 
> Presumably "XOR" means "exclusive or," although it is unclear whether this is
> intended to be a boolean or bitwise operation.
> 
> The "<==" and "<==>" notation is also without explanation, and I can't even make
> an educated guess at their intended meaning.
> 
> There is no explanation what it might mean to combine a series of assignment
> operations (and one comparison operation) with these XOR, <==, and <==> symbols.
> 
> This needs to be clarified (for all of figures 7 through 9) before the
> document can go to IETF last call.

To do

> ---------------------------------------------------------------------------
> 
> §3.8.1.1.1:
> 
> >  Above describes the range decoding, encoding is defined as any
> >  process which produces a decodable bytestream.
> 
> Would it be reasonable to include a non-normative example of one such
> process?

To do

> ---------------------------------------------------------------------------
> 
> §3.8.1.2:
> >  exact contexts used are best described by the
> >  following code, followed by some comments.
> 
> I can't find the referenced comments.

To do

> ---------------------------------------------------------------------------
> 
> §3.8.1.2:
> 
> > pseudo-code                                                   | type
> > --------------------------------------------------------------|-----
> >  void put_symbol(RangeCoder *c, uint8_t *state, int v, int \   |
> >  is_signed) {                                                  |
> >      int i;                                                    |
> >      put_rac(c, state+0, !v);                                  |
> >      if (v) {                                                  |
> >          int a= abs(v);                                        |
> >          int e= log2(a);                                       |
> >                                                                |
> >          for (i = 0; i < e; i++) {                             |
> >              put_rac(c, state+1+min(i,9), 1); //1..10         |
> > }                                                     |
> >                                                                |
> >          put_rac(c, state+1+min(i,9), 0);                      |
> >          for (i = e-1; i >= 0; i--) {                          |
> >              put_rac(c, state+22+min(i,9), (a>>i)&1); //22..31 |
> > }                                                     |
> >                                                                |
> >          if (is_signed) {                                      |
> >              put_rac(c, state+11 + min(e, 10), v < 0); //11..21|
> > }                                                     |
> > }                                                         |
> > }                                                             |
> 
> The "type" column here appears superfluous, and should either employed or
> removed.

To do

> The behavior of the function "put_rac" isn't defined anywhere. While the purpose
> of its first two parameters can be deduced from context, the purpose of the
> third parameter is baffling. For avoidance of doubt, please add a description of
> what this function does, and what its parameters mean.

To do

> ---------------------------------------------------------------------------
> 
> §3.8.1.6:
> 
> >  The alternative state transition table has been built using iterative
> >  minimization of frame sizes and generally performs better than the
> >  default.  To use it, the coder_type (see the section on coder_type
> >  (#codertype)) MUST be set to 2 and the difference to the default MUST
> >  be stored in the "Parameters", see the section on Parameters
> >  (#parameters).  The reference implementation of FFV1 in FFmpeg uses
> >  this table by default at the time of this writing when Range coding
> >  is used.
> 
> Please replace the MD-style anchors with section numbers.

Updated all such anchors in the pull request.

> ---------------------------------------------------------------------------
> 
> §3.8.2.1:
> 
> > pseudo-code                                                   | type
> > --------------------------------------------------------------|-----
> >  int get_ur_golomb(k) {                                        |
> >      for (prefix = 0; prefix < 12; prefix++) {                 |
> >          if (get_bits(1)) {                                    |
> >              return get_bits(k) + (prefix << k)                |
> > }                                                     |
> > }                                                         |
> >      return get_bits(bits) + 11                                |
> > }                                                             |
> >                                                                |
> >  int get_sr_golomb(k) {                                        |
> >      v = get_ur_golomb(k);                                     |
> >      if (v & 1) return - (v >> 1) - 1;                         |
> >      else       return   (v >> 1);                             |
> >  }
> 
> The "type" column here appears superfluous, and should either employed or
> removed.

To do.

> Please clarify the meaning of "ur" and "sr" in the function names.

To do, define function names with references to ur / sr definitions.

> ---------------------------------------------------------------------------
> 
> §3.8.2.1.1:
> 
> >                       +----------------+-------+
> >                       | bits           | value |
> >                       +================+=======+
> >                       | 1              | 0     |
> >                       +----------------+-------+
> >                       | 01             | 1     |
> >                       +----------------+-------+
> >                       | ...            | ...   |
> >                       +----------------+-------+
> >                       | 0000 0000 0001 | 11    |
> >                       +----------------+-------+
> >                       | 0000 0000 0000 | ESC   |
> >                       +----------------+-------+
> 
> This is pretty confusing, not least of all because the base of the number on the
> right is unclear (given that all the shown values contain 1's and 0's
> exclusively). I would suggest:
> 
>                         +----------------+-------+
>                         | bits           | value |
>                         +================+=======+
>                         | 1              | 0     |
>                         +----------------+-------+
>                         | 01             | 1     |
>                         +----------------+-------+
>                         | ...            | ...   |
>                         +----------------+-------+
>                         | 00 0000 0001   | 9     |
>                         +----------------+-------+
>                         | 000 0000 0001  | 10    |
>                         +----------------+-------+
>                         | 0000 0000 0001 | 11    |
>                         +----------------+-------+
>                         | 0000 0000 0000 | ESC   |
>                         +----------------+———+

To do

> ---------------------------------------------------------------------------
> 
> §3.8.2.2.1:
> 
> > pseudo-code                                                   | type
> > --------------------------------------------------------------|-----
> > log2_run[41]={                                                |
> >   0, 0, 0, 0, 1, 1, 1, 1,                                      |
> >   2, 2, 2, 2, 3, 3, 3, 3,                                      |
> >   4, 4, 5, 5, 6, 6, 7, 7,                                      |
> >   8, 9,10,11,12,13,14,15,                                      |
> > 16,17,18,19,20,21,22,23,                                      |
> > 24,                                                           |
> > };                                                            |
> >                                                                |
> >  if (run_count == 0 && run_mode == 1) {                        |
> >      if (get_bits(1)) {                                        |
> >          run_count = 1 << log2_run[run_index];                 |
> >          if (x + run_count <= w) {                             |
> > run_index++;                                      |
> > }                                                     |
> >      } else {                                                  |
> >          if (log2_run[run_index]) {                            |
> >              run_count = get_bits(log2_run[run_index]);        |
> >          } else {                                              |
> >              run_count = 0;                                    |
> > }                                                     |
> >          if (run_index) {                                      |
> > run_index--;                                      |
> > }                                                     |
> >          run_mode = 2;                                         |
> > }                                                         |
> > }                                                             |
> 
> 
> The "type" column here appears superfluous, and should either employed or
> removed.
> 
> 
> >  The log2_run function is also used within [ISO.14495-1.1999].
> 
> The reference to "log2_run" as a "function" is perplexing, as log2_run is defined
> as an array in the above pseudocode.

To do

> ---------------------------------------------------------------------------
> 
> §3.8.2.3:
> 
> Some light documentation of the "state" structure would make this pseudocode
> much easier to follow.

To do

> ---------------------------------------------------------------------------
> 
> §4:
> 
> >  Within the following sub-sections, pseudo-code is used to explain the
> >  structure of each FFV1 bitstream component, as described in the
> >  section on Pseudo-Code (#pseudocode).
> 
> Please replace the MD-style anchor with a section number.
> 
> > +--------+-------------------------------------------+
> >         | Symbol | Definition |
> > +========+===========================================+
> >         | u(n)   | unsigned big endian integer using n bits |
> > +--------+-------------------------------------------+
> >         | sg     | Golomb Rice coded signed scalar symbol |
> >         |        | coded with the method described in Signed |
> >         |        | Golomb Rice Codes (#golomb-rice-mode) |
> > +--------+-------------------------------------------+
> >         | br     | Range coded Boolean (1-bit) symbol with |
> >         |        | the method described in Range binary |
> >         |        | values (#range-binary-values) |
> > +--------+-------------------------------------------+
> >         | ur     | Range coded unsigned scalar symbol coded |
> >         |        | with the method described in Range non |
> >         |        | binary values (#range-non-binary-values) |
> > +--------+-------------------------------------------+
> >         | sr     | Range coded signed scalar symbol coded |
> >         |        | with the method described in Range non |
> >         |        | binary values (#range-non-binary-values) |
> > +--------+-------------------------------------------+
> 
> Please replace the several MD-style anchors with a section number.

Updated all such anchors in the pull request.

> ---------------------------------------------------------------------------
> 
> §4:
> 
> >  The same context that is initialized to 128 is used for all fields in
> >  the header.
> 
> This phrasing is confusing, as it reads like it's referring to some
> previously-mentioned context that is best identified as containing "128." After
> convincing myself that I didn't miss such a thing, I *think* you might mean:
> 
>    All fields in the header use the same context, which is
>    initialized to 128.
> 
> Although, even with that adjustment, I'm not sure which header you're talking
> about. The only header mentioned anywhere earlier in the document is the
> "Slice Header," and that seems unlikely to be what you're referring to.

To do

> ---------------------------------------------------------------------------
> 
> §4.1:
> 
> >              state_transition_delta[ i ]                       | sr
> ...
> >          QuantizationTableSet( i )                             |
> 
> This psuedo-code appears to use both "[]" and "()" as array operators.  I
> understand this is only pseudocode, but please try to remain consistent with
> the meaning of operators.
> 
> Also, "QuantiztionTableSet" is missing a type designation.

To do

> ---------------------------------------------------------------------------
> 
> §4.1.16:
> 
> > +-------+--------------------------------------------+
> >         | value | error detection/correction type |
> > +=======+============================================+
> >         | 0     | 32-bit CRC on the global header |
> > +-------+--------------------------------------------+
> >         | 1     | 32-bit CRC per slice and the global header |
> > +-------+--------------------------------------------+
> >         | Other | reserved for future use |
> > +-------+--------------------------------------------+
> 
> I'm still perplexed by the intended meaning of "header.”

To do

> ---------------------------------------------------------------------------
> 
> §4.2.3.4:
> 
> >  FFV1 SHOULD use "V_FFV1" as the Matroska "Codec ID".
> 
> Normative requirements on external documents make those documents normative
> dependencies. Please move "[Matroska]" from the "Informative References" section
> to the "Normative References" section.

Updated in the pull request.

> ---------------------------------------------------------------------------
> 
> §4.1.17:
> 
> >             +-------+-------------------------------------+
> >             | value | relationship                        |
> >             +=======+=====================================+
> >             | 0     | Frames are independent or dependent |
> >             |       | (keyframes and non keyframes)       |
> >             +-------+-------------------------------------+
> >             | 1     | Frames are independent (keyframes   |
> >             |       | only)                               |
> >             +-------+-------------------------------------+
> >             | Other | reserved for future use             |
> >             +-------+-------------------------------------+
> >
> >                                 Table 14
> 
> I'm having a hard time understanding "Other" in this table. Section 4.3
> defines "keyframe" to be of "br" type. I'm confused about how a boolean
> variable can take on more than two values.

To do

> ---------------------------------------------------------------------------
> 
> §4.4:
> 
> >  Background: due to some non conforming
> >  encoders, some bitstreams where found with 40 extra bits
> >  corresponding to "error_status" and "slice_crc_parity", a decoder
> >  conforming to the revised specification could not do the difference
> >  between a revised bitstream and a buggy bitstream.
> 
> This is hard to parse. I suggest rephrasing like:
> 
>    Background: Due to some non-conforming
>    encoders, some bitstreams were found with 40 extra bits
>    corresponding to "error_status" and "slice_crc_parity". As a
>    result, a decoder conforming to the revised specification could not
>    distinguish between a revised bitstream and a buggy bitstream.

Updated to your recommendation in the pull request.

> ---------------------------------------------------------------------------
> 
> §4.6:
> 
> > pseudo-code                                                   | type
> > --------------------------------------------------------------|-----
> >  SliceContent( ) {                                             |
> >      if (colorspace_type == 0) {                               |
> >          for (p = 0; p < primary_color_count; p++) {           |
> >              for (y = 0; y < plane_pixel_height[ p ]; y++) {   |
> >                  Line( p, y )                                  |
> > }                                                 |
> > }                                                     |
> >      } else if (colorspace_type == 1) {                        |
> >          for (y = 0; y < slice_pixel_height; y++) {            |
> >              for (p = 0; p < primary_color_count; p++) {       |
> >                  Line( p, y )                                  |
> > }                                                 |
> > }                                                     |
> > }                                                         |
> > }                                                             |
> 
> Without a "type" indicated on the "Line( p, y )" lines, this encoding appears
> to be ambiguous.

To do

> ---------------------------------------------------------------------------
> 
> §4.7:
> 
> > pseudo-code                                                   | type
> > --------------------------------------------------------------|-----
> >  Line( p, y ) {                                                |
> >      if (colorspace_type == 0) {                               |
> >          for (x = 0; x < plane_pixel_width[ p ]; x++) {        |
> >              sample_difference[ p ][ y ][ x ]                  |
> > }                                                     |
> >      } else if (colorspace_type == 1) {                        |
> >          for (x = 0; x < slice_pixel_width; x++) {             |
> >              sample_difference[ p ][ y ][ x ]                  |
> > }                                                     |
> > }                                                         |
> > }                                                             |
> 
> Although redundant with the request above regarding section 4.6, please
> add "type" indicators for the "sample_difference[ p ][ y ][ x ]" lines.

To do

> ---------------------------------------------------------------------------
> 
> §4.7.4:
> 
> >  "Sample" value is computed based on median predictor and context
> >  described in the section on Samples (#samples).
> 
> Please replace the MD-style anchor with a section number.

Updated all such anchors in the pull request.

> ---------------------------------------------------------------------------
> 
> §4.9
> 
> >  The Quantization Table Sets are stored by storing the number of equal
> >  entries -1 of the first half of the table (represented as "len - 1"
> >  in the pseudo-code below) using the method described in Range Non
> >  Binary Values (#range-non-binary-values).
> 
> Please replace the MD-style anchor with a section number.

Updated all such anchors in the pull request.

> ---------------------------------------------------------------------------
> 
> §6:
> 
> While it does provide compression (my understanding is that a typical
> ratio is about 2:1), FFV1 still produces a very large bitstream, due to
> its lossless properties. This does present some security challenges that
> need to be cited.
> 
> I would expect this document to include text that is roughly equivalent to the
> final three paragraphs of RFC 4175, Section 8.

To do

> ---------------------------------------------------------------------------
> 
> §7:
> 
> Please replace the seven various MD-style anchors with section numbers.

Updated all such anchors in the pull request.

> ---------------------------------------------------------------------------
> 
> §7:
> 
> >  This registration is done using the template defined in [RFC6838] and
> >  following [RFC4855].
> 
> To avoid the need to dereference the cited RFC, please also include the IANA
> table name ("Media Types") in this paragraph.

To do

> ---------------------------------------------------------------------------
> 
> §7:
> 
> >  This
> >  media type is framed binary data Section 4.8 of [RFC6838].
> 
> I think this means to say something like:
> 
>    This media type is framed binary data; see Section 4.8 of [RFC6838].

To do

> ---------------------------------------------------------------------------
> 
> §7:
> 
> >  Published specification:
> >
> >  [I-D.ietf-cellar-ffv1] and RFC XXXX.
> 
> I can't figure out why two documents are mentioned here -- it would seem that
> both of these refer to the same document.

To do

> ---------------------------------------------------------------------------
> 
> > 9.  Appendixes
> >
> > 9.1.  Decoder implementation suggestions
> >
> > 9.1.1.  Multi-threading Support and Independence of Slices
> 
> This seems like an unnecessarily deep nesting for its contents. Consider
> collapsing into a single section instead of three nested sections.

To do

> ---------------------------------------------------------------------------
> 
> Section 10 should contain instructions to the RFC editor to remove it prior to
> publication.

To do

> Thanks!
> 
> /a
>