Subject: Re: [Cellar] [FFmpeg-devel] [PATCH] FFV1: make RGB48 support as non-experimental

From: Jerome Martinez <[email-masked]>
Date: Sun, 7 Jan 2018 18:39:34 +0100

On 06/01/2018 23:21, Michael Niedermayer wrote:
> On Sat, Jan 06, 2018 at 04:54:18PM +0100, Jerome Martinez wrote:
>> On 06/01/2018 02:05, Michael Niedermayer wrote:
>>>>   ffv1enc.c |    4 ----
>>>>   1 file changed, 4 deletions(-)
>>>> acfc60c913b311b148f2eeef2d2d6ea9e37afcf7  0001-avcodec-ffv1enc-mark-RGB48-support-as-non-experiment.patch
>>>>  From 303f63fb7e6172fdb7de66da1f8a4006b79a535f Mon Sep 17 00:00:00 2001
>>>> From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
>>>> Date: Fri, 5 Jan 2018 11:09:01 +0100
>>>> Subject: [PATCH] avcodec/ffv1enc: mark RGB48 support as non-experimental
>>>>
>>>> Resulting bitstream was tested with a conformance checker
>>>> using the last draft of FFV1 specifications.
>>> But has the way this is stored been optimized ?
>>>
>>> Once its marked as non exerimental all future decoders must support the exact
>>> way.
>> Although this is considered experimental in the encoder, the implementation
>> adheres to the description in the specification. The bitstream specification
>> does not provide a bitdepth limit so with the current draft of the
>> specification, another FFV1 encoder could already encode 16-bit (and more)
>> content. The work on the specification has been careful to not break
>> compatibility with former drafts and at this point the FFV1 bitstream
>> specification for versions 0, 1, and 3 should be considered already
>> non-experimental for all bit depths. All current decoders should already
>> support the exact way it is currently specified.
>>
>> To make a change in the specification at this point would have cascading
>> consequences as we’d have to retract the part of the specification which
>> states that micro_version 4 of version 3 is the first stable variant. Worse,
>> it is impossible to indicate a bitstream change in FFV1 version 1, which
>> permits RGB 16-bit content too, so it would be difficult for a decoder to
>> detect a bitstream not conforming to the bitstream created by the current
>> version of FFmpeg encoder.
> Are you not making this look alot more complex than it is ?
> Or maybe we talk about slightly different things here
>
> with the next version we can introduce any method of storing 16bit or 9-15 bit
> and we certainly do not support in the implementation all possible bit depths
>
>  From what i remember I had always wanted to improve the way that
> more than 8bit is handled, not just 16. Although 16 is a bit special
>
> Consider this:
> If we improve get_context() in the next version for >8bit
> we still have to support 9-15 bit with the old definition
> if we now declare 16bit non experimental then we also must support that with
> an old get_context() in the decoder.
> the 16bit path is seperate from the lower bit depth. So this is an Additional
> codepath that we have to carry in the future
>
> isnt it smarter now that if we want to improve get_context() that we
> dont now extend what can be generated with the current get_context ?
>
> or are such current get_context() style files already widespread ?
> if so then its probably best to accept it and keep supporting it

I am lost.
Looks like we talk about 2 different subjects: FFV1 bitstream 
specifications and FFmpeg implementation.
Let separate subjects:

FFV1 bitstream specifications:
Since at least 2012 [1] get_context (in the bitstream) is defined and 
flagged as stable for **all** bit depths for versions 0, 1, 3.
It is still the case today [2].
There was a consensus on discussing about FFV1 bitstream on CELLAR 
mailing list
There was a consensus for not changing the bitstream for version 0, 1, 3 
so we can standardize it ASAP without breaking current implementations 
(reason we document bugs in the main encoder, but the idea is not to 
accept more changes)
If the FFV1 bitstream for version 0, 1, or 3 must be changed in current 
stable version, it would be a major break in the consensus and it should 
be discussed on CELLAR list (in copy as they are concerned), but I doubt 
the consensus would be for breaking the bitstream compatibility as the 
discussion from day 0 on CELLAR is to document current bitstream without 
changing it for versions 0, 1, 3. The fact that there was no (publicly 
available) RGB48 encoder in the wild is not an excuse for breaking the 
compatibility with current draft of specifications. Same if someone 
decides to do an encoder for e.g. RGB3000.
It is possible to change the bitstream with version 4 (experimental 
version), and it is a good place for changing get_context for 16-bit 
content (whatever is the color space, BTW).

FFmpeg implementation:
FFV1 YCbCr 16-bit is already flagged as non experimental, so there is 
already some non experimental 16-bit support in FFmpeg FFV1 encoder. 
16-bit is not new and for RGB48 the actual encoding after RGB to YCbCr 
transformation is just 1 bit more (17-bit).
FFmpeg experimental flag is for the status of the encoder, not for the 
status of the bitstream (the bitstream for versions 0, 1, 3 is already 
considered stable for RGB48 and others)
FFV1 RGB48 support in FFmpeg is conform to FFV1 bitstream specifications.
Optimizing FFmpeg for better compression of FFV1 RGB48 as specified in 
spec is not blocked after this change.
The only reason for keeping the experimental flag for RGB48 support is 
if the resulting file does not comply to the FFV1 specification, and 
this is not the case (tested with a conformance checker complying to 
FFV1 specifications).

As a result, the comment about get_contet for RGB48 is blocking for 
removing experimental status of version 4, but the suggested patch does 
not touch on version 4.

[1] 
https://github.com/FFmpeg/FFV1/blob/cbb560873e54bfe2d2042e898a210fe2abd079e0/ffv1.lyx#L659
[2] 
https://github.com/FFmpeg/FFV1/blob/fdc65b73282633cc8867c185e83a6d21e5c65f3f/ffv1.md#context 

[3] 
https://github.com/FFmpeg/FFV1/blob/fdc65b73282633cc8867c185e83a6d21e5c65f3f/ffv1.md#micro_version

---

From: Michael Niedermayer <[email-masked]>
Date: Sun, 7 Jan 2018 20:08:06 +0100

Hi

On Sun, Jan 07, 2018 at 06:39:34PM +0100, Jerome Martinez wrote:
> Looks like we talk about 2 different subjects: FFV1 bitstream specifications
> and FFmpeg implementation.
> Let separate subjects:
> 

[...]

> already some non experimental 16-bit support in FFmpeg FFV1 encoder. 16-bit
> is not new and for RGB48 the actual encoding after RGB to YCbCr

This is correct but i think misleading a bit
the 17bit case this is about uses a seperate codepath. So if its enabled
we generate new files that have never been generated before in some sense
AFAIK

If we change the bitstream in a future version, which i belive we 
should consider. Then we have an additional "old" version of the 17bit path
that needs to be supported in implementations.

> considered stable for RGB48 and others)

I think i added the flag check. The reason behind the check was so that the
bitstream syntax can be changed without the need to support every iteration
of the bitstream.
I had hoped someone would fund research and testing of further improvments 
to the various fine details of higher bit depth encoding beyond 8bit.
IIRC No further funding was provided, Noone worked on it as a volunteer,
No changes where proposed  ...

But it wasnt intended as a "bitstream is in stone, encoder might mismatch
the spec" at the time.

We have a draft spec now that does not limit bitdepth in any way, this may
have been a mistakke but i dont see this as a big problem honestly. I do not
propose to change this. But i would not oppose it if people want to change it

If someone creates a 19bit per sample file based on the draft spec it also
will not decode with most real world implementations.
There is no question that with a unlimited bitdepth, real implementations
will never support some depths

I just dont want to create a new variant of files _IF_ we plan to work on
improving the "bitstream syntax" in the next version.
Of course if these files are already out in the wild then we must support
this in the implementation. And then most of this discussion is meaningless

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.

---

From: Jerome Martinez <[email-masked]>
Date: Sun, 7 Jan 2018 21:20:15 +0100

On 07/01/2018 20:08, Michael Niedermayer wrote:
> the 17bit case this is about uses a seperate codepath.

For your (FFmpeg) encoder and decoder. Not mine (same code path is used 
for 8/10/12/16/... bit RGB and YUV with my encoder and decoder).
Again, we mix up bitstream specification and FFmpeg implementation.
In the bitstream specifications, there is currently only one path 
(except for handling files in the wild which are not conform to the 
unique path, we take care of them for historical reasons).

>   So if its enabled

- MediaConch conformance checker can check files up to 30 bits per 
component, and already implements the way it is written in spec for all 
supported bit depths, because the spec was flagged as stable whatever is 
the bit depth for a long time, without having anyone saying that 16 or 
17-bit for RGB (reminder: it is already set as "stable" for YCbCr 
16-bit) may have a different paths later for versions 0, 1, 3. If you 
change the bitstream of RGB48 or any stream with bitdepth <=30, you 
break it despite the fact it is validating correctly and would be a 
major issue for this tool (breaking trust about the tool or FFV1, as 
there would be 2 variants of FFV1 RGB48).
- I have an early (not yet public, for testing the spec only for the 
moment) version of an encoder conforming to the spec for all bit depths 
up to 30-bit per component.
- I have heard about other FFV1 encoders able to encode RGB48

> that needs to be supported in implementations.

question: How would you detect old version, from all encoders (not only 
FFmpeg)? there is nothing permitting that in the bitstream AFAIK (for 
v1: no micro_version; for v3: micro version >4 must be compatible with 
v4 as v4 is flagged stable).
So it is "bitstream is in stone" now, for at least versions 0, 1, 3. 
Reason I speak about R&D with version 4 bitstream (I don't speak about 
FFmpeg), which is still flagged as experimental for all files.

> No changes where proposed  ...

This is planned on my side after FFV1 versions 0, 1, 3 are finalized. 
With version 4 (the experimental version).
but it is possible only if we all are in sync and don't do something 
against the consensus, and as far I as I understand the consensus is 
that versions 0, 1, 3 are "bitstream is in stone" now (and actually from 
the beginning of CELLAR) based on the draft of FFV1 specifications, 
which is detailed before going to IETF but not changed. All the 
communication, at IETF and conferences, around FFV1 versions 0, 1, 3 is 
that the bitstream specification is stable, and discussing about 
breaking this consensus may harm FFV1 spread.

> But it wasnt intended as a "bitstream is in stone, encoder might mismatch

I disagree: the idea behind CELLAR is that all encoders **must** match 
the spec, or there is misunderstanding to clarify. Else everyone does 
his own version of FFV1, and we can not work together on an unique 
lossless compression format. The spec is expected to be the reference 
(for versions 0, 1, 3 for the moment) and encoders are expected to match 
the spec.

> will not decode with most real world implementations.

It will with my conformance checker. You can not know what is "real 
world", as a couple of encoders may be in house only.
we still mix up bitstream specification and one implementation.
The idea behind CELLAR is to have FFV1 a standardized video format, so 
we can not decide for 1 encoder to change the bitstream. We need to find 
a consensus on CELLAR mailing-list first. The consensus is what is 
written in the draft and there was no post on CELLAR mailing-list for 
limiting to some bit depths.

> will never support some depths

Whatever is the support from the encoders you know, you can not know 
what was created by others based on the bitstream specification. so the 
bitstream is specified (for versions 0, 1, 3) for all bit depths and we 
can not change that now, as it is written for a long time in spec that 
the spec is flagged stable for these versions.

The question is not what is the support from decoder x, the question is 
that if there is a plan to break the compatibility between current 
version of FFV1 draft and future versions of this document.

> improving the "bitstream syntax" in the next version.

I don't understand: yes, we plan to improve the bitstream, and this is 
the reason we plan a version 4, 5, 6... But it does not mean that 
versions 0, 1, 3 are not used by FFmpeg or any other encoder for all bit 
depths.

But again, here (ffmpeg-devel), my goal was to speak about FFmpeg 
encoder, it was not intended to speak about FFV1 specification, and it 
is weird for me to have to speak about FFV1 specification on 
ffmpeg-devel (it should be on CELLAR only).
Being sure that FFmpeg is creating a bitstream conforming to FFV1 
specifications (so versions 0, 1, 3) should be the only criteria.

> this in the implementation. And then most of this discussion is meaningless

I confirm that RGB48 files are already in the wild, as it is needed by 
some archives, and I confirm that they rely on MediaConch for testing 
the conformance compared to the specification.

I still don't see a reason to block 1 encoder to create RGB48 with 
version 1 or 3, as the spec is clear for a long time about how the 
bitstream is for such bit depth. Note that the FFV1 RGB48 decoder is not 
flagged as experimental, so any old version of FFmpeg should be able to 
decode new files you want to create (I don't see how they could do that 
as they don't know the get_context you want to implement) if you don't 
want to have complain that an old stable version of FFmpeg is not able 
to decode a FFV1 RGB48 file you just created with the new FFmpeg 
implementation.

Again, the experimental flag is for FFmpeg encoder, not the FFV1 
bitstream, the FFV1 bitstream is flagged as stable in FFV1 
specification, including RGB48.

Additionally, FFmpeg FFV1 decoder is currently able to decode RGB48 as 
specified in FFV1 specification and created by encoders conforming to 
the spec (FFmpeg "experimental" included) without having to set "-strict 
experimental" on the command line.

For these reasons, I still argue to remove the experimental flag for 
this encoder about RGB48 as the output is conform to FFV1 specifications 
and current and older versions of FFmpeg would not be able to decode 
FFV1 RGB48 if get_context is changed.

Jérôme

---

From: Michael Niedermayer <[email-masked]>
Date: Mon, 8 Jan 2018 00:41:58 +0100

On Sun, Jan 07, 2018 at 09:20:15PM +0100, Jerome Martinez wrote:
> For your (FFmpeg) encoder and decoder. Not mine (same code path is used for
> 8/10/12/16/... bit RGB and YUV with my encoder and decoder).

The seperation of 16bit and >16bit results out of the data type size
fitting or not fitting in "int16".

A space/cache efficient implementation, possibly one using SIMD would
use seperate codepaths for 16 and > 16 on a normal architecture thats based on
8bit bytes.

Its part of spec design what "an efficient implementation" would/could do

[...]
> FFmpeg)? there is nothing permitting that in the bitstream AFAIK (for v1: no
> micro_version; for v3: micro version >4 must be compatible with v4 as v4 is
> flagged stable).
> So it is "bitstream is in stone" now, for at least versions 0, 1, 3. Reason
> I speak about R&D with version 4 bitstream (I don't speak about FFmpeg),
> which is still flagged as experimental for all files.

very simple, the lowest version that we can make that change in without
causing any pain to people using ffv1 and without breaking the spec.

[...]
> I disagree: the idea behind CELLAR is that all encoders **must** match the

you disagree what i mean in a years old commit in ffmpeg i authored ?

> spec, or there is misunderstanding to clarify. Else everyone does his own
> version of FFV1, and we can not work together on an unique lossless
> compression format. The spec is expected to be the reference (for versions
> 0, 1, 3 for the moment) and encoders are expected to match the spec.

you make no sense
This code was intended to be used for developing the next version, that didnt
happen. If it did happen the code in the implementation would have changed
and once it was looking optimal a suggestion with test results would have
been posted to CELLAR to update the draft/spec. If that update did happen
then once its "stable" the check would have been removed from the encoder
implementation so people could use it.

[...]

> Whatever is the support from the encoders you know, you can not know what
> was created by others based on the bitstream specification. so the bitstream
> is specified (for versions 0, 1, 3) for all bit depths and we can not change
> that now, as it is written for a long time in spec that the spec is flagged
> stable for these versions.

I can know what is possible with real implementations build by others because
they are constrained by the same physical laws and space and matter available
in the observable universe.

> 
> The question is not what is the support from decoder x, the question is that
> if there is a plan to break the compatibility between current version of
> FFV1 draft and future versions of this document.

I think its more like a circular reference
The draft/spec intended to describe the existing status quo
and while in the implementation something was marked experimental this 
detail was lost in the spec 

[...]

> I confirm that RGB48 files are already in the wild, as it is needed by some
> archives, and I confirm that they rely on MediaConch for testing the
> conformance compared to the specification.

Thanks, then its probably best if we support this format in en and decoder
implementations. As its out there already

[...]

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin