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

[Dave Rice <[e-mail]>]

Hi all,
I’m providing an update to my status update on this review, the prior one is at https://mailarchive.ietf.org/arch/msg/cellar/UPdYs9A1FivzQJWiiW3uHLPSYwE/ <https://mailarchive.ietf.org/arch/msg/cellar/UPdYs9A1FivzQJWiiW3uHLPSYwE/>. To simply the reading of this email, I’m simply going to remove what (IMHO) has already been addressed by https://github.com/FFmpeg/FFV1/pull/185 <https://github.com/FFmpeg/FFV1/pull/185>.

> On Jan 9, 2020, at 11:02 AM, Dave Rice <[e-mail]> wrote:

[…]

>> 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.
>> 
>> ---------------------------------------------------------------------------
>> 
>> §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?

Michael Niedermayer resolved this in https://github.com/FFmpeg/FFV1/pull/187 <https://github.com/FFmpeg/FFV1/pull/187>.

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

Michael Niedermayer resolved this in https://github.com/FFmpeg/FFV1/pull/187 <https://github.com/FFmpeg/FFV1/pull/187>.

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

Michael Niedermayer resolved this in https://github.com/FFmpeg/FFV1/pull/187 <https://github.com/FFmpeg/FFV1/pull/187>.

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

Michael Niedermayer resolved this in https://github.com/FFmpeg/FFV1/pull/187 <https://github.com/FFmpeg/FFV1/pull/187> and https://github.com/FFmpeg/FFV1/pull/188/files <https://github.com/FFmpeg/FFV1/pull/188/files>.

>> ---------------------------------------------------------------------------
>> 
>> §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.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:
>> 
>>> 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.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.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
> 
>> ---------------------------------------------------------------------------
>> 
>> §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:
>> 
>>> 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
>> 
> 

Best Regards,
Dave Rice