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

Support for reading writing ICC profiles in Jpeg #74

Closed
JimBobSquarePants opened this issue Jan 10, 2017 · 14 comments
Closed

Support for reading writing ICC profiles in Jpeg #74

JimBobSquarePants opened this issue Jan 10, 2017 · 14 comments

Comments

@JimBobSquarePants
Copy link
Member

Currently we lack support for reading/writing ICC color profiles within Jpeg.

This directly negatively affects the color conversion process that we use for converting YCCK or CMYK jpeg images and prevents us from supporting DCI-P3

Reading in C# should be achievable using code based on Drew Noakes MetaDataExtractor (Apache 2.)

https://github.com/drewnoakes/metadata-extractor-dotnet/blob/5411bd3cad10facfd01deb4b1f6878ee242d98a0/MetadataExtractor/Formats/Icc/IccReader.cs

We'll have to additionally figure out how the conversion process takes place. there's info in WikiPedia but it's limited. @antonfirsov Could you possibly have a dig around libjeg-turbo and see if you can find how it's dealt with there? - My C is poor so I struggle to follow that codebase.

We probably don't need the ability to write a profile, just to store it within the image @dlemstra Perhaps we can work in your plan to store different profiles here?

Please comment below on your thoughts everyone.

@SepiaGroup
Copy link

SepiaGroup commented Feb 27, 2017

@JimBobSquarePants

attached is a jpg that has its colors muted when saved back to a new jpg file. i get the same results when i save this out of photoshop into sRGB.

hth

sample-sunset

@JimBobSquarePants JimBobSquarePants self-assigned this Mar 21, 2017
@JBildstein
Copy link
Contributor

I wrote a color library in C# which has ICC reading/writing already built in.
If you are interested I'd be willing to add that code to ImageSharp. I'll have to do some refactoring but it is very complete and works well. It does not include conversion though (started it but it's not nearly complete).
Here are the relevant files: https://github.com/JBildstein/NCM/tree/master/ColorManager/ICC
The rest of the library overlaps a lot with the one from @tompazourek but has a different conversion approach.

@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Mar 21, 2017

Hi @JBildstein,

Thanks so much for getting in touch and helping out! ICC profiles are are complicated beast and your experience is most welcome!

I've made a start in a feature/icc branch by adding a large number of the tags and descriptions from the specification.

be197bd

I've shifted my focus just now though to porting the color conversion code from Colourful.NET into our library replacing the current colorspaces (We can't use your conversion code as a basis as it uses reflection.Emit) I'll be doing a lot of work within that conversion to boost performance. I've made a good start, just not pushed anything yet.

If you could focus on ICC reading/writing that would be great!

Managing the combination of our work is the trickiest part and I think the best way to do this is as follows.

  • I give you temporary write access to the repo.
  • I create a work-in-progress PR from my feature branch.
  • We can continue to push to the feature branch until we are happy with the changes.

Doing the above make handling merges much easier and allows us to move rapidly.

How does that sound?

Cheers

James

@JBildstein
Copy link
Contributor

sure, no problem! I'm happy that my endless reading of the ICC specs helps someone 😄
Your start is great, because I don't have any documentation on my enum members (yet).
I'll use the Exif code as a reference point on how to do things if that's alright.
Getting write access to the branch works for me, definitely makes things easier.

Using Colorful.NET as a basis for the color conversion is a good idea since, as you said, my conversion is not compatible with .Net Core and his is very likely also easier to read.
Still, it might be useful for cross referencing:
https://github.com/JBildstein/NCM/tree/master/ColorManager/Conversions
Even though the actual conversion is pieced together dynamically, most of the formulas can be found in that folder.
You have to be a bit careful though because sometimes things like range checking and whitepoint conversions are done elsewhere.

@JimBobSquarePants
Copy link
Member Author

I've created the PR here. If you work on the feature/icc branch it'll automatically update.

#144

I'll be having a very close look at your library to combine the best of both worlds.

@JimBobSquarePants
Copy link
Member Author

This is almost ready to close. A few conversion bits and we're laughing 👏

@pkese
Copy link

pkese commented Sep 14, 2017

Jim's branch had been merged back in May.

Is there anything more to do, or can this ticket be closed?

@JimBobSquarePants
Copy link
Member Author

@pkese We still don't have conversion using those classes in our jpeg decoder. Once we have them I'll close this.

@JBildstein
Copy link
Contributor

ICC conversion is tracked here: #273
I have written about half of the unit tests for the calculation bits (not pushed yet) and hope to do the rest soon. Work's keeping me a bit busy at the moment so it's going slower than I'd like.

@JimBobSquarePants
Copy link
Member Author

@JBildstein No worries there, we've got time 😄 . If there's anything I can help with please let me know.

@JBildstein
Copy link
Contributor

@JimBobSquarePants good to know ^^ but I think it should definitely be done by 1.0
thanks, I'll need some help (or some pointers) for integration and I'd love some feedback on the design. I'm not entirely happy about it yet. But I'll give you a ping on gitter when I'm ready.

@JimBobSquarePants
Copy link
Member Author

Good man, no worries

@JimBobSquarePants
Copy link
Member Author

I'm closing this. We can read/write Icc Profiles now. Converting between is a different matter covered by #1567

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

No branches or pull requests

4 participants