Subject: [Cellar] [PATCH FFV1] Replace slice_count because it is not precisely defined

From: Jerome Martinez <[email-masked]>
Date: Fri, 13 May 2016 10:44:29 +0200

In the spec, I see:
"*slice_count* indicates the number of slices in the current frame, 
slice_count is 1 if it is not explicitly coded."
and:
"for(i=0; i<slice_count; i++)"

but slice_count is not explicitly defined (based on bitstream elements).
As a slice can have a width or height more than 1, slice_count is not 
num_h_slices * num_v_slices, it depends of slice_width and slice_height 
of each slice.

I understand that Range coder state variables (see 4.6.1.3) are per 
slice_x and slice_y in the slice raster.
Is it correct if I say that if for frame 0 a slice with slice_x=0, 
slice_y=0, slice_width=2, slice_height=1 (so no frame 0 slice with 
slice_x=1, slice_y=0), for frame 1 a slice with slice_x=1, slice_y=0 
will have Range coder state variables set to their initial state?

I propose the following patch for being more explicit about slices.
- add "maximal" to num_h/v_slices because we could have less elements 
(e.g. if num_h_slices is 4, we could have a scenario with only 3 slices 
horizontal, e.g. slices with slice_x=0 have always slice_width=2)
- remove slice_count definition and replace it by a test about the 
presence of remaining bytes at the end of the frame
- replace "[i]" by "[slice_x][slice_y]"

Jérôme

From 3cbb0b1803b8a835339222d124e55c71a5aa1725 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Fri, 13 May 2016 09:26:43 +0200
Subject: [PATCH] Replace slice_count because it is not precisely defined

- add "maximal" to num_h/v_slices because we could have less
elements (e.g. if num_h_slices is 4, we could have a scenario
with only 3 slices horizontal, e.g. slices with slice_x=0 have
always slice_width=2)
- remove slice_count definition and replace it by a test about
the presence of remaining bytes at the end of the frame
- replace "[i]" by "[slice_x][slice_y]"
---
 ffv1.md | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/ffv1.md b/ffv1.md
index e48a7ee..3fd0507 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -516,17 +516,17 @@ See [NUT](#references) for more information about elements.
 |    keyframe                                       |  br|
 |    if( keyframe && !ConfigurationRecordIsPresent )|    |
 |         Parameters( )                             |    |
-|    for( i = 0; i \< slice\_count; i++ )           |    |
-|        Slice( i )                                 |    |
+|    while ( remaining_bits_in_bitstream() )        |    |
+|        Slice( )                                   |    |
 |}                                                  |    |
 
 ## Slice
 
 |                                                                    |
 |------------------------------------------------------------|:------|
-|Slice( i ) {                                                | type  |
+|Slice( )   {                                                | type  |
 |    if( version \> 2 )                                      |       |
-|        SliceHeader( i )                                    |       |
+|        SliceHeader( )                                      |       |
 |    if( colorspace\_type == 0) {                            |       |
 |        for( p = 0; p \< primary\_color\_count; p++ ) {     |       |
 |            Plane( p )                                      |       |
@@ -535,7 +535,7 @@ See [NUT](#references) for more information about elements.
 |            for( p = 0; p \< primary\_color\_count; p++ ) { |       |
 |                Line( p, y )                                |       |
 |    }                                                       |       |
-|    if( i \|\| version \> 2 )                               |       |
+|    if( slice_x \|\| slice_y \|\| version \> 2 )            |       |
 |        slice\_size                                         | u(24) |
 |    if( ec ) {                                              |       |
 |        error\_status                                       |  u(8) |
@@ -565,13 +565,13 @@ The CRC generator polynom used is the standard IEEE CRC polynom (0x104C11DB7) wi
 
 |                                                            |    |
 |------------------------------------------------------------|---:|
-| SliceHeader( i ) {                                         |type|
+| SliceHeader( ) {                                           |type|
 |    slice\_x                                                | ur |
 |    slice\_y                                                | ur |
 |    slice\_width - 1                                        | ur |
 |    slice\_height - 1                                       | ur |
 |    for( j = 0; j \< quant\_table\_index\_count; j++ )      |    |
-|        quant\_table\_index [ i ][ j ]                      | ur |
+|        quant\_table\_index [ slice\_x ][ slice\_y ][ j ]   | ur |
 |    picture\_structure                                      | ur |
 |    sar\_num                                                | ur |
 |    sar\_den                                                | ur |
@@ -755,10 +755,10 @@ Decoders SHOULD accept and interpret bits_per_raw_sample = 0 as 8.
 | 0     | transparency plane is not present|
 | 1     | transparency plane is present    |
 
-**num\_h\_slices** indicates the number of horizontal elements of the slice raster.
+**num\_h\_slices** indicates the maximal number of horizontal elements of the slice raster.
 Inferred to be 1 if not present.
 
-**num\_v\_slices** indicates the number of vertical elements of the slice raster.
+**num\_v\_slices** indicates the maximal number of vertical elements of the slice raster.
 Inferred to be 1 if not present.
 
 **quant\_table\_count** indicates the number of quantization table sets.
@@ -776,8 +776,6 @@ Inferred to be 0 if not present.
 pred = j ? initial\_states[ i ][j - 1][ k ] : 128
 initial\_state[ i ][ j ][ k ] = ( pred + initial\_state\_delta[ i ][ j ][ k ] ) & 255
 
-**slice\_count** indicates the number of slices in the current frame, slice\_count is 1 if it is not explicitly coded.
-
 **ec** indicates the error detection/correction type.
 
 |value | error detection/correction type          |
-- 
2.7.0.windows.1

---

From: Michael Niedermayer <[email-masked]>
Date: Fri, 13 May 2016 20:04:37 +0200

On Fri, May 13, 2016 at 10:44:29AM +0200, Jerome Martinez wrote:
> As a slice can have a width or height more than 1, slice_count is
> not num_h_slices * num_v_slices, it depends of slice_width and
> slice_height of each slice.

i think it was meant to be the number of slices if one follows the
chain of slice_size  (or in a badly damaged frame the number of byte
ranges that would have valid looking headers and coded sizes)

> will have Range coder state variables set to their initial state?

I think the behavior when the slice segmentation changes in a non
key slice is undefined (litterally so as the spec does not say what to
do for that)
i would tend toward leaving that undefined / disallowed
instead of trying to specify some behavior without a bit of study of
its effects and alternatives
disallowing it is easy for implementations

> elements (e.g. if num_h_slices is 4, we could have a scenario with
> only 3 slices horizontal, e.g. slices with slice_x=0 have always
> slice_width=2)

> 74f93a5ba93c4fd91e1857e7a3a5a224e6db8a1e  0001-Replace-slice_count-because-it-is-not-precisely-defi.patch

iam a bit sceptic on this
first it does not work for multitreading, maybe its not wrong but
one cannot implement multitreading if one has to decode the previous
slice before knowing the start of the next

also theres the question about extensibility when defined by
more bits instead of teh slice sizes
i _think_ we currently could add things before slice_size without
breaking the bitstream but maybe iam missing something

>  Inferred to be 1 if not present.

i would define the raster resolution (whatever one wants to call it)
and then have slices span multiple raster elements if thats what they
do
as in the resolution is X by Y elements and each element of that is
either part of the blue rectangle or not ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship: All citizens are under surveillance, all their steps and
actions recorded, for the politicians to enforce control.
Democracy: All politicians are under surveillance, all their steps and
actions recorded, for the citizens to enforce control.

---

From: Jerome Martinez <[email-masked]>
Date: Fri, 13 May 2016 21:00:11 +0200

> ranges that would have valid looking headers and coded sizes)

but it is not in the bitstream, I see no way to define it in another way 
(currently, it is not defined at all so we can not keep it as is), this 
is the reason I propose to replace it.
but depending of your answer on the other questions below, I could 
define it.

> key slice is undefined (litterally so as the spec does not say what to

this is the reason I raise the issue and propose something ;-).

> i would tend toward leaving that undefined

undefined in the spec does not seem good.

>   / disallowed

if it is disallowed, slice_x, slice_y, slice_width, slice_height are 
useless (they can be copied from the previous frame) and only extra bits 
not optimal for compression ratio.
Anyway, it is the definition of an existing bitstream so let say that we 
add restrictions.
Before I propose another patch, please confirm that it is OK to forbid a 
non-key slice with slice_x, slice_y, slice_width, slice_height not 
identical to a slice present in the previous frame.

> slice before knowing the start of the next

It does: the way it is described in the bitstream does not forbid 
another method for decoding it, and we provide something for doing the 
multithread (the slice byte size at the end).

> breaking the bitstream but maybe iam missing something

this way to define it definitely forbids things after the end of the 
current slice bitstream.
but it is actually the way it is defined in the current spec (and/or it 
is not clear, so we need to define it).
The way you are thinking about (reading slice byte size at the end of 
the frame and "reading back" all slice byte size) adds a constraint: we 
must receive the last byte of the frame before starting to decode the 
2nd slice.
As is, the first slice can be decoded until the last pixel, and we wait 
for the last byte of the frame in order to get the real slice byte size.
If we have 256 slices in a frame streamed by Internet, we must receive 
the last byte of the frame before starting to decode 255 slices. In some 
scenarios, it could be considered as a problem because it adds some 
delay in the delivery of the decoded frame. This also means that the 
decoder must keep the complete frame in memory (it can not decode then 
drop bytes), the decoder can not decode without seeking in the 
bytstream, I am not not sure it is pertinent but it is a potential issue.

So I can propose a patch with 2 passes:
- first pass for getting slice_count and all slices byte size
- second pass for decoding each slice.

Before I propose another patch, please confirm that it is OK with the 
constraints added due to this way to implement the spec.

> either part of the blue rectangle or not ...

I see.
I just don't really have ideas about how to call it. from my point of 
view, "slice raster" was a bit misleading (not taking the possibility 
that a slice can have a slice_width not 1).
I'll try to find a way to better define it.

Jérôme

---

From: Michael Niedermayer <[email-masked]>
Date: Fri, 13 May 2016 23:52:13 +0200

On Fri, May 13, 2016 at 09:00:11PM +0200, Jerome Martinez wrote:
> but it is not in the bitstream, I see no way to define it in another
> way (currently, it is not defined at all so we can not keep it as
> is), this is the reason I propose to replace it.
> useless (they can be copied from the previous frame) and only extra
> bits not optimal for compression ratio.

remove them, transmit it with some packet loss and try to decode
and you will see why they are usefull

slices should be independant of each other, so at the very least
one needs to know where the slice is located in the slice or pixel
raster. The width & height would generally be 1 so they take almost
no space

> Anyway, it is the definition of an existing bitstream so let say
> that we add restrictions.
> Before I propose another patch, please confirm that it is OK to
> forbid a non-key slice with slice_x, slice_y, slice_width,
> slice_height not identical to a slice present in the previous frame.

I think as the spec does not say what to do about this and i think
the implementation wouldnt behave sane either adding this restriction
to the current bitstream version does just state more clearly how it
already is

[...]
> but it is actually the way it is defined in the current spec (and/or
> it is not clear, so we need to define it).
> The way you are thinking about (reading slice byte size at the end
> of the frame and "reading back" all slice byte size) adds a
> constraint: we must receive the last byte of the frame before
> starting to decode the 2nd slice.
> As is, the first slice can be decoded until the last pixel, and we
> wait for the last byte of the frame in order to get the real slice
> byte size.
> If we have 256 slices in a frame streamed by Internet, we must
> receive the last byte of the frame before starting to decode 255
> slices. In some scenarios, it could be considered as a problem
> because it adds some delay in the delivery of the decoded frame.
> This also means that the decoder must keep the complete frame in
> memory (it can not decode then drop bytes), the decoder can not
> decode without seeking in the bytstream, I am not not sure it is
> pertinent but it is a potential issue.

This is something we should look into for the next version of the
bitstream
for now we could cheat a bit by either aligning slices with
network packets in some way so their start is known by external means
or also to allow and define decoding to be valid both by the
slice_sizes and by the  remaining_bits_in_bitstream loop if that
works, but that would only help single threaded decoding

> Before I propose another patch, please confirm that it is OK with
> the constraints added due to this way to implement the spec.

iam fine with defining the decoding both ways (if that actually works)
or the way you describe above

[...]

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes

---

From: Jerome Martinez <[email-masked]>
Date: Tue, 17 May 2016 11:08:20 +0200

> or the way you describe above

I think that having the possibility to decode from the end (from 
slice_size values) and from the beginning (parsing the bitstream 
sequentially) may be useful for error recovery and choice of the decoder 
to decide how to decode.
Additionally, the current spec (and the old one) never say that it is 
possible to have additional bits/bytes between Line( p, y ) and 
slice_size so it would be weird to say now, for the current version, 
that it is possible.
In next version of FFV1, we could add after "Line( p, y )" e.g. an 
"extension_present" flag (so only one bit) + "extension_size" value (in 
bytes) for letting the possibility to extend the bitstream at this point 
without removing any decoding possibility (from begin or from end).
so I prefer to define the bitstream in the bitstream sequential order 
(and letting the possibility to decode in sequential order).

updated .patch :

In Parameter():
I keep "raster", no better idea.
I remove "elements", I think it may be misleading (may be understood as 
"slices" or making understanding difficult), I prefer to use "size of 
the slice raster". Better idea from someone else?

In Slice():
I added byte alignment function (as currently specified, slice_size is 
not byte-aligned but FFmpeg decoder mandates byte alignment)

In Restrictions:
Added the need of having 1 and only 1 slice for each position in the 
slice raster (no missing part in the slice raster, no overlapping)
no change of slices positions when it is not a keyframe.

>From d8b479a93ac1bc6d0e29c0d33cd79640c33d7431 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Tue, 17 May 2016 11:00:48 +0200
Subject: [PATCH] Replace slice_count because it is not precisely defined

- remove slice_count definition and replace it by a test about
the presence of remaining bytes at the end of the frame
- replace "[i]" by "[slice_x][slice_y]"
- add byte alignment test
- add restrictions about slice_x, slice_y, slice_width,
slice_height
---
 ffv1.md | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/ffv1.md b/ffv1.md
index 80e3cf9..0fd1249 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -153,6 +153,8 @@ __a...b__ means any value starting from a to b, inclusive.
 
 **remaining\_bits\_in\_bitstream( )** means the count of remaining bits after the current position in the bitstream. It is computed from the NumBytes value multiplied by 8 minus the count of bits already read by the bitstream parser.
 
+**byte\_aligned( )** means the last parsed bit in the bitstream is the last bit in a byte.
+
 \pagebreak
 
 # General Description
@@ -522,17 +524,17 @@ See [NUT](#references) for more information about elements.
 |    keyframe                                       |  br|
 |    if( keyframe && !ConfigurationRecordIsPresent )|    |
 |         Parameters( )                             |    |
-|    for( i = 0; i \< slice\_count; i++ )           |    |
-|        Slice( i )                                 |    |
+|    while ( remaining_bits_in_bitstream() )        |    |
+|        Slice( )                                   |    |
 |}                                                  |    |
 
 ## Slice
 
 |                                                                    |
 |------------------------------------------------------------|:------|
-|Slice( i ) {                                                | type  |
+|Slice( )   {                                                | type  |
 |    if( version \> 2 )                                      |       |
-|        SliceHeader( i )                                    |       |
+|        SliceHeader( )                                      |       |
 |    if( colorspace\_type == 0) {                            |       |
 |        for( p = 0; p \< primary\_color\_count; p++ ) {     |       |
 |            Plane( p )                                      |       |
@@ -541,7 +543,9 @@ See [NUT](#references) for more information about elements.
 |            for( p = 0; p \< primary\_color\_count; p++ ) { |       |
 |                Line( p, y )                                |       |
 |    }                                                       |       |
-|    if( i \|\| version \> 2 )                               |       |
+|    while ( !byte_aligned() )                               |       |
+|        padding                                             |  u(1) |
+|    if( slice_x \|\| slice_y \|\| version \> 2 )            |       |
 |        slice\_size                                         | u(24) |
 |    if( ec ) {                                              |       |
 |        error\_status                                       |  u(8) |
@@ -571,13 +575,13 @@ The CRC generator polynom used is the standard IEEE CRC polynom (0x104C11DB7) wi
 
 |                                                            |    |
 |------------------------------------------------------------|---:|
-| SliceHeader( i ) {                                         |type|
+| SliceHeader( ) {                                           |type|
 |    slice\_x                                                | ur |
 |    slice\_y                                                | ur |
 |    slice\_width - 1                                        | ur |
 |    slice\_height - 1                                       | ur |
 |    for( j = 0; j \< quant\_table\_index\_count; j++ )      |    |
-|        quant\_table\_index [ i ][ j ]                      | ur |
+|        quant\_table\_index [ slice\_x ][ slice\_y ][ j ]   | ur |
 |    picture\_structure                                      | ur |
 |    sar\_num                                                | ur |
 |    sar\_den                                                | ur |
@@ -593,10 +597,10 @@ Inferred to be 0 if not present.
 **slice_y** indicates the y position on the slice raster formed by num_v_slices.
 Inferred to be 0 if not present.
 
-**slice_width** indicates the width on the slice raster.
+**slice_width** indicates the width on the slice raster formed by num_h_slices.
 Inferred to be 1 if not present.
 
-**slice_height** indicates the height on the slice raster.
+**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 ).
@@ -761,10 +765,10 @@ Decoders SHOULD accept and interpret bits_per_raw_sample = 0 as 8.
 | 0     | transparency plane is not present|
 | 1     | transparency plane is present    |
 
-**num\_h\_slices** indicates the number of horizontal elements of the slice raster.
+**num\_h\_slices** indicates the horizontal size of the slice raster.
 Inferred to be 1 if not present.
 
-**num\_v\_slices** indicates the number of vertical elements of the slice raster.
+**num\_v\_slices** indicates the vertical size of the slice raster.
 Inferred to be 1 if not present.
 
 **quant\_table\_count** indicates the number of quantization table sets.
@@ -782,8 +786,6 @@ Inferred to be 0 if not present.
 pred = j ? initial\_states[ i ][j - 1][ k ] : 128
 initial\_state[ i ][ j ][ k ] = ( pred + initial\_state\_delta[ i ][ j ][ k ] ) & 255

 |value | error detection/correction type          |
@@ -852,6 +854,10 @@ MAX\_CONTEXT\_INPUTS is 5.
 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.
 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).
+
+For each Frame with keyframe value of 0, each slice MUST have the same value of slice\_x, slice\_y, slice\_width, slice\_height as a slice in the previous frame.
+
 # Changelog
 
 See <https://github.com/FFmpeg/FFV1/commits/master>
-- 
2.7.0.windows.1

---

From: Michael Niedermayer <[email-masked]>
Date: Tue, 17 May 2016 14:52:30 +0200

Hi

On Tue, May 17, 2016 at 11:08:20AM +0200, Jerome Martinez wrote:
> sequentially) may be useful for error recovery and choice of the
> decoder to decide how to decode.

i think for error recovery slices should be decodable with both
slices before and after them being destroyed beyond recognization

> Additionally, the current spec (and the old one) never say that it
> is possible to have additional bits/bytes between Line( p, y ) and

The issue is the range coder, or more precissely switching from the
range coder back to storing bits and bytes

i am not sure its specified restricted enough to know where the end is
exactly (thats why i said "if that works" earlier ...

IIRC it basically just says any bytestream that decodes correctly
is fine, leaving it to the encoder to add an unneeded byte or 2 at
the end. (Which simplifies the encoder)
If such extra bytes would be allowed then future specs could also use
the area to hold some usefull information

ill look at he patch in a moment

[...]

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.

---

From: Jerome Martinez <[email-masked]>
Date: Tue, 17 May 2016 17:08:12 +0200

> range coder back to storing bits and bytes

Right. This is something I am currently not comfortable with.
I proposed in the patch a byte_aligned test, usually used with 
bitstreams without range coder, not sure it is useful here (does a range 
coder always "eat" 8 bits whatever is the implementation?) so I ask for 
comment (maybe saying that we don't use anymore the range coder at this 
point is a better method)

> the area to hold some usefull information

Issue I have is: what is "decodes correctly"?
FFmpeg reads the end of the frame in order to get all slice_size values. 
But the idea is that FFmpeg will not be the only decoder in the future .
The method from FFmpeg is a method, but not the only one: reading from 
the beginning of the stream, without reading slice_size first, looks 
like currently possible when we read the current spec (a decoder could 
decide to ignore slice_size values).
In that case, it is not possible to authorize an encoder to add an 
unneeded byte or 2 at the end even if it simplifies the encoder.
If we authorize an encoder to add arbitrary bytes at the end, a decoder 
not waiting for the a complete frame before decoding would not decode 
correctly.
The main question to answer is if we authorize or not a decoder to read 
sequentially a FFV1 frame (or if a decoder could easily decode a frame 
missing the last 2 bytes), as it is currently more or less authorized in 
the spec (slice_size right after Line( p, y ), no place of "unneeded 
byte or 2").

for example, if slice_size of the last slice is corrupted (1 byte of 
corruption), FFmpeg decodes only the 1st slice but my own decoder (not 
for release, just a proof of concept) relying on a sequential decode of 
slices and on the idea there is no unneeded byte or 2 at the end of any 
slice can decode all slices without any loss of detail. If unneeded byte 
or 2 is authorized in the bitstream (and already created by FFmpeg 
encoder?), such decoder can not exist so it is important to know now 
what is authorized in the bitstream.

---

From: Michael Niedermayer <[email-masked]>
Date: Tue, 17 May 2016 17:52:42 +0200

>  Inferred to be 1 if not present.

is this really clearer ?
size could be in anything from pixels, elements, even inch

> +For each Frame with keyframe value of 0, each slice MUST have the same value of slice\_x, slice\_y, slice\_width, slice\_height as a slice in the previous frame.

i was wondering if reset_contexts = 1 should allow changing the layout
it doesnt use the previous so it should be fine in terms of being
well specified, but maybe i miss something

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

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

---

From: Jerome Martinez <[email-masked]>
Date: Tue, 17 May 2016 18:12:08 +0200

> size could be in anything from pixels, elements, even inch

It is for me but I understand that it is not for others, I definitely 
didn't find a good way for having it obvious.
I remove the change for the moment.

> well specified, but maybe i miss something

My role for the moment is to write the specification based on what 
exists, not to create the future bitstream.
I currently mainly focus on FFV1 "frozen" versions, and reset_contexts 
does not exist in frozen versions.
Anyway, I added "except if reset_contexts is 1" but note that I plan to 
remove (in a specific branch) any reference to FFV1 >= version 4 before 
proposing the spec to IETF, so this addition would be removed (and we'll 
be able to change it before version 4 freeze if necessary).

New patch attached.
>From cdd324b2db682ecf600890a8b07bda8d32fc28a1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Tue, 17 May 2016 18:09:21 +0200
Subject: [PATCH] Replace slice_count because it is not precisely defined

- remove slice_count definition and replace it by a test about
the presence of remaining bytes at the end of the frame
- replace "[i]" by "[slice_x][slice_y]"
- add byte alignment test
- add restrictions about slice_x, slice_y, slice_width,
slice_height
---
 ffv1.md | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/ffv1.md b/ffv1.md
index 80e3cf9..8298c6d 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -153,6 +153,8 @@ __a...b__ means any value starting from a to b, inclusive.

 
 # General Description
 
 ## Slice
 
 |                                                                    |
 
 |                                                            |    |

 

 
+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).
+
+For each Frame with keyframe value of 0, each slice MUST have the same value of slice\_x, slice\_y, slice\_width, slice\_height as a slice in the previous frame, except if reset\_contexts is 1.
+
 # Changelog

---

From: Michael Niedermayer <[email-masked]>
Date: Tue, 17 May 2016 18:27:28 +0200

On Tue, May 17, 2016 at 05:08:12PM +0200, Jerome Martinez wrote:
> bitstreams without range coder, not sure it is useful here (does a
> range coder always "eat" 8 bits whatever is the implementation?) so
> I ask for comment (maybe saying that we don't use anymore the range
> coder at this point is a better method)
> 
> FFmpeg reads the end of the frame in order to get all slice_size
> values. But the idea is that FFmpeg will not be the only decoder in
> the future .
> The method from FFmpeg is a method, but not the only one: reading
> from the beginning of the stream, without reading slice_size first,
> looks like currently possible when we read the current spec (a
> decoder could decide to ignore slice_size values).
> If we authorize an encoder to add arbitrary bytes at the end, a
> decoder not waiting for the a complete frame before decoding would
> not decode correctly.
> The main question to answer is if we authorize or not a decoder to
> read sequentially a FFV1 frame (or if a decoder could easily decode
> a frame missing the last 2 bytes), as it is currently more or less
> authorized in the spec (slice_size right after Line( p, y ), no
> place of "unneeded byte or 2").
>

> for example, if slice_size of the last slice is corrupted (1 byte of

the chance of this is 1 in 10000+ or so if you have a single byte error

robust decoders should search for slices not relying on sequential
decoding from either side

> corruption), FFmpeg decodes only the 1st slice but my own decoder
> (not for release, just a proof of concept) relying on a sequential
> decode of slices and on the idea there is no unneeded byte or 2 at
> the end of any slice can decode all slices without any loss of
> detail. If unneeded byte or 2 is authorized in the bitstream (and
> already created by FFmpeg encoder?), such decoder can not exist so
> it is important to know now what is authorized in the bitstream.

I possibly misremembered and this ambiguity was already fixed
either way sequential forward decoding is something that should really
work.
if theres mismatch between spec and encoder in a bit at the end it
might make sense to treat that as a bug in the spec given that there
is just one encoder and existing files generated by it

[...]

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

---

From: Michael Niedermayer <[email-masked]>
Date: Fri, 20 May 2016 12:13:34 +0200

On Tue, May 17, 2016 at 06:12:08PM +0200, Jerome Martinez wrote:
> It is for me but I understand that it is not for others, I
> definitely didn't find a good way for having it obvious.
> I currently mainly focus on FFV1 "frozen" versions, and
> reset_contexts does not exist in frozen versions.
> Anyway, I added "except if reset_contexts is 1" but note that I plan
> to remove (in a specific branch) any reference to FFV1 >= version 4
> before proposing the spec to IETF, so this addition would be removed
> (and we'll be able to change it before version 4 freeze if
> necessary).
> 
> New patch attached.

> 7010297ac6631ec16f5ea3a4f5fd525b2a47fc54  0001-Replace-slice_count-because-it-is-not-precisely-defi.patch

please split this in one patch per issue

> +**byte\_aligned( )** means the last parsed bit in the bitstream is the last bit in a byte.

"last bit in a byte" is unclear

> +|    while ( remaining_bits_in_bitstream() )        |    |

has it been tested that this works with both coders ?

The text should also mention that decoders SHOULD parse slices using
the slice_size as it allows multthreading and is independant of
errors within slices

> +|        padding                                             |  u(1) |

There is no Description in the spec what "padding" is

> +|        quant\_table\_index [ slice\_x ][ slice\_y ][ j ]   | ur |

This raises an interresting related question

If most arrays are based on slice coordinates in the raster and
not slices the arrays would become much larger if the entries are
not conditionally allocated. Theres alot of state per slice

of course this is a spec and not an implementation but implementations
might follow the wording used in the spec rather closely 

This problem should be mentioned in the spec so implementations avoid
having problems with files with fine rasters and slices spaning many
raster elements

> +**slice_width** indicates the width on the slice raster formed by num_h_slices.

slice_width The width of the slice in slice raster elements

[...]

---

From: Jerome Martinez <[email-masked]>
Date: Fri, 20 May 2016 13:20:07 +0200

> please split this in one patch per issue

Will do.

> "last bit in a byte" is unclear

Will change to "means remaining\_bits\_in\_bitstream( ) % 8 is 0" ("%" 
is defined in arithmetic operators)

> has it been tested that this works with both coders ?

Yes. Actually because there is no change impacting the decoding compared 
to previous specification (which was not defining slice_count).
It just clarifies it.

> the slice_size as it allows multthreading and is independant of

 From my point of view, we describe a bitstream and not how a decoder 
should decode it, so such sentence should not be in the specification: 
we specify a bitstream and we should not try to force a decoder to 
choose one way rather than another one (as the H264 specification does 
not say that a decoder should find slices boundaries before doing the 
decoding). The feature of FFV1 is to permit multithreading, not that 
decoder should do mutlithreading (for various reasons)

> There is no Description in the spec what "padding" is

I can add something like:
"**padding** bit without any significance and used only for byte alignment"

That said, I am not sure this is needed, the issue is about we switch 
from the range coder to a more classical bit/bytestream.
for the moment, I use br/ur/sr for the range coder and u(n) for the 
classic bitstream.
I would like to have comments/proposals about how to define this switch. 
Is such padding bit info coherent and enough from your point of view?

> having problems with files with fine rasters and slices spaning many

 From my point of view, the spec should not go to that level, because it 
is optimization. The spec should be clear about what is forbidden and 
this is the reason I added the restriction paragraph. I think that the 
restriction paragraph (copied below for reference) is the answer to this 
question.
In the future, we should think to add profiles with restrictions about 
the count of slices per frame instead of limits about num_h_slices / 
num_v_slices and it will be even more clear.

[...]

---

From: Jerome Martinez <[email-masked]>
Date: Fri, 20 May 2016 13:58:55 +0200

>From c0f2da9aa007332ac123bea6ee0159170ef5faa4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Fri, 20 May 2016 13:48:58 +0200
Subject: [PATCH 1/3] Slice header members restrictions

slice_x, slice_y, slice_width, slice_height restrictions are explicit.
---
 ffv1.md | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ffv1.md b/ffv1.md
index 80e3cf9..738f68e 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -852,6 +852,10 @@ MAX\_CONTEXT\_INPUTS is 5.
 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.
 Note: 101376 is the frame size in pixels of a 352x288 frame also known as CIF ("Common Intermediate Format") frame size format.

---

From: Jerome Martinez <[email-masked]>
Date: Fri, 20 May 2016 13:59:22 +0200

>From 79e1aac0bf021758672524e0e3cbd3c67abca237 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Fri, 20 May 2016 13:50:55 +0200
Subject: [PATCH 2/3] slice_width and slice_height definition update

More coherency with slice_x and slice_y definitions.
---
 ffv1.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ffv1.md b/ffv1.md
index 738f68e..cf26a10 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -593,10 +593,10 @@ Inferred to be 0 if not present.
 **slice_y** indicates the y position on the slice raster formed by num_v_slices.
 Inferred to be 0 if not present.

---

From: Jerome Martinez <[email-masked]>
Date: Fri, 20 May 2016 14:00:36 +0200

Request for comments, as I am not sure it is the best method for a 
switch between Range Coder and classic bitstream.
>From cbd66f3955c2e6709765250927b2ded46e090b8e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Fri, 20 May 2016 13:55:31 +0200
Subject: [PATCH 3/3] Add byte alignment test

This is a method for defining the switch from a Range Coder parsing
(br/ur/sr) to a classic bitstream parsing (u(n)).
---
 ffv1.md | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ffv1.md b/ffv1.md
index cf26a10..d8a9328 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -153,6 +153,8 @@ __a...b__ means any value starting from a to b, inclusive.

+**byte\_aligned( )** means remaining\_bits\_in\_bitstream( ) % 8 is 0.
+
 \pagebreak
 
 # General Description
@@ -541,6 +543,8 @@ See [NUT](#references) for more information about elements.
 |            for( p = 0; p \< primary\_color\_count; p++ ) { |       |
 |                Line( p, y )                                |       |
 |    }                                                       |       |
+|    while ( !byte_aligned() )                               |       |
+|        padding                                             |  u(1) |
 |    if( i \|\| version \> 2 )                               |       |
 |        slice\_size                                         | u(24) |
 |    if( ec ) {                                              |       |
@@ -551,6 +555,8 @@ See [NUT](#references) for more information about elements.
 
 **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.
+
 **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.
 
-- 
2.7.0.windows.1

---

From: Jerome Martinez <[email-masked]>
Date: Fri, 20 May 2016 15:24:54 +0200

From a remark I received, I add "MUST be 0".

>From 686345ac408f4f30c4b71b04823ce01a19059d5c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Fri, 20 May 2016 15:00:05 +0200
Subject: [PATCH 3/3] Add byte alignment test

This is a method for defining the switch from a Range Coder parsing
(br/ur/sr) to a classic bitstream parsing (u(n)).
---
 ffv1.md | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ffv1.md b/ffv1.md
index cf26a10..db149d4 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -153,6 +153,8 @@ __a...b__ means any value starting from a to b, inclusive.

 
 # General Description
@@ -551,6 +555,9 @@ See [NUT](#references) for more information about elements.

+MUST be 0.
 
-- 
2.7.0.windows.1

---

From: Michael Niedermayer <[email-masked]>
Date: Fri, 20 May 2016 18:33:17 +0200

[...]
> should decode it, so such sentence should not be in the
> specification: we specify a bitstream and we should not try to force
> a decoder to choose one way rather than another one (as the H264
> specification does not say that a decoder should find slices
> boundaries before doing the decoding). The feature of FFV1 is to
> permit multithreading, not that decoder should do mutlithreading
> (for various reasons)

I dont like the idea that someone follwing the spec and implementing
everyhing it mentions would end up with a decoder that doesnt work
(for some cases like realtime playback of some streams)

One shouldnt need to fully understand everything in a spec and then
twist it backward to end up with a good implementation

> That said, I am not sure this is needed, the issue is about we
> switch from the range coder to a more classical bit/bytestream.
> I would like to have comments/proposals about how to define this
> switch. Is such padding bit info coherent and enough from your point
> of view?

our implementation works in bytes, also the specification seems to
work in bytes for the range coder
so iam not sure where this padding comes from if its for RC->VLC

> From my point of view, the spec should not go to that level, because
> it is optimization. The spec should be clear about what is forbidden
> and this is the reason I added the restriction paragraph. I think
> that the restriction paragraph (copied below for reference) is the
> answer to this question.
> In the future, we should think to add profiles with restrictions
> about the count of slices per frame instead of limits about
> num_h_slices / num_v_slices and it will be even more clear.
> 

it matters for someone implementing the spec if an array could have
10 million entryies with only 100 used or not
that is if you want things to interoperate well then the kind of
values that encoders are allowed to set must be known by the one
writing a decoder
as is its easy for a encoder author to write a raster of 1000x1000
in which a few slices are defined. While a decoder author could easily
think that this doesnt occur and the current implementation also
doesnt produce this so he wouldnt notice when testing

[...]

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle

---

From: Michael Niedermayer <[email-masked]>
Date: Fri, 20 May 2016 18:42:47 +0200

On Fri, May 20, 2016 at 01:58:55PM +0200, Jerome Martinez wrote:

> 04bc9c65d1240c797956476b50d4e1d12e1c3f88  0001-Slice-header-members-restrictions.patch

applied

thx

[...]

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus

---

From: Luca Barbato <[email-masked]>
Date: Fri, 20 May 2016 20:25:51 +0200

> twist it backward to end up with a good implementation

That could be another document or an appendix. Mixing implementation
advises with the bitstream description can result in quite a bit of
confusion.

> so iam not sure where this padding comes from if its for RC->VLC

Keep in mind that an independent implementer would be legally bound not
to read any code by third parties and might decide that there wouldn't
be any padding in case you end up with some spare bits.

Being explicit or even pedantic doesn't hurt.

lu

---

From: Michael Niedermayer <[email-masked]>
Date: Fri, 20 May 2016 21:44:10 +0200

> advises with the bitstream description can result in quite a bit of

I was also thinking of puting it in another document, didnt think
of an appendix, that might indeed be even better

[...]

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin

---

From: Michael Niedermayer <[email-masked]>
Date: Sat, 21 May 2016 01:09:35 +0200

On Fri, May 20, 2016 at 01:59:22PM +0200, Jerome Martinez wrote:

> eea29be2530a26d6e845b81d54dd97d8461c3dc5  0002-slice_width-and-slice_height-definition-update.patch

applied

thanks

[...]

Democracy is the form of government in which you can choose your dictator