Subject: Re: [Cellar] Quick Review of draft-ietf-cellar-ffv1-00

From: Jerome Martinez <[email-masked]>
Date: Thu, 4 Jan 2018 12:55:45 +0100

I added some Pull Requests on https://github.com/FFmpeg/FFV1 in order to 
fix some of the issues raised in this very useful post, comments inline:

On 28/07/2017 16:20, Derek Buitenhuis wrote:
> Hi all,
>
> Dave Rice pointed me at draft-ietf-cellar-ffv1-00 and draft-ietf-cellar-ebml-03,
> and these are my thoughts upon a cursory look at the former.
>
> I'm new to this list / IETF stuff in general, so some of my comments may be moot
> or a bit fuzzy.
>
> Comments follow inline below.
>
>> 2.2.9.1.  remaining_bits_in_bitstream
>>
>>     "remaining_bits_in_bitstream( )" means the count of remaining bits
>>     after the current position in that bitstream component.  It is
>>     computed from the NumBytes value multiplied by 8 minus the count of
>>     bits of that component already read by the bitstream parser.
> A bit confusing that this is named 'remaining_bits_in_bitstream', but in
> reality it is 'remaining_bits_in_bitstream_component', not the whole
> bitstream.

I prefer "block" instead of "bitstream_component" (too long IMO)
PR: https://github.com/FFmpeg/FFV1/pull/87

>>     o  one column of samples to the left of the coded slice is assumed as
>>        identical to the samples of the leftmost column of the coded slice
>>        shifted down by one row
> This doesn't specify what the top sample value should be, once the
> column has been shifted down by one. Based on the diagram below, it
> should be 0.

PR: https://github.com/FFmpeg/FFV1/pull/88

>>                  left16s = l  >= 32768 ? ( l  - 65536 ) : l
>>                  top16s  = t  >= 32768 ? ( t  - 65536 ) : t
>>                  diag16s = tl >= 32768 ? ( tl - 65536 ) : tl
> Does the IETF specs generally define when arithmetic is signed/unsigned, or
> do they defer to the C spec on integer promotion, etc.?

We use the word "integer" everywhere, IMO it is meaning 
https://en.wikipedia.org/wiki/Integer and not related to C. Not sure it 
is worth it to describe integers as "Z".

>> 3.7.2.  JPEG2000-RCT
> It's kind of a bit odd that this is titled like this, considering
> section 3.7 is titled 'Color space' and JPEG2000-RCT is not a
> color space, but a transform used to store RGB(A).

I modified 'Color space' to "pixel transformation", with more details 
about ho RGB content in encoded in FFV1.
PR: https://github.com/FFmpeg/FFV1/pull/89

>> 3.8.2.  Huffman coding mode
>>
>>     This coding mode uses Golomb Rice codes.  The VLC code is split into
> Redundant 'code'. Variable Length Code code.

I also change more things in order to use "Golomb Rice" everywhere.
PR: https://github.com/FFmpeg/FFV1/pull/90

>>     if (run_count == 0 && run_mode == 1) {                        |
>>         if (get_bits1()) {                                        |
>>             run_count = 1 << log2_run[run_index];                 |
>>             if (x + run_count <= w)                               |
>>                 run_index++;                                      |
>>         } else {                                                  |
>>             if (log2_run[run_index])                              |
>>                 run_count = get_bits(log2_run[run_index]);        |
> get_bits1() and get_bits() first appear here, with no definition.
> Should they not be defined in '2.2.9. Bitstream functions'?

this is a part I did not yet work on, anyway let's add the definition 
now so we don't forget.
PR: https://github.com/FFmpeg/FFV1/pull/91

>>     "frame_pixel_width" is defined as frame width in pixels.
>>
>>     "frame_pixel_height" is defined as frame height in pixels.
> The document, up until this point, and after this point, always refers to
> samples. Why is it suddenly pixels?

Looking carefully at pixel vs sample in the doc, there are sometimes 
inversion between the meaning of each word. I changed them and added a 
definition.
PR: https://github.com/FFmpeg/FFV1/pull/92

>>     pseudo-code                                                   | type
>>     --------------------------------------------------------------|-----
>>     ConfigurationRecord( NumBytes ) {                             |
>>         ConfigurationRecordIsPresent = 1                          |
>>         Parameters( )                                             |
>>         while( remaining_bits_in_bitstream( NumBytes ) > 32 )     |
>>             reserved_for_future_use                               | u(1)
>>         configuration_record_crc_parity                           | u(32)
>>     }
> Style changes completely half way through the spec, and also adds
> types to the types column. It's pretty obvious the earlier pseudo-code
> was pasted directly from libavcodec, but this code probably isn't
> written by Michael at all. Very different stuff; e.g. the previous
> code has variable types in the psuedo-code, but this does not.
> Is this specific to bitstream descriptions?

As you say, one is a copy/paste, and I did not yet touch to the copy/paste.
I plan to make it less dependent of C (i.e. without variable types, as 
it is expected to b C-like code but not with limitations of C about 
types) when I work on these parts.

>
>> 4.1.3.1.  In AVI File Format
> Poor English. Perhaps remove the "In". Ditto for others.

Dave did a PR for it (already merged).

>>     The Configuration Record extends the sample description box ("moov",
>>     "trak", "mdia", "minf", "stbl", "stsd") with a "glbl" box which
>>     contains the ConfigurationRecord bitstream.  See [ISO.14496-12.2015]
>>     for more information about boxes.
> Where is 'glbl' defined? If it's only this spec, then it conflicts with
> existing custom boxes for e.g.:
>
>      https://wiki.xiph.org/Oggless#Example_and_Discussion_of_the_MOV_container
>
> Not sure if this ever actually existed in practice or not, but it's
> something.

I am not sure the 'glbl' atom should be more defined here, as it is 
generic. Part of another discussion in the thread, I let it as is for 
the moment.

>
>>     FFV1 SHOULD use "V_FFV1" as the Matroska "Codec ID".  For FFV1
>>     versions 2 or less, the Matroska "CodecPrivate" Element SHOULD NOT be
>>     used.  For FFV1 versions 3 or greater, the Matroska "CodecPrivate"
>>     Element MUST contain the FFV1 Configuration Record structure and no
>>     other data.  See [Matroska] for more information about elements.
> Why only SHOULD?

 From IETF definitions:
"SHOULD   This word, or the adjective "RECOMMENDED", mean that there
    may exist valid reasons in particular circumstances to ignore a
    particular item, but the full implications must be understood and
    carefully weighed before choosing a different course."

There may be good reasons for using another identifier (e.g 
"V_MS/VFW/FOURCC", as this one is the only supported by old version of 
FFmpeg, so for compatibility reasons).
To be debated, but I am not in favor of MUST.

>
>>                    +-------+----------------------------+
>>                    | value | slice coding mode          |
>>                    +-------+----------------------------+
>>                    |   0   | normal Range Coding or VLC |
>>                    |   1   | raw PCM                    |
>>                    | Other | reserved for future use    |
>>                    +-------+----------------------------+
> This is the first and only time 'PCM' appears in this document. Should
> it be defined, at least, or have a reference?

this is from an early draft of v4 bitstream, not part of the current 
work. I suggest to completely remove this part for the moment.
PR: https://github.com/FFmpeg/FFV1/pull/93

>
>>     pseudo-code                                                   | type
>>     --------------------------------------------------------------|-----
>>     Line( p, y ) {                                                |
>>         if (colorspace_type == 0) {                               |
>>             for( x = 0; x < plane_pixel_width[ p ]; x++ )         |
>>                 Pixel( p, y, x )                                  |
>>         } else if (colorspace_type == 1) {                        |
>>             for( x = 0; x < slice_pixel_width; x++ )              |
>>                 Pixel( p, y, x )                                  |
>>         }                                                         |
>>     }                                                             |
> This is the first and only time Pixel() appears in the document, and
> without explanation.

Work in progress of the document.

>
>> 4.8.5.  colorspace_type
>>
>>     "colorspace_type" specifies the color space.
>>
>>                      +-------+-------------------------+
>>                      | value | color space used        |
>>                      +-------+-------------------------+
>>                      |   0   | YCbCr                   |
>>                      |   1   | JPEG2000-RCT            |
>>                      | Other | reserved for future use |
>>                      +-------+-------------------------+
> Same comment as before: JPEG2000-RCT is not a color space.

Same answer as before :).

>
>> 4.8.8.  h_chroma_subsample
>>
>>     "h_chroma_subsample" indicates the subsample factor between luma and
>>     chroma width ("chroma_width = 2^(-log2_h_chroma_subsample) *
>>     luma_width").
>>
>> 4.8.9.  v_chroma_subsample
>>
>>     "v_chroma_subsample" indicates the subsample factor between luma and
>>     chroma height ("chroma_height=2^(-log2_v_chroma_subsample) *
>>     luma_height").
> I get that the spec is trying to be mathematically correct, but why
> can't this contain the method that literally every single implementation
> will ever use: bitshifting by {v,h}_chroma_subsample. It's even a
> defined operator in "2.2.1. Arithmetic operators", and is used in
> e.g. the JPEG2000-RCT equations.
>
> Further, the name 'log2_h_chroma_subsample', in the definition, is
> incorrect.

Was in the former draft, and in code if I remember well. Right, I see 
some issue with having both {v,h}_chroma_subsample and 
log2_{v,h}_chroma_subsample in the spec. We are not bind to any external 
code, so we could change that.
I switched all to log2_{v,h}_chroma_subsample.
PR: https://github.com/FFmpeg/FFV1/pull/94

***

Lot of Pull Requests in a batch, don't hesitate to comment.

Jérôme

---

From: Michael Niedermayer <[email-masked]>
Date: Thu, 4 Jan 2018 13:14:20 +0100

On Thu, Jan 04, 2018 at 12:55:45PM +0100, Jerome Martinez wrote:
> I added some Pull Requests on https://github.com/FFmpeg/FFV1 in order to fix
> some of the issues raised in this very useful post, comments inline:
> 

Block in many video and image codecs generally refers to a 
rectgangular spatial area of pixels.
Macro block would refer to a larger square made of blocks.

We should not redefine commonly used terms. That could confuse people who
have worked with other video codecs

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued

---

From: Jerome Martinez <[email-masked]>
Date: Thu, 4 Jan 2018 13:53:04 +0100

> have worked with other video codecs

I reused the "Block" term from Matroska ( 
https://www.matroska.org/technical/specs/index.html#block_structure ) in 
order not to reinvent the wheel and this word is not used in FFV1, but I 
fully understand that it may be misleading in the video coding world and 
I have no opinion about which word to use, I just need a different word 
because "bitstream" is misleading as reported. Please suggest an 
alternative.

Jérôme

---

From: Dave Rice <[email-masked]>
Date: Thu, 4 Jan 2018 10:34:51 -0500

> On Jan 4, 2018, at 7:53 AM, Jerome Martinez <[email-masked]> wrote:
> 
> I reused the "Block" term from Matroska ( https://www.matroska.org/technical/specs/index.html#block_structure ) in order not to reinvent the wheel and this word is not used in FFV1, but I fully understand that it may be misleading in the video coding world and I have no opinion about which word to use, I just need a different word because "bitstream" is misleading as reported. Please suggest an alternative.

I added several reviews within the active pull requests at https://github.com/FFmpeg/FFV1/pulls <https://github.com/FFmpeg/FFV1/pulls>. Overall these changes help with the document a lot. Thanks to Derek and Jerome.

I agree with Michael that the term Block may not be best. The NumBytes definition already uses `FFV1 components`. Could also consider packet, segment, or IMHO `bitstream component` was fine though long.

Kind Regards,
Dave Rice

---

From: Ashley Blewer <[email-masked]>
Date: Thu, 4 Jan 2018 15:44:47 -0500

Thanks Jerome!

I brought up a related issue in https://github.com/FFmpeg/FFV1/pull/92
about use of luma vs luminance and am noting it here, as it is a separate
issue from the PR at hand, which I will now approve (but still think
changes should be made).

Ashley

On Thu, Jan 4, 2018 at 10:34 AM, Dave Rice <[email-masked]> wrote:

> I reused the "Block" term from Matroska ( https://www.matroska.org/
> technical/specs/index.html#block_structure ) in order not to reinvent the
> wheel and this word is not used in FFV1, but I fully understand that it may
> be misleading in the video coding world and I have no opinion about which
> word to use, I just need a different word because "bitstream" is misleading
> as reported. Please suggest an alternative.
>
>
> I added several reviews within the active pull requests at
> https://github.com/FFmpeg/FFV1/pulls. Overall these changes help with the
> document a lot. Thanks to Derek and Jerome.
>
> I agree with Michael that the term Block may not be best. The NumBytes
> definition already uses `FFV1 components`. Could also consider packet,
> segment, or IMHO `bitstream component` was fine though long.
>
> Kind Regards,
> Dave Rice
>
>
> _______________________________________________
> Cellar mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/cellar
>
>

---

From: Katherine Frances <[email-masked]>
Date: Fri, 5 Jan 2018 09:59:00 +1300

Luma v. luminance - good catch, Ash! Charles Poynton wrote a great article
about that.

On Fri, Jan 5, 2018 at 9:44 AM, Ashley Blewer <[email-masked]>
wrote:

>> I reused the "Block" term from Matroska ( https://www.matroska.org/techn
>> ical/specs/index.html#block_structure ) in order not to reinvent the
>> https://github.com/FFmpeg/FFV1/pulls. Overall these changes help with
>> the document a lot. Thanks to Derek and Jerome.
>>

---

From: Ashley Blewer <[email-masked]>
Date: Thu, 4 Jan 2018 15:59:19 -0500

Patch for aforementioned issue in #92 is now open here for review:
https://github.com/FFmpeg/FFV1/pull/97

---

From: Ashley Blewer <[email-masked]>
Date: Thu, 4 Jan 2018 16:03:21 -0500

> Luma v. luminance - good catch, Ash! Charles Poynton wrote a great article
Haha, yes, and it has been very fresh in my mind lately! I linked to it in
the github issue if anyone is interested.