[Cellar] review of draft-niedermayer-cellar-ffv1-01

[Dave Rice <[e-mail]>]

Hi all,

I thought I'd provide feedback on the current state of the FFV1 specification as found at https://datatracker.ietf.org/doc/draft-niedermayer-cellar-ffv1/. Please note this review isn't complete and there are still many parts of the specification that I don't completely understand.

== Introduction ==

Overall this section could be expanded to provide more characteristics about FFV1, what encoding technologies it is based upon, and how it may be used. I think at this point the sentence "The latest version of this document is available..." could be removed.

== Definitions ==

These definitions could be reordered to prevent a need for forward reference. For instance "RCT" references "RGB" and "YCbCr" but "YCbCr" and "RGB" come after "RCT".

"VLC" explains the abbreviation, but does not offer an explanation of the meaning as is done for the other defined terms.

The term "symbol" is used within the Definitions section and throughout the document. I think "symbol" and perhaps "sample" could themselves use definitions.

== Comparison operators ==

I think there is a typo in the "a || b" section:   "a || b" means Boolean logical "or" of a and b.
Perhaps this should be "a or b".

In "!a" I suggest that the "not" should be changed to "not a".

== Mathematical functions ==

The definition for "abs(a)" uses an undefined function "sign(a)".

== remaining_bits_in_bitstream ==

I suggest using a variable value within "remaining_bits_in_bitstream( )" such as "remaining_bits_in_bitstream( a )" and then using "a" within the definition. Same with the next function "byte_aligned ( )".

== Border ==

Some issues are already discussed at https://github.com/FFmpeg/FFV1/issues/42, but the language could be more clear.

== Median predictor ==

This section starts right away with what looks like a function. It could use some context, for instance to start with "Three neighboring samples are used to create a median predictor:".

== Context ==

The diagram could use an explanation for what "X" is, as well as to say that 'tl' is 'top-left', 't' is 'top', etc. Although the diagram appears to use "X" as a reference point, the "X" is not discussed or used within the equation.

The form of "Q_0[a-b]" is introduced here, but the meaning is not explained. Are the pipes in the equation used to mean absolute value, as in "|Q_0|" is the "absolute value of the zeroth quantization value"?

Perhaps the Quantization section should preceed the Context section.

Instead of "-context" it could say something like "the absolute value of context".

== Quantization ==

In this equation "  Q_{i}[a-b]=Table_{i}[(a-b)&255]" what do "a" and "b" signify? I presume this would be from the list of sample differences, but it could be more clear.

== JPEG2000-RCT ===

These equations are familiar but the section could use an introductory paragraph to say what they are for.

== Coding of the sample difference ==

The "(or n+1)" should be "(or n+1, in the case of RCT)" to clarify what the parenthetical is referring to. Alternatively the section could be divided into an explanation in the case of RCT and an explanation for non-RCT.

== Range coding mode ==

Is the anecdote about early experimental versions of FFV1 needed? Does this document define those early experimental versions? If not perhaps the anecdote could be removed.

=== Range binary values ==

Some values are defined in the introduction of this section but many are missing. For instance what does r_i, R_i, l_i, L_i, and t_i mean? Left, right, top? Also in the equations what is "k" representing? Is it the same "k" as from the Huffman coding mode section?

S_{0,i} is defined as the i-th initial state, but why is there a "0," in the subscript to express that?

The first equation uses C_i within the subscript of S_i. Is that intention or are C_i and S_i intended to be at the same level?

The second equation starts with S with "i+1,C_i" as a subscript. From the introduction statement S with "0,i" means the i-th State. What is the "0" meaning here? And how is the comma read in words? So is the second equation implying that it considers the C_i-th state?

Also note that I've tried in https://github.com/FFmpeg/FFV1/pull/50 to express these equations in an RFC-friendly manner, but to represent the subscript (particularly when there are subscripts and subscripts) is particularly challenging. Is there a method to express the same without subscript?

== Range non binary values ==

The introduction contains some think-out-loud style considerations about data encoding, this language could be made more concise and direct.

Noting that the backslashed comments included in the function break the barrier within the table.

== State transition table ==

This section is equation-only. It could benefit from a introductory sentence to give context.

== default state transition ==

This section is only the table and no introduction. It should note that the coder_type MUST be set to '1' when using this table.

== alternative state transition table ==

"has been build" -> "has been built"

Note that here it's called the "alternative state transition table" but in the coder_type section it is called the "custom state transition table". Are these phrases meaning the same thing, or does "custom" imply any user-customized table?

== Prefix ==

Does the table here imply that the Prefix of the VLC code MUST be between 1 and 12 bits? There are likely some requirements of the prefix that could be stated here.

== Examples ==

Based upon what I can gather from the Prefix and Suffix tables, I am not able to fully comprehend what is communicated by the examples section. Some narrative could help.

== Mapping FFV1 into Containers ==

This section is a large subsection of the "Bitstream" section. It may be out of place and perhaps would be better places at the same sectional level as "Bitstream".

== Frame ==

Within the Frame structure is data called "Parameters" is this the same as the "Parameters" within the Configuration Record? If not, perhaps they should use distinct terms. Also there is a lowercase "parameters" used throughout the document. If "parameters" uses a locally defined meaning then it should be in title case.

== slice_coding_mode ==

This references a manner of slice coding via PCM but there is no other reference to this mode. When would PCM encoding be used and not Range or VLC?

== error_status ==

I'm not certain when error status would not equal zero. If the ffv1 encoder expected to detect writing errors and record them here? What sorts of conditions would trigger that?

== slice footer ==

The footer contains the slice size which IIUC allows some bidirectional seeking and resilience. It may be worthwhile to say here how this works in the event that some slices are corrupted.

Best Regards,
Dave Rice