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

Add ICC profile to ImageDecoder #1858

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Conversation

sophie-h
Copy link
Contributor

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.


Giving this a go. Not sure why &mut self was suggested in the issue

Closes #1832

@fintelia
Copy link
Contributor

Thanks for this PR! The reason to require &mut self is because some future decoders might require it. In particular, we don't want to force every decoder to parse the ICC profile in its new() method.

@sophie-h
Copy link
Contributor Author

Changed all to &mut

@HeroicKatora
Copy link
Member

👍 Though I now loathe how ImageDecoder::read_image takes self instead of &mut self. For instance, in the PNG decoder the ICC profile may not be part of the header but be placed at some arbitrary location in the data stream. The caller can not generically know if the information has already been read until the image parsing was 'done'. The whole design deserves a good rework.

Going to ignore the deny warnings for this PR, it looks like rav1e and criterion had new releases we should bump to.

@sophie-h
Copy link
Contributor Author

in the PNG decoder the ICC profile may not be part of the header

I guess I should mention in the docs that it could only be available after completely reading the image?

The whole design deserves a good rework.

Not sure if that is about this MR? I mean, that sounds like a fundamental issue. But for ICC profiles, it seems like it's fine to only have them after the complete image is decoded.

For EXIF, it would be much more relevant to know it as early as possible because knowing the size and orientation of the image is very helpful for the presentation, even before complete decoding.

Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

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

Not sure if that is about this MR? I mean, that sounds like a fundamental issue. But for ICC profiles, it seems like it's fine to only have them after the complete image is decoded.

It's not, PR itself looks good to me.

@fintelia fintelia merged commit 2ca903c into image-rs:master Feb 10, 2023
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.

Access ICC profile data
3 participants