Subject: [Cellar] Barry Leiba's Discuss on draft-ietf-cellar-ffv1-17: (with DISCUSS and COMMENT)

From: Barry Leiba via Datatracker <[email-masked]>
Date: Fri, 18 Sep 2020 21:38:44 -0700

Barry Leiba has entered the following ballot position for
draft-ietf-cellar-ffv1-17: Discuss

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/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

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 3.8.1.1 —
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 3.8.1.2 —

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

I see nothing in Section 3.8.1.1 that describes get_rac.

— Section 3.8.1.3 —

   At keyframes all Range coder state variables are set to their initial
   state.

What does “at keyframes” mean?

— Section 3.8.1.5 —
This is just a list of numbers with no explanation.  It needs text explaining
what it means.

— Section 3.8.2.2 —

   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?

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

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 2.2.9.1 —

   "remaining_bits_in_bitstream( )" means the count of remaining bits

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

---

From: "Murray S. Kucherawy" <[email-masked]>
Date: Thu, 24 Sep 2020 02:07:22 -0700

On Fri, Sep 18, 2020 at 9:38 PM Barry Leiba via Datatracker <
[email-masked]> wrote:

> [...]

There were enough issues raised in Barry's DISCUSS that I decided to push
this document off of this week's telechat agenda to the next one (in two
weeks) rather than having the whole IESG look at it.  Could we get a new
version out by, say, October 4th, that clarifies as much of this as
possible?

-MSK

---

From: Jerome Martinez <[email-masked]>
Date: Thu, 24 Sep 2020 11:10:36 +0200

On 24/09/2020 11:07, Murray S. Kucherawy wrote:
>
> There were enough issues raised in Barry's DISCUSS that I decided to 
> push this document off of this week's telechat agenda to the next one 
> (in two weeks) rather than having the whole IESG look at it.  Could we 
> get a new version out by, say, October 4th, that clarifies as much of 
> this as possible?

We are working on the issues, October 4th seems a good dead line for a 
new version.

Jérôme