-
-
Notifications
You must be signed in to change notification settings - Fork 840
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
Color conversion with ICC profiles #1567
base: main
Are you sure you want to change the base?
Conversation
I wonder why the test |
@brianpopow It'll be an accuracy issue most likely. (I hope it's not a JIT issue). It should be possible to inspect the result and see. |
Codecov Report
@@ Coverage Diff @@
## main #1567 +/- ##
======================================
Coverage ? 87%
======================================
Files ? 1023
Lines ? 55212
Branches ? 7052
======================================
Hits ? 48227
Misses ? 5768
Partials ? 1217
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
The issue only happens with a Release build. I think i found the reason, but it seems very weird. This can be seen with the testoutput:
|
@brianpopow Woah! That's bonkers! |
I have reported this issue: dotnet/runtime#55623 They confirmed the issue, but they say its unlikely to be fixed because netcore2.1 is out of support in august. |
@JimBobSquarePants It would be really nice, if we could bring this PR forward. This would be a good addition to ImageSharp. I thought, I may ask you, if you know what the PR needs (besides tests) to be finished? What would be the right way to apply an embedded color profile? My first attempt was:
Here is an example image with adobe rgb color profile: This does not seems to work, the colors seem to be wrong. Here are more example images |
@brianpopow Honestly..... I don't know. I was hoping the OP would come back to finish things off. I've just kept things updated over the years and hadn't gotten involved at all in the implementation as yet. Judging from the old comments in the previous PR I believe the code is based somewhat based on the following https://github.com/InternationalColorConsortium/DemoIccMAX/tree/master/IccProfLib As for accuracy. That result looks like it's just spitting out the sRGB values again. I do agree that it would be an awesome addition to the library and would save a ton of headaches. I hoped we'd get it in V3 but that's a mighty big ask. |
I think we definitely need a reference implementation to compare the results against. I tried BABL which gnome is using, but i could not get it to work on windows. I will take a look at DemoIccMAX |
I'm in love with these diagrams.🤩 |
I'm in love with Excalidraw. 😍 I didn't even know if the pictures would be useful, but I had too much fun drawing them! |
@brianpopow @saucecontrol I've been trying to debug the port of the Clut 4d interpolator but I'm drawing a blank because I have no idea how to run DemoIccMAX IccProfLib to do a comparison. Any hints? My money is on the port being sound but I'm doing something stupid porting the pcs to CIELab to convert via the color converter in |
@JimBobSquarePants I struggle myself to understand how DemoIccMAX is supposed to work. I would suggest trying to understand how Little-CMS does it. There are alot of tests included in Little-CMS, maybe we can try to replicate one of them (maybe
edit: There is no need to generate the visual studio project files, there are already ones in the repo: https://github.com/mm2/Little-CMS/tree/master/Projects/VC2022 |
Thanks @brianpopow I can't find anything useful there unfortunately. |
I added some code and notes to the converter to help debugging. What I'm seeing is the results of the lut transform result in very minor differences to the output value for each pixel 1e05. I'm wondering if somehow, we have a scaling problem. |
What is the state of this? Lack of ICC support is a major blocker for us. |
The state is as discussed in the various comments above, without contribution this PR will not be completed. We support ICC profile preservation but not transformation during decode. |
Prerequisites
Description
Note: This is a replacement for the original PR #273 from @JBildstein that was automatically closed by our Git LFS history rewrite. Individual commits have unfortunately been lost in the process. Help is very much needed to complete the work.
As the title says, this adds methods for converting colors with an ICC profile.
Architecturally, the idea is that the profile is checked once for available and appropriate conversion methods and a then a delegate is stored that only takes the color values to convert and returns the calculated values. The possible performance penalty for using a delegate is far smaller than searching through the profile for every conversion. I'm open for other suggestions though.
There are classes to convert from the profile connection space (=PCS, can be XYZ or Lab) to the data space (RGB, CMYK, etc.) and vice versa. There are also classes to convert from PCS to PCS and Data to Data but they are only used for special profiles and are not important for us now but I still added them for completeness sake.
A challenge here is writing tests for this because of the complexity of the calculations and the big amount of different possible conversion paths. This is a rough list of the paths that exist:
The three main approaches in that list are
The most used approaches are Color Trc for RGB profiles and LutAToB/LutBToA for CMYK profiles.
Todo list:
Help and suggestions are very welcome.