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 >> <linuxwolf+i...@outer-planes.net >> <mailto:linuxwolf+i...@outer-planes.net>> 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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
