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

From: "Murray S. Kucherawy" <[email-masked]>
Date: Tue, 28 Apr 2020 22:15:28 -0700

Hi there,

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.

I think this needs one more round before it can be sent to the IESG.
Fortunately most of this is editorial, but there's a lot, and there are one
or two substantive things I urge you to address.

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.
* "TBA" is not used in this document other than in the glossary.  It should
probably be removed.

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

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

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.

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?
* It looks a little odd to quote an equation, especially where that's not
done elsewhere in the document so far.

Section 3.7.2:
* The spacing around and within the equations in this section is not
consistent with the rest of the document.
* "offsetted" should be "offset"

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

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

Section 3.8.2.1:
* "2 parts" should be "two parts"
* "the prefix stores ..." should be the start of a new sentence.
* Another table with an unused "type" column.

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

Section 3.8.2.1.3:
* Same issue about "value".

Section 3.8.2.2.1:
* "2 parts" -- "two parts"; "2nd' -- "second"
* "the prefix" should begin a new sentence
* another table with an unused "type" column

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.
* The final paragraph appears to be made up of two sentences run together.

Section 3.8.2.3:
* sign_extend() is undefined.

Section 4:
* "1 or more" -- "one or more"
* This is the first time there's a reference to a "header".  Is more
context needed here?

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

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

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

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?

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

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

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

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

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

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

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

Section 4.4:
* Addressing normative language to authors of revisions to this document is
novel.  I suggest using non-normative prose here.
* "an other" should be "another"

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.

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?
* But does this function really output nothing?

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

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

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"?
* Same problem on the next line, coupled with quoted inline equations again.

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

Section 7:
* Does the subtype intend to be case-sensitive?  i.e., couldn't "ffv1" work?
* Required parameters should be "N/A" if you have none to register.
* "This parameter is used to signal ..." -- to what is this referring?

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

Section 9:
* Appendices go after the references.

-MSK

---

From: Jerome Martinez <[email-masked]>
Date: Wed, 29 Apr 2020 22:48:27 +0200

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

> consistent with the rest of the document.

Fixed.

> * "offsetted" should be "offset"

Fixed.

> * "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>:
> * 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>:
> * "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".

> * Same issue about "value".

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

> * "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>:

TO DO.

> * "1 or more" -- "one or more"

Fixed.

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

TO DO.

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

TO DO.

> 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

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

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

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

TO DO.

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

Fixed.

> Section 4.2.3.1 <http://4.2.3.1>:

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.

> * Same comment as before about inline formulae.

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

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

Fixed.

> * Same issue with quoted inline equations.

Fixed.

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

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.

> * This can be merged with Section 7.

Fixed.

> * Appendices go after the references.

TO DO.

Jérôme

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Wed, 29 Apr 2020 18:51:34 -0700

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

Sorry, disregard.  This referred to an issue I had with an earlier section
but I changed my mind, and deleted that one but forgot to delete this one.

> identifier found in AVI files (0x41564920 in hexadecimal).
Interesting.  You might want to make that clear, because I expect the RFC
Editor will have exactly the same question.

> SliceContent() could move into Slice() but IMO it is less readable.

This goes back to not understanding how to interpret these these "type"
columns.  But also, this is another place where a forward reference to
Line() would be helpful.  And then down in Line(), there are lines of
pseudo-code that appear to be standalone array references.  In the table in
Section 4.1, those lines have types, which I thought means there's some
output happening.  Is that not the case here?  If not, then this is a
nested set f function calls that do some computation but don't appear to
yield anything.  That's why I'm confused here.

> But it means that all is in one chapter, IMO it is a lot of formulae.
You don't have to put them all in one chapter.   I was thinking of just
this for 4.6.1, for example:

"primary_color_count" is defined as:

    1 + ( chroma_planes ? 2 : 0 ) + ( extra_plane ? 1 : 0 )

That even parses in C. :-)

-MSK