Subject: [Cellar] FFV1 JPEG2000-RCT not really as described for bit depth 9 to 15? With patch

From: Jerome Martinez <[email-masked]>
Date: Tue, 11 Apr 2017 20:57:06 +0200

Hi CELLAR,

In the experience of creating an independent FFV1 decoder from the 
specification, I found an issue with JPEG2000-RCT color space and bit 
depth from 9 to 15. During decoding, the colors are not the expected ones.

For 8 and 16 -bit, JPEG2000-RCT is used as expressed in the draft 
specification.
For 9/10/12/14 -bit, looks like this is not JPEG2000-RCT but a variant, 
inverting b and g.

If I follow correctly the source code:
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/ffv1dec_template.c#L151-L160
- for 8-bit (and below), it is transformed to packed "bgr0" FFmpeg pixel 
format
- else for 16-bit (and above), it is transformed to planar "gbrpX" (X= 
bit depth) FFmpeg pixel format
- else it is transformed to planar "bgrpX" FFmpeg pixel format in a 
"gbrpX" FFmpeg pixel format, see 
https://github.com/FFmpeg/FFmpeg/blob/549045254c4614d5d61b5c36e340171a6914d57c/libavcodec/ffv1dec.c#L667-L673
Reading encoding part, looks like same, I understand it as like if order 
of planes was considered as the same as order of bytes for 8-bit packed 
order but it was a bug (bgr vs gbr).
Thus g (green) is not the "reference" for RGB to YCbCr transform, but b 
(blue) is used instead.

If I am correct, I suggest the attached patch.
In the details:
- for bit depth from 9 to 15, I suggest to support the "bug" for 
hypothetical bit depths of 11, 13, and 15 too so we have a test >=9 && 
<=15 instead of a list of values. We have this exception about bit 
depths 9 to 15 in order to support files already in the wild.
- as for the bug in the context computing, I suggest to remove this bug 
in the next incompatible version (version 4) so we will have a version 4 
with a more coherent specification (identical formulas whatever is the 
bit depth).

Jérôme

From 3da378d034c7f1756f1a7766ee21874649d4f150 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <[email-masked]>
Date: Tue, 11 Apr 2017 18:43:58 +0200
Subject: [PATCH] JPEG2000-RCT formula update

In order to be compatible with files in the wild
---
 ffv1.md | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/ffv1.md b/ffv1.md
index 0af341b..3bd7d0e 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -272,6 +272,41 @@ RFC:```
 RFC:b=Cb+g
 RFC:```
 
+Exception for the transform:  
+if bits_per_raw_sample is between 9 and 14 included, the following transform MUST be used:
+
+PDF:$$Cb=g-b$$
+RFC:```
+RFC:Cb=g-b
+RFC:```
+
+PDF:$$Cr=r-b$$
+RFC:```
+RFC:Cr=r-b
+RFC:```
+
+PDF:$$Y=b+(Cb+Cr)>>2$$
+RFC:```
+RFC:Y=b+(Cb+Cr)>>2
+RFC:```
+
+PDF:$$b=Y-(Cb+Cr)>>2$$
+RFC:```
+RFC:b=Y-(Cb+Cr)>>2
+RFC:```
+
+PDF:$$r=Cr+b$$
+RFC:```
+RFC:r=Cr+b
+RFC:```
+
+PDF:$$g=Cb+b$$
+RFC:```
+RFC:g=Cb+b
+RFC:```
+
+Background: when bits_per_raw_sample is more than 8, GBR planes where used as BGR planes during encoding then decoding in all known implementations of FFV1 bitstream. Note that when the issue is discovered, the only configurations of all known implementations being impacted are with bits_per_raw_sample of 9, 10, 12, 14, but it is suggested to simplify the check with all between 9 and 14. In the meanwhile, 16-bit JPEG2000-RCT color space was implemented without this issue in one implementation and validated by one conformance checker. It is expected (to be confirmed) to remove this exception for the transform in the next version of the bitstream.
+
 [@!ISO.15444-1.2016]
 
 An FFV1 frame using JPEG2000-RCT MUST use one of the following arrangements:
-- 
2.7.0.windows.1

---

From: Luca Barbato <[email-masked]>
Date: Tue, 11 Apr 2017 21:44:29 +0200

> with a more coherent specification (identical formulas whatever is the

Sounds good, I would suggest to make sure version 4 doesn't have
anything new but just remove all the quirks so it is a little more regular.

lu

---

From: Jerome Martinez <[email-masked]>
Date: Tue, 11 Apr 2017 22:08:57 +0200

> anything new but just remove all the quirks so it is a little more regular.

I think that having a version bump only for removal of all the quirks is 
not necessary, we can have both quirks removed and new features in the 
same version, the version number being for incompatibility info for 
decoder.

BTW, the patch I sent was an outdated one, attached is the one I wanted 
to send.
>From 071a9277035a64d93966db7273490699a65f5900 Mon Sep 17 00:00:00 2001

diff --git a/ffv1.md b/ffv1.md
index 0af341b..f55ebe9 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -272,6 +272,41 @@ RFC:```
 RFC:b=Cb+g
 RFC:```
 
+Exception for the reversible conversions between YCbCr and RGB:
+if bits_per_raw_sample is between 9 and 15 inclusive, the following reversible conversions between YCbCr and RGB MUST be used instead of the ones above:
+
+PDF:$$Cb=g-b$$
+RFC:```
+RFC:Cb=g-b
+RFC:```
+
+PDF:$$Cr=r-b$$
+RFC:```
+RFC:Cr=r-b
+RFC:```
+
+PDF:$$Y=b+(Cb+Cr)>>2$$
+RFC:```
+RFC:Y=b+(Cb+Cr)>>2
+RFC:```
+
+PDF:$$b=Y-(Cb+Cr)>>2$$
+RFC:```
+RFC:b=Y-(Cb+Cr)>>2
+RFC:```
+
+PDF:$$r=Cr+b$$
+RFC:```
+RFC:r=Cr+b
+RFC:```
+
+PDF:$$g=Cb+b$$
+RFC:```
+RFC:g=Cb+b
+RFC:```
+
+Background: At the time of this writing, in all known implementations of FFV1 bitstream, when bits_per_raw_sample was between 9 and 15 inclusive, GBR planes were used as BGR planes during both encoding and decoding. In the meanwhile, 16-bit JPEG2000-RCT color space was implemented without this issue in one implementation and validated by one conformance checker. It is expected (to be confirmed) to remove this exception for the transform in the next version of the bitstream.
+
 [@!ISO.15444-1.2016]

---

From: Dave Rice <[email-masked]>
Date: Wed, 12 Apr 2017 12:10:29 -0400

> On Apr 11, 2017, at 4:08 PM, Jerome Martinez <[email-masked]> wrote:
> 
> I think that having a version bump only for removal of all the quirks is not necessary, we can have both quirks removed and new features in the same version, the version number being for incompatibility info for decoder.
> 
> BTW, the patch I sent was an outdated one, attached is the one I wanted to send.
> <0001-JPEG2000-RCT-formula-update.patch>_______________________________________________

Minor wording suggestions:

(use the same terminology as the intro to the RCT formulae)
"the following reversible conversions" -> "the following formulae for reversible conversions"

(could be more specific about the next version)
"in the next version of the bitstream" -> "in the first stable variant of version 4 of the bitstream"

Other than that, LGTM,
Dave

---

From: "Peter B." <[email-masked]>
Date: Thu, 13 Apr 2017 10:01:26 +0200

On 04/11/2017 08:57 PM, Jerome Martinez wrote:
> - as for the bug in the context computing, I suggest to remove this
> bug in the next incompatible version (version 4) so we will have a
> version 4 with a more coherent specification (identical formulas
> whatever is the bit depth).

Summing it up to make sure I understand you correctly:

  * This means that the current FFV1 sourcecode uses a different order
of RGB values below 8bpc than >8bits?
  * Your suggestion is to keep this behavior as-is (for backwards
compatibility) and apply the coherent RGB order in future FFV1 versions?

Right?
:)

Regards,
Pb

---

From: Jerome Martinez <[email-masked]>
Date: Thu, 13 Apr 2017 10:08:34 +0200

> of RGB values below 8bpc than >8bits?

RGB is not stored as is, it is converted (lossless) to YCbCr
So a different formula i.e. Y = (R + G + 2B) / 4 instead of Y = (R + 2G 
+ B) / 4 and similar difference for Cb and Cr.
(again, this is what I understand, don't hesitate to counter-argument if 
someone thinks this is wrong)

> compatibility) and apply the coherent RGB order in future FFV1 versions?

Right.

---

From: Michael Niedermayer <[email-masked]>
Date: Thu, 13 Apr 2017 12:29:29 +0200

On Thu, Apr 13, 2017 at 10:08:34AM +0200, Jerome Martinez wrote:
> So a different formula i.e. Y = (R + G + 2B) / 4 instead of Y = (R +
> 2G + B) / 4 and similar difference for Cb and Cr.
> (again, this is what I understand, don't hesitate to
> counter-argument if someone thinks this is wrong)
> 

> >compatibility) and apply the coherent RGB order in future FFV1 versions?

I think we should take the oppertunity and improve the color
decorrelation transform.
Better decorrelation and compression should be achievable with probably
minor complexity

If people agree, ideas, papers and comparissions in that direction
would be interresting ...

the suggested patch LGTM

thanks

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

Never trust a computer, one day, it may think you are the virus. -- Compn

---

From: Jerome Martinez <[email-masked]>
Date: Thu, 13 Apr 2017 12:36:18 +0200

> would be interresting ...

Beyond my skills :p

> the suggested patch LGTM

updated one, with first change proposal from Dave.
I am not in favor of the second change proposal as it is a copy/paste 
from the first exception text, I suggest to have this change in a more 
global patch related to wording of exceptions.
>From 2df09d4de2b00003ede06367e2ed60cead490728 Mon Sep 17 00:00:00 2001

diff --git a/ffv1.md b/ffv1.md
index 0af341b..706f340 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -272,6 +272,41 @@ RFC:```
 RFC:b=Cb+g
 RFC:```
 
+Exception for the reversible conversions between YCbCr and RGB:
+if bits_per_raw_sample is between 9 and 15 inclusive, the following formulae for reversible conversions between YCbCr and RGB MUST be used instead of the ones above:
+
+PDF:$$Cb=g-b$$
+RFC:```
+RFC:Cb=g-b
+RFC:```
+
+PDF:$$Cr=r-b$$
+RFC:```
+RFC:Cr=r-b
+RFC:```
+
+PDF:$$Y=b+(Cb+Cr)>>2$$
+RFC:```
+RFC:Y=b+(Cb+Cr)>>2
+RFC:```
+
+PDF:$$b=Y-(Cb+Cr)>>2$$
+RFC:```
+RFC:b=Y-(Cb+Cr)>>2
+RFC:```
+
+PDF:$$r=Cr+b$$
+RFC:```
+RFC:r=Cr+b
+RFC:```
+
+PDF:$$g=Cb+b$$
+RFC:```
+RFC:g=Cb+b
+RFC:```
+
+Background: At the time of this writing, in all known implementations of FFV1 bitstream, when bits_per_raw_sample was between 9 and 15 inclusive, GBR planes were used as BGR planes during both encoding and decoding. In the meanwhile, 16-bit JPEG2000-RCT color space was implemented without this issue in one implementation and validated by one conformance checker. It is expected (to be confirmed) to remove this exception for the transform in the next version of the bitstream.
+
 [@!ISO.15444-1.2016]

---

From: Luca Barbato <[email-masked]>
Date: Thu, 13 Apr 2017 13:12:49 +0200

> Better decorrelation and compression should be achievable with probably

I would not conflate the two in the same iteration, there are loads of
interesting techniques we could consider for an evolution (daala's ones
might be the quickest to import and test IMHO), but would take more time
than just amend a small quirk and go forward in having a good base
standardized.

lu

---

From: Michael Niedermayer <[email-masked]>
Date: Thu, 13 Apr 2017 15:37:17 +0200

> than just amend a small quirk and go forward in having a good base

Everything we add to the standard adds burden to people implementing
it.
I think its better if we have 2 instead of 3 variants that need to
be supported

[...]

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact

---

From: Dave Rice <[email-masked]>
Date: Thu, 13 Apr 2017 13:55:37 -0400

> On Apr 13, 2017, at 4:01 AM, Peter B. <[email-masked]> wrote:
> 

AFAIK there has never been an FFV1 implementation that supported <8 bit RGB, thus for instance a Road Pizza encoding could not be encoded losslessly in FFV1. This issue is specific to bits depths from 9-15 inclusive.

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