Subject: [Gen-art] Genart early review of draft-ietf-cellar-ffv1-03

From: Matthew Miller <[email-masked]>
Date: Thu, 05 Jul 2018 14:00:33 -0700

Reviewer: Matthew Miller
Review result: On the Right Track

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-cellar-ffv1-03
Reviewer: Matthew A. Miller
Review Date: 2018-07-05
IETF LC End Date: N/A
IESG Telechat date: N/A

Summary:

This document is moving in the right direction, but needs
work.  Overall, the document feels unfinished.  It's clear
a lot of work has gone into this, and there's been tremendous
effort put into the details.  However, it's lacking some
clarity, that was present in older revisions that were removed;
speculatively is it due to more generic coverage that
later revisions covered?

The introduction is quite helpful in answering the inevitable
question "What is a FFV1?".  For the most part, the flow of
the document seems to make sense.

Major issues:

I can understand relying on pseudo-code for something very math-
and/or algorithm-heavy, but some prose would be quite helpful in
understanding how and why the parts relate to one another.  The
Definitions section is essentially what I would look for, but only
accounts for some of the terms used within the rest of the
document.  As written, this document depends entirely on the
reader being intimately familiar with the subject matter.

Minor issues:

* Please consider moving the text of Section 2.2.6. "Pseudo-code"
up a level to 2.2. "Conventions" or as the first sub-section under
2.2. "Conventions".  This points seems to me to warrant more
significance than it currently has.

* In reading this, I wondered if it would help improve the
understanding of this document if Sections 3. and 4. swapped
places.  I think it's worth considering, but accept this
suggestion could be rejected.

* Please consider moving Section 4.8. "Parameters" to immediately
proceed from Section 4.1. "Configuration Record".  I think it
would help with understanding the document.

* Please consider moving the ASCII diagram of a Frame from
Section 9.1.1. "Multi-threading support and independence of slices"
to Section 4.2. "Frame".

Nits/editorial comments:

* In Section 2.1. "Definitions", a short description of what a
"Frame" and "Slice" are conceptually would be very helpful.

* In Section 2.2.4. "Mathematical functions", the definition of
"ceil(a)"" appears to be a copy from "floor(a)"; I would suggest:

   """
   ceil(a) the smallest integer greater than or equal to a
   """

* In Section 2.2.6. "Pseudo-code", the word "as" ought to be
"and" in "as uses its".

* The first use of "JEP2000-RCT" (in Section 3.3.) is not
immediately followed with its reference.

* In Section 3.7.2. "RGB", there is a paragraph that is solely
""[ISO.15444-1.2016]"", almost as if it's a reference that was
meant a section heading.

* In Section 3.8.1. "Range coding mode", there appears to be some
odd formatting with "_G. Nigel_ and "N. Martin_"; is this an
attempt to italicize?

* Most of the subsection contents with Section 4. seem to have
extraneous newlines (e.g., 4.1.1. "reserved_for_future_use").
  
* In Section 4.4.9. "sar_den", the text is identical to the
preceding section.  I think this is meant to be the denominator
to complement "sar_num" as the numerator.

* In Section 4.5.1. "primary_color_count", the pseudo-code is
inline with no quotes, which is inconsistent with the rest of the
document.

* In Section 4.7.1. "slice_size", the "Note:" appears to be two
sentence fragments.  I think the following conveys the same
meaning:

   """
   Note: this allows finding the start of slices before previous
   slices have been fully decoded, and allows parallel decoding as
   well as error resilience.
   """

* Section 11. "ToDo" still as one item remaining.

---

From: Dave Rice <[email-masked]>
Date: Fri, 6 Jul 2018 11:23:06 -0400

Hi,
Thank you Matthew for this detailed review. Here are my comments related to a PR at https://github.com/FFmpeg/FFV1/pull/119 <https://github.com/FFmpeg/FFV1/pull/119> which responds to some of these comments. Other comments below may require more discussion.

> On Jul 5, 2018, at 5:00 PM, Matthew Miller <[email-masked]> wrote:
> 
> Reviewer: Matthew Miller

I’m not sure I understand as it hasn’t seemed like many parts have been removed during the drafting work. Could you provide an example?

> reader being intimately familiar with the subject matter.

This has been discussed before and I’ve appreciated how some other IETF documents which include pseudo-code include an “as read” narrative version of the code as well. I think there may have been some concern of any risk of discrepancy between what the pseudo-code and the narrative of the pseudo-code communicate. If anyone know some boilerplate for managing this, please share.

> significance than it currently has.

I started a pull request at https://github.com/FFmpeg/FFV1/pull/119 <https://github.com/FFmpeg/FFV1/pull/119> and moved this section.

> suggestion could be rejected.

I reviewed these sections and feel hesitant swapping the order outright. Any other comments from cellar on this proposal?

> would help with understanding the document.

I agree. I moved it in the pull request.

> to Section 4.2. "Frame”.

I agree. I moved it in the pull request.

> "Frame" and "Slice" are conceptually would be very helpful.

I agree. I moved it in the pull request. Comments welcome on these definitions:

`Frame`: An encoded representation of a complete static image.

`Slice`: A spatial sub-section of a `Frame` that is encoded separately from an other region of the same frame.

>   “""

Already fixed in https://github.com/FFmpeg/FFV1/commit/a6191aae2beb29879ac7f82530fa92a950f8a4bb <https://github.com/FFmpeg/FFV1/commit/a6191aae2beb29879ac7f82530fa92a950f8a4bb>.

> "and" in "as uses its”.

Fixed in PR.

> immediately followed with its reference.

I added the reference to the first use in the PR.

> ""[ISO.15444-1.2016]"", almost as if it's a reference that was

Fixed in https://github.com/FFmpeg/FFV1/commit/8b6304ebf25764b5d2820497be1ad90ebadc624f <https://github.com/FFmpeg/FFV1/commit/8b6304ebf25764b5d2820497be1ad90ebadc624f>.

> odd formatting with "_G. Nigel_ and "N. Martin_"; is this an

Removed in PR.

> extraneous newlines (e.g., 4.1.1. "reserved_for_future_use”).

I suggest some discussion on this. I remember that it was discussed by not resolved. See https://github.com/FFmpeg/FFV1/pull/85 <https://github.com/FFmpeg/FFV1/pull/85>.

> to complement "sar_num" as the numerator.

Thanks. Fixed in PR.

> inline with no quotes, which is inconsistent with the rest of the

Thanks. Fixed in PR.

>   “""

Thanks. Fixed in PR.

> * Section 11. "ToDo" still as one item remaining.

This is moved to a github issue.

Thanks again,
Dave Rice

---

From: "Matthew A. Miller" <[email-masked]>
Date: Fri, 6 Jul 2018 11:50:37 -0600

Thanks Dave for the quick work!  I've read through the PR and commits
you provided, and think it will be in much better shape.  I look forward
to reviewing the next published revision.

I've made contextual responses below to outstanding questions.

On 18/07/06 09:23, Dave Rice wrote:
> Hi,
> Thank you Matthew for this detailed review. Here are my comments related
> to a PR at https://github.com/FFmpeg/FFV1/pull/119 which responds to
> some of these comments. Other comments below may require more discussion.
> 
>> On Jul 5, 2018, at 5:00 PM, Matthew Miller
>> <[email-masked]
>> <mailto:[email-masked]>> wrote:
>>
>> Reviewer: Matthew Miller
> I’m not sure I understand as it hasn’t seemed like many parts have been
> removed during the drafting work. Could you provide an example?
> 

When I first opened this document, I felt overwhelmed by some of the
terminology and unclear on what the structure/architecture was, so I did
some spelunking.  It's largely why I took my review is five days late
despite having 30 days to get through it (that, and I still
procrastinated some in the middle).

I went all the way back to versions prior to WG adoption and saw there
were some additional prose throughout; although it seemed the earlier
revisions were more restricted in what could be represented in FFV1 than
the current document allows, it helped me better understand.

I apologize for not taking more detailed note of which sections in those
previous revisions helped me the most, but I can try to do that.
However, I think the additions and reorganization in the PR goes a long
way to helping already.

> This has been discussed before and I’ve appreciated how some other IETF
> documents which include pseudo-code include an “as read” narrative
> version of the code as well. I think there may have been some concern of
> any risk of discrepancy between what the pseudo-code and the narrative
> of the pseudo-code communicate. If anyone know some boilerplate for
> managing this, please share.
> 

My experience has been that (pseud-)code is great to cover the details,
but the overall forest can get lost reading each tree.  I think the
additional terms added plus some of the reorganizing addresses this
concern, but I'd need to re-read the document with all the changes to be
sure.

> I started a pull request at https://github.com/FFmpeg/FFV1/pull/119 and
> moved this section.
> 
> I reviewed these sections and feel hesitant swapping the order outright.
> Any other comments from cellar on this proposal?
> 

I personally felt I had a better grasp of the document after reading
Section 4 and re-reading Section 3.  If such a big change is too much,
think it might be possible to expand the introduction or background a
bit to restate/duplicate some of the high-level structure; e.g., how
Containers, Frames, and Slices fit together.

> Already fixed
> in https://github.com/FFmpeg/FFV1/commit/a6191aae2beb29879ac7f82530fa92a950f8a4bb.
> 
> Fixed
> in https://github.com/FFmpeg/FFV1/commit/8b6304ebf25764b5d2820497be1ad90ebadc624f.
> 
> I suggest some discussion on this. I remember that it was discussed by
> not resolved. See https://github.com/FFmpeg/FFV1/pull/85.
> 

I read the commentary from that PR, and appreciate the argument.  I'm
also anticipating the RFC Editor coming back with proposed changes that
better match their conventions.

If I may suggest after read the PR comments, taking the current line
breaks and treating them as paragraphs seems like a reasonable editorial
change to me that appears to align with the comments.

> This is moved to a github issue.

And thank you,

- m&m

Matthew A. Miller