Date
1 - 7 of 7
FileTransform interface changes
Malcolm Humphreys <malcolmh...@...>
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
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:
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
Andrew Hunter <and...@...>
Hey Jeremy,
toggle quoted message
Show quoted text
On Thu, Sep 15, 2011 at 8:50 PM, Jeremy Selan <jeremy...@...> wrote:
[...]
to be able to support ICC characterization directly in ocio but that
can wait until it's ready if there exists a means for me to get the
characterization of my display into an ocio supporting app.
[...]
Cheers,
Andrew
[...]
In the meantime, a compromise (suitable for 1.0.0) could be to add anThis would be absolutely suitable for my workflow. It would be ideal
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.
to be able to support ICC characterization directly in ocio but that
can wait until it's ready if there exists a means for me to get the
characterization of my display into an ocio supporting app.
[...]
Cheers,
Andrew
Malcolm Humphreys <malcolmh...@...>
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.
I leave it up to you to decide what makes it into 1.0. At the end of the day I would just be happy to somehow replace these:
FileTransform::getCCCId()
FileTransform::setCCCId(const char * id)
But I think for 2.0 a full blown solution would be a great addition.
Could we find a halfway point?
FileTransform::getOptionString()
FileTransform::setOptionString(const char * id)
This could take a 'token1=value1;token2=value2' string. This is a bit ugly but does allow for the interface to be not so format specific while also allowing to add other options if they are needed for fixing bugs on the path to 2.0.
eg.
FileTransform::setOptionString("CCCId=myfavshot;");
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
I think that is a great idea and should be 2.0 as it's a big interface change. I like it even more after trying to wrap all the transforms types which is a hell of lot of boilerplate code.. (grumble)
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?
yep just for the ui.
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'.
I would hope this would be just a straight FileTransform rather than a separate *Transform.
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 was more thinking of being able to add it in a 1.xx minor version rather than it being ready for 1.0.
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.
Well I'm all for that as it's originally what I suggested :) it would be good if this API also extended to adding additional file formats.
I would hope the long term goal would be to roll new formats into the core but this would allow adding/modifying existing support.
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.
Sure, you kind can do that now as Ben has just posted so it's not really a big issue for 1.0 for me.
.malcolm
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
>
Kai-Uwe Behrmann <ku...@...>
Am 15.09.11, 17:50 -0700 schrieb Jeremy Selan:
Btw. the functions above are not present inside lcms' code. Usually lcms runs on a wide scale of systems, from embedded to many flawours of desktop systems. I would be astonished if there where unresolvable or even hard portability problems.
kind regards
Kai-Uwe Behrmann
--
developing for colour management www.behrmann.name + www.oyranos.org
*everywhere*? For example, at SPI we have a vm with an old libstdc++That looks like cmake output. But littleCMS (alias lcms) uses autotools and is written in C not C++. Did you check with lcms' native build system?
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':
Btw. the functions above are not present inside lcms' code. Usually lcms runs on a wide scale of systems, from embedded to many flawours of desktop systems. I would be astonished if there where unresolvable or even hard portability problems.
kind regards
Kai-Uwe Behrmann
--
developing for colour management www.behrmann.name + www.oyranos.org
Jeremy Selan <jeremy...@...>
Would FileTransform::setOptionString(...) introduce the same complexity?
Say you wanted to create the OCIOFileTransform in nuke. In this
system, would you expect to see a single knob, 'optionString', or
multiple knobs (cccid, etc)? If a single knob is exposed, how would
the user know what to set? Would we just provide helptext which
documented the options?
What would the string format be? JSON/YAML?
What keys would be supported in 1.0? shaper interpolation + cccid?
How about normal interpolation?
-- Jeremy
On Fri, Sep 16, 2011 at 2:40 AM, Malcolm Humphreys
<malcolmh...@...> wrote:
Say you wanted to create the OCIOFileTransform in nuke. In this
system, would you expect to see a single knob, 'optionString', or
multiple knobs (cccid, etc)? If a single knob is exposed, how would
the user know what to set? Would we just provide helptext which
documented the options?
What would the string format be? JSON/YAML?
What keys would be supported in 1.0? shaper interpolation + cccid?
How about normal interpolation?
-- Jeremy
On Fri, Sep 16, 2011 at 2:40 AM, Malcolm Humphreys
<malcolmh...@...> wrote:
Could we find a halfway point?
FileTransform::getOptionString()
FileTransform::setOptionString(const char * id)
This could take a 'token1=value1;token2=value2' string. This is a bit ugly
but does allow for the interface to be not so format specific while also
allowing to add other options if they are needed for fixing bugs on the path
to 2.0.
eg.
FileTransform::setOptionString("CCCId=myfavshot;");
Malcolm Humphreys <malcolmh...@...>
Hi,
The shaper interpolation should be what we agreed.
Interpolation FileTransform::getShaperInterpolation() const;
void FileTransform::setShaperInterpolation(Interpolation interp);
bool FileTransform::hasShaper();
The normal interpolation should stay as it is.
Would FileTransform::setOptionString(...) introduce the same complexity?
It would be get to the same end, so yes it would be similar.
Say you wanted to create the OCIOFileTransform in nuke. In this
system, would you expect to see a single knob, 'optionString', or
multiple knobs (cccid, etc)? If a single knob is exposed, how would
the user know what to set? Would we just provide helptext which
documented the options?
That would be up to the client app to parse and display as it wishes, it would be nice in the nuke case to have multiple knobs.
What would the string format be? JSON/YAML?
I would go for something simple as it would be replaced in 2.0 anyway. I would go for a ';' separated list of key=value pairs.
What keys would be supported in 1.0? shaper interpolation + cccid?
How about normal interpolation?
For now it would be just be 'cccid'. This just allows to add additional options between 1.0 and 20 if it's needed with out changing the public interface.
.malcolm
-- Jeremy
On Fri, Sep 16, 2011 at 2:40 AM, Malcolm Humphreys
<malcolmh...@...> wrote:
>
> Could we find a halfway point?
> FileTransform::getOptionString()
> FileTransform::setOptionString(const char * id)
>
> This could take a 'token1=value1;token2=value2' string. This is a bit ugly
> but does allow for the interface to be not so format specific while also
> allowing to add other options if they are needed for fixing bugs on the path
> to 2.0.
>
> eg.
> FileTransform::setOptionString("CCCId=myfavshot;");