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 protected]> > 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
_______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
