Ballot deferred by Murray Kucherawy on 2020-09-21.
Summary: Has a DISCUSS. Has enough positions to pass once DISCUSS positions are resolved.
There are a number of things I’d like to discuss, because I find them not understandable. Perhaps it’s simply because I’m not a codec expert, but, while I understand that this is written for readers who will actually be implementing he FFV1 codec, some of them will also not be “experts”. That said, I’m sure some of this is just a case of “give Barry some clue and it’s fine.” — Section 2.1 — You use the term “symbol” here and later, without defining it, and I don’t know what it is. A byte? A character? A string of bits? What length? — Section 126.96.36.199 — I’m not sure how to interpret the stuff in this section. First, I don’t know why there are seven “figures”, with no captions nor other explanation. Second, I’m having trouble making sense out of things like this: S_(i + 1, C_(i)) = zero_state_(S_(i, C_(i))) AND l_(i) = L_(i) AND t_(i) = R_(i) - r_(i) <== b_(i) = 0 <==> L_(i) < R_(i) - r_(i) Can you explain how this is meant to be read? Maybe it’s just me, and maybe after you explain it I’ll whack myself on the head and say, “Doh!” Third, you say, “S_(0, i) is the i-th initial state,” but you haven’t previously introduced the term “state”, and I don’t know what it means. — Section 188.8.131.52 — "get_rac" returns a boolean, computed from the bytestream as described in Section 184.108.40.206. I see nothing in Section 220.127.116.11 that describes get_rac. — Section 18.104.22.168 — At keyframes all Range coder state variables are set to their initial state. What does “at keyframes” mean? — Section 22.214.171.124 — This is just a list of numbers with no explanation. It needs text explaining what it means. — Section 126.96.36.199 — The level is identical to the predicted one. The run and the first different level are coded. What does “level” mean? It’s not defined anywhere. — Section 4.3.1 — "reserved_for_future_use" has semantics that are reserved for future use. Yes, that seems rather obvious, though it’s oddly worded. But then you say what to do with “this value”, and nowhere do you say what the value is. How does one know?
Some editorial comments that I ask you to please consider: General: It’s jarring to see the defined terms, such as “Sample” and “Pixel” in quotes every time they’re used. More common practice is to quote them when they’re first defined, and then to just use the capitalization to show that they’re defined terms. I find that the quotes affect the readability and make it somewhat more of a challenge to read. — Section 2.1 — "Plane": A discrete component of a static image comprised of Total, total nit, and pet peeve: “composed of” or “comprising”, but not “comprised of”. — Section 2.2.2 — You’re missing “a % b”, which is referenced in Section 2.2.6. — Section 2.2.4 — "a > b" means a is greater than b. "a >= b" means a is greater than or equal to b. "a < b" means a is less than b. "a <= b" means a is less than or equal b. "a == b" means a is equal to b. "a != b" means a is not equal to b. I don’t think anyone will misunderstand this, so it’s a very small thing, but… might this be better said as, ‘ "a > b" is true when a is greater than b. ‘ (and similarly for the rest)? — Section 2.2.5 — "min(a,b)" means the smallest of two values a and b. "max(a,b)" means the largest of two values a and b. Nit: “smaller” and “larger”, for only two. — Section 2.2.7 — "a...b" means any value starting from a to b, inclusive. “starting”? I would leave that word out. — Section 188.8.131.52 — "remaining_bits_in_bitstream( )" means the count of remaining bits Section 184.108.40.206 uses remaining_bits_in_bitstream( NumBytes ), as do Sections 4.4 and 4.5. This should too, yes? — Section 3.2 — Figure 3: A depiction of how relatively positions Samples are references within this document. I think you mean “positioned” and “referenced”, yes? — Section 3.3 — Background: a two's complement signed 16-bit signed integer was used I’d remove the first “signed”. — Section 3.8.2 — The end of the bitstream of the "Frame" is filled with 0-bits until that the bitstream contains a multiple of 8 bits. Is “that” just an extra word that needs to be struck, or is something else wrong here? And is this meant to be what we usually call “padding”? — Section 4 — Within the following sub-sections, pseudo-code is used to explain the structure of each FFV1 bitstream component, as described in Section 2.2.1. I read this as saying that Section 2.2.1 describes the structure of each FFV1 bitstream component, until I went back there and looked. I suggest reordering the sentence to help the reader a bit: NEW Within the following sub-sections, pseudo-code is used, as described in Section 2.2.1, to explain the structure of each FFV1 bitstream component. END — Section 4.5 — Encoders SHOULD NOT fill these bits. Decoders SHOULD ignore these bits. Before this you describe two kinds of bits, and it’s not clear whether these are talking about both kinds, or just the latter. I think changing “these” to “reserved” is better. — Section 6 — A frequent security problem in image and video codecs is also to not check for integer overflows, for example to allocate "frame_pixel_width * frame_pixel_height" in "Pixel" count computations without considering that the multiplication result may have overflowed the arithmetic types range. This is somewhat complex and awkward (arguably a comma splice), and I suggest splitting the sentence thus: NEW A frequent security problem in image and video codecs is failure to check for integer overflows. An example is allocating "frame_pixel_width * frame_pixel_height" in "Pixel" count computations without considering that the multiplication result may have overflowed the arithmetic types range. END I might also make the “must” in the last sentence of that paragraph be “MUST”.
[[ comments ]] [ section 3.3 ] * Suggest either defining colorspace_type value and coder_type values before using them, or provide a reference in this section (i.e. just include a reference to sections 4.2.3 and 4.2.5). [[ nits ]] [ section 3.2 ] * In the description of Figure 3: "relatively positions" -> "relatively positioned", I think. [ section 3.3 ] * "signed 16-bit signed integer": choose any one instance of 'signed', I'd say [ section 3.7.2 ] * "and then if used transparency" -> "and then, if used, transparency"