ocio-dev@lists.aswf.io

Re: Review: added new yaml-cpp with YAML::Newline feature

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

#198

Is there a reason to not remove yaml-cpp-0.2.5* from ext?

Other than that, looks good to me.

-- Jeremy

On Mon, Oct 25, 2010 at 5:22 AM, Malcolm Humphreys
<malcolmh...@...> wrote:
- added yaml-cpp r443 which contains the new YAML::Newline emitter manipulator
http://stackoverflow.com/questions/3982901/how-to-emit-a-blank-line-using-yaml-cpp
- added some 'out << YAML::Newline' to tryout this feature

http://github.com/malcolmhumphreys/OpenColorIO/commit/0de8ccb6842b223f942789f1ae68d768dab9de29

.malcolm

Re: Review: Inital pass at GCC visibility

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

#197

Looks good to me.
(I tested this, as well as all the related post-email updates).

-- Jeremy

On Thu, Oct 21, 2010 at 1:44 PM, Malcolm Humphreys
<malcolmh...@...> wrote:
- Inital pass at GCC visibility http://gcc.gnu.org/wiki/Visibility
 to hide unnecessary public symbols in an effort to maintain ABI
- Added -fvisibility-inlines-hidden and -fvisibility=hidden to the yaml-cpp patch, YAML::* symbols should now be hidden in libOpenColorIO.so
- Added export/OpenColorIO/OpenColorABI.h
- Added some '#pragma GCC visibility' for some external libs

Before Visibility:
:nm -C -D src/core/libOpenColorIO.so | grep " T " | wc -l
958

After Visibility:
:nm -C -D src/core/libOpenColorIO.so | grep " T " | wc -l
306

http://github.com/malcolmhumphreys/OpenColorIO/commit/aa888b4d4f969e6c903eb73d7c20a9d9f546298f
http://github.com/malcolmhumphreys/OpenColorIO/commit/08b4208717221426ba90058b6324e3358930d376
http://github.com/malcolmhumphreys/OpenColorIO/commit/3ec1be5204291aaf40e56cdbdb1c7c0f757664eb

.malcolm

Reference !<Colorspace>

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

#209

Do you think the 'reference' colorspace should in a separate section in the profile? This would be similar to the PCS in the icc world. It could be an extra line 'reference: !<ColorSpace> ...' before the colorspaces in the profile. The 'reference' name could be reserved so that colorspace -> reference transforms could work the same.

I can see this working as roles with ROLE_REFERENCE but it really feels like a more central OCIO concept than roles and should be separated.

Also looking at the serialization with to_referance and from_referance, I'm thinking it would be easy to read if it was structured like this? what do you think?

--from--
   to_reference: !<GroupTransform>
     children:
       - !<FileTransform> {src: "", interpolation: unknown}
       - !<CineonLogToLinTransform>
         max_aim_density: [2.046, 2.046, 2.046]
         neg_gamma: [0.6, 0.6, 0.6]
         neg_gray_reference: [0.434995, 0.434995, 0.434995]
         linear_gray_reference: [0.18, 0.18, 0.18]
   from_reference: !<GroupTransform>
   children:
     - !<ExponentTransform> {value: [1, 1, 1, 1]}
--from--

--to--
   reference:
     to: !<GroupTransform>
       children:
         - !<FileTransform> {src: "", interpolation: unknown}
         - !<CineonLogToLinTransform>
           max_aim_density: [2.046, 2.046, 2.046]
           neg_gamma: [0.6, 0.6, 0.6]
           neg_gray_reference: [0.434995, 0.434995, 0.434995]
           linear_gray_reference: [0.18, 0.18, 0.18]
     from: !<GroupTransform>
       children:
         - !<ExponentTransform> {value: [1, 1, 1, 1]}
--to--

.malcolm

Review: added new yaml-cpp with YAML::Newline feature

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

#207

- added yaml-cpp r443 which contains the new YAML::Newline emitter manipulator
http://stackoverflow.com/questions/3982901/how-to-emit-a-blank-line-using-yaml-cpp
- added some 'out << YAML::Newline' to tryout this feature

http://github.com/malcolmhumphreys/OpenColorIO/commit/0de8ccb6842b223f942789f1ae68d768dab9de29

.malcolm

Re: Review: Inital pass at searchpath impl

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

#202

- fixed a bug with abs searchpath entries
- empty '::' searchpaths now resolve to the profile cwd

http://github.com/malcolmhumphreys/OpenColorIO/commit/5d922362259e86073592481d428926de1054414d

.malcolm

Show quoted text

On 25/10/2010, at 6:59 PM, Malcolm Humphreys wrote:

- removed the EnvExpand recusion limits as this didn't work in the production env which
has more than 30 env vars. might want to add limits in later if this becomes a problem.

http://github.com/malcolmhumphreys/OpenColorIO/commit/a172426e59ddebc4ccd0691b796bfe493677ec9f

.malcolm

On 25/10/2010, at 6:17 PM, Malcolm Humphreys wrote:

Inital pass at searchpath impl
- added environment variable string expansion code in PathUtils.h (GetEnvMap() & EnvExpand())
- added FileExists() function to PathUtils.h
- added searchpath functionality into Config:: class (setSearchPath(path), getSearchPath(expand), findFile(filename)) this hasn't be plugged into anything yet.
- added ocio_core_tests.sh.in so we can pass in some env vars for the unit tests

http://github.com/malcolmhumphreys/OpenColorIO/commit/80bfd194bd7fd3d787e7a8795ad63be6050702c6

.malcolm

Re: Review: Inital pass at searchpath impl

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

#201

- removed the EnvExpand recusion limits as this didn't work in the production env which
has more than 30 env vars. might want to add limits in later if this becomes a problem.

http://github.com/malcolmhumphreys/OpenColorIO/commit/a172426e59ddebc4ccd0691b796bfe493677ec9f

.malcolm

Show quoted text

On 25/10/2010, at 6:17 PM, Malcolm Humphreys wrote:

Inital pass at searchpath impl
- added environment variable string expansion code in PathUtils.h (GetEnvMap() & EnvExpand())
- added FileExists() function to PathUtils.h
- added searchpath functionality into Config:: class (setSearchPath(path), getSearchPath(expand), findFile(filename)) this hasn't be plugged into anything yet.
- added ocio_core_tests.sh.in so we can pass in some env vars for the unit tests

http://github.com/malcolmhumphreys/OpenColorIO/commit/80bfd194bd7fd3d787e7a8795ad63be6050702c6

.malcolm

Review: Inital pass at searchpath impl

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

#200

Inital pass at searchpath impl
- added environment variable string expansion code in PathUtils.h (GetEnvMap() & EnvExpand())
- added FileExists() function to PathUtils.h
- added searchpath functionality into Config:: class (setSearchPath(path), getSearchPath(expand), findFile(filename)) this hasn't be plugged into anything yet.
- added ocio_core_tests.sh.in so we can pass in some env vars for the unit tests

http://github.com/malcolmhumphreys/OpenColorIO/commit/80bfd194bd7fd3d787e7a8795ad63be6050702c6

.malcolm

Review: fix the gcc visibility on linux

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

#195

- removed some #pragma's which were breaking the build on linux,
there are a few more warnings now to do with template<> visibility
in yaml-cpp but it should now link on linux (was working on OSX)

http://github.com/malcolmhumphreys/OpenColorIO/commit/378d9fb3729255bd665f216ef68a487657bd666d

.malcolm

Review: Inital pass at GCC visibility

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

#196

- Inital pass at GCC visibility http://gcc.gnu.org/wiki/Visibility
 to hide unnecessary public symbols in an effort to maintain ABI
- Added -fvisibility-inlines-hidden and -fvisibility=hidden to the yaml-cpp patch, YAML::* symbols should now be hidden in libOpenColorIO.so
- Added export/OpenColorIO/OpenColorABI.h
- Added some '#pragma GCC visibility' for some external libs

Before Visibility:
:nm -C -D src/core/libOpenColorIO.so | grep " T " | wc -l
958

After Visibility:
:nm -C -D src/core/libOpenColorIO.so | grep " T " | wc -l
306

http://github.com/malcolmhumphreys/OpenColorIO/commit/aa888b4d4f969e6c903eb73d7c20a9d9f546298f
http://github.com/malcolmhumphreys/OpenColorIO/commit/08b4208717221426ba90058b6324e3358930d376
http://github.com/malcolmhumphreys/OpenColorIO/commit/3ec1be5204291aaf40e56cdbdb1c7c0f757664eb

.malcolm

Re: Review: Misc Yaml Updates

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

#192

- Can we call AddBaseTransformPropertiesToYAMLMap = EmitBaseTransformKeyValues
- Can we call ReadBaseTransformPropertiesFromYAMLMap = ReadBaseTransformKeyValues, and can the argument order be swaped to match the other >> operators ie. (TransformRcPtr & t, const YAML::Node & node) -> (const YAML::Node & node, TransformRcPtr &)Done.
-- jeremy

OCIO 0.7.0 posted

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

#191

The first release based on YAML is now ready for download. Thanks to
Malcolm for his help on this.

An updated set of 0.7-format color configs (with many more color
spaces!) will be posted tomorrow.

Version 0.7.0 (Oct 21 2010):
    * Switched file format from XML to Yaml

-- Jeremy

Re: Review: Additional Yaml bugfixes

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

#194

All LGTM, sorry most of these were introduced by me in the switch

Show quoted text

On 21/10/2010, at 2:39 PM, Jeremy Selan wrote:

These commits fix a bunch of bugs we discovered when testing our
in-house apps on real production color configurations.

* Yaml serialization fix, resource_path is updated properly
* Relative path loading works again (for FileTransform)
* Yaml output doesn't include default transform direction. (need to
generalize this not to emit default values in general)
* Fixed processing bug related to handling of null transforms
* Yaml bugfix, previously color spaces which defined the transform
"from_reference" were hosed.

js/master

8e6e90b822f21e02b055e9ef9a95e78ac88dc15f
8046c604171470c3a3b8dd31fc51a69d127371a4
e24312c91420a92158658f418e05032ad934d97f
f381144da667b388da86a8492ce28ca7e19a1a2a
c4a68d76662fb74676ac6e94b3f0abcd24c68de7
4a0063ce32c35cffb13bab77eef024971a615266

-- Jeremy

Re: Review: renamed config.writeToStream to config.serialize

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

#193

Yep, I was thinking about doing that but it's not straight forward if we don't want yaml-cpp in the public interfaces for the Transforms()s.

It could also be possible if we have some form of Transform()s parameter types / storage. So something in the core lib can just serialize these params into this ostream. This could pave the way for having Transform plugins which can be serialize themselves with out knowing about yaml-cpp.

.malcolm

Show quoted text

On 21/10/2010, at 2:30 PM, Jeremy Selan wrote:

Thinking about this topic further, I think the right thing to do it to
have __str__ call through to the C++ ostream implementation for each
class.  (If that happens to use yaml, great! But it's not a problem if
it doesn't either).

-- Jeremy


Did you see my comment in one of the threads about also supporting the .__str__() method?

Review: Additional Yaml bugfixes

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

#188

These commits fix a bunch of bugs we discovered when testing our
in-house apps on real production color configurations.

* Yaml serialization fix, resource_path is updated properly
* Relative path loading works again (for FileTransform)
* Yaml output doesn't include default transform direction. (need to
generalize this not to emit default values in general)
* Fixed processing bug related to handling of null transforms
* Yaml bugfix, previously color spaces which defined the transform
"from_reference" were hosed.

js/master

8e6e90b822f21e02b055e9ef9a95e78ac88dc15f
8046c604171470c3a3b8dd31fc51a69d127371a4
e24312c91420a92158658f418e05032ad934d97f
f381144da667b388da86a8492ce28ca7e19a1a2a
c4a68d76662fb74676ac6e94b3f0abcd24c68de7
4a0063ce32c35cffb13bab77eef024971a615266

-- Jeremy

Re: Review: renamed config.writeToStream to config.serialize

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

#187

Thinking about this topic further, I think the right thing to do it to
have __str__ call through to the C++ ostream implementation for each
class.  (If that happens to use yaml, great! But it's not a problem if
it doesn't either).

-- Jeremy

Show quoted text

Did you see my comment in one of the threads about also supporting the .__str__() method?

Re: Review: Misc Yaml Updates

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

#190

Looking at this quickly it would be a pretty small patch to add out << YAML::NewLines(3);

Something along the lines of:

--emittermanip.h--
	struct _NewLines {
		_NewLines(int value_): value(value_) {}
		int value;
	};
	
	inline _NewLines NewLines(int value) {
		return _NewLines(value);
	}
--emittermanip.h--

-- emitter.h --
...
Emitter& Write(const _NewLines& lines);
...
inline Emitter& operator << (Emitter& emitter, const _NewLines& v) { return emitter.Write(v); }
...
-- emitter.h --

-- emmiter.cpp --
	Emitter& Emitter::Write(const _NewLines& lines)
	{
		if(!good())
			return *this;
		for(unsigned int i=0; i< lines.value; ++i)
			m_stream << "\n";
		return *this;
	}
-- emmiter.cpp --

.malcolm

Show quoted text

On 21/10/2010, at 11:24 AM, Malcolm Humphreys wrote:

This looks all pretty good to me

A bunch of changes related to yaml serialization:

* ColorSpace transform a base Transform, instead of GroupTransform.
* GroupTransform Yaml serialization supports nesting
* Direction is now serialized for all yaml elements- Can we call AddBaseTransformPropertiesToYAMLMap = EmitBaseTransformKeyValues
- Can we call ReadBaseTransformPropertiesFromYAMLMap = ReadBaseTransformKeyValues, and can the argument order be swaped to match the other >> operators ie. (TransformRcPtr & t, const YAML::Node & node) -> (const YAML::Node & node, TransformRcPtr &)

* Roles survive a yaml round trip+1 I like this being a map, I'm not sure if order is important.

* Most transforms serialize in Flow form for better readability

Commits: js/master

5f53929774b53ebe6dd9f626e320f70a19251592
a417123d6ec39a5df8add746c3047685be7b8553
dc66f63326b7766c70d8bafd392c826a3a8b37d3
8ec63860a00ebf6301d249270ff43f7dadf8f568
a788950e36d107ec07adb1e2daaf8b399fe90936

I am not totally happy with the look of the Yaml output.  Ideally, I'd
like to add blank lines after each colorspace definition (this is
valid yaml), but havent been able to figure out how do to so with
yaml-cpp.Yeah most of our yamls are hand written with big breaks between sections, so this hasn't really been an issue.

One possible way would be using YAML::Comment with some new line chars, but it would be much nicer to have something like out << YAML::NewLines(3) that you could use.

void MultiLineComment(YAML::Emitter& out, std::string& desiredOutput)
{
	out << YAML::BeginSeq;
	out << "item 1" << YAML::Comment("really really long\ncomment that couldn't possibly\nfit on one line");
	out << "item 2";
	out << YAML::EndSeq;
	desiredOutput = "---\n- item 1  # really really long\n          # comment that couldn't possibly\n          # fit on one line\n- item 2";
}

.malcolm

Re: Review: Misc Yaml Updates

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

#189

This looks all pretty good to me

A bunch of changes related to yaml serialization:

* ColorSpace transform a base Transform, instead of GroupTransform.
* GroupTransform Yaml serialization supports nesting
* Direction is now serialized for all yaml elements- Can we call AddBaseTransformPropertiesToYAMLMap = EmitBaseTransformKeyValues
- Can we call ReadBaseTransformPropertiesFromYAMLMap = ReadBaseTransformKeyValues, and can the argument order be swaped to match the other >> operators ie. (TransformRcPtr & t, const YAML::Node & node) -> (const YAML::Node & node, TransformRcPtr &)

* Roles survive a yaml round trip+1 I like this being a map, I'm not sure if order is important.

* Most transforms serialize in Flow form for better readability

Commits: js/master

5f53929774b53ebe6dd9f626e320f70a19251592
a417123d6ec39a5df8add746c3047685be7b8553
dc66f63326b7766c70d8bafd392c826a3a8b37d3
8ec63860a00ebf6301d249270ff43f7dadf8f568
a788950e36d107ec07adb1e2daaf8b399fe90936

I am not totally happy with the look of the Yaml output.  Ideally, I'd
like to add blank lines after each colorspace definition (this is
valid yaml), but havent been able to figure out how do to so with
yaml-cpp.Yeah most of our yamls are hand written with big breaks between sections, so this hasn't really been an issue.

One possible way would be using YAML::Comment with some new line chars, but it would be much nicer to have something like out << YAML::NewLines(3) that you could use.

void MultiLineComment(YAML::Emitter& out, std::string& desiredOutput)
{
	out << YAML::BeginSeq;
	out << "item 1" << YAML::Comment("really really long\ncomment that couldn't possibly\nfit on one line");
	out << "item 2";
	out << YAML::EndSeq;
	desiredOutput = "---\n- item 1  # really really long\n          # comment that couldn't possibly\n          # fit on one line\n- item 2";
}

.malcolm

Review: Misc Yaml Updates

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

#186

A bunch of changes related to yaml serialization:

* ColorSpace transform a base Transform, instead of GroupTransform.
* GroupTransform Yaml serialization supports nesting
* Direction is now serialized for all yaml elements
* Roles survive a yaml round trip
* Most transforms serialize in Flow form for better readability

Commits: js/master

5f53929774b53ebe6dd9f626e320f70a19251592
a417123d6ec39a5df8add746c3047685be7b8553
dc66f63326b7766c70d8bafd392c826a3a8b37d3
8ec63860a00ebf6301d249270ff43f7dadf8f568
a788950e36d107ec07adb1e2daaf8b399fe90936

I am not totally happy with the look of the Yaml output.  Ideally, I'd
like to add blank lines after each colorspace definition (this is
valid yaml), but havent been able to figure out how do to so with
yaml-cpp.

http://stackoverflow.com/questions/3982901/how-to-emit-a-blank-line-using-yaml-cpp

-- Jeremy

Re: Review: renamed config.writeToStream to config.serialize

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

#184

I did see that comment, forgot to address it in the last post.

I'd prefer to not expose the exact yaml serialization using __str__().

I'm pretty biased against "non-obvious" operator overloading (both in
C and python), so if people want to get the serialized representation
for output purposes I'd prefer an explicit call.

Granted, it may be super handy to have str(config) print out a fully
informative description of the object.  This could be as simple as
<class Ocio.Config FULL YAML EMBEDDED HERE>).   I just want to make
sure the str representation is not valid parsable yaml, so people dont
accidentally rely on str() when they should be using .serialize().

-- Jeremy



On Tue, Oct 19, 2010 at 3:31 PM, Malcolm Humphreys
<malcolmh...@...> wrote:
LGTM,

Did you see my comment in one of the threads about also supporting the .__str__() method?

On 20/10/2010, at 9:05 AM, Jeremy Selan wrote:

This makes the naming unified across C++ / python, and also gets rid
of "getXML" (which is no longer xml).

js/master
commits:
bd559608bdfbbc930634ef78eebd42cda83647da

Re: Review: added missing cmake NAME arg to add_test()

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

#180

looks good to me.

On Tue, Oct 19, 2010 at 5:35 PM, Malcolm Humphreys
<malcolmh...@...> wrote:
- Added missing NAME arg to add_test() as this was failing to work on linux (worked on OSX, small cmake bug)

http://github.com/malcolmhumphreys/OpenColorIO/commit/8e80331866f003b99bbc2bfae627b730980cf8b0

.malcolm