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

Dave Rice <dave@dericed.com> Tue, 31 March 2020 15:17 UTC

Return-Path: <dave@dericed.com>
X-Original-To: cellar@ietfa.amsl.com
Delivered-To: cellar@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 801883A22C5; Tue, 31 Mar 2020 08:17:57 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 0.655
X-Spam-Level:
X-Spam-Status: No, score=0.655 tagged_above=-999 required=5 tests=[HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_NEUTRAL=0.652, URIBL_BLOCKED=0.001] autolearn=no autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rIi-APoPbj5v; Tue, 31 Mar 2020 08:17:54 -0700 (PDT)
Received: from server172-3.web-hosting.com (server172-3.web-hosting.com [68.65.122.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id CAB3B3A16E0; Tue, 31 Mar 2020 08:17:54 -0700 (PDT)
Received: from cpe-104-162-94-162.nyc.res.rr.com ([104.162.94.162]:39587 helo=[10.0.1.6]) by server172.web-hosting.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from <dave@dericed.com>) id 1jJIe9-001pM5-Su; Tue, 31 Mar 2020 11:17:52 -0400
From: Dave Rice <dave@dericed.com>
Message-Id: <36024E3E-9672-4950-B9AA-1F07667DD311@dericed.com>
Content-Type: multipart/alternative; boundary="Apple-Mail=_8E485551-3CF7-48A0-9839-B79D420EC761"
Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.40.2.2.4\))
Date: Tue, 31 Mar 2020 11:17:43 -0400
In-Reply-To: <9CF47172-A0AD-4ABF-B5BF-59D42E284867@dericed.com>
Cc: Adam Roach <adam@nostrum.com>, Codec Encoding for LossLess Archiving and Realtime transmission <cellar@ietf.org>
To: draft-ietf-cellar-ffv1.all@ietf.org
References: <b71e9496-c924-b970-9094-2b29ba173bdd@nostrum.com> <7DDCA42B-8BDA-4EBD-9C04-F0615265E021@dericed.com> <9CF47172-A0AD-4ABF-B5BF-59D42E284867@dericed.com>
X-Mailer: Apple Mail (2.3608.40.2.2.4)
X-OutGoing-Spam-Status: No, score=-2.4
X-AntiAbuse: This header was added to track abuse, please include it with any abuse report
X-AntiAbuse: Primary Hostname - server172.web-hosting.com
X-AntiAbuse: Original Domain - ietf.org
X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12]
X-AntiAbuse: Sender Address Domain - dericed.com
X-Get-Message-Sender-Via: server172.web-hosting.com: authenticated_id: dave@dericed.com
X-Authenticated-Sender: server172.web-hosting.com: dave@dericed.com
X-Source:
X-Source-Args:
X-Source-Dir:
X-From-Rewrite: unmodified, already matched
Archived-At: <https://mailarchive.ietf.org/arch/msg/cellar/EXdNLoj_z51wQHHXe16gHbY-tl8>
Subject: Re: [Cellar] AD Review: draft-ietf-cellar-ffv1
X-BeenThere: cellar@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Codec Encoding for LossLess Archiving and Realtime transmission <cellar.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/cellar>, <mailto:cellar-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/cellar/>
List-Post: <mailto:cellar@ietf.org>
List-Help: <mailto:cellar-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/cellar>, <mailto:cellar-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 31 Mar 2020 15:17:58 -0000

A few updates in regards to https://github.com/FFmpeg/FFV1/pull/193 <https://github.com/FFmpeg/FFV1/pull/193>

> On Mar 31, 2020, at 10:07 AM, Dave Rice <dave@dericed.com> wrote:
> 
> 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 <dave@dericed.com <mailto:dave@dericed.com>> wrote:
> 
> […]
> 
>>> On Jan 2, 2020, at 6:26 PM, Adam Roach <adam@nostrum.com <mailto:adam@nostrum.com>> 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

Reworded to "This section completes the media type registration template defined in [@!RFC6838] and following [@!RFC4855]” in https://github.com/FFmpeg/FFV1/pull/193 <https://github.com/FFmpeg/FFV1/pull/193>.

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

Addressed in https://github.com/FFmpeg/FFV1/pull/193 <https://github.com/FFmpeg/FFV1/pull/193>.

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

Addressed in https://github.com/FFmpeg/FFV1/pull/193 <https://github.com/FFmpeg/FFV1/pull/193>.

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

I reduced the 3 levels into "Appendix A: Multi-theaded decoder implementation suggestions” in https://github.com/FFmpeg/FFV1/pull/193 <https://github.com/FFmpeg/FFV1/pull/193>.

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

Addressed in https://github.com/FFmpeg/FFV1/pull/193 <https://github.com/FFmpeg/FFV1/pull/193>.

[…]

Best Regards,
Dave Rice