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

From: Derek Buitenhuis <[email-masked]>
Date: Fri, 28 Jul 2017 15:20:38 +0100

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.

>    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.

>                 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.?

> 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).

> 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.

>    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'?

>    "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?

>    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?

> 4.1.3.1.  In AVI File Format

Poor English. Perhaps remove the "In". Ditto for others.

>    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.

>    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?

> 
>                   +-------+----------------------------+
>                   | 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?

>    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.

> 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.

> 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.

[...]

Hopefully these comments have some value!

Cheers,
- Derek

---

From: Michael Niedermayer <[email-masked]>
Date: Fri, 28 Jul 2017 17:37:18 +0200

[...]

> color space, but a transform used to store RGB(A).

Its meant to be the colorspace formed by the JPEG2000-RCT
I see this isnt ideally worded but iam ATM not sure how to word this
better
Maybe calling it YCbCr (JPEG2000-RCT) would be an option

Besides the wording the colorspace formed by a reversible colorspace
transform is as much its own colorspace as YCbCr is distinct from RGB

It may be that implementations treat colorspaces which can be
losslessly converted to RGB as RGB. But from a specification point of
view these are not RGB and we may add other lossless transforms from
RGB in the future

[...]

> samples. Why is it suddenly pixels?

a RGB pixel represents 3 samples
do you see  an error in how the terms are used ?

[...]
>     https://wiki.xiph.org/Oggless#Example_and_Discussion_of_the_MOV_container

I think glbl was used as a generic box to hold a codec specific
global header.

if one looks at the bigger picture and not just ffv1 then a single
box should be simpler than a codec specific one.
a (de)muxer would otherwise probably need to have a list of these
boxes

[...]

Thanks for reviewing the spec!

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras

---

From: Derek Buitenhuis <[email-masked]>
Date: Fri, 28 Jul 2017 16:49:01 +0100

> view these are not RGB and we may add other lossless transforms from

All good points.

Ideally, I'd just like to see it made clearer that the goal, or one of the
goals, is to store RGB. It's not entirely clear to me how to best write this.

> do you see  an error in how the terms are used ?

It wasn't clear that "frame width in pixels" really meant that, to me.
A width in pixels, to me, means the width in pixels that is displayed.

Maybe a note can be added, or their description modified to reflect that?

> I think glbl was used as a generic box to hold a codec specific

Was this ever written anywhere (Apple docs, or some ISO spec)?

> a (de)muxer would otherwise probably need to have a list of these

Aren't custom boxes like this kind of standard procedure for codec
global data in MP4/MOV? Like hev1, avc1, etc. Unfortunate, though.

Cheers,

- Derek

---

From: Jerome Martinez <[email-masked]>
Date: Fri, 28 Jul 2017 19:18:01 +0200

> I'm new to this list / IETF stuff in general, so some of my comments may be moot

Thank you for reviewing.
and we are new to IETF too (our first work), but experimented people are 
usually there when we do a mistake (BTW: thank you IETF for that).

> reality it is 'remaining_bits_in_bitstream_component', not the whole

Good point.
This is the remaining bits in the current block (the Configuration 
Record or the Frame). I am not sure "component" fits here. I'll try to 
find an alternative suggestion.

> column has been shifted down by one. Based on the diagram below, it

OK.

> do they defer to the C spec on integer promotion, etc.?

 From my point of view it is integer as defined in 
https://en.wikipedia.org/wiki/Integer (so "Z"). I'll check if it is 
defined somewhere else I'll make it explicit.

> color space, but a transform used to store RGB(A).

 From the discussion you got with Michael, I suggest to change the 
wording of colorspace_type in order to be more explicit about what it 
is, something like:
0: YCbCr (planar)
1: YCbCr (line, from JPEG2000-RCT)
because in practice YCbCr samples are stored also with colorspace_type 
of 1 (and there is a difference of how it stored, plane and lines inside 
plane or line and planes inside lines)

Maybe some informational text (the C like text is the reference) too.

> Redundant 'code'. Variable Length Code code.

OK

> Should they not be defined in '2.2.9. Bitstream functions'?

OK.

> samples. Why is it suddenly pixels?

Definitely an issue, more eyes are good for catching such issue.
We need to define Pixel (and Sample).
Reading on the net, I see that sometimes sample is synonym of pixel, so 
we definitely need to make the difference more explicit with clear 
definition that a Pixel is the sum of all (1 to 4) primary samples (and 
change Pixel() to Sample() ).

> Is this specific to bitstream descriptions?

I started to reformat the pseudo-code, in order to be less tied to 
libavcodec or C, I did not yet reformatted the beginning (e.g. C uint8_t 
still there but not defined).

> Poor English. Perhaps remove the "In". Ditto for others.

My English :(.
I can remove "In", but need help for fixing "Poor English".

> Not sure if this ever actually existed in practice or not, but it's

I don't catch "it conflicts", in my understanding it is actually exactly 
same (a "global header" is stored here, for both Vorbis and FFV1).
Note that this is a description of how files in the wild are, we can not 
change that (e.g. doing like ISO who defines avc1, hvcC...)
We don't have the money and time for going throw the ISO process, and I 
think that e.g. Dolby does not do any registration for similar atoms 
(dac3, dec3...)
http://mp4ra.org/atoms.html has a very small list.
As far as I know, it was an arbitrary decision from FFmpeg developers 
for storing any global header in a generic atom instead of having 
different atom name for each format (format is already provided 
elsewhere, no need of different atom name), and I see no reason not to 
use that by default.

If it is not considered to have this part here because it is not 
something accepted by the MP4 standard body nor Apple (for MOV), we 
could say that this is informational only.

>>     other data.  See [Matroska] for more information about elements.

Why not?
We would like to avoid AVI compatibility layer (the files in the wild, 
using a template from Microsoft), and we would like to have a native 
binding in Matroska.

> it be defined, at least, or have a reference?

This is for FFV1 v4, out of scope (and only "experimental" in FFmpeg), 
I'll remove this part, and we add it again when we work on v4.

> This is the first and only time Pixel() appears in the document, and

Right, this is the missing part in the spec, on my ToDo-list.
(Note: to be replaced by "Sample()" or other suggestion)

> Same comment as before: JPEG2000-RCT is not a color space.

See answer before :).

> e.g. the JPEG2000-RCT equations.

OK.

> Further, the name 'log2_h_chroma_subsample', in the definition, is

OK

> Hopefully these comments have some value!

They definitely are!
This is good to have a new pair of eyes on it, this permits to see 
errors we don't see because we read the doc too many times and consider 
some parts as obvious but they are not.

I'll prepare a bunch of Pull Requests on GitHub (I'll ping on the list 
when it is ready, for comments)

Jérôme

---

From: Thomas Daede <[email-masked]>
Date: Fri, 28 Jul 2017 17:21:22 -0700

> global data in MP4/MOV? Like hev1, avc1, etc. Unfortunate, though.

For Opus in MP4, we used a custom sample entry box ("Opus"), which
turned out to be easy to register:

http://mp4ra.org/codecs.html

The FLAC in MP4 mapping likewise uses a custom one ("fLaC"), see:

https://bug1286097.bmoattachments.org/attachment.cgi?id=8805161

It would be nice for FFV1 to be consistent. However, if there are
actually a lot of FFV1 MP4 files in the wild, it would be good to at
least informationally document it for old versions. In practice, there's
nothing inside the stds box for it to conflict with.

---

From: Jerome Martinez <[email-masked]>
Date: Sat, 29 Jul 2017 10:08:37 +0200

> http://mp4ra.org/codecs.html

I understand that the discussion moved from the "Specific Box" atom to 
the "Sample Entry Code", this is a bit disturbing and I am a bit lost. I 
try to summarize what exists and issues.

I understand that "Opus" it for the "Sample Entry Codes" part, and I see 
"Opus" in the list on mp4ra.org, good.
for FFV1, "Sample Entry Code" is "FFV1" (registration pending), I see 
that it is so "obvious" in my mind that I forgot to write it in the 
corresponding part, I'll fix that (same for AVI)
As I understand, the "Specific Box" is "dOps" for Opus, and MP4/FFV1 
files in the wild have a "Specific Box" of "glbl".

> https://bug1286097.bmoattachments.org/attachment.cgi?id=8805161

Just to be clear:
"fLaC" is the "Sample Entry Code",
"dfLa" is the "Specific Box" atom name.

> nothing inside the stds box for it to conflict with.

I guess that it is like "V_FFV1" mapping for Matroska, we would have to 
move to "common practices" with a "dfv1"/"fv1C" (letter to add/letter to 
remove/uppercase/lowercase to be defined) atom instead of current "glbl" 
generic atom name specified nowhere for the moment, with a transition 
plan in FFmpeg/libav as we do for "V_FFV1", if we all agree.
On my side, I like the idea of "glbl" atom common to several formats 
(for demuxer, it is easier to pass this atom to the decoder during 
parsing than checking which atom is the initialization specific box, 
this is more generic) but I understand the consistency request, so no 
strong opinion now (just a small one for "glbl" as we already have files 
in the wild with that). Others?

Note: the initialisation (stream not decodable if missing) "Specific 
boxes" for some formats:
- AVC: "avcC"
- HEVC: "hvcC"
- FLAC: "dfLa"
- Opus: "dOps"

We also have:
- AC-3: "dac3"
- E-AC-3: "dec3"
- DTS: "ddts"
but theses box are not initialisation (if they are missing, the stream 
is still decodable, as far as I understand)

"???C" are listed in "Box types contained in specific Sample Entries" in 
http://mp4ra.org/codecs.html
"d???" are not listed in "Box types contained in specific Sample 
Entries" in http://mp4ra.org/codecs.html
Was there a reason to choose "d???" instead of "???C" for initialisation 
(called "Configuration" by ISO) e.g. some atom names reserved by ISO?
Any downside to use "glbl" "Specific Box" with 2 different "Sample Entry 
Codes"?