ICC profile reading improvements, especially for PSD

[lgritz]

The main thing here is that it turns out that when trying to parse the ICC profiles from Photoshop PSD/PSB files, we weren't storing the attributes in the correct way. Everything else, we shoved in both the "common" and "composite" imagespecs, but we left ICC data out of the composite spec.

While I'm at it, I made two other ICC related changes:

• Among the several formats with ICC profiles, we were inconsistent about whether we report errors for corrupt ICC profiles and treat the whole image as broken, or just ignore the errors entirely. Fixed in various places to make a consistent policy of always checking the errors, and reporting/terminating if the "imageinput:strict" global attribute is true, allowing it to just ignore the broken ICC record if that attribute is false.

• Found a spot in JPEG handling of ICC where we were dangerously trusting of the sizes of things. Replace a risky memcpy with a safer spancpy that can't overrun the buffer bounds when copying.

[zachlewis]

Sidebar: out of curiosity, can OIIO do any color processing itself w.r.t. embedded ICC metadata? I know OCIO's API has methods for converting ICC profiles (e.g. provided by the OS at the system level) into "virtual displays" attached in place to an OCIO config held in memory; and likewise for baking Display / View transforms to ICC data. Maybe it would be cool if OIIO could optionally apply embedded ICC data as an "autocc"-type color transform (e.g., if a color space cannot be ascertained from file rules). And likewise, it might be kind of neat if OIIO had a means for embedding ad-hoc XYZ-D50_to_Display-View transforms as ICC data when writing to supported formats. (In fact, it should even be possible to derive a match from embedded ICC profile to existing OCIO config display color spaces / colorimetric views, if one exists...)

I don't personally use ICC profiles in my work, so I'm not sure if this is even desired behavior for those who do; but the recent discussions surrounding color-encoding-related EXR metadata have got me thinking about heuristics for handling other methods of embedding and communicating "implicitly managed" color encoding data (such as CICP / NCLX tuples used by AVIF and various video codecs).

Just a thought.

[lgritz]

@zachlewis That would be a really helpful addition, and it is one beyond my very thin knowledge of ICC and OCIO. Honestly, I'm just a plumber who can, upon request, connect an OIIO API call to an obvious OCIO one, but I do so with minimal understanding about how it works (beyond what you pick up by osmosis with a career in VFX studios). I'm regularly surprised to discover the existence of features that have been in OCIO for years without my awareness. I'm sure that somebody well versed in the ICC support in OCIO and how it should be used could rig up a half dozen extremely useful OIIO features in a matter of days, if not hours.

[lgritz]

Anybody object to the changes in this PR?

[zachlewis]

Anybody object to the changes in this PR?

(No objections from me, although I honestly haven't really reviewed this PR)

I'm sure that somebody well versed in the ICC support in OCIO and how it should be used could rig up a half dozen extremely useful OIIO features in a matter of days, if not hours.

I could definitely slug together a proof of concept in Python pretty quickly to get juices marinating, if you're interested -- but OCIO is looking at providing helpers for interpreting and generating embeddable metadata for various formats, and I think OIIO would be best served for now by helping to guide those efforts, so OCIO can do as much heavy lifting as possible.

[lgritz]

Yes, all things being equal, I'd prefer to just outsource the problem to OCIO and assume that there's a future OCIO API call that directly does what we need. OIIO's pieces of the puzzle should really be limited to:

• knowing how to read/write the ICC profile for each type of image file
• exposing/wrapping the OCIO functionality via an OIIO API call and an oiiotool action/cli argument (much as we do for other important OCIO functionality)

[lgritz]

(No objections from me, although I honestly haven't really reviewed this PR)

My theory of code review is that most of the value is just getting a second pair of eyes on it so that we are truly minimizing the number of things directly committed without any 2nd party oversight. High priority items are:

• Does the approach/design -- revealed mostly by the commit comment -- seem like the right way to solve the problem and the right direction for the project, and does it appear to correspond to what the code seems to be doing at a high level.
• Is there anything that appears extraneous or malicious sneaking in that seems to not be a legit part of the PR's purpose.
• Is the functionality tested somehow, if not plainly obvious.
• Are any public API changes getting the scrutiny they deserve, and are any backward compatibility breaks or behavior changes appropriate for the current project stage or the likely release branch backports we'll do of this PR.

All of that can usually be done with a cursory look by anybody who is a competent programmer even if not previously familiar with that section of the code. A deep line-by-line expert understanding of exactly how it works (to the degree that you could identify any bugs just by reading the code) would be ideal, but realistically it is rare to achieve in practice, and that's ok because it's only a tiny portion of the code review value.

[zachlewis]

Well in that case, LGTM...!

(Although if this is exhaustive of all the plugins that currently implement ICC support, I guess it's worth at least noting somewhere that HEIF, AVIF, and JPEG XL all permit embedded ICC profiles as well...!)

[lgritz]

(Although if this is exhaustive of all the plugins that currently implement ICC support, I guess it's worth at least noting somewhere that HEIF, AVIF, and JPEG XL all permit embedded ICC profiles as well...!)

Good point! I just spot-checked, and currently our heif and ffmpeg(avif) readers do not consider icc at all, and jpegxl reads it from the file then but does not add it to the ImageSpec. :-) So yeah, let's remember that there are other ICC-related touch-ups that somebody should do (ideally, the people who are most active in each of those format reader implementations).