Re: [Cellar] draft-ietf-cellar-ffv1-13 comments

[Jerome Martinez <[e-mail]>]

Hi,

On 29/04/2020 07:15, Murray S. Kucherawy wrote:
> I've reviewed -13 of this document, and the results are below.  I also 
> used Adam's prior AD review as a checklist of sorts.

Thank you for your review. I put comments below to say 'Fixed' if the 
referenced pull request addresses it. Some of 'TO DO' and in a few 
places, I mention your name in the comment as I would like clarification.
https://github.com/FFmpeg/FFV1/pull/199


> Section 2..1:
> * "RCT" doesn't appear as a standalone term in this document other 
> than as part of the string "JPEG2000-RCT", so I don't know if it's 
> worth including in your glossary.

Fixed.
"RCT" definition is repeated when we explain "JPEG2000-RCT", so I 
removed it.


> * "TBA" is not used in this document other than in the glossary.  It 
> should probably be removed.

Fixed.
It is used in FFV1 v4 development branch but not in v3, so I flagged it 
as v4 only, it will not be in the next IETF doc.


> Sections 2.1 and 3.5:
> * "1 or more" should become "one or more".  Similarly, "1 of the 5" 
> should be "one of the five".

Fixed.


> Section 2.2.2:
> * The sentence starting "With" should be attached to the previous 
> sentence with a comma.  It's not a sentence by itself.
> * "This is equivalent to, b times dividing a by 2 with rounding toward 
> negative infinity." -- I suggest: "This is equivalent to dividing a by 
> 2, b times, with rounding toward negative infinity."

Fixed.


> Section 2.2.5:
> * The layout of this table is a little hard to parse. I would quote 
> the terms you're describing, or put colons after them, or something 
> like that.

Fixed.
I used the same method as previous chapters: "(name)" means (meaning).


> Section 3.3:
> * The "Background" paragraph starts out talking in the past tense, 
> then switches to present ("was used", then "when the issue is 
> discovered").  Then it switches back in a later sentence.  That should 
> all be past tense, right?

Fixed.


> * It looks a little odd to quote an equation, especially where that's 
> not done elsewhere in the document so far.

Fixed.


> Section 3.7.2:
> * The spacing around and within the equations in this section is not 
> consistent with the rest of the document.

Fixed.


> * "offsetted" should be "offset"

Fixed.


> Section 3.8.1.1.1:
> * "3 modes" should be "three modes"
> * "1 byte shorter" should be "one byte shorter"
> * "3 places" should be "three places"
> * "Above describes the range decoding, encoding is defined as ..." -- 
> this should be two separate sentences

Fixed.


> Section 3.8.1.2 <http://3.8.1.2>:
> * As Adam mentioned in his review: "The exact contexts used are best 
> described by Figure 15, followed by some comments.", but there are no 
> comments after Figure 15 nor after this prose.
> * Adam also pointed out that this table appears to have a "type" 
> column that serves no purpose.  It should either be used or removed.  
> Or, if as I think I figured out much later, this layout is actually a 
> way to indicate the types of output values when the pseudo-code yields 
> something, you really should explain that either here, or maybe 
> earlier where you're describing conventions used in this document.
> * Adam also pointed out that the put_rac() function is defined nowhere 
> in this document nor is a definition for it referenced.

We still have to handle these issues (separate fixes)


> Section 3.8.2.1 <http://3.8.2.1>:
> * "2 parts" should be "two parts"
> * "the prefix stores ...." should be the start of a new sentence.

Fixed.


> * Another table with an unused "type" column.

On the unused column global issue, the corresponding patch is in discussion:
https://github.com/FFmpeg/FFV1/pull/196#issuecomment-620697043


> Section 3.8.2.1.2:
> * The prose in the ESC line uses too many commas. Maybe: "the value - 
> 11, in MSB first order; ESC may only ..."

Fixed.
As there is a "may" which is used not really in a RFC 2119 manner, I 
extracted the sentence from the table and used "MUST NOT".


> Section 3.8.2.1.3:
> * Same issue about "value".

Murray, I didn't get it (no comma there)


> Section 3.8.2.2.1:
> * "2 parts" -- "two parts"; "2nd' -- "second"
> * "the prefix" should begin a new sentence

Fixed.


> * another table with an unused "type" column

Same as with other tables.


> Section 3.8.2.2.2:
> * This code uses a function defined in the next section.  They should 
> be swapped perhaps, or at least a forward reference would be helpful.

TO DO.


> * The final paragraph appears to be made up of two sentences run together.

Fixed.


> Section 3.8.2.3 <http://3.8.2.3>:
> * sign_extend() is undefined.

TO DO.


> Section 4:
> * "1 or more" -- "one or more"

Fixed.


> * This is the first time there's a reference to a "header".  Is more 
> context needed here?

TO DO.


> Section 4.1:
> * QuantizationTableSet() isn't defined until much later in the 
> document.  Again, a forward reference would be helpful.

TO DO.


> Section 4.1.1:
> * "... version 2 files SHOULD NOT exist ..." -- what's an implementer 
> supposed to do with this normative instruction?

Fixed.
We indicate that it is possible for a decoder to meet such stream, and 
the implementer is not required to support it, but it is also not 
required not to support it. It is just not part of the standard.
I rephrased in order to just say to reject such content.


> Section 4.1.3:
> * "... there is no known implementations ..." -- change "is" to "are", 
> or use "implementation"

Fixed.


> Section 4.1.5:
> * The normative language at the end here is pretty crisp, but what 
> should an implementation do if it receives something that doesn't comply?

Fixed.
I rephrased and say to reject such content


> Section 4.1.7:
> * The sentence at the end appears to be two sentences run together.

Fixed.


> Section 4.1.8, 4.1.9:
> * The spacing in the equations here isn't consistent with the rest of 
> the document.

Fixed.


> Section 4.1.15:
> * This conditional syntax is incomplete since the ":" part is missing.

TO DO.
Not missing in source file (in Markdown) and not missing in IETF HTML 
output.
Issue is in text output.


> Section 4.1.16:
> * As Adam pointed out, another unexplained reference to "header" 
> ("global" in this case).

TO DO.


> Section 4.2:
> * First-ever reference to a "track header".

TO DO.


> Section 4.2.2:
> * Change "crc" to "CRC" (two instances).  It's correct the third time.

Fixed.


> Section 4.2.3.1 <http://4.2.3.1>:
> * "AVI " should be just "AVI" (i.e., remove the trailing space)

Murray, the "trailing" space is required. It is a part of a 4-letter 
identifier found in AVI files (0x41564920 in hexadecimal).


> Section 4.4:
> * Addressing normative language to authors of revisions to this 
> document is novel.  I suggest using non-normative prose here.

Fixed.
I pushed the prose to the end in an informative appendix.


> * "an other" should be "another"

Fixed.


> Section 4.5.5:
> * Rather than separating this formula away from the text as was done 
> in prior sections, here it's quoted inline.  I find the former much 
> more readable.

Fixed.

> Section 4.6:
> * Another table with an empty "type" column.  I think I see now that 
> the column is populated against each line that outputs something, but 
> can it not just be omitted when there's no output to the function?

Same as with other tables.


> * But does this function really output nothing?

Murray, not sure I understand the question.
This function read nothing from the input, and call Line() which calls 
Pixel().
It is in a standalone pseudo-code block for an having blocks easy to 
understand (SliceHeader(), SliceContent(), SliceHeader()).
SliceContent() could move into Slice() but IMO it is less readable.


> Sections 4.6.1 through 4.6.4:
> * Same comment as before about inline formulae.

Fixed.
But it means that all is in one chapter, IMO it is a lot of formulae.


> Section 4.7:
> * This should have indicators for output types, I think.

Fixed.
I added a "sd" type.


> Section 4.7.1:
> * I don't understand the second sentence.  I parse it as: "X" and "Y" 
> value is "Z".  Did you mean "The values of X and Y are Z"?

Fixed.
I reworded a bit this part.


> * Same problem on the next line, coupled with quoted inline equations 
> again.

Fixed.


> Sections 4.7.2 and 4.7.3:
> * Same issue with quoted inline equations.

Fixed.


> Section 7:
> * Does the subtype intend to be case-sensitive?  i.e., couldn't "ffv1" 
> work?

Fixed.


> * Required parameters should be "N/A" if you have none to register.

Fixed.


> * "This parameter is used to signal ..." -- to what is this referring?

TO DO.


> Section 8:
> * This can be merged with Section 7.

Fixed.


> Section 9:
> * Appendices go after the references.

TO DO.

Jérôme