Date   

Re: Review: Disable envvar unit tests

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

hmm thats a bit weird the test should be creating all the files it is looking for.

--snip--
std::string lut2(two_dir+"somelut2.lut");

std::ofstream somelut2(lut2c_str());
somelut2.close();
--snip--

I'm guessing
OCIO_TEST_AREA env isn't being set correctly, i'll check it out when I am back. But I'm guessing this might change with the work you are doing with the globals.

.malcolm


On 06 Nov, 2010,at 11:22 AM, Jeremy Selan <jere...@...> wrote:

Unit tests for envvars were failing, referencing files that weren't
checked into the source tree. Disabling these specific tests for now.

Commits:
0a052d9090c9d693f63cdb409260627406790d75

-- Jeremy


Review: Updated Nuke OCIOFileTransform

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

Added explicit control of transform direction + interpolation to the
Nuke OCIOFileTransform node.

Commits:
3143ccdb38fee9ec774a1be94150c8f04d9e70c5

-- Jeremy


Re: Review: Lut1D optimization

Rod Bogart <bog...@...>
 

I guess my preference would be to allow someone to ask for
optimization or not. In the glorious future when OCIO is involved in
the creation of every pixel in every movie, the animated CTL example
may be rather common, and an option to avoid clever internal
heuristics might be a good debugging and processing tool.

RGB

On Wed, Nov 10, 2010 at 4:15 PM, Jeremy Selan <jeremy...@...> wrote:
Man, I can't get anything past you guys!   :)

The clamp was omitted on purpose, but I could probably be convinced
either way on this issue.*

If this code were inside a compiler, there would be no discussion -
clamp would be the only correct answer.

But ... in the context of writing a color processing library I would
propose that we have a bit of leeway here.  I tend to have a strong
distaste for proactively clamping outputs. Why destroy data if you
don't need to?  Who is to say that the extra data (previously
scheduled to be clamped) couldn't prove useful further down the
pipeline in certain circumstances?   Clamping is easy to add
downstream if/when needed.  I.e., if a subsequent image processing
operation depends on a clamped input in order to produce sensible
results, it can can always do it at that time.  And if it's not
needed, even better!

Our implementation of ASC CDL SOP already does this.

// out = clamp( (in * slope) + offset ) ^ power
// Note: the clamping portion of the CDL is only applied if
// a non-identity power is specified.

The same rationale applies.   The CDL transformation math strictly
requires a clamp between [0,1], making it useless on HDR data.
However, in practice many portions of the math (gain, sat) work great
on HDR data so it seems unnecessary to always clamp the output.  We
thus only clamp if the exponent potion is specified, which was the
intent as specified in the CDL docs).


The downside of this approach (in both the lut1d case, and the cdl
case) is that if someone ends up relying on either of these behaviors,
they are in a semi-precarious position.  I.e., their configuration is
sitting with magic numbers at certain values that happen to not
introduce this clamping, and this behavior will probably be
discontinuous (unintuitive?) over the parameter space.   Picture an
animating CDL exponent, where it goes from [0.95, 1.05].  For some
portion of the range there will be clamping, then no clamping, then
clamping again.

I agree this is not ideal, and that a sensible individual may prefer
the alternative, but this downside is a price I'm inclined to pay for
the benefit of preserving data in the commoner cases.

-- Jeremy


*(Technically the clamped range is not 0,1 but instead
[lut1d.from_min, lut1d.from_max] but this is inconsequential to the
overall point).


On Wed, Nov 10, 2010 at 3:14 PM, Rod Bogart <bog...@...> wrote:
Is this semantically correct if the presence of the 1D lut also
triggers clipping to 0-1?  Is there now a missing clamp?

RGB

On Wed, Nov 10, 2010 at 1:56 PM, Jeremy Selan <jeremy...@...> wrote:
On Lut loading, 1D luts are checked to determine if the underlying
transform represents 'identity'.  If so, they are skipped during op
generation.  This is particularly useful when working with 3D luts
that have shaper luts, which are often left as identity.

This check is added to the lut1d-finalize call, which takes a
'tolerance' as an arg as the underlying file quantization is file
dependent.

Commits:
177bf402f6ff95ba70224aa4466e7773c5a89f96

-- Jeremy


Re: Review: Lut1D optimization

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

Man, I can't get anything past you guys! :)

The clamp was omitted on purpose, but I could probably be convinced
either way on this issue.*

If this code were inside a compiler, there would be no discussion -
clamp would be the only correct answer.

But ... in the context of writing a color processing library I would
propose that we have a bit of leeway here. I tend to have a strong
distaste for proactively clamping outputs. Why destroy data if you
don't need to? Who is to say that the extra data (previously
scheduled to be clamped) couldn't prove useful further down the
pipeline in certain circumstances? Clamping is easy to add
downstream if/when needed. I.e., if a subsequent image processing
operation depends on a clamped input in order to produce sensible
results, it can can always do it at that time. And if it's not
needed, even better!

Our implementation of ASC CDL SOP already does this.

// out = clamp( (in * slope) + offset ) ^ power
// Note: the clamping portion of the CDL is only applied if
// a non-identity power is specified.

The same rationale applies. The CDL transformation math strictly
requires a clamp between [0,1], making it useless on HDR data.
However, in practice many portions of the math (gain, sat) work great
on HDR data so it seems unnecessary to always clamp the output. We
thus only clamp if the exponent potion is specified, which was the
intent as specified in the CDL docs).


The downside of this approach (in both the lut1d case, and the cdl
case) is that if someone ends up relying on either of these behaviors,
they are in a semi-precarious position. I.e., their configuration is
sitting with magic numbers at certain values that happen to not
introduce this clamping, and this behavior will probably be
discontinuous (unintuitive?) over the parameter space. Picture an
animating CDL exponent, where it goes from [0.95, 1.05]. For some
portion of the range there will be clamping, then no clamping, then
clamping again.

I agree this is not ideal, and that a sensible individual may prefer
the alternative, but this downside is a price I'm inclined to pay for
the benefit of preserving data in the commoner cases.

-- Jeremy


*(Technically the clamped range is not 0,1 but instead
[lut1d.from_min, lut1d.from_max] but this is inconsequential to the
overall point).

On Wed, Nov 10, 2010 at 3:14 PM, Rod Bogart <bog...@...> wrote:
Is this semantically correct if the presence of the 1D lut also
triggers clipping to 0-1?  Is there now a missing clamp?

RGB

On Wed, Nov 10, 2010 at 1:56 PM, Jeremy Selan <jeremy...@...> wrote:
On Lut loading, 1D luts are checked to determine if the underlying
transform represents 'identity'.  If so, they are skipped during op
generation.  This is particularly useful when working with 3D luts
that have shaper luts, which are often left as identity.

This check is added to the lut1d-finalize call, which takes a
'tolerance' as an arg as the underlying file quantization is file
dependent.

Commits:
177bf402f6ff95ba70224aa4466e7773c5a89f96

-- Jeremy


Re: Review: Lut1D optimization

Rod Bogart <bog...@...>
 

Is this semantically correct if the presence of the 1D lut also
triggers clipping to 0-1? Is there now a missing clamp?

RGB

On Wed, Nov 10, 2010 at 1:56 PM, Jeremy Selan <jeremy...@...> wrote:
On Lut loading, 1D luts are checked to determine if the underlying
transform represents 'identity'.  If so, they are skipped during op
generation.  This is particularly useful when working with 3D luts
that have shaper luts, which are often left as identity.

This check is added to the lut1d-finalize call, which takes a
'tolerance' as an arg as the underlying file quantization is file
dependent.

Commits:
177bf402f6ff95ba70224aa4466e7773c5a89f96

-- Jeremy


Review: Lut1D optimization

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

On Lut loading, 1D luts are checked to determine if the underlying
transform represents 'identity'. If so, they are skipped during op
generation. This is particularly useful when working with 3D luts
that have shaper luts, which are often left as identity.

This check is added to the lut1d-finalize call, which takes a
'tolerance' as an arg as the underlying file quantization is file
dependent.

Commits:
177bf402f6ff95ba70224aa4466e7773c5a89f96

-- Jeremy


Review: Added Truelight .cub support

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

Added support for Truelight .cub luts. (Both 1D shaper + 3d lut). 1D
shaper lut currently uses linear interpolation, until something better
comes along.

Note: This checkin tested with a limited number of luts, all written
out from Nuke. If anyone has any non-nuke examples of .cub luts I
would love to test them! Thanks!

Commits:
4308349e005866b6c32c1cc6e8aba96c6309ad2c
cf885ba396a41f3a9718d5fdc9b1bc4959204963

-- Jeremy


Review: Config.roles data storage is map

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

The list of defined roles within a config previously maintained order.
Conceptually, this is actually a mapping type where order does not
matter (and the yaml serialization dropped ordering). This switches
the underlying storage to use a map instead.

Commits:
3942e6d8bd72b721b23d3e9991e434f2537a2759


Review: EnvExpand optimization

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

Minor optimization: the potentially expensive EnvExpand call (which
expands envvars in file paths), does an early exit for paths which do
not contain special characters. ($ + %)

Commits:
38d53d46239ec3f7f090509f2273edbedd703d7e

-- Jeremy


Re: Review: CSP bugfix

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

In the absence of any objections, I will be merging this (and the
previous commits) into OCIO.

-- Jeremy


Re: Review: Inital pass at searchpath impl

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

Malcolm, I like your suggestions.

My thoughts:

* The name 'globals' is good.

* I like pre-declaring the list of allowed variables beforehand for the config.

* I like the use of env-vars as defaults. This seems very user-friendly.

* I like supporting only strings for now. In the future, I can also
see numbers being supported (such as allowing for CDL blocks that rely
on envvars), but this can be added as needed.

* The FileTransform will probably have to get a bit smarter. Say
you're using a pershot lut, and for some shots you dont want any such
table. This could be solved by either not installing a lut in the
expected location, or by installing an identity lut. Which is
preferable? In the current implementation, if a lut is not found
this is always an error condition. Maybe we should introduce on the
FilmTransform a 'silent error mode', where this would just skip the
application of that specific LUT?

* The "globals" pre-declaration could also define an optional default
(used if the envvar is not set). If the envvar is not set, and no
default is specified, any references to the global variable would
probably be an error.

* I have some apprehensions about the config->getGlobal,
config->setGlobal implementation. My primary concern is that the
GlobalConfig() is const (by design), so clients couldnt actually call
setGlobals on the config in use. Instead, they would have to do
something like:

c = GetGlobalConfig().createEditableCopy()
c.setGlobals(...)
SetGlobalConfig( c )

This is a lot of copying, particularly as the globals often are tied
to the clip. If you're A/B-ing two clips in an image viewer,
conceptually there really is only one config, and two sets of globals.
Why force the client to copy and have duplicate configs?

I will mockup some ideas which solve this and see what the possible
solutions look like.

-- Jeremy


Review: CSP bugfix

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

Upon further testing, it appears the channel swizzling was backwards
during CSP lut importing.

I used Nuke as the baseline in terms of lut channel order, so it's
possible that I've actually broken things if Nuke's CSP Vector node is
broken. But if Nuke's works, OCIO's now agrees with the results.
Can anyone confirm Nuke's Vectorfield implementation of .csp files
works?

I am including the file I used for testing. This gains down the red
channel by 0.5 - green and blue are unchanged.

Commits:
d483d6a3660cec5cafa6868294f3cb62c05f4472

I've confirmed that our 3dl handing is correct.

-- Jeremy


Review: Added two additional Nuke OCIO nodes

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

* LogConvert, which is a handy node that does the colorspace transform
between the SCENE_LINEAR role, and the COMPOSITING_LOG role. While
this isnt strictly needed (it could be accomplished using the
ColorSpace node) it's convenient for the artists to have so they dont
have to worry about all the options. They can just think, "give me
the right log to lin".

* FileTransform, which allows for the application of individual files
(i.e., luts). While not needed in general use, this will let users
apply any lut that OCIO knows about, independent of the normal OCIO
workflow.

Commits:
27371ec44b878ae787326c89405400ee22dfa1c8
745a7c495b4b3e5ab3faab17e5a40dbb3325d4f0

-- Jeremy


Review: Switched auto_ptr -> ptr

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

In the public API we were storing references to m_impl using autoptrs,
which really was unnecessary. This only saved one line of code per
use, and made the memory model less obvious. This doesnt affect user
code at all, as it's impl related anyways.

Commits:
089aaa9297dfedffa59c2998bf4111cd65d53c5d

-- Jeremy


Review: Disable envvar unit tests

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

Unit tests for envvars were failing, referencing files that weren't
checked into the source tree. Disabling these specific tests for now.

Commits:
0a052d9090c9d693f63cdb409260627406790d75

-- Jeremy


Re: Idea: Remove boost:shared_ptr

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

Definitely,

I went to add tr1 on mac, and realized we're already doing it. From
export/OpenColorIO/OpenColorTypes.h,

#ifdef __APPLE__
#include <tr1/memory>
#define OCIO_SHARED_PTR std::tr1::shared_ptr
#define OCIO_DYNAMIC_POINTER_CAST std::tr1::dynamic_pointer_cast
#else
#include <boost/shared_ptr.hpp>
#define OCIO_SHARED_PTR boost::shared_ptr
#define OCIO_DYNAMIC_POINTER_CAST boost::dynamic_pointer_cast
#endif

So I'll have to see why this didnt work on OSX without boost. Looks
like it all should have worked...

-- Jeremy


Re: Idea: Remove boost:shared_ptr

Colin Doncaster <colin.d...@...>
 

Would

#include <tr1/memory>

be an option for now?

On 03-11-2010, at 6:00 AM, Malcolm Humphreys wrote:

+1 it should be pretty easy to add boost back as a dependency if we need to. I have seen a lot of projects copy the shared_ptr so it seems fairly common practice.

.malcolm

On 03 Nov, 2010,at 09:29 AM, Jeremy Selan <jeremy...@...> wrote:

Would anyone be against us copying the shared_ptr implementation
internally into OCIO? That would let us make the OCIO boost
dependency optional, which would let it compile on a plain-vanilla OSX
install. The only other part of the code that relies on boost is the
testing framework, which is already if def'd. So if the user has
boost, you'll get the testing If the user doesnt have boost, it will
skip testing.

Thoughts?

-- Jeremy


Re: Review: Inital pass at searchpath impl

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

Hi,

I'm going to be out of town for the remainder of the week, perhaps we
could try an internet call early next week to discuss these specifics
further? (Skype, iChat, etc). Many of these points can probably be
hashed out easier live, and then we can summarize our thoughts back to
the list.
 
It might have to wait a few weeks, I'm in Hong Kong right now and I'm going to be MIA in asia for a bit.

But yes a chat would be good when we are both free.

Your approach to implementing per-shot looks - using environment
variables within search paths - is clever. However, I'm not sold
The portion I'm concerned about is the use of environment variables as
a communications mechanism.
 
They are a common mechanism for setting up initial state for a lot of apps. This being similar is a nice entry point for integration with existing systems.

But I agree the env vars should only be used to setup the initial state.

Even if it is possible to change this state with setenv()'s I would discourage this use with a better interface for changing state during execution, similar to what you have suggested.

Most places have some form of setshot system which bootstraps the env for a shot, not just for color but things like the required toolchain for that artist session doing task A (I'm guessing you have something similar at SPI).

I can guarantee that I would need to write something that grabbed env's and setup an initial state through which ever interface we settle on. I would prefer that this ocio env wrapper type code was internal rather than each shop doing it slightly differently.

For an application (such as a compositor) where you're not changing
show / shot / sequence at runtime the use of envvars is no big deal.
For example, say you have a per-shot display lut
(/job/$SHOT/.../shotgrade.3dl). You could run a script which sets up
the shot environment, launch the APP, and everything just works.
 
+1

Being able to setup/enfoce the initial state like this is important for multiple dept / group workflows. Who are most likely using diffrent toolchains but require to start with the same consistent viewing env between them.

In this style of setup each person gets pushed the currently approved viewing env for that session and are not concerned with modification of it.

However, consider an image-viewing tool which needs to flipbook
multiple shots (or whole sequences) back to back. I.e., the display
transform, as queried from OCIO, would be different based on the shot
name Would this approach be possible using an envvar mechanism? I
dont believe so.

Neither do I, that would be a bit clumsy for that specific problem.

Envvars are commonly locked down at launch time.
And, even if you could modify it at runtime as playback was in
progress, would you want to? How would you notifiy OCIO that it had
changed? This seems like a tricky implementation, and strikes me as
going down the path of evil.
 
I think a mixture of the two ideas could work well (see below).

I would prefer to promote this concept of 'variables' to be an
explicit communications mechanism, rather then using envvars as a
back-door. Alternate implementation options include:

* a config->setVariable option, where variables were available for
use in color transforms. (They could even retain the $SHOW $SHOT
syntax). This way the OCIO plugin could set it as needed, and with
the python API exposed in client apps the list of variables could even
be amended with facility code, as needed at runtime!
 
I was thinking the same kind of thing, but with the possibility of these grabbing their initial value from the env (if it exists).

I'm also thinking it might be better to be explicit with globals eg. config->addglobal(name) these would end up as a list in the yaml profile. Only the $globals in this list would be expanded and also the only ones initialized from the env. (This does resolve my niggles with the current expand code which has the potential to be a bit slow, if used in many)

These globals could be set/get dynamically at run-time ie config->getglobal(), config->setglobal(). I would prefer to call these globals over variables as it suggests their scope.

Right now I only see a direct use for globals with string parameters and would only want to add support for other types if we had a strong need for them.

As an example this would be nearly a complete list of globals I could see a use for ($JOB, $SCENE, $SEQ, $SHOT, $TASK, $USERGROUP, $BALLANCE_GRADE, $CREATIVE_GRADE, $EDIT_GRADE). I'm sure these have slightly different names per shop but represent the same concepts.

* Alternatively, with a less stateful implementation the variables /
arguments could be passed along in the config->getProcessor call. This
may be preferable to those computer scientists among us.
 
I definitely would like some form of dynamic parameters/variables, but these should be something different to globals.

Either way, this passing of variables as arguments to a transforms is
half of the work towards allowing fully dynamic colorspaces. (The
other 1/2 being the plugin API to define the lists of transforms at
runtime).
 
With the prospect of multiple image sequences / clips with transforms, configs and or grades. For these case are you proposing that we setup a single config and then store some kind of ocio processor relationship per clip?

Perhaps this is good reason to move forward on both ASAP?
 
It would be great to flesh this out as it would be good for client apps not to need to manage the details of this per clip state storage if this is the way we are thinking of going.

.malcolm


Re: Idea: Remove boost:shared_ptr

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

+1 it should be pretty easy to add boost back as a dependency if we need to. I have
seen a lot of projects copy the shared_ptr so it seems fairly common practice.

.malcolm

On 03 Nov, 2010,at 09:29 AM, Jeremy Selan <jere...@...> wrote:

Would anyone be against us copying the shared_ptr implementation
internally into OCIO? That would let us make the OCIO boost
dependency optional, which would let it compile on a plain-vanilla OSX
install. The only other part of the code that relies on boost is the
testing framework, which is already if def'd. So if the user has
boost, you'll get the testing. If the user doesnt have boost, it will
skip testing.

Thoughts?

-- Jeremy


Re: Review: Added missing role enum to python

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

LGTM

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

Trivial addition, exposed ROLE_DEFAULT to python.

commit 82dd5a4666f7c5456361e9ec7d78107742e5b427

-- Jeremy

1961 - 1980 of 2217