Re: FileTransform interface changes


Jeremy Selan <jeremy...@...>
 

Malcolm,

Sorry for the delay in responding.

First, I agree with you that dynamically defined FileTransform options
would have more elegant. (prman is a great example of how such an API
can stay awesomely forwards compatible even decades later).

But considering that commercial apps are chomping at the bit to
release OCIO 1.0 binaries, and that we've promised an absolutely ready
to ship '1.0' just 2 weeks from now, I think it's too late in the game
to be considering non-trivial API changes. <tear/> Would you
concur? My gut tells me this is too much API design work to flesh out
in too short a time.

Of course I'm very open to considering such a change as a part of a
1.2 or 2.0 API. (As it wouldnt change the .ocio format, 1.2 is
possible). To do this issue 'right' we'd potentially want to give the
same treatment to all transform types, not just FileTransform. (As a
side effect all the static typed transforms would evaporate into a
single Transform with getType() !) See this #issue (from last week)
where we spec'd out some of the details.
https://github.com/imageworks/OpenColorIO/issues/155

So if you'd concur, let's go ahead and just add set/get
ShaperInterpolation in the meantime? What's the use case for
hasShaper? Is that for interface visibility?


In terms of ICCTransform support, I have mixed feelings. First off, I
totally understand the use case, and I'm completely committed to
solving that workflow with OCIO's 'ecosystem'.

But in terms of adding it ICCTransfrom to the core library, I have a
concern. How confident are we that littlecms can be build
*everywhere*? For example, at SPI we have a vm with an old libstdc++
for compatibility testing. Core OCIO builds fine on this machine, but
littlecms has linking issues. (below).

[ 48%] Building CXX object
src/core/CMakeFiles/OpenColorIO.dir/Transform.cpp.o
.libs/cmserr.o: In function `feof_unlocked':
/usr/include/bits/stdio.h:123: multiple definition of `feof_unlocked'
.libs/cmscnvrt.o:/usr/include/bits/stdio.h:123: first defined here
.libs/cmserr.o: In function `ferror_unlocked':

This particular error may be trivial to solve (I didnt even try), but
I wonder if adding littlecms to the core library may adversely impact
portability?

And making ICCTransform optional for core OCIO is a path I dont want
to go down. One of the best parts of OCIO is portability between apps,
and If we make an ICC transform optional at build time, OCIO will
become fractured into apps that support .icc, and those that don't.
What if you need .icc support, but one of the OCIO apps has chosen to
not build with support for it? If .icc profile support isnt
required, they're essentially useless. (The same argument applies
to Truelight, by the way).

So do essentially would have to *require* core littlecms / icc support
to consider it. Are we willing to make that jump at this point? Is
OCIO's ICCTransfrom ready to ship? (and be locked down?).
https://github.com/imageworks/OpenColorIO/pull/87

I think my preference would be in the medium term, to add an OCIO API
where transforms such as ICC / Truelight / CTL could be built
externally - as needed - without introducing the build-time
dependency. Supporting apps wouldnt need to ship with these optional
plugins - the uses would compile them as required.

In the meantime, a compromise (suitable for 1.0.0) could be to add an
additional external app (or part of ocio2icc?) that handles the
functionality of ICCTransform, and bakes the result into a 3dlut
suitable for OCIO. That way folks who rely on ICC calibration would
have a command to generate the needed 3d display luts - it just would
happen as a preprocess, and not at runtime.

Thoughts?

-- Jeremy



On Wed, Sep 14, 2011 at 7:00 AM, Malcolm Humphreys
<malcolmh...@...> wrote:
Hi,
As we are fast approaching 1.0 I know it's a bit late in the day for some
API changes. But I noticed these while doing the JNI wrapping for the
android port some things which would be really good to change before 1.0.
Could we add the following to control the interpolation of the shaper lut
Interpolation FileTransform::getShaperInterpolation() const;
void FileTransform::setShaperInterpolation(Interpolation interp);
bool FileTransform::hasShaper();
Also these seem a bit smelly and very format specific in the current
interface.
FileTransform::getCCCId();
FileTransform::setCCCId(const char * id);
Could this be replaced with some kind of key, type, value interface.
Maybe along the lines of (I would think the order of vars/parameters would
be important for ui's which is why it's all index based):
int FileTransform::getNumVars();
VarType FileTransform::getVarType(int index);
const char * FileTransform::getVarName(int index);
int FileTransform::getVarIndex(const char * name);
void FileTransform::setStringVar(int index, const char * value);
const char * FileTransform::getStringVar(int index);
void FileTransform::setFloatVar(int index, float value);
float FileTransform::getFloatVar(int index);
enum VarType
{
        VAR_TYPE_NONE = 0,
        VAR_TYPE_STRING = 1,
        VAR_TYPE_FLOAT = 2
};
eg.
--snip--
int index = ptr->getVarIndex("CCCId");
if(ptr->getVarType(index) == VAR_TYPE_STRING)
ptr->setStringVar(, "MYSHOTCCC");
--snip--
This would allow me to move the ICCTransfrom code into a FileTransform impl
which would be nice.
If your down with the idea I'm happy to whip something up when I have
sometime after work.
.malcolm

Join ocio-dev@lists.aswf.io to automatically receive all group messages.