Matt, thanks for your (long-ago) review. Dave, thanks for addressing Matt’s comments.
Alissa > On Jul 6, 2018, at 1:50 PM, Matthew A. Miller > <linuxwolf+i...@outer-planes.net> wrote: > > 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 > > _______________________________________________ > Gen-art mailing list > Gen-art@ietf.org > https://www.ietf.org/mailman/listinfo/gen-art _______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
