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

Prophoto RGB support #1744

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@aurelienpierre
Contributor

aurelienpierre commented Oct 10, 2018

Add support for Prophoto RGB :

  • as input profile and gamut clipping
  • as output profile
  • as a softproofing option
  • as a display color management option.

It works in input, but in output, it produces magenta highlights and I can't figure out why.

@aurelienpierre aurelienpierre changed the title from Prophotorgb to Prophoto RGB support Oct 10, 2018

@LebedevRI

This comment has been minimized.

Show comment
Hide comment
@LebedevRI

LebedevRI Oct 10, 2018

Member

Don't touch the submodule, please.

Member

LebedevRI commented Oct 10, 2018

Don't touch the submodule, please.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 10, 2018

Contributor

Sorry, I didn't notice this one got trapped in the commit

Contributor

aurelienpierre commented Oct 10, 2018

Sorry, I didn't notice this one got trapped in the commit

@LebedevRI

This comment has been minimized.

Show comment
Hide comment
@LebedevRI

LebedevRI Oct 10, 2018

Member

Uhm :)

Member

LebedevRI commented Oct 10, 2018

Uhm :)

@LebedevRI

This comment has been minimized.

Show comment
Hide comment
@LebedevRI

LebedevRI Oct 10, 2018

Member

Don't touch means don't touch.
Rebase your branch, and make sure that none of the commits touches (changes the version, deletes) the submodule.
Not 'nah just drop it'.

Member

LebedevRI commented Oct 10, 2018

Don't touch means don't touch.
Rebase your branch, and make sure that none of the commits touches (changes the version, deletes) the submodule.
Not 'nah just drop it'.

@LebedevRI

This comment has been minimized.

Show comment
Hide comment
@LebedevRI

LebedevRI Oct 10, 2018

Member

Also, pardon me that i yet again comment on a pr in the repo i no longer develop,
but all this has worrying amount of justification/reasoning, basically no commits messages,
and while the pr talks about adding another colorspace,
also some random (no reasons given) module regrouping is also stashed here :/

Member

LebedevRI commented Oct 10, 2018

Also, pardon me that i yet again comment on a pr in the repo i no longer develop,
but all this has worrying amount of justification/reasoning, basically no commits messages,
and while the pr talks about adding another colorspace,
also some random (no reasons given) module regrouping is also stashed here :/

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 10, 2018

Contributor

Now, it's clean… I hate git.

Contributor

aurelienpierre commented Oct 10, 2018

Now, it's clean… I hate git.

@@ -57,7 +57,8 @@ typedef enum dt_colorspaces_color_profile_type_t
DT_COLORSPACE_VENDOR_MATRIX = 13,
DT_COLORSPACE_ALTERNATE_MATRIX = 14,
DT_COLORSPACE_BRG = 15,
DT_COLORSPACE_LAST = 16
DT_COLORSPACE_LAST = 16,

This comment has been minimized.

@TurboGit

TurboGit Oct 11, 2018

Member

I think LAST should stays in last position, no?

@TurboGit

TurboGit Oct 11, 2018

Member

I think LAST should stays in last position, no?

This comment has been minimized.

@aurelienpierre

aurelienpierre Oct 11, 2018

Contributor

It makes sense, I didn't wanted to change the order since I wasn't sure it would be safe.

@aurelienpierre

aurelienpierre Oct 11, 2018

Contributor

It makes sense, I didn't wanted to change the order since I wasn't sure it would be safe.

@@ -38,6 +38,7 @@
#endif
static const cmsCIEXYZ d65 = {0.95045471, 1.00000000, 1.08905029};
//static const cmsCIEXYZ d50 = {0.96422000, 1.00000000, 0.82490000};

This comment has been minimized.

@TurboGit

TurboGit Oct 11, 2018

Member

A comment added, but I don't see why? If there is noting to say about it, better remove this line otherwise can you add a blurb about what it is about?

@TurboGit

TurboGit Oct 11, 2018

Member

A comment added, but I don't see why? If there is noting to say about it, better remove this line otherwise can you add a blurb about what it is about?

This comment has been minimized.

@aurelienpierre

aurelienpierre Oct 11, 2018

Contributor

I tried to use the same structure of code that is used for Adobe RGB, but none of these constants are actually used in the code, which is strange because we are supposed to correct the XYZ illuminant (D50 or D65) at some point when applying the conversion matrices between color spaces. So anyway, I dropped that here only for consistency, but they might come in handy at some point.

@aurelienpierre

aurelienpierre Oct 11, 2018

Contributor

I tried to use the same structure of code that is used for Adobe RGB, but none of these constants are actually used in the code, which is strange because we are supposed to correct the XYZ illuminant (D50 or D65) at some point when applying the conversion matrices between color spaces. So anyway, I dropped that here only for consistency, but they might come in handy at some point.

@@ -945,7 +963,8 @@ static const char *_profile_names[] =
N_("standard color matrix"),
N_("enhanced color matrix"),
N_("vendor color matrix"),
N_("alternate color matrix")
N_("alternate color matrix"),
N_("Prophoto RGB"),

This comment has been minimized.

@TurboGit

TurboGit Oct 11, 2018

Member

Bear with me, I'm no expert. But is it ok to have a Prophoto RGB color space into the list of matrix? I mean we don't have Adobe RGB here for example.

@TurboGit

TurboGit Oct 11, 2018

Member

Bear with me, I'm no expert. But is it ok to have a Prophoto RGB color space into the list of matrix? I mean we don't have Adobe RGB here for example.

This comment has been minimized.

@aurelienpierre

aurelienpierre Oct 11, 2018

Contributor

That's an error, it shouldn't be there, I will fix that.

@aurelienpierre

aurelienpierre Oct 11, 2018

Contributor

That's an error, it shouldn't be there, I will fix that.

@TurboGit TurboGit self-assigned this Oct 11, 2018

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 15, 2018

Contributor

I will close that for now. I think I am re-inventing the wheel here. It would be more relevant to clean-up the hard-coded standard spaces in dt (Adobe RGB, sRGB, REC 709 / 2020), replace them by ICC profiles with ICC v4 support. I will try to add https://github.com/ellelstone/elles_icc_profiles.git as a submodule and ship her profiles instead.

Contributor

aurelienpierre commented Oct 15, 2018

I will close that for now. I think I am re-inventing the wheel here. It would be more relevant to clean-up the hard-coded standard spaces in dt (Adobe RGB, sRGB, REC 709 / 2020), replace them by ICC profiles with ICC v4 support. I will try to add https://github.com/ellelstone/elles_icc_profiles.git as a submodule and ship her profiles instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment