windows build, apparent bugs


LaszloSebo <laszl...@...>
 

Hi there,

I am a bit of a newbie here, so please be gentle :-)

I built openColorIO yesterday on the windows platform, and while the
process had parts that i had to do manually (for yaml and tinyxml), it
was not extremely hard.

However, i had to mod the source code in a couple of places, which i
think might just be bugs, and not OS specific changes necessarily:

Issue #1: in core/ColorSpaceTransform.cpp:

Line 178: BuildColorSpaceOps function. When converting a lut to an
icc, no default config is used (as far as i understand), so the color
spaces defined are all totally default. Thus, they have no 'allocation
variables', at least nothing initialized for me for that. 2 base color
spaces were made automatically:
RawInput and ProcessedOutput, with basically empty structures (which i
think is fine).

However, around line 203, there is this line:
srcColorSpace->getAllocationVars(&srcAllocation.vars[0])
Since srcAllocation.vars is empty, this will result in an out of
bounds error.

Same thing further down in the function for dstAllocation, around line
235ish.

Issue #2: in core/pystring/pystring.cpp:

Line 254: std::string do_strip() function. The used variables are all
of type std::string::size_type. At least on the windows platform,
thats unsigned int64. That means that it wraps around if you go below
zero, so a crucial test could fail when the string length was zero:

j = len; // so j is initialized with 0

then later:

j--; // now j is, instead of being -1, is equal to the largest
number unsigned int64 can represent

while (j >= i && ::isspace(str[j])) // <-- since j>=i is true,
isspace(str[j]) is tested, and will fail with out of bounds error.
I fixed this by wrapping the whole test in a if len>0 and i=0, j=0
variable initialization.


So with all that said,.. the icc conversion goes through without
errors. However, photoshop cs5 refuses to load them :-\ So i have a
feeling i might still be missing some configuration setting..


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

Laslo,

Thanks for this detailed report.

My apologies that you've had issues running on Windows. (and sorry for not replying sooner in the week).

Are you comfortable fixing the bugs you've found and submitting a pull request on guthub?  If you're up for this, please feel free to do so.  If not, just let me know and I'll be happy to try and address what you've reported.

As far as the final icc profile being invalid, which OCIO configuration are you using?  One from our online examples, I presume?  What command-line are you using to generate the profile?   ociobakelut <...>

I'm happy to try and re-generate the icc profile on an alternate os, so we can confirm the expected result.

-- Jeremy



On Thu, Apr 5, 2012 at 9:53 AM, LaszloSebo <laszl...@...> wrote:
Hi there,

I am a bit of a newbie here, so please be gentle :-)

I built openColorIO yesterday on the windows platform, and while the
process had parts that i had to do manually (for yaml and tinyxml), it
was not extremely hard.

However, i had to mod the source code in a couple of places, which i
think might just be bugs, and not OS specific changes necessarily:

Issue #1: in core/ColorSpaceTransform.cpp:

Line 178: BuildColorSpaceOps function. When converting a lut to an
icc, no default config is used (as far as i understand), so the color
spaces defined are all totally default. Thus, they have no 'allocation
variables', at least nothing initialized for me for that. 2 base color
spaces were made automatically:
RawInput and ProcessedOutput, with basically empty structures (which i
think is fine).

However, around line 203, there is this line:
srcColorSpace->getAllocationVars(&srcAllocation.vars[0])
Since srcAllocation.vars is empty, this will result in an out of
bounds error.

Same thing further down in the function for dstAllocation, around line
235ish.

Issue #2: in core/pystring/pystring.cpp:

Line 254: std::string do_strip() function. The used variables are all
of type std::string::size_type. At least on the windows platform,
thats unsigned int64. That means that it wraps around if you go below
zero, so a crucial test could fail when the string length was zero:

j = len; // so j is initialized with 0

then later:

j--;   // now j is, instead of being -1, is equal to the largest
number unsigned int64 can represent

while (j >= i && ::isspace(str[j]))   //  <-- since j>=i  is true,
isspace(str[j]) is tested, and will fail with out of bounds error.
I fixed this by wrapping the whole test in a if len>0 and i=0, j=0
variable initialization.


So with all that said,.. the icc conversion goes through without
errors. However, photoshop cs5 refuses to load them :-\ So i have a
feeling i might still be missing some configuration setting..


László Sebő <laszl...@...>
 

Hi Jeremy,


Sure thing, ill make a fork and do a pull request.

Re: what config i was using:

I tried a couple of ways of creating the ICC, first without a config (i noticed in the sourcecode that when you supply a lut as an input, the iconfig parameter is ignored / not parsed):

ociobakelut -lut halfred_truelight.3dl -format icc mylut.icc

When using the -lut params, a "new (temporary) configuration is synthesized with the transformation embedded in the colorspace" (from line 183 in main.cpp)
It sets up a RawInput and a ProcessedOutput colorspace, but both are basically all default values, there are no allocation variables defined.

Then i tried without giving an input lut, just using some standard color spaces:

ociobakelut -iconfig config.ocio --inputspace lg10 --outputspace srgb8 -format icc mytest.icc

I was trying it with various ocio configs that i grabbed from Nuke 6.3v7. Maybe that was a mistake :-)


cheers,
laszlo



On Sat, Apr 7, 2012 at 4:14 PM, Jeremy Selan <jeremy...@...> wrote:
Laslo,

Thanks for this detailed report.

My apologies that you've had issues running on Windows. (and sorry for not replying sooner in the week).

Are you comfortable fixing the bugs you've found and submitting a pull request on guthub?  If you're up for this, please feel free to do so.  If not, just let me know and I'll be happy to try and address what you've reported.

As far as the final icc profile being invalid, which OCIO configuration are you using?  One from our online examples, I presume?  What command-line are you using to generate the profile?   ociobakelut <...>

I'm happy to try and re-generate the icc profile on an alternate os, so we can confirm the expected result.

-- Jeremy



On Thu, Apr 5, 2012 at 9:53 AM, LaszloSebo <laszl...@...> wrote:
Hi there,

I am a bit of a newbie here, so please be gentle :-)

I built openColorIO yesterday on the windows platform, and while the
process had parts that i had to do manually (for yaml and tinyxml), it
was not extremely hard.

However, i had to mod the source code in a couple of places, which i
think might just be bugs, and not OS specific changes necessarily:

Issue #1: in core/ColorSpaceTransform.cpp:

Line 178: BuildColorSpaceOps function. When converting a lut to an
icc, no default config is used (as far as i understand), so the color
spaces defined are all totally default. Thus, they have no 'allocation
variables', at least nothing initialized for me for that. 2 base color
spaces were made automatically:
RawInput and ProcessedOutput, with basically empty structures (which i
think is fine).

However, around line 203, there is this line:
srcColorSpace->getAllocationVars(&srcAllocation.vars[0])
Since srcAllocation.vars is empty, this will result in an out of
bounds error.

Same thing further down in the function for dstAllocation, around line
235ish.

Issue #2: in core/pystring/pystring.cpp:

Line 254: std::string do_strip() function. The used variables are all
of type std::string::size_type. At least on the windows platform,
thats unsigned int64. That means that it wraps around if you go below
zero, so a crucial test could fail when the string length was zero:

j = len; // so j is initialized with 0

then later:

j--;   // now j is, instead of being -1, is equal to the largest
number unsigned int64 can represent

while (j >= i && ::isspace(str[j]))   //  <-- since j>=i  is true,
isspace(str[j]) is tested, and will fail with out of bounds error.
I fixed this by wrapping the whole test in a if len>0 and i=0, j=0
variable initialization.


So with all that said,.. the icc conversion goes through without
errors. However, photoshop cs5 refuses to load them :-\ So i have a
feeling i might still be missing some configuration setting..



László Sebő <laszl...@...>
 

Hi Jeremy,


Sent a pull request just now.

One additional thing i noticed is that there is a duplicate of pystring.cpp used in the ociobakelut project:

src/apps/share/pystring.cpp
src/apps/share/pystring.h

With also the originals being here:

src/core/pystring/pystring.cpp
src/core/pystring/pystring.h

I didn't dare touch that, but i dont think that's needed for ociobakelut as its already being built into the OpenColorIO library.
Maybe its required by one of the other apps? Maybe just a couple of files left over from earlier, as the other 2 libs in the share folder seem to be unique, only pystring being a duplicate.


cheers,
laszlo


On Tue, Apr 10, 2012 at 10:32 AM, László Sebő <laszl...@...> wrote:
Hi Jeremy,


Sure thing, ill make a fork and do a pull request.

Re: what config i was using:

I tried a couple of ways of creating the ICC, first without a config (i noticed in the sourcecode that when you supply a lut as an input, the iconfig parameter is ignored / not parsed):

ociobakelut -lut halfred_truelight.3dl -format icc mylut.icc

When using the -lut params, a "new (temporary) configuration is synthesized with the transformation embedded in the colorspace" (from line 183 in main.cpp)
It sets up a RawInput and a ProcessedOutput colorspace, but both are basically all default values, there are no allocation variables defined.

Then i tried without giving an input lut, just using some standard color spaces:

ociobakelut -iconfig config.ocio --inputspace lg10 --outputspace srgb8 -format icc mytest.icc

I was trying it with various ocio configs that i grabbed from Nuke 6.3v7. Maybe that was a mistake :-)


cheers,
laszlo




On Sat, Apr 7, 2012 at 4:14 PM, Jeremy Selan <jeremy...@...> wrote:
Laslo,

Thanks for this detailed report.

My apologies that you've had issues running on Windows. (and sorry for not replying sooner in the week).

Are you comfortable fixing the bugs you've found and submitting a pull request on guthub?  If you're up for this, please feel free to do so.  If not, just let me know and I'll be happy to try and address what you've reported.

As far as the final icc profile being invalid, which OCIO configuration are you using?  One from our online examples, I presume?  What command-line are you using to generate the profile?   ociobakelut <...>

I'm happy to try and re-generate the icc profile on an alternate os, so we can confirm the expected result.

-- Jeremy



On Thu, Apr 5, 2012 at 9:53 AM, LaszloSebo <laszl...@...> wrote:
Hi there,

I am a bit of a newbie here, so please be gentle :-)

I built openColorIO yesterday on the windows platform, and while the
process had parts that i had to do manually (for yaml and tinyxml), it
was not extremely hard.

However, i had to mod the source code in a couple of places, which i
think might just be bugs, and not OS specific changes necessarily:

Issue #1: in core/ColorSpaceTransform.cpp:

Line 178: BuildColorSpaceOps function. When converting a lut to an
icc, no default config is used (as far as i understand), so the color
spaces defined are all totally default. Thus, they have no 'allocation
variables', at least nothing initialized for me for that. 2 base color
spaces were made automatically:
RawInput and ProcessedOutput, with basically empty structures (which i
think is fine).

However, around line 203, there is this line:
srcColorSpace->getAllocationVars(&srcAllocation.vars[0])
Since srcAllocation.vars is empty, this will result in an out of
bounds error.

Same thing further down in the function for dstAllocation, around line
235ish.

Issue #2: in core/pystring/pystring.cpp:

Line 254: std::string do_strip() function. The used variables are all
of type std::string::size_type. At least on the windows platform,
thats unsigned int64. That means that it wraps around if you go below
zero, so a crucial test could fail when the string length was zero:

j = len; // so j is initialized with 0

then later:

j--;   // now j is, instead of being -1, is equal to the largest
number unsigned int64 can represent

while (j >= i && ::isspace(str[j]))   //  <-- since j>=i  is true,
isspace(str[j]) is tested, and will fail with out of bounds error.
I fixed this by wrapping the whole test in a if len>0 and i=0, j=0
variable initialization.


So with all that said,.. the icc conversion goes through without
errors. However, photoshop cs5 refuses to load them :-\ So i have a
feeling i might still be missing some configuration setting..