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

First pass at a ICCTransform #87

Conversation

malcolmhumphreys
Copy link
Contributor

Had sometime on the plane so did a first pass at a ICCTransform, which will be needed for 'iv color profiles/LUTs' on the https://github.com/OpenImageIO/oiio/wiki/GSoC-Idea-Page.

Would like some feedback on where the filepath resolution should be? It's easy for me to add this in the Op but most other things have this in the Transform, is this where this logic should sit?

I have not implemented the setInputMem(const void * inputPtr, int size) functions yet, but have added these to the transform interface, these would be used when loading icc profile embedded in file headers.

@jeremyselan
Copy link
Contributor

Ah, the joys of airplane coding! :)

  • Yes, I'd prefer for the filepath resolution logic to be in the Transform, at op creation time. Please see FileTransform.cpp, BuildFileOps(...) . I
  • There's no caching in there; currently the cmsHPROFILEs are re-read every time the processor is created. This will need to be addressed before we roll it in. This is the unfortunate consequence of not riding on the FileTransform rails. I understand why this class was created instead of extending FileTransform (as it does the linking from an input ICC profile to an output one), so it will need an independent caching implementation.

I think my preference is for the caching to be implemented inside the ICCOp. Ideally, if the arguments haven't chagned to the profile cmsOpenProfileFromFile,cmsCreateTransform will never be called. (only cmsDoTransform will be called live in the hit cache case).

  • ICCTransform::getIntput is spelled wrong
  • Let's drop the exposed API that isn't currently hooked up. I really hate dead code paths. They can always be added in the future, as needed, in a binary compatible manner. I don't see any advantage to adding em now. If you'd like them as placeholders so people know what will be coming, let's at least comment them out.

API that isnt hooked up:

  • setInputMem functions
  • IccIntent (this is probably an oversight, INTENT_PERCEPTUAL is hard-coded)
  • Proofing profiles. What is the idea for this? Why include it as a separate option? Couldnt I just pass in the proofing profile as my icc profile? (Forgive me if I'm misunderstanding ICC proofing)
  • blackpointCompensation
  • gamutCheck
  • softProofing
  • Linking fails on my linux box:

Linking CXX shared library libOpenColorIO.so
[ 77%] /usr/bin/ld: ../../ext/dist/lib/liblcms2.a(cmserr.o): relocation R_X86_64_32S against `.text' can not be used when making a shared object; recompile with -fPIC
../../ext/dist/lib/liblcms2.a: could not read symbols: Bad value
collect2: ld returned 1 exit status

  • Also, there's a compile warning:
    src/core/ICCOp.cpp:195: warning: conversion to ‘cmsUInt32Number’ from ‘long int’ may alter its value

@jeremyselan
Copy link
Contributor

Thinking about it further, it's unfortunate this can't just be a FileTransform. Under what circumstances is it ok to apply an ICC profile in isolation (as opposed to a src/dst pair). Nuke's VectorApply node let's you apply an ICC profile, for example, and it only requires a single functionality. If what we've done is a superset of that, shouldn't we at least have that single file functionality? Maybe we should extend FileTransform (which already takes 'src') to have an optional 'dst' filename option? If src ends in .icc, we could then unhide 'dst'. And then all the other parameter (such as iccIntent) could be exposed as optional FileTransform options.

Will this make ocio2icc go away? Will ociobakelut supplant that for icc profile generation?

@malcolmhumphreys
Copy link
Contributor Author

I did start down that path but thought that bloating out the FileTransform interface with icc specific parameters would be an undesirable change. Also with the new MPE tags this avoids any need for handling data range precession issues with sampling these out into 1D and 3D luts.

How would you want to handle being able to do fwd and rev icc transforms where 3D luts are only allowed to go in one?

Soft Proofing requires 3 profiles 1) the profile of the file your working on 2) the profile of your display device 3) the profile of the device your emulating.

Sorry I didn't hook everything up as I didn't want to spend all the time before nutting out if this is the right direction.

It could go either way, but I do feel like FileTransform is really for baked out data while ICCTransform is integrating part of another system so it does feel a bit like it's own beast.

I do feel if we could somehow tag a colorspace with an icc profile this could clean up some of the need for these extra parameters, as it does feel a bit clunky and limiting to have to reference both the src and dst icc profiles. I'm not 100% sure this is what we want as it feels like this ties two !'s heavily together which could cause some problems.

@dbr dbr mentioned this pull request Oct 26, 2011
@scoopxyz scoopxyz closed this Oct 27, 2017
@scoopxyz scoopxyz reopened this Oct 27, 2017
@hodoulp
Copy link
Member

hodoulp commented Jun 27, 2018

Now Refer to #541

@scoopxyz
Copy link
Contributor

Closing in favor of #541 : Will continue to use as another reference implementation

@scoopxyz scoopxyz closed this Sep 16, 2018
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.

None yet

4 participants