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.

Sign up for GitHub

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

Jump to bottom

set NCLX defaults to match sRGB and change handling of unspecified NCLX values #1017

Merged
merged 4 commits into from
Oct 30, 2023
Merged

set NCLX defaults to match sRGB and change handling of unspecified NCLX values #1017

merged 4 commits into from
Oct 30, 2023

Conversation

farindk
Copy link
Contributor

@farindk farindk commented Oct 30, 2023

This is an alternative implementation of #1015.

kleisauke
kleisauke reviewed Oct 30, 2023
View reviewed changes
libheif/nclx.cc Outdated
m_colour_primaries = heif_color_primaries_ITU_R_BT_709_5;
}

if (m_transfer_characteristics == heif_color_primaries_unspecified) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (m_transfer_characteristics == heif_color_primaries_unspecified) {
if (m_transfer_characteristics == heif_transfer_characteristic_unspecified) {

@farindk farindk force-pushed the unspecified-nclx-handling branch from c428e08 to 136cf01 Compare October 30, 2023 13:08
kmilos
kmilos reviewed Oct 30, 2023
View reviewed changes
examples/heif_enc.cc Outdated
uint16_t nclx_matrix_coefficients = 6;
uint16_t nclx_colour_primaries = 2;
uint16_t nclx_transfer_characteristic = 2;
uint16_t nclx_matrix_coefficients = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 5 for sRGB

libheif/nclx.cc Outdated
m_matrix_coefficients = 6;
m_colour_primaries = 1;
m_transfer_characteristics = 13;
m_matrix_coefficients = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

5 for sRGB

libheif/nclx.cc Outdated
void color_profile_nclx::replace_undefined_values_with_defaults()
{
if (m_matrix_coefficients == heif_matrix_coefficients_unspecified) {
m_matrix_coefficients = heif_matrix_coefficients_ITU_R_BT_709_5;
Copy link
Contributor

Choose a reason for hiding this comment

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

heif_matrix_coefficients_ITU_R_BT_470_6_System_B_G (i.e. 5) for sRGB

@farindk
Copy link
Contributor Author

farindk commented Oct 30, 2023

Are there any references what the values should be for sRGB? I found this random piece of code: https://observablehq.com/@eeeps/color-space-determiner that also seems to favor 1,13,1.

@kmilos
Copy link
Contributor

kmilos commented Oct 30, 2023 •
edited
Loading

I found this random piece of code: https://observablehq.com/@eeeps/color-space-determiner that also seems to favor 1,13,1.

Plain wrong. See https://www.itu.int/rec/T-REC-H.273 for IEC 61966-2-1 (sRGB/sYCC) legal values.

@farindk
Copy link
Contributor Author

farindk commented Oct 30, 2023 •
edited
Loading

Accoding to H273, color_primaries=1. I think we agree here.
It also says to use transfer_characteristics=13.
Then, H273 says to use matrix_coefficients=0 for SRGB. Well ok, but that's not what we're looking for.
How do you derive that matrix_coefficients should be 5 or 1 ?

@kleisauke
Copy link
Contributor

I found these two comments:
AOMediaCodec/av1-avif#195 (comment)
MPEGGroup/MIAF#2 (comment)

Which says that matrix_coefficients defaults to 5 or 6 for sRGB.

@kmilos
Copy link
Contributor

kmilos commented Oct 30, 2023 •
edited
Loading

Then, H273 says to use matrix_coefficients=0 of SRGB. Well ok, but that's not what we're looking for.
How do you derive that matrix_coefficients should be 5 or 1 ?

IEC 61966-2-1 mentions sYCC representation that specifically uses matrix coeffs 5 (601).

However, note that the same wikipedia page also mentions sRGB conversion to XYZ using the 709 coeffs earlier...

Also note that the JPEG/JFIF specification uses 601 matrix coeffs.

@kleisauke kleisauke mentioned this pull request Oct 30, 2023
Merged
@farindk
Copy link
Contributor Author

farindk commented Oct 30, 2023

Ok, that makes sense. Then we'll go for matrix_coefficients=5.

@kleisauke
Copy link
Contributor

For reference, libavif seems to default to 13/1/6:
https://github.com/AOMediaCodec/libavif/blob/f9dd31fea43f748ce1a6e49be4cd9e68b916ebe6/apps/avifenc.c#L2269-L2276
https://github.com/AOMediaCodec/libavif/blob/f9dd31fea43f748ce1a6e49be4cd9e68b916ebe6/apps/avifenc.c#L1460

@farindk
Copy link
Contributor Author

farindk commented Oct 30, 2023

For reference, libavif seems to default to 13/1/6: https://github.com/AOMediaCodec/libavif/blob/f9dd31fea43f748ce1a6e49be4cd9e68b916ebe6/apps/avifenc.c#L2269-L2276 https://github.com/AOMediaCodec/libavif/blob/f9dd31fea43f748ce1a6e49be4cd9e68b916ebe6/apps/avifenc.c#L1460

Yes. But since matrix_coefficients 5 and 6 are exactly the same, this does not matter.

@kmilos
Copy link
Contributor

kmilos commented Oct 30, 2023 •
edited
Loading

For reference, libavif seems to default to 13/1/6

Yeah, IIRC this was because either Chrome or Safari originally only implemented parsing that combo and not (IMHO more canonical) 13/1/5 in addition?!

@farindk farindk merged commit d5a5559 into master Oct 30, 2023
30 checks passed
@farindk
Copy link
Contributor Author

farindk commented Oct 30, 2023

Thanks, @kleisauke , @kmilos

farindk added a commit that referenced this pull request Oct 30, 2023
@farindk
use MC=6 for sRGB because of potential incompatibility of Safari/iOS …
4809d30 
…with MC=5 (#1017)
farindk added a commit that referenced this pull request Oct 30, 2023
@farindk
use MC=6 for sRGB because of potential incompatibility of Safari/iOS …
7cd873f 
…with MC=5 (#1017)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kleisauke kleisauke kleisauke left review comments

@kmilos kmilos kmilos left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@farindk @kmilos @kleisauke @novomesk