Re: [Cellar] Genart early review of draft-ietf-cellar-ffv1-03

[Dave Rice <[e-mail]>]

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 <[e-mail]> wrote:
> 
> 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?

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?

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

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.

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

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

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

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

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

I agree. I moved it in the pull request.

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

I agree. I moved it in the pull request.

> Nits/editorial comments:
> 
> * In Section 2.1. "Definitions", a short description of what a
> "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.

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

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

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

Fixed in PR.

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

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

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

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

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

Removed in PR.

> * Most of the subsection contents with Section 4. seem to have
> 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>.

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

Thanks. Fixed in PR.

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

Thanks. Fixed in PR.

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

Thanks. Fixed in PR.

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

This is moved to a github issue.

Thanks again,
Dave Rice