Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace color space wording by more adapted transformation wording #89

Closed
wants to merge 1 commit into from

Conversation

JeromeMartinez
Copy link
Contributor

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

Issue reported by @dwbuiten

ffv1.md Outdated
@@ -239,7 +239,7 @@ top16s = t >= 32768 ? ( t - 65536 ) : t
diag16s = tl >= 32768 ? ( tl - 65536 ) : tl
```

Background: a two's complement signed 16-bit signed integer was used for storing pixel values in all known implementations of FFV1 bitstream. So in some circumstances, the most significant bit was wrongly interpreted (used as a sign bit instead of the 16th bit of an unsigned integer). Note that when the issue is discovered, the only configuration of all known implementations being impacted is 16-bit YCbCr color space with Range Coder coder, as other potentially impacted configurations (e.g. 15/16-bit JPEG2000-RCT color space with Range Coder coder, or 16-bit any color space with Golomb Rice coder) were implemented nowhere. In the meanwhile, 16-bit JPEG2000-RCT color space with Range Coder coder 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 media predictor in the next version of the FFV1 bitstream.
Background: a two's complement signed 16-bit signed integer was used for storing pixel values in all known implementations of FFV1 bitstream. So in some circumstances, the most significant bit was wrongly interpreted (used as a sign bit instead of the 16th bit of an unsigned integer). Note that when the issue is discovered, the only configuration of all known implementations being impacted is 16-bit YCbCr without pixel transformation with Range Coder coder, as other potentially impacted configurations (e.g. 15/16-bit JPEG2000-RCT with Range Coder coder, or 16-bit content with Golomb Rice coder) were implemented nowhere. In the meanwhile, 16-bit JPEG2000-RCT with Range Coder coder 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 media predictor in the next version of the FFV1 bitstream.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ffv1.md Outdated

FFV1 supports two color spaces: YCbCr and JPEG2000-RCT. Both color spaces allow an optional Alpha plane that can be used to code transparency data.
FFV1 supports 1 Luma component, optionaly 2 Chroma components and 1 Alpha component that can be used to code transparency data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

& optionaly was a typo

ffv1.md Outdated

In YCbCr color space, the Cb and Cr planes are optional, but if used then MUST be used together. Omitting the Cb and Cr planes codes the frames in grayscale without color data. An FFV1 `Frame` using YCbCr MUST use one of the following arrangements:
Transformations permit to modify the pixel data in a reversible way in order to improve the compression ratio, before being compressed. An example of usage is to compress RGB content which could have each component independently compressed but with a less optimal compression ratio.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ffv1.md Outdated

### No transformation

There is no transformation, the source content is directly compressed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@JeromeMartinez
Copy link
Contributor Author

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)

Copy link
Contributor

@dericed dericed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@retokromer retokromer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JeromeMartinez
Copy link
Contributor Author

Rebased.

@JeromeMartinez
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

dericed commented Jan 12, 2018

+1

ffv1.md Outdated
@@ -1103,19 +1110,22 @@ If state_transition_delta is not present in the FFV1 bitstream, all Range coder

### colorspace_type

`colorspace_type` specifies the color space.
`colorspace_type` specifies source color space losslessly encoded, Pixel transformation used by the encoder, as well as interleave method.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Pixel capitalised here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ffv1.md Outdated
@@ -37,7 +37,7 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S

`RGB`: A reference to the method of storing the value of a Pixel by using three numeric values that represent Red, Green, and Blue.

`YCbCr`: A reference to the method of storing the value of a Pixel by using three numeric values that represent the luma of the Pixel (Y) and the chrominance of the Pixel (Cb and Cr).
`YCbCr`: A reference to the method of storing the value of a Pixel by using three numeric values that represent the luma of the Pixel (Y) and the chrominance of the Pixel (Cb and Cr). YCbCr word is used for historical reasons and currently references any color space relying on 1 luma and 2 chrominances e.g. YCbCr, YCgCo or ICtCp. Exact meaning of the three numeric values is provided by the container.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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».)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ffv1.md Outdated

### JPEG2000-RCT

JPEG2000-RCT is a Reversible Color Transform that codes RGB (red, green, blue) planes losslessly in a modified YCbCr color space. Reversible conversions between YCbCr and RGB use the following formulae.
JPEG2000-RCT is a Reversible Color Transform that codes RGB (red, green, blue) planes losslessly in a modified YCbCr color space. Reversible Pixel transformations between YCbCr and RGB use the following formulae.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pixel instead of Pixel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dericed dericed mentioned this pull request Jan 14, 2018
ffv1.md Outdated

FFV1 supports two color spaces: YCbCr and JPEG2000-RCT. Both color spaces allow an optional Alpha plane that can be used to code transparency data.
FFV1 supports 1 Luma component, optionally 2 Chroma components, and optionally 1 Alpha component that can be used to code transparency data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ffv1.md Outdated

There is no Pixel transformation, the source content is directly compressed.

The Cb and Cr planes are optional, but if used then MUST be used together. Omitting the Cb and Cr planes codes the frames in grayscale without color data. An FFV1 `Frame` using YCbCr MUST use one of the following arrangements:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ffv1.md Outdated
@@ -1099,23 +1107,25 @@ If state_transition_delta is not present in the FFV1 bitstream, all Range coder

### colorspace_type

`colorspace_type` specifies the color space.
`colorspace_type` specifies the Pixel transformation used by the encoder, as well as interleave method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Member

"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
Copy link
Contributor Author

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
Copy link
Contributor

dericed commented Jan 26, 2018

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.

@michaelni michaelni closed this in 5e928cf Jan 26, 2018
@JeromeMartinez JeromeMartinez deleted the transformations branch January 26, 2018 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants