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

Alissa Cooper <alissa@cooperw.in> Thu, 24 September 2020 02:31 UTC

Return-Path: <alissa@cooperw.in>
X-Original-To: cellar@ietfa.amsl.com
Delivered-To: cellar@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 381413A17A5; Wed, 23 Sep 2020 19:31:53 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.12
X-Spam-Level:
X-Spam-Status: No, score=-2.12 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=cooperw.in header.b=dPhBEldY; dkim=pass (2048-bit key) header.d=messagingengine.com header.b=AGWbcE4V
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id mqGQMRG_vOVJ; Wed, 23 Sep 2020 19:31:51 -0700 (PDT)
Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id F2FA33A1773; Wed, 23 Sep 2020 19:31:42 -0700 (PDT)
Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 42A8F5C011D; Wed, 23 Sep 2020 22:31:42 -0400 (EDT)
Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Wed, 23 Sep 2020 22:31:42 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cooperw.in; h= content-type:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; s=fm1; bh=W KOYgjdAw9V1F6w9Nj4cMSs2U45SyCPInoP1OofmmMU=; b=dPhBEldYCECe1LXdA WK6qZQ0tDuXu8Px+lr2J5RN2/ZeYiCkXFJypcCyEs48ZEsYeg+rg2mnUc73EsiVx jxPxvsBccmekWyuPBMU1mKFYfcK36nqAxmZZ3fv4Kg1Rv3FersUvOf2AbnwuzvU3 qc0CZhpx80VH7nrg4nCy98lI0CBAmlAu+t14URn0BkKmKMlFxnY6dZ75XBOB/YLl mVpfTs8XMD9dDn/4MUskdN5zNsH2c3guxFkZfpy6z/CbXR9iUAPSJL9zPHxsvUbR uld71Gk2j3MNGzXm7V99vZvwik/q5wrh5wpxuS7rLNlBZE6ZuvkzKhN3QM0rFPvn yKzJA==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=WKOYgjdAw9V1F6w9Nj4cMSs2U45SyCPInoP1Oofmm MU=; b=AGWbcE4VXVGcytYDQA4uBpxGn1uJgcsbizxWRYkcuvDvVVm5ozZIGjBjL KKonCRK1DxZx5j6Fqt6JInX0yryRJodKy69Dc8HnN6NUrf1Iw41mnYpR30qRpXQv Yg/cZAviNt9GBzPQgY4TyXyIXBffkLpnD76b3qOiZE9AFNjCgqfH3CY6Klw8JA65 29zcKVTupatog8iVmKvQNgXGDcvTJPyUZi+1DCGuujs8yLCWjZOHRLmmkTEUhcL3 xYurw1ntMazlAhtZ4o/GJeTFSa3emuTxulw7ZyvCOMmR5P+h3CzqzydeNih7SAd2 66Fw6UbI/El72bOTfuEIG2Je6CodQ==
X-ME-Sender: <xms:DgVsX_MTaHxUTk2YVHC5D7VXKozzXxncfMC6N20I23hG_mA5_pOXLg> <xme:DgVsX5_sNuD5sFMS7wNqxELAmkJb0WWgLFSE0J6flX8e-3S46tLWoedbYOXWIwr7F 6PER1zqXnL44BAf-Q>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrudejgdehkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpegtggfuhfgjfffgkfhfvffosehtqhhmtdhhtdejnecuhfhrohhmpeetlhhishhs rgcuvehoohhpvghruceorghlihhsshgrsegtohhophgvrhifrdhinheqnecuggftrfgrth htvghrnheptdejtdeuteekieejuddvuefhjeeggfeuffffkeeltedugfdtfedtjeduieet veehnecuffhomhgrihhnpehgihhthhhusgdrtghomhdpihgvthhfrdhorhhgnecukfhppe dujeefrdefkedruddujedrjeejnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghm pehmrghilhhfrhhomheprghlihhsshgrsegtohhophgvrhifrdhinh
X-ME-Proxy: <xmx:DgVsX-Tx7cvNLZNOYw_fPl0zZXb338WNaRk_ZEewvBUVi_fubUp_Og> <xmx:DgVsXztCtpZO6jgy1WqHqvsW4GQ1J4bt5mzbQb0aHGjEMgerI0cFOQ> <xmx:DgVsX3f5wl8mCyOxP_2BPaoFP4WV1sLh2b-zKzpiE1gc0Gcd-9D9tQ> <xmx:DgVsX4oeESWtAf7BZ3Okloc9LoRh2p9RaBeICn2svJdGKWCJYgqoPw>
Received: from rtp-alcoop-nitro2.cisco.com (unknown [173.38.117.77]) by mail.messagingengine.com (Postfix) with ESMTPA id B15CA306467E; Wed, 23 Sep 2020 22:31:41 -0400 (EDT)
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\))
From: Alissa Cooper <alissa@cooperw.in>
In-Reply-To: <31b43c76-1c3f-a669-fc1d-9f08999cebf3@outer-planes.net>
Date: Wed, 23 Sep 2020 22:31:41 -0400
Cc: Dave Rice <dave@dericed.com>, gen-art@ietf.org, cellar@ietf.org, draft-ietf-cellar-ffv1.all@ietf.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <5395D1B9-6229-4AC1-8F09-9AAC1E9089D2@cooperw.in>
References: <153082443369.26281.17163340643478720739@ietfa.amsl.com> <4016F335-7C19-4F25-AED8-B4A97E0F3A97@dericed.com> <31b43c76-1c3f-a669-fc1d-9f08999cebf3@outer-planes.net>
To: "Matthew A. Miller" <linuxwolf+ietf@outer-planes.net>
X-Mailer: Apple Mail (2.3608.120.23.2.1)
Archived-At: <https://mailarchive.ietf.org/arch/msg/cellar/XEUKUHurxbavs7uXgj3mWOTHPuo>
Subject: Re: [Cellar] [Gen-art] Genart early review of draft-ietf-cellar-ffv1-03
X-BeenThere: cellar@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Codec Encoding for LossLess Archiving and Realtime transmission <cellar.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/cellar>, <mailto:cellar-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/cellar/>
List-Post: <mailto:cellar@ietf.org>
List-Help: <mailto:cellar-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/cellar>, <mailto:cellar-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 24 Sep 2020 02:31:59 -0000

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+ietf@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+ietf@outer-planes.net
>>> <mailto:linuxwolf+ietf@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