ocio-dev@lists.aswf.io

Review: Support for per-shot looks

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

Commits:
1a54ec8d76

Resolves Issue #16:
http://github.com/imageworks/OpenColorIO/issues#issue/16

This commit enables color transforms to leverage environment variables
+ search paths to determine the lut location.  This will enable
workflows where the looks changes across different
shots/sequences/etc.   For apps which work in the context of a single
shot, no changes are required.  And, for apps which span multiple
shots (such as a playback utility), a mechanism is provided to update
the variables at runtime.

We introduce a new class, OCIO::Context, that is responsible for all
of the work involved in turning a short filename into full path. (it
handles both envvar interpolation and search paths).  Profiles, on
construction, automatically get a default context which contains the
full environment.   New versions of getProcessor have been added that
take a context as an argument, allowing the default context to be
overridden at runtime.  If no context is provided, the default is
used.

-- Jeremy

Re: Review: Add config.sanityCheck

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

#320

Thanks, I'm going to check this in then.

-- Jeremy

On Wed, Dec 22, 2010 at 3:07 PM, Malcolm Humphreys
<malcolmh...@...> wrote:
On 23/12/2010, at 5:38 AM, Jeremy Selan wrote:
Either of our approaches will work, of course, but I'm still inclined
to go with the exception route.

A bunch of calls in OCIO can throw exceptions (the common one being
config->getProcessor(...) ), so most of the time you'll already have
try / catch blocks.  You can usually thus add a "sanityCheck" call in
a single extra line.

If we instead added: bool isValid(const char * msg)
this requires a separate code block to explicitly check it, probably 3
or 4 lines of code.   Which isn't a problem, just an observation.

I'm also not the biggest fan of functions with two return arguments.
Consider the python API... If we go the exception route, it ports
naturally.   If we use an isValid call, would it return the tuple
"(bool, str)"?  People would probably expect it to just return a bool.yeah I guess you would either need to do that or have some config.getErrorMsg() function.

Perhaps you object to the name 'sanityCheck'? (Which I admit is a bit
weird).  How about 'validate', instead?No I think it's ok esp if people are used to the ilm code.

The ilm code does  call header.sanityCheck when using InputFile (), OutputFile(), TiledInputFile(), TiledOutputFile().

OCIO::GetCurrentConfig() will always return a valid profile object.
It currently will throw an exception only iff $OCIO is defined, but
points to a profile that has parse problems.

I'd prefer to not enforce 'auto validation' after loading at the library level.I would say throwing inside sanityCheck is fine then.

If you think having a pointer to a profile which has problems is needed for something else then not throwing from in GetCurrentConfig() is also ok.

.malcolm

Re: Review: Add config.sanityCheck

Malcolm Humphreys <malcolmh...@...>

#321

On 23/12/2010, at 5:38 AM, Jeremy Selan wrote:
Either of our approaches will work, of course, but I'm still inclined
to go with the exception route.

A bunch of calls in OCIO can throw exceptions (the common one being
config->getProcessor(...) ), so most of the time you'll already have
try / catch blocks.  You can usually thus add a "sanityCheck" call in
a single extra line.

If we instead added: bool isValid(const char * msg)
this requires a separate code block to explicitly check it, probably 3
or 4 lines of code.   Which isn't a problem, just an observation.

I'm also not the biggest fan of functions with two return arguments.
Consider the python API... If we go the exception route, it ports
naturally.   If we use an isValid call, would it return the tuple
"(bool, str)"?  People would probably expect it to just return a bool.yeah I guess you would either need to do that or have some config.getErrorMsg() function.

Perhaps you object to the name 'sanityCheck'? (Which I admit is a bit
weird).  How about 'validate', instead?No I think it's ok esp if people are used to the ilm code.

The ilm code does  call header.sanityCheck when using InputFile (), OutputFile(), TiledInputFile(), TiledOutputFile().

OCIO::GetCurrentConfig() will always return a valid profile object.
It currently will throw an exception only iff $OCIO is defined, but
points to a profile that has parse problems.

I'd prefer to not enforce 'auto validation' after loading at the library level.I would say throwing inside sanityCheck is fine then.

If you think having a pointer to a profile which has problems is needed for something else then not throwing from in GetCurrentConfig() is also ok.

.malcolm

Re: Review: Add config.sanityCheck

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

#319

Either of our approaches will work, of course, but I'm still inclined
to go with the exception route.

A bunch of calls in OCIO can throw exceptions (the common one being
config->getProcessor(...) ), so most of the time you'll already have
try / catch blocks.  You can usually thus add a "sanityCheck" call in
a single extra line.

If we instead added: bool isValid(const char * msg)
this requires a separate code block to explicitly check it, probably 3
or 4 lines of code.   Which isn't a problem, just an observation.

I'm also not the biggest fan of functions with two return arguments.
Consider the python API... If we go the exception route, it ports
naturally.   If we use an isValid call, would it return the tuple
"(bool, str)"?  People would probably expect it to just return a bool.

Perhaps you object to the name 'sanityCheck'? (Which I admit is a bit
weird).  How about 'validate', instead?

OCIO::GetCurrentConfig() will always return a valid profile object.
It currently will throw an exception only iff $OCIO is defined, but
points to a profile that has parse problems.

I'd prefer to not enforce 'auto validation' after loading at the library level.

-- Jeremy

Re: Review: removed config.getEditableColorSpace

Malcolm Humphreys <malcolmh...@...>

#318

LGTM

Show quoted text

On 22 Dec, 2010,at 09:26 AM, Jeremy Selan <jere...@...> wrote:

This checkin removed configgetEditableColorSpace(...). Users should
use the existing getColorSpace, and addColorSpace as replacements.

getEditableColorSpace has always been a wart on the OCIO API, as this
is the only function that lets you manipulate the contents of a
configuration behind its back (not using a "set" calls). Removing
this function now lets us perform internal caching with 100%, where
all changes (such as addColorSpace) will clear caches as appropriate.

Commits:
03301d29018

-- Jeremy

Re: Review: Add config.sanityCheck

Malcolm Humphreys <malcolmh...@...>

#317

On 21 Dec, 2010,at 02:23 PM, Jeremy Selan <jere...@...> wrote:

Your idea would also work. Not sure if either is better.

sanityCheck() was inspired by OpenEXR, which has a function of the
same name, and performs a similar action. Guess I just had that model
in mind when writing this.

Are you against exceptions?

Not really.

Would you ever expect to call OCIO::GetCurrentConfig() and get back a pointer to an invalid profile? Throwing an exception from inside GetCurrentConfig() feels sensible, while throwing in sanityCheck() just feels a bit strange ie.
...
OCIO::ConstConfigRcPtr config = OCIO::GetCurrentConfig();
try {
config->sanityCheck();
} catch(...) {
...

vs

OCIO::ConstConfigRcPtr config = OCIO::GetCurrentConfig();
if(config->sanityCheck(msg)) {
...

vs

try {
OCIO::ConstConfigRcPtr config = OCIO::GetCurrentConfig();
} catch(...) {
...
if(config->sanityCheck(msg)) {
...

But I do like the idea of a sanityCheck type of function for when we are building a profile in code and need to check it is vaild.

If we went with your idea, why an enum instead of a bool?
"bool isValid()"

I would expect users to only want to know if there is an error or not.
Under what circumstances would a user be returned the "UNKNOWN" value?
The reason I have 3 states internally is to know if i've computed the
value, and if so, if it succeeded or not. The internal 'unknown'
state is just an implementation detail, the user is not aware of it.

a bool is even better.

.malcolm

Re: Adding multiple bit depths to one <!ColorSpace>

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

#316

Malcolm - interesting idea!

I do agree that right now families feel a bit like a second-class
concept, and I would like them to be easier to deal with.  I also
agree that it's appealing to present the user as short a list of colorspaces,
as much as possible, and not to burden them with bitdepth options if we
can avoid it.

(I'm not sure this relates to contexts.)

One side note: currently family names *are* used as an
optimization within the library, and it's assumed that all transformed
to/from colorspaces of the same family can be considered a no-op.
I.e., if you define an image as being in the 'lg10' space, and ask to
transform it to 'lgf', it will determine that both spaces are in the
same family and no work will be done.  This is actually a nice
optimization for a lot of reasons related to internal performance.

So, Joseph, in the case of of the IIF's adx10 vs adx16, these would
currently need to be separate families anyways.  (In the Academy Density
Encoding spec (ADX), the 10 and the 16 bit representation differ in
more than just precision, the 16-bit spec also encodes a greater
density range).

I'd like to consider this further, but the next big API extension I'm
looking at is the per-shot look / "Context" stuff.   Let's bring it up
again when that is complete.. (i.e., in a few weeks)

-- Jeremy

On Thu, Dec 16, 2010 at 3:05 PM, Malcolm Humphreys
<malcolmh...@...> wrote:
Hi did you guys have any more thoughts on this? Jeremy do you see this
fitting in somehow with contexts?

.malcolm

Review: removed config.getEditableColorSpace

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

#315

This checkin removed config.getEditableColorSpace(...).  Users should
use the existing getColorSpace, and addColorSpace as replacements.

getEditableColorSpace has always been a wart on the OCIO API, as this
is the only function that lets you manipulate the contents of a
configuration behind its back (not using a "set" calls).  Removing
this function now lets us perform internal caching with 100%, where
all changes (such as addColorSpace) will clear caches as appropriate.

Commits:
03301d29018

-- Jeremy

Re: Review: Add config.sanityCheck

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

#313

Your idea would also work. Not sure if either is better.

sanityCheck() was inspired by OpenEXR, which has a function of the
same name, and performs a similar action. Guess I just had that model
in mind when writing this.

Are you against exceptions?

If we went with your idea, why an enum instead of a bool?
"bool isValid()"

I would expect users to only want to know if there is an error or not.
Under what circumstances would a user be returned the "UNKNOWN" value?
 The reason I have 3 states internally is to know if i've computed the
value, and if so, if it succeeded or not.  The internal 'unknown'
state is just an implementation detail, the user is not aware of it.

-- Jeremy

Re: Review: Add config.sanityCheck

Malcolm Humphreys <malcolmh...@...>

#314

Could we call this config.validate(), I know we are all a bit insane ;)

enum ConfigValid
{
    CONFIG_UNKNOWN = 0,
    CONFIG_VALID,
    CONFIG_INVALID
}

What do you think about not throwing an exception but returning the enum and the error text?

something like:
OCIO::ConstConfigRcPtr config = OCIO::GetCurrentConfig();
if(config.validate(&errstr) == CONFIG_INVALID)
// do something sane

.malcolm

Show quoted text

On 21 Dec, 2010,at 01:26 PM, Jeremy Selan <jere...@...> wrote:

This will prevent the issues Blake encountered from happening again.
http://groups.google.com/group/ocio-dev/browse_thread/thread/c073e0f4fde995f7

This also addresses an outstanding issue:
https://github.com/imageworks/OpenColorIO/issues#issue/17

* Configs get a 'sanityCheck' call, which will validate the
configuration and throw an exception if any errors are detected. An
example of an error condition would be a role reference to a
colorspace that does not exist. Repeated calls to sanityCheck are
cached (and fast).

* Nuke plugins call sanityCheck() before the getProcessor() call.

* getColorSpace call is now not sensitive to case differences. Aka,
linear == LINEAR.

* The default role is only returned as the colorspace of 'last resort'
if strict_parsing is explicitly disabled. This will prevent
colorspace errors to silently roll by, unless requested.

Commits:
2ae25fc437
993773f9ad
3607e94d29

-- Jeremy

Review: Add config.sanityCheck

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

#312

This will prevent the issues Blake encountered from happening again.
http://groups.google.com/group/ocio-dev/browse_thread/thread/c073e0f4fde995f7

This also addresses an outstanding issue:
https://github.com/imageworks/OpenColorIO/issues#issue/17

* Configs get a 'sanityCheck' call, which will validate the
configuration and throw an exception if any errors are detected. An
example of an error condition would be a role reference to a
colorspace that does not exist.  Repeated calls to sanityCheck are
cached (and fast).

* Nuke plugins call sanityCheck() before the getProcessor() call.

* getColorSpace call is now not sensitive to case differences.  Aka,
linear == LINEAR.

* The default role is only returned as the colorspace of 'last resort'
if strict_parsing is explicitly disabled.  This will prevent
colorspace errors to silently roll by, unless requested.

Commits:
2ae25fc437
993773f9ad
3607e94d29

-- Jeremy

Re: Testing Nuke Plugins

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

#311

An updated nuke-default profile has been posted that should resolve
all your issues:
http://sites.google.com/site/opencolorio/downloads

Sorry for the trouble!

(I'll be submitting a patch later today to avoid the issue in the future.)

-- Jeremy

Re: Testing Nuke Plugins

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

#310

Blake,
Thanks for the testing!

* Glad to hear OCIOFileTransform is working.  My goal is to support
(at a minimum) all of Nuke's lut formats in the near future.

* We're lucky that the nuke cineon transform happens to match Log2Lin.
The nuke-default OCIO profile is desogned to match all the lut
transforms used in the read-write nodes.  (see
src/testbed/nuke_to_ocio.py for how we generated the nuke-default OCIO
profile).  Nuke's log2lin node does an analytical transform based on
its configurable parameters, so it's only a fortunate circumstance
they match by default.

* OCIODisplay has it's default display transform set to 'None' by
default in the nuke-default profile. If you switch this to srgb you'll
see the node is working.  (This is also why switching input colorspace
has no effect.  If you select 'no processing', no matter what the
input is no work will be done).

* OCIOLogConvert doesn't work because the config.ocio file has a typo
in it.  You can hand edit it to fix the issue.  At the top of the
config is the compositing_log space, which is set to "cineon".  But
down below the colorspace is listed as "Cineon".  (Note the case
difference).  If you fix the role's capitalization everything will
work.

What's happening is that the 'cineon' (lower case) role is not found,
so it's relying on the default role, "data". By design, any conversion
to or from a 'data' space is a no-op, so no processing occurs.

This 'silent error' is a critical mistake; I'll get in the appropriate
library fixes ASAP making sure this doesn't bite any other people.
I'll also update the Nuke profile today to fix the errors as well.

Thanks!

-- Jeremy

Show quoted text

On Mon, Dec 20, 2010 at 2:04 PM, bsloan <bsl...@...> wrote:
Gah. Correction:

The OCIODisplay node seems to ignore the 'input colorspace' knob.
Display transform and exposure work as expected.

As for the LogConvert node, switching the operation from loglin to
linlog seems not to change anything.

Thanks again,

-blake

On Dec 20, 1:42 pm, bsloan <bsl...@...> wrote:
Hi all,

I've tested the OCIO 0.7.3 Nuke suite in Nuke 6.1v3 and have noticed
some possible bugs (or user errors ).

First of all, I downloaded the configs for 0.7 and pointed my OCIO env
var to the nuke-default/configs.ocio.

Then I start Nuke and create an instance of each of the four plugins:

Display
File_Transform
LogConvert
ColorSpace

OCIOFile_Transform worked well with the .csp I fed it (Bravo)
OCIOColorspace did the right thing with my cineon image (identical
result to Nuke's vanilla LogLin (Yippee!))

The other two, OCIODisplay and OCIOLogConvert seem to have no effect
(regardless of the values I set in the controls). Could I be doing
something wrong?

BTW, thanks Jeremy et al for all the hard work.

-blake

Re: Testing Nuke Plugins

bsloan <bsl...@...>

#309

Gah. Correction:

The OCIODisplay node seems to ignore the 'input colorspace' knob.
Display transform and exposure work as expected.

As for the LogConvert node, switching the operation from loglin to
linlog seems not to change anything.

Thanks again,

-blake

Show quoted text

On Dec 20, 1:42 pm, bsloan <bsl...@...> wrote:
Hi all,

I've tested the OCIO 0.7.3 Nuke suite in Nuke 6.1v3 and have noticed
some possible bugs (or user errors ).

First of all, I downloaded the configs for 0.7 and pointed my OCIO env
var to the nuke-default/configs.ocio.

Then I start Nuke and create an instance of each of the four plugins:

Display
File_Transform
LogConvert
ColorSpace

OCIOFile_Transform worked well with the .csp I fed it (Bravo)
OCIOColorspace did the right thing with my cineon image (identical
result to Nuke's vanilla LogLin (Yippee!))

The other two, OCIODisplay and OCIOLogConvert seem to have no effect
(regardless of the values I set in the controls). Could I be doing
something wrong?

BTW, thanks Jeremy et al for all the hard work.

-blake

Testing Nuke Plugins

bsloan <bsl...@...>

#308

Hi all,

I've tested the OCIO 0.7.3 Nuke suite in Nuke 6.1v3 and have noticed
some possible bugs (or user errors ).

First of all, I downloaded the configs for 0.7 and pointed my OCIO env
var to the nuke-default/configs.ocio.

Then I start Nuke and create an instance of each of the four plugins:

Display
File_Transform
LogConvert
ColorSpace

OCIOFile_Transform worked well with the .csp I fed it (Bravo)
OCIOColorspace did the right thing with my cineon image (identical
result to Nuke's vanilla LogLin (Yippee!))

The other two, OCIODisplay and OCIOLogConvert seem to have no effect
(regardless of the values I set in the controls). Could I be doing
something wrong?


BTW, thanks Jeremy et al for all the hard work.

-blake

Re: Adding multiple bit depths to one <!ColorSpace>

Malcolm Humphreys <malcolmh...@...>

#307

Hi did you guys have any more thoughts on this? Jeremy do you see this fitting in somehow with contexts?

.malcolm

Show quoted text

On 14 Dec, 2010,at 01:35 PM, Malcolm Humphreys <malcol...@...> wrote:

I like it. However I'd like to keep the option for a family designation. For simple colorspaces I see this as great way to simplify things. The family designation is still useful for instances like in IIF:ACES where the 10 and 16 bit ADX transformations are significantly different.

Yeah I can see 'family' would be really useful for grouping colorspaces based on purpose which could be as abstract as the profile builder would like. In the examples I couldn't see a case where family wasn't being used for bit-depth, but I agree it should stay. I agree if a bitdepth change significantly changes the transform required, this would suggest a different !<ColorSpace>.

For differences in bitdepth, I guess the usual case will be buffer A at a certain depth and buffer B at a certain depth, give me the transform from color space A -> B.

I was thinking something along the lines of:
ConstProcessorRcPtr processor = config->getProcessor(
    OCIO::ROLE_COMPOSITING_LOG,
    OCIO::ROLE_SCENE_LINEAR,
    OCIO::BIT_DEPTH_UINT10,
    OCIO::BIT_DEPTH_F16);

This would give you back the correct 10ui log -> f16 linear, without needing to mess around with colorspace names. This would likely end up looking a bit different with the 'context' ideas Jeremy and I chatted about.

Under the hood the colorspace could define the family implicitly from the name.

The SPI examples use the family designation in a very simple way. I could see a reason to place all video space transforms under a single family, for example. This could results in a family with 2 colorspaces that are named differently with the same bit depth.

Could you see a colorspace existing in two families? maybe we could have ',' separated tags which would allow for some powerful grouping of colorspace, I'm guessing families are mostly used for UI purposes, ie give me list of all 'video' color spaces. These would be similar to 'tag clouds' www people use to have items in multiple categories / groups.

eg.
- !<ColorSpace>
   name: vdhd
   family: video, hd
   ...

- !<ColorSpace>
   name: vdntsc
   family: video, ntsc
   ...

Maybe that's just a little bit two much over-engineering, for simple grouping.

.malcolm

0.7.3 Released

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

#306

Version 0.7.3 (Dec 16 2010):
    * Added example applications: ociodisplay, ocioconvert
    * Makefile: Add boost header dependency
    * Makefile: Nuke plugins are now version specific
    * Fixed bug in GLSL MatrixOp

-- Jeremy

Re: Review: Added example image viewer

Malcolm Humphreys <malcolmh...@...>

#305

LGTM

Show quoted text

On 15 Dec, 2010,at 06:46 AM, Jeremy Selan <jere...@...> wrote:

I've added a working GLSL image viewer example.

It relies on OpenImageIO to load an arbitrary image, and then will
display it using OpenColorIO. It's pretty awesome to see how little
code in practice is needed to implement a full-fledged monitor
interface!
(Including fstop exposure controls, channel swizzling, etc).

For those interested in adding OCIO natively to existing viewers, this
is a great place to start

Commits:
18ac496924e06
899b272d710ea

Caveot : this doesn't yet work with the CMake makefile, I hope to get
that added soon.

-- Jeremy

Re: Review: updated readme and usage example

Malcolm Humphreys <malcolmh...@...>

#304

LGTM

Show quoted text

On 15 Dec, 2010,at 06:42 AM, Jeremy Selan <jere...@...> wrote:

Commits:

1c2d95e82e

Not much to this one...

-- Jeremy

Re: Review: Bugfix for GLSL GPU

Malcolm Humphreys <malcolmh...@...>

#303

LGTM

Show quoted text

On 15 Dec, 2010,at 05:40 AM, Jeremy Selan <jere...@...> wrote:

Commits:
54b464d255

Fixed the bug where the application of MatrixOps on the GPU (for GLSL
only) had the wrong multiplication order.

-- Jeremy