Re: [Cellar] I-D Action: draft-ietf-cellar-ffv1-15.txt

["Murray S. Kucherawy" <[e-mail]>]

Thanks, this looks much nicer!

On Thu, Jun 25, 2020 at 10:34 AM Dave Rice <[e-mail]> wrote:

> Hi Murray,
>
> > On Jun 25, 2020, at 1:19 PM, Jerome Martinez <[e-mail]>
> wrote:
> >
> > Hi Murray,
> >
> > My answers inline:
> >
> > On 25/06/2020 18:24, Murray S. Kucherawy wrote:
> >> Hi, thanks for this.  I'm looking now only at the diff between -13 (the
> last version I looked at) and this one.
> >>
> >> First and foremost, this is mostly better.  Kudos for putting the work
> in.
> >>
> >> Hooray, most of those "type" columns are gone!  But, alas, not all of
> them.  They're still present in Sections 4.1 and below.  I counted at least
> seven of them just in the diff I'm reviewing.  As before, they should also
> be removed, or at least explained.
> >>
> >> A minor point: Prior to Section 3.8.2.3, it looks like all of the
> examples are in C.  After that, they switch to pseudo code.  Any reason we
> can't be consistent?
> > The figures before section 4 are autonomous functions, used for
> describing a part of the algorithm and not used for directly reading out
> any symbol from the bitstream, so it was making sense to remove the "type"
> column at these places (this column was not used there). They could be more
> pseudocode nonetheless, as the C types ("int" etc) are not really relevant
> (they depends on the programming language).
> >
> > The figures in section 4 are for explaining the bitstream parsing order,
> and the "type" column is used for indicating the kind of value it is (types
> are described in
> https://tools.ietf.org/id/draft-ietf-cellar-ffv1-15.html#table-4 , I see
> that it is called "Symbol" here, we'll change "Symbol" to "Type" in this
> table).
> > This kind of method for describing a video bitstream is very classic,
> for example in:
> > - H.264, see the full spec (PDF to download)
> https://www.itu.int/rec/T-REC-H.264-201906-I/en
> > - AV1, see an online direct example at
> https://aomediacodec.github.io/av1-spec/#frame-obu-syntax
> >
> > As in theses specs, we have some figures without usage of the "type"
> column because they are intermediate figures (the "type" column is used in
> the figures using this figure and the figures called by this figure), split
> from another figure, in order to have the spec more readable (focusing on a
> specific topic). AV1 uses it for example at
> https://aomediacodec.github.io/av1-spec/#decode-tile-syntax
> >
> >> I suggest that the prose below Table 8, which is a mix of pseudocode
> and English, would be better broken apart.
> > We'll try to find a better wording for this line.
> >
> >> Chairs: RFC 4732 is a downref; please make sure the shepherd calls this
> out in the writeup.
> > (Removing my question as Spencer answered my question before I send it
> :-p )
> >
> >> There are a few code expressions that would benefit from being wrapped
> differently.  For example:
> >>
> >> (chroma_planes == 1 && (p == 1 || p == 2)) ? ceil(slice_pixel_height
> >> / (1 << log2_v_chroma_subsample)) : slice_pixel_height
> >>
> >> Maybe this?
> >>
> >> chroma_planes == 1 && (p == 1 || p == 2)
> >>   ? ceil(slice_pixel_height / (1 << log2_v_chroma_subsample))
> >>   : slice_pixel_height
> > It is on a full line in the specification source code, I see that the
> raw text version and the HTML version have different automatic line break
> position, we'll force some line breaks.
>
> I add line-breaks to this equation and a related one as you suggested in
> this commit
> https://github.com/FFmpeg/FFV1/pull/215/commits/1a4f83fb05591a1cfb4ef197682d06212ecf0b56.
> I also reviewed other equations and moved many to code blocks and manually
> set the line-wrapping. This work is to be reviewed in the pull request at
> https://github.com/FFmpeg/FFV1/pull/215.
>
> Best Regards,
> Dave Rice
>
>