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

["Matthew A. Miller" <[e-mail]>]

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

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.

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

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.

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

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.

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

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.

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

And thank you,


- m&m

Matthew A. Miller