[Cellar] Benjamin Kaduk's Abstain on draft-ietf-cellar-ffv1-18: (with COMMENT)

[Benjamin Kaduk via Datatracker <[e-mail]>]

Benjamin Kaduk has entered the following ballot position for
draft-ietf-cellar-ffv1-18: Abstain

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-cellar-ffv1/



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

I support Roman's Discuss.

I am balloting Abstain in that I am not confident that this
document has received sufficient review, but have not identified
specific flaws or areas of uncertainty that sould justify blocking
publication, and accordingly do not wish to block its publication.  In
particular, I am not confident that I myself understand how all the
pieces fit together -- even though some of my comments below might
indicate internal inconsistencies or other issues that would justify a
Discuss position, I am not fully confident in that, nor am I confident
that I could judge that they have been adequately addressed by any
proposed changes to the document's text.  (This is in large part related
to Barry's Discuss.)

That said, my review comments on the document appear below.

It might make sense to have a bit of discussion on what types of things
would (or would not) change in a new micro version, vs new "macro"
version, and what could be said to be "invariants" of the format overall.

Should we give more treatment (or even an IANA registry) for how
extensibility will be achieved for (e.g.) new "coder_type" and
"colorspace_type" values?

Section 2.2.9.4

   "get_bits( i )" is the action to read the next "i" bits in the
   bitstream, from most significant bit to least significant bit, and to
   return the corresponding value.  [...]

[I guess this "value" is always in the form of an unsigned integer.]

Section 3.3

   It is expected (to be confirmed) to remove this exception for the
   median predictor in the next version of the FFV1 bitstream.

When can/will the confirmation process happen?  When version 4 is
finalized?

Section 3.6

The discussion seems to imply that the chroma planes are not used in
FFV4 (and so the check for "version <= 3" in the logic here is just for
future compatibility); is that correct?

Section 3.7

   color space.  In YCbCr for each "Plane", each "Line" is coded from
   top to bottom and for each "Line", each "Sample" is coded from left
   to right.  In JPEG2000-RCT for each "Line" from top to bottom, each

Is there a reason that we use "JPEG2000-RCT" in the prose here, but
"RGB" for the section 3.7.2 heading?

Section 3.8.1.1

The defined terms do not cover all terms used (so I conclude that there
are unnamed intermediate values) and the formulae themself do not do a
great job of conveying where the overall inputs/outputs are, thus the
flow of the encoding logic.  Captions on the figures might help, which
could include indications of which intermediate values from from formula
to formula.  It also seems like some of the intermediate values could be
usefully named, e.g., R_(i) seems to serve the role of the remaining
range at a given step.

Section 3.8.1.2

Does "non binary" mean "non-integer", i.e., floating point?
We don't seem to use the "non binary" term elsewhere in the document.

I'm also not sure I'm reading the formula properly -- state is a uint8_t
but the indexing into it seems to make more sense to me if treated as
bit indexing, not byte indexing.  (E.g., 'a' is only doubled when
iterating through 22..31, indicating bitwise representation.)

       int e = 0;
       while (get_rac(c, state + 1 + min(e, 9)) { //1..10
           e++;
       }

It looks like this enters an infinite loop if get_rac(c, state + 10)
returns true; is there something to enforce that this does not happen?

   "get_rac" returns a boolean, computed from the bytestream as
   described in Section 3.8.1.1.

I suggest saying a little bit more about how this works, e.g., that the
boolean is a single-bit integer value, and how this functional notation
maps to the interfaces of the expressions in Section 3.8.1.1.

Section 3.8.2.1

What is 'bits' (the argument to the last get_bits() call) in Figure 20?
(Is it the same "bits_per_raw_sample" from §3.8?  If so, that
description should be qualified to apply more broadly than just "the
equation below".)

Section 3.8.2.2.1

What are 'x' and 'w' (and how are they set)?

What are the semantics of "run_mode" values 1 vs 2?

Section 3.8.2.3

I guess it's implicit that "output_number" is of the appropriate (width)
type for the desired output bit width.

Section 3.8.2.4

I guess "i += i" vs "i *= 2" is just a matter of style.

Where does the 'bits' input to sign_extend() come from?
(Is it the same "bits_per_raw_sample" from §3.8?  If so, that
description should be qualified to apply more broadly than just "the
equation below".)

Section 3.8.2.4.1

I suggest rewording or using a section reference for "normal difference
coding" as the phrase "difference coding" does not suffice (via text
search) to find the referenced procedure, since this is the only
instance of the phrase in the document.

Section 4.1

I'm not sure how to parse "number of equal entries -1".  In combination
with the example I can guess what is meant, but I still can't parse the
text.

The "Table" listed in the example does not seem to conform to "second
half [...] is identical to the first with flipped sign".

Also, the QuantizationTable() pseudocode suggests that the Table should
have 256 entries, but this example only has 16.  (That may be fine for
example purposes, but the difference should be called out.)

Section 4.2

C uses 0-indexed arrays, but the pseudocode seems to use 1-indexed
arrays (e.g., state_transition_delta[]).  Perhaps this is noteworthy.

Section 4.5

If 'reserved' is only present when version <=1, it seems unlikely that a
revision of this specification would assign semantics to it.

Section 4.6.x

nit: "if not present" could be taken to suggest that this field is
optional at field-level granularity (vs. at header-/version-level
granularity); "if header not present" might be slightly more clear.

Section 6

If we are attempting to differentiate between "format" and "codec", are
the uses of "codec" in this section all correct?

It may (or may not) be worth calling out the importance of internal
consistency within an implementation as to whether the Parameters()
appear in the ConfigurationRecord() or in each Frame().

   In all of the conditions above, the decoder and encoder was run
   inside the [VALGRIND] memory debugger as well as clangs address
   sanitizer [Address-Sanitizer], which track reads and writes to

nit: "clang's" with apostrophe.

Section 10

I don't see a particular reason that RFC 6716 needs to be a normative
reference.

(I also agree with Roman that there doesn't seem to be need to reference
both C90 and C18.)

Appendix A, B

It is surprising to see BCP 14 keywords in these nominally informative
sections.