Subject: [Cellar] [PATCH FFV1 0/2] Clarification about versions and slice structure

From: Jerome Martinez <[email-masked]>
Date: Wed, 25 May 2016 22:40:43 +0200

This patch is for some formatting, because it is becoming slightly 
incoherent after some additions:
- version value of 2 is set as reserved but this value is seen in 
different places, I removed any reference to this value.
- applicable version is difficult to catch (e.g. sometimes >=2, 
sometimes >1), I think that showing the version number when a feature 
begins ("if >= 3") or stops (<=1) can avoid some mistakes by people who 
implement.
- there is a slice header element but no slice footer despite the fact a 
slice footer is clearly visible (not part of the decoding) and we 
discuss about it in the appendix. I propose to split in 3 parts (Header, 
Content, Footer) to better see the architecture of the slice, and we 
could add some text about each part in a future patch (in this patch, 
parts of the slice are moved to their own paragraph without any change)

---

From: Jerome Martinez <[email-masked]>
Date: Wed, 25 May 2016 22:41:23 +0200

From 95489ba2b6d670ef6027e134625c744869bc1572 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Wed, 25 May 2016 20:24:40 +0200
Subject: [PATCH 1/2] More explicit version tests

Use everywhere the version number when a feature begins or stop
Avoid usage of the reserved version value 2
Remove tests specific to reserved version value 2
---
 ffv1.md | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/ffv1.md b/ffv1.md
index 11d0faa..588330f 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -472,7 +472,7 @@ Default values at the decoder initialization phase:
 
 ## Configuration Record
 
-In the case of a bitstream with version >= 2, a configuration record is stored in the underlying container, at the track header level.
+In the case of a bitstream with version >= 3, a configuration record is stored in the underlying container, at the track header level.
 It contains the parameters used for all frames.
 The size of the configuration record, NumBytes, is supplied by the underlying container.
 
@@ -533,7 +533,7 @@ See [NUT](#references) for more information about elements.
 |                                                                    |
 |------------------------------------------------------------|:------|
 |Slice( i ) {                                                | type  |
-|    if( version \> 2 )                                      |       |
+|    if( version \>= 3 )                                     |       |
 |        SliceHeader( i )                                    |       |
 |    if( colorspace\_type == 0) {                            |       |
 |        for( p = 0; p \< primary\_color\_count; p++ ) {     |       |
@@ -546,7 +546,7 @@ See [NUT](#references) for more information about elements.
 |    if ( coder\_type == 0 )                                 |       |
 |        while ( !byte\_aligned() )                          |       |
 |            padding                                         |  u(1) |
-|    if( i \|\| version \> 2 )                               |       |
+|    if( version \>= 3 )                                     |       |
 |        slice\_size                                         | u(24) |
 |    if( ec ) {                                              |       |
 |        error\_status                                       |  u(8) |
@@ -589,7 +589,7 @@ The CRC generator polynom used is the standard IEEE CRC polynom (0x104C11DB7) wi
 |    picture\_structure                                      | ur |
 |    sar\_num                                                | ur |
 |    sar\_den                                                | ur |
-|    if( version \> 3 ) {                                    |    |
+|    if( version \>= 4 ) {                                   |    |
 |        reset\_contexts                                     | br |
 |        slice\_coding\_mode                                 | ur |
 |    }                                                       |    |
@@ -607,7 +607,7 @@ Inferred to be 1 if not present.
 **slice_height** indicates the height on the slice raster formed by num_v_slices.
 Inferred to be 1 if not present.
 
-**quant\_table\_index\_count** is defined as 1 + ( ( chroma_planes || version \< 4 ) ? 1 : 0 ) + ( alpha_plane ? 1 : 0 ).
+**quant\_table\_index\_count** is defined as 1 + ( ( chroma_planes || version \<= 3 ) ? 1 : 0 ) + ( alpha_plane ? 1 : 0 ).
 
 **quant\_table\_index** indicates the index to select the quantization table set and the initial states for the slice.
 Inferred to be 0 if not present.
@@ -649,27 +649,27 @@ Inferred to be 0 if not present.
 |------------------------------------------------------------|----:|
 | Parameters( ) {                                            | type|
 |   version                                                  | ur  |
-|   if( version \> 2 )                                       |     |
+|   if( version \>= 3 )                                      |     |
 |       micro\_version                                       | ur  |
 |   coder\_type                                              | ur  |
 |   if( coder\_type \> 1 )                                   |     |
 |       for( i = 1; i \< 256; i++ )                          |     |
 |           state\_transition\_delta[ i ]                    | sr  |
 |   colorspace\_type                                         | ur  |
-|   if( version \> 0 )                                       |     |
+|   if( version \>= 1 )                                      |     |
 |       bits\_per\_raw\_sample                               | ur  |
 |   chroma\_planes                                           | br  |
 |   log2( h\_chroma\_subsample )                             | ur  |
 |   log2( v\_chroma\_subsample )                             | ur  |
 |   alpha\_plane                                             | br  |
-|   if( version \> 1 ) {                                     |     |
+|   if( version \>= 2 ) {                                    |     |
 |       num\_h\_slices - 1                                   | ur  |
 |       num\_v\_slices - 1                                   | ur  |
 |       quant\_table\_count                                  | ur  |
 |   }                                                        |     |
 |   for( i = 0; i \< quant\_table\_count; i++ )              |     |
 |       QuantizationTable( i )                               |     |
-|   if( version \> 1 ) {                                     |     |
+|   if( version \>= 2 ) {                                    |     |
 |       for( i = 0; i \< quant\_table\_count; i++ ) {        |     |
 |           states\_coded                                    | br  |
 |           if( states\_coded )                              |     |
@@ -684,8 +684,8 @@ Inferred to be 0 if not present.
 
 **version** specifies the version of the bitstream.
 Each version is incompatible with others versions: decoders SHOULD reject a file due to unknown version.
-Decoders SHOULD reject a file with version < 2 && ConfigurationRecordIsPresent == 1.
-Decoders SHOULD reject a file with version >= 2 && ConfigurationRecordIsPresent == 0.
+Decoders SHOULD reject a file with version =< 1 && ConfigurationRecordIsPresent == 1.
+Decoders SHOULD reject a file with version >= 3 && ConfigurationRecordIsPresent == 0.
 
 |value   | version                 |
 |:-------|:------------------------|
@@ -857,7 +857,7 @@ MAX\_CONTEXT\_INPUTS is 5.
 
 ### Restrictions
 
-To ensure that fast multithreaded decoding is possible, starting version 2 and if frame\_width * frame\_height is more than 101376, slice\_width * slice\_height MUST be less or equal to num\_h\_slices * num\_v\_slices / 4.
+To ensure that fast multithreaded decoding is possible, starting version 3 and if frame\_width * frame\_height is more than 101376, slice\_width * slice\_height MUST be less or equal to num\_h\_slices * num\_v\_slices / 4.
 Note: 101376 is the frame size in pixels of a 352x288 frame also known as CIF ("Common Intermediate Format") frame size format.
 
 For each frame, each position in the slice raster MUST be filled by one and only one slice of the frame (no missing slice position, no slice overlapping).
-- 
2.7.0.windows.1

---

From: Jerome Martinez <[email-masked]>
Date: Wed, 25 May 2016 23:01:58 +0200

From 1eba18046858df76c8fa329c31335de819fdd730 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Wed, 25 May 2016 22:36:46 +0200
Subject: [PATCH 2/2] Slice structure clarification

Split of Slice() in 3 different parts (Header, Content, Footer).
---
 ffv1.md | 93 ++++++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 57 insertions(+), 36 deletions(-)

diff --git a/ffv1.md b/ffv1.md
index 588330f..cf98f9a 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -535,46 +535,17 @@ See [NUT](#references) for more information about elements.
 |Slice( i ) {                                                | type  |
 |    if( version \>= 3 )                                     |       |
 |        SliceHeader( i )                                    |       |
-|    if( colorspace\_type == 0) {                            |       |
-|        for( p = 0; p \< primary\_color\_count; p++ ) {     |       |
-|            Plane( p )                                      |       |
-|    } else if( colorspace\_type == 1 ) {                    |       |
-|        for( y = 0; y \< height; y++ )                      |       |
-|            for( p = 0; p \< primary\_color\_count; p++ ) { |       |
-|                Line( p, y )                                |       |
-|    }                                                       |       |
+|    SliceContent( )                                         |       |
 |    if ( coder\_type == 0 )                                 |       |
 |        while ( !byte\_aligned() )                          |       |
 |            padding                                         |  u(1) |
 |    if( version \>= 3 )                                     |       |
-|        slice\_size                                         | u(24) |
-|    if( ec ) {                                              |       |
-|        error\_status                                       |  u(8) |
-|        slice\_crc\_parity                                  | u(32) |
-|    }                                                       |       |
+|        SliceFooter( )                                      |       |
 |}                                                           |       |
 
-**primary\_color\_count** is defined as 1 + ( chroma_planes ? 2 : 0 ) + ( alpha_plane ? 1 : 0 ).
-
 **padding** specifies a bit without any significance and used only for byte alignment.
 MUST be 0.
 
-**slice_size** indicates the size of the slice in bytes.
-Note: this allows finding the start of slices before previous slices have been fully decoded. And allows this way parallel decoding as well as error resilience.
-
-**error_status** specifies the error status.
-
-| value |     error status                     |
-|-------|--------------------------------------|
-| 0     | no error                             |
-| 1     | slice contains a correctable error   |
-| 2     | slice contains a uncorrectable error |
-| Other | reserved for future use              |
-
-**slice\_crc\_parity** 32 bits that are choosen so that the slice as a whole has a crc remainder of 0.
-This is equivalent to storing the crc remainder in the 32-bit parity.
-The CRC generator polynom used is the standard IEEE CRC polynom (0x104C11DB7) with initial value 0.
-
 ## Slice Header
 
 |                                                            |    |
@@ -643,6 +614,53 @@ Inferred to be 0 if not present.
 | 1     | raw PCM                      |
 | Other | reserved for future use      |
 
+## Slice Content
+
+|                                                                    |
+|------------------------------------------------------------|:------|
+|SliceContent( ) {                                           | type  |
+|    if( colorspace\_type == 0) {                            |       |
+|        for( p = 0; p \< primary\_color\_count; p++ ) {     |       |
+|            Plane( p )                                      |       |
+|    } else if( colorspace\_type == 1 ) {                    |       |
+|        for( y = 0; y \< height; y++ )                      |       |
+|            for( p = 0; p \< primary\_color\_count; p++ ) { |       |
+|                Line( p, y )                                |       |
+|    }                                                       |       |
+|}                                                           |       |
+
+**primary\_color\_count** is defined as 1 + ( chroma_planes ? 2 : 0 ) + ( alpha_plane ? 1 : 0 ).
+
+## Slice Footer
+
+Note: slice footer is always byte aligned.
+
+|                                                                    |
+|------------------------------------------------------------|:------|
+|SliceFooter( ) {                                            | type  |
+|    slice\_size                                             | u(24) |
+|    if( ec ) {                                              |       |
+|        error\_status                                       |  u(8) |
+|        slice\_crc\_parity                                  | u(32) |
+|    }                                                       |       |
+|}                                                           |       |
+
+**slice_size** indicates the size of the slice in bytes.
+Note: this allows finding the start of slices before previous slices have been fully decoded. And allows this way parallel decoding as well as error resilience.
+
+**error_status** specifies the error status.
+
+| value |     error status                     |
+|-------|--------------------------------------|
+| 0     | no error                             |
+| 1     | slice contains a correctable error   |
+| 2     | slice contains a uncorrectable error |
+| Other | reserved for future use              |
+
+**slice\_crc\_parity** 32 bits that are choosen so that the slice as a whole has a crc remainder of 0.
+This is equivalent to storing the crc remainder in the 32-bit parity.
+The CRC generator polynom used is the standard IEEE CRC polynom (0x104C11DB7) with initial value 0.
+
 ## Parameters
 
 |                                                            |     |
@@ -870,24 +888,27 @@ For each Frame with keyframe value of 0, each slice MUST have the same value of
 
 ### Multi-threading support and independence of slices
 
-The bitstream is parsable in two ways: in sequential order as described in this document or with the pre-analysis of slice\_size fields. slice\_size fields pre-analysis allows multi-threading as well as independence of slice content (a bitstream error in a slice content has no impact on the decoding of the other slices).
+The bitstream is parsable in two ways: in sequential order as described in this document or with the pre-analysis of the footer of each slice. Each slice footer contains a slice\_size field so the boundary of each slice is computable without having to parse the slice content. That allows multi-threading as well as independence of slice content (a bitstream error in a slice header or slice content has no impact on the decoding of the other slices).
 
 After having checked keyframe field, a decoder SHOULD parse slice_size fields, from slice\_size of the last slice at the end of the frame up to slice\_size of the first slice at the beginning of the frame, before parsing slices, in order to have slices boundaries. A decoder MAY fallback on sequential order e.g. in case of corrupted frame (frame size unknown, slice\_size of slices not coherant...) or if there is no possibility of seek into the stream.
 
 Architecture overwiew of slices in a frame:
--
+
 |                                                               |
 |:--------------------------------------------------------------|
+| first slice header                                            |
 | first slice content                                           |
-| first slice slice\_size + error\_status + slice\_crc\_parity  |
+| first slice footer                                            |
 |---------------------------------------------------------------|
+| second slice header                                           |
 | second slice content                                          |
-| second slice slice\_size + error\_status + slice\_crc\_parity |
+| second slice footer                                           |
 |---------------------------------------------------------------|
 | ...                                                           |
 |---------------------------------------------------------------|
+| last slice header                                             |
 | last slice content                                            |
-| last slice slice\_size + error\_status + slice\_crc\_parity   |
+| last slice footer                                             |

 # Changelog
-- 
2.7.0.windows.1

---

From: Luca Barbato <[email-masked]>
Date: Wed, 25 May 2016 23:18:56 +0200

>  |        while ( !byte\_aligned() )                          |       |

Might go to the Slice Content section as well

so the Slice Content could specify the coder-specific details inside there.

About that, Michael, is it specified somewhere why the state 129 is used
to reset both coders and why 0 is put at the start in one case while in
the other it is used to pad and terminate?

lu

---

From: Michael Niedermayer <[email-masked]>
Date: Thu, 26 May 2016 05:03:29 +0200

> different places, I removed any reference to this value.

does "version >=3"  really refer less to version 2 than version >=2 ?
i think in cases like above its better to choose the point so that
its most correct

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.

---

From: Dave Rice <[email-masked]>
Date: Wed, 25 May 2016 23:16:18 -0400

> On May 25, 2016, at 11:03 PM, Michael Niedermayer <[email-masked]> wrote:
> 

I think you mean ">=3" versus ">2”.

I prefer the references such as >=3. In cases where version is set to ‘2’, the spec implies that this means a ‘reserved’ value rather than any implication of a ‘version 2’. Since 2 is reserved as a version value, I’d prefer not make any reference to "version 2” excepting the existing side note comment that says that version 2 files shouldn’t exist. Since they shouldn’t exist and the value is reserved, it seems contradictory for the specification to also define or infer to a “version 2” and make references such as “greater than version 2”, whereas “greater than or equal to version 3” makes clearer sense to me.

Dave Rice

---

From: Michael Niedermayer <[email-masked]>
Date: Thu, 26 May 2016 05:38:52 +0200

> +To ensure that fast multithreaded decoding is possible, starting version 3 and if frame\_width * frame\_height is more than 101376, slice\_width * slice\_height MUST be less or equal to num\_h\_sl…
                                                                    ^^^^^^^^^

> _______________________________________________
> Cellar mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/cellar

Those who are best at talking, realize last or never when they are wrong.

---

From: Dave Rice <[email-masked]>
Date: Wed, 25 May 2016 23:49:01 -0400

> On May 25, 2016, at 11:38 PM, Michael Niedermayer <[email-masked]> wrote:
> 

That makes sense to me. Since 2 as a version is reserved, I think that anchoring comparators to a version that should not exist is confusing. I suggest that the specification does not try to define an FFV1 version 2 except for remarking on why the version number is reserved. The cellar charter as well, https://datatracker.ietf.org/wg/cellar/charter/, only references FFV1 0, 1, and 3 to be defined in the specification.

---

From: Jerome Martinez <[email-masked]>
Date: Thu, 26 May 2016 09:20:43 +0200

> i think in cases like above its better to choose the point so that

This is on purpose : "version 2" is not defined (version field with 
value 2 is set as reserved), so it is difficult (it makes the spec less 
clear, it is not coherent) to refer to this undefined version.
The goal of this patch is to have coherency: we have the sentence 
"Version 2 was never enabled in the encoder thus version 2 files 
SHOULD NOT exist, and this document does not describe them to keep the 
text simpler", so any reference to version with a value 2 is removed.

As 2 does not exist, >=3 refers to the exact same versions of FFV1 as 
 >=2 or >2 without referencing a version of FFV1 which does not exist.

---

From: Luca Barbato <[email-masked]>
Date: Fri, 27 May 2016 15:47:16 +0200

> i think in cases like above its better to choose the point so that

If version 2 is non-defined in the specification would be better not
even hint that it could be decoded somehow in the specification or even
in the implementation advices section.

It can get quite confusing, probably would be safer even remove it from
the code so nobody would look at it and maybe overlook the fact it is
experimental and not to be used...

lu

---

From: Michael Niedermayer <[email-masked]>
Date: Fri, 27 May 2016 18:44:50 +0200

On Thu, May 26, 2016 at 09:20:43AM +0200, Jerome Martinez wrote:
> value 2 is set as reserved), so it is difficult (it makes the spec
> less clear, it is not coherent) to refer to this undefined version.

the patch also does this:
(this does not look coherent to me if removing version 2 is teh goal)

> SHOULD NOT exist, and this document does not describe them to keep
> the text simpler", so any reference to version with a value 2 is
> removed.
> 
> As 2 does not exist, >=3 refers to the exact same versions of FFV1
> as >=2 or >2 without referencing a version of FFV1 which does not
> exist.

It was not enabled by default, and probably hard to create them
by mistake but it was possible to generate such files though with
the right command line parameters

i have a mixed feeling about adjusting the version thresholds to
be less correct for version 2

[...]

What does censorship reveal? It reveals fear. -- Julian Assange

---

From: Jerome Martinez <[email-masked]>
Date: Fri, 27 May 2016 19:21:29 +0200

> +|   if( version \>= 2 ) {                                    |     |

Was not my goal.
Updated patch attached.

> the right command line parameters

I understand that such file can be created, but they are explicitly non 
standard (experimental) streams.
We decided that we standardize FFV1 version 0, 1, 3, so we should not 
consider that we take care of version 2 files in the wild.
That does not prevent a decoder to support such stream if the developer 
wants to support it, just that we don't standardize it.

> be less correct for version 2

As it is currently defined, we will not be able to decode version 2 
because we don't handle all exceptions about it, so it is "less correct" 
for only an unsupported version, from my point of view it is weird.

Additionally, In the patch, I do:
-|    if( i \|\| version \> 2 ) |       |
+|    if( version \>= 3 ) |       |

(I remove the "i" part because "i" is always 0 with versions 0 and 1. I 
will also be able to have simplifications in other parts of the spec 
without this "i" here)
and it is weird to keep a (relatively) more complex specification for a 
version we don't support completely (not decodable with this spec), in 
addition to a reference to a version we say that we don't support.

Jérôme
>From a007a06a6b3f4fbda6b08d9393e927899509235d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Fri, 27 May 2016 19:04:10 +0200
Subject: [PATCH 1/2] More explicit version tests

diff --git a/ffv1.md b/ffv1.md
index 11d0faa..cadb3e5 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -472,7 +472,7 @@ Default values at the decoder initialization phase:
 
 ## Configuration Record

 
 
+|   if( version \>= 3 ) {                                    |     |
+|   if( version \>= 3 ) {                                    |     |

 |value   | version                 |
 
 ### Restrictions

---

From: Dave Rice <[email-masked]>
Date: Fri, 27 May 2016 13:41:49 -0400

> On May 27, 2016, at 1:21 PM, Jerome Martinez <[email-masked]> wrote:
> 
> I understand that such file can be created, but they are explicitly non standard (experimental) streams.
> We decided that we standardize FFV1 version 0, 1, 3, so we should not consider that we take care of version 2 files in the wild.
> That does not prevent a decoder to support such stream if the developer wants to support it, just that we don't standardize it.

+1

> As it is currently defined, we will not be able to decode version 2 because we don't handle all exceptions about it, so it is "less correct" for only an unsupported version, from my point of view it is weird.
> 
> (I remove the "i" part because "i" is always 0 with versions 0 and 1. I will also be able to have simplifications in other parts of the spec without this "i" here)
> and it is weird to keep a (relatively) more complex specification for a version we don't support completely (not decodable with this spec), in addition to a reference to a version we say that we don't support.

I agree. Also I find it contradictory for the spec to claim both that '2' is reserved as a version value and to then go on to define many aspects relative to version 2. If version 2 is defined in the specification, then '2' should not be reserved; however, I prefer Jerome's approach to consider version 2 as out of scope of the specification and to remove its mention to only a note to explain why '2' is reversed.
Dave Rice

---

From: Michael Niedermayer <[email-masked]>
Date: Sat, 28 May 2016 17:32:47 +0200

> I agree. Also I find it contradictory for the spec to claim both that '2' is reserved as a version value and to then go on to define many aspects relative to version 2. If version 2 is defined in t…

patch applied

Avoid a single point of failure, be that a person or equipment.

---

From: Michael Niedermayer <[email-masked]>
Date: Sat, 28 May 2016 18:22:20 +0200

> the other it is used to pad and terminate?

Does the way the coder is implemented result in a diferent bytestream
than what the specification allows ?
I thought Jerome had writen his own implementation based on the spec
and it worked, but i too was a bit unsure about the range coder
termination matching exactly between spec and implementation.

[...]

---

From: Jerome Martinez <[email-masked]>
Date: Sat, 28 May 2016 19:01:14 +0200

> termination matching exactly between spec and implementation.

Not on the spec as it is currently.
This is actually the reason we want to improve the spec, because for the 
moment it is not possible to implement a decoder only based on the spec. 
this state 129 was something we had to peek in the FFmpeg source code if 
I remind well (I am not the one who wrote this part of our decoder, I'll 
ask for confirmation; maybe we missed it in the current spec?), beside 
several other small things (I already patched the spec for some of them 
e.g. the padding bits for Golomb Rice coder was not in the spec and it 
was making the decoder fail, for the last thing I patched, and other 
patches are in the queue e.g. slice_count is in the spec but is never 
defined so it is not usable). This state 129 is something I would like 
to have more clear in the spec, and unfortunately my knowledge of Range 
Coder is not enough for proposing good sentences about it (I know it is 
required, I don't know the reason and how to well write it in the spec), 
so this part is postponed for the moment.

BTW Michael, a small ping about the Patch 2/2 to comment or accept.

---

From: Michael Niedermayer <[email-masked]>
Date: Sat, 28 May 2016 22:54:20 +0200

On Sat, May 28, 2016 at 07:01:14PM +0200, Jerome Martinez wrote:
[...]
> BTW Michael, a small ping about the Patch 2/2 to comment or accept.

applied

thanks

[...]

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus