Replace color space wording by more adapted transformation wording

[JeromeMartinez]

"Color Space" is not the right wording for this part of the spec.

Issue reported by @dwbuiten

[dericed]

would prefer no pixel transformation over without pixel transformation to match the later references of colorspace_type.

[dericed]

actually i see below uses no transformation. I suggest being consistent with either no pixel transformation or no transformation.

[dericed]

suggest 1 Luma component, optionally 2 Chroma components, and optionally 1 Alpha component

& optionaly was a typo

[dericed]

re "in order to improve the compression ratio" is this why? Aren't there transformations to allow FFV1 to support non-YCbCr colorspaces (RGB and maybe someday Bayer formats). Suggesting maybe: "Transformations permit to modify the pixel data in a reversible way in order to store alternate color spaces"

[JeromeMartinez]

RGB could be compressed without transformation, just saying that order is RGB instead of YCbCr, transformation is here for efficient compression.

I could remove the explanation if needed.

[dericed]

perhaps this section should be merged with the info on line 284

[JeromeMartinez]

PR updated.
Using now Pixel transformation everywhere (with uppercase P due to a comment about having "Pixel" with upper P in another PR). I don't use "Color Transformation" because IMO transformations are generic and could be on something else than color in the future.
I added a commit for moving the plane order section (without change on the text)

[dericed]

LGTM

[retokromer]

LGTM

[JeromeMartinez]

Rebased.

[JeromeMartinez]

Small update, keeping the "color space" column, but filling it with the actual color space (YCbCr or RGB), the transformation column containing "JPEG2000-RCT" when relevant.

[michaelni]

This is a bit confusing, YCbCr is itself a "transformed" form of RGB. Talking about one YCbCr variant as a transformation while another YCbCr is not a transformation sounds a bit odd to me. At least without some explanation in the text
FFV1 Supports "1 Luma component, optionally 2 Chroma components," <- and RGB

Also if the colorspace field would be extended to provide information about which (lossy) YCbCr transform is used as in BT709 vs BT601 this whole text would become even more confusing

[JeromeMartinez]

This is a bit confusing, YCbCr is itself a "transformed" form of RGB.

IMO there is a limit between what is done outside of the bitstream from the bitstream itself. If the encoder receives YCbCr content, how it was transformed previously is not part of the specification.

FFV1 Supports "1 Luma component, optionally 2 Chroma components," <- and RGB

IMO "FFV1 supports two color spaces" wording is misleading as one can think that RGB planes could be encoded separately (without transformation), and JPEG2000-RCT is not a color space.

As far as I understand, RGB is not directly supported i.e. you can not compress RGB without transformation, and e.g. Parameters( ) contains chroma_planes and h/v_chroma_subsample even if RGB colorspace is used (could be changed in version 4). Additionally, Slice ( ) expects YCbCr content only. So this is different, order of the RGB input is not relevant (could be RGB or GBR, it does not mater, there is a transformation which maps RGB components to YCbCr order). So RGB info should not be in "Components" paragraph. This is the reason I split content in 2 paragraphs: "Components" and "Pixel transformations" (the later one talks about how RGB can be losslessly encoded).

At least without some explanation in the text

I added a paragraph about it.

Also if the colorspace field would be extended to provide information about which (lossy) YCbCr transform

I didn't want to change all in 1 PR as I was requested to split my patches, "colorspace_type" and "YCbCr" were used before the patch, so issue is there before and after the patch. preview: color_space field would be more a coding method, and colour_primaries / transfer_characteristics / matrix_coefficients / HDR stuff need to be indicated in dedicated fields (at the end of Parameters()?). Adding a "colorspace_type" for color definition would break a lot of files which currently rely on the container for indicating BT.601/BT.709/BT.2020/YCoCg/ICtCp/.... But let put that in another PR.

---

I added 3 patches for review.
First one puts back a "Color space" paragraph with information what can be losslessly compressed. IMO that permits to clarify the lossless compression capability of FFV1 (YCbCr and RGB) and the way it is encoded (YCbCr only, RGB being encoded only after a transformation).
Second one removes redundant code and clarifies restriction when colorspace_type is RGB
Third one clarifies the use of YCbCr as for historical reason but is now used for storing other related color spaces (YCgCo, ICtCp...), this is similar to the way ITU did for H.265.

Let me know if/when I should merge patches.

[michaelni]

IMO there is a limit between what is done outside of the bitstream from the bitstream itself. If the
encoder receives YCbCr content, how it was transformed previously is not part of the specification.

It maybe is orthogonal to this issue but encoding is generally not normative / part of a specification.
The specification describes what is allowed in the bitstream and what it means. How to create that is outside the spec. A encoder could take RGB as input or Luma/Chroma from a compatible input like some jpeg2000 and store that. It could even convert RGB lossy to YCbCr and store that. That might in fact make sense if exact losslessness is not required as it might allow better compression. I will take a look at the new patches

[JeromeMartinez]

bump, as it would conflict with incoming patches (e.g. "more planes" ones), it is a bit frustrating to adapt again due to conflicting modified lines.

[dericed]

+1

[retokromer]

Why is Pixel capitalised here?

[JeromeMartinez]

Following this comment about titlecase for defined word

[retokromer]

The main word is transformation, not pixel, but I am not English speaker.

[retokromer]

I don’t think that repeating errors for «historical reasons» is a good move.

[JeromeMartinez]

I simplifies the patch and separates issues. A dedicated patch could be done changing that.
Note that e.g. ITU decided to not change the spec and add a similar wording for H.265 when they introduce YCgCo and ICtCp in color metadata.

[retokromer]

The fact that ITU uses a wrong wording doen’t make it correct, yet I can survive also with YUV…
Other point: Do you mean chrominance or chroma?

[JeromeMartinez]

I added this sentence for clarifying from #89 (comment), but no problem for removing it if it has no consensus and can help in being merged.
chrominance or chroma: I took the nearest word used in order to have my PR accepted, I have no problem for changing it if it makes my PR merged. I plan to add a PR for changing chrominance to chroma everyhere, I just don't want to do too many PRs in paralel as I have often line edition conflicts (would be the case here whatever word I take).

[retokromer]

OK, it’s fine with me to merge.
(I haven’t checked if chrominance should read chroma everywhere, therefore I would suggest to check every occurrence instead of changing systematically by a «find & replace».)

[michaelni]

I think this should not refer to the container like this because not all supported containers are able to store this information. As in asking one to look at the container which then has no standard way to contain this information

[JeromeMartinez]

Will change to "Exact meaning of the three numeric values is unspecified"

[retokromer]

pixel instead of Pixel?

[JeromeMartinez]

Following this comment about titlecase for defined word

[michaelni]

JPEG2000-RCT did imply RGB, the new text looses this

[michaelni]

this sounds odd: "improve the compression ratio, before being compressed."
I dont think declaring pixel transforms to be reversible is needed. Iam undecided if the justification for the feature, that is improving compression makes sense in the specification
"Pixel transformation is indicated in the bitstream in order to permit the decoder to configure the output accordingly to encoder input.", this just doesnt work. Theres no way to know from the bitstream if the encoder had jpeg2000 YCbCr or RGB input nor if it was planar or packed or BGR or RGB

[michaelni]

The act of encoding is generally not normative. Thats how all ITU/ISO video specs are written IIRC. I dont think we should imply too much about the encoder as this would make it very ambigous if a encoder doing something different was compliant.

[JeromeMartinez]

JPEG2000-RCT did imply RGB, the new text looses this

No loss, just moved.
This is on purpose, because FFV1 does not support to store RGB directly (without cheating e.g. flagging RGB as YUV444).
Y, Cb, Cr, Alpha planes order are indicated for both supported colors spaces, without referencing R, G or B planes, this sounds weird after reading ("it supports RGB but no order indicated?").

I separated in 3 parts for this reason, so suggestion is to have 3 clearly separated parts:

• "colors spaces" part: general overview about the supported color spaces for lossless compression (YCbCr and RGB are indicated here)
• "components" part: the stored planes (Y, Cb, Cr, A). No indication of RGB here because this is not supported as stored planes.
• "transformations" past: retrieving RGB from stored YCbCr through JPEG2000-RCT (I stipulated here JPEG2000-RCT).

I think it is better explained with such split, without being misleading (e.g. JPEG2000-RCT is not a color space but it is described as such, and RGB is not indicated as a color space)

I dont think declaring pixel transforms to be reversible is needed

I think it is important to be clear about this, but maybe redundant, removed.

this sounds odd: "improve the compression ratio, before being compressed."

removed

"Pixel transformation is indicated in the bitstream in order to permit the decoder to configure the output accordingly to encoder input.", this just doesnt work.

Was already removed

I dont think we should imply too much about the encoder

Was already removed

---

I added a commit removing remaining unnecessary and ambiguous sentences, I think the current PR addresses all concerns (except the "JPEG2000-RCT did imply RGB" as I disagree about it for the moment).

If the "JPEG2000-RCT did imply RGB, the new text looses this" is still a blocker, I suggest I remove this part from this PR and move it to another one, but IMO the patch would look less coherent. Note that in both cases I may add another PR for clarifying, here I didn't want to touch everywhere in a single PR, I tried to focused on the clarification of transformations and supported colors space (i.e. removing "JPEG2000-RCT color space" wrong wording).

I added a commit for letting reviewers review modifications, should I merge all of them now?

[michaelni]

"colors spaces" part: general overview about the supported color spaces for lossless compression (YCbCr and RGB are indicated here)
"components" part: the stored planes (Y, Cb, Cr, A). No indication of RGB here because this is not supported as stored planes.
"transformations" past: retrieving RGB from stored YCbCr through JPEG2000-RCT (I stipulated here JPEG2000-RCT).

The terminology here is confusing. If you see a color space as a vector space. Then a transformation from one colorspace takes you to another color space not to some components. That would make no sense. Its components before as much or as little as it is after the transform.

I added a commit for letting reviewers review modifications, should I merge all of them now?

I think adding suboptimal text in a commit and then removing it again in a subsequent one in a single pull req would be quite confusing if its pushed that way. The history of ffv1.md should stay easy readable

[JeromeMartinez]

I removed the split in 3 parts, just replacing "JPEG2000-RCT" by "RGB" when the purpose is the color space and not the transformation ("JPEG2000-RCT color space" is confusing IMO).

The history of ffv1.md should stay easy readable

I was meaning merging the commits in a single commit before the merge, so the history has only 1 line.
Done now (1 single commit)

YCbCr definition update is still there (without reference to container) but I can move it to another patch (it is here for answering to a comment about 601/709 but it is not completely linked to this PR)

[dericed]

anything blocking here? If so I suggest proceeding with #98 as-is to facilitate an draft update on the cellar site. Am chatting with mp4reg about the isobmff registration of ffv1 but there is no active draft at the moment.