Fix SPI1D and SPI3D LUT parsing failing depending on locale

[enkore]

Ain't pretty but I wanted to keep the amount of changes minimal. Unit tests pass and they look like they're fairly valid for checking if the parsing works.

This leaves only a few more spots in the codebase that use sscanf (and thus break depending on locale):

OpenColorIO/src> ag sscanf
OpenColorIO/Platform.h
29:#define sscanf sscanf_s

OpenColorIO/fileformats/FileFormatDiscreet1DL.cpp
412:        const int nummatched = sscanf(InString, "%*s %d %d %s", &numtables, &length, dstDepthS, 16);
414:        const int nummatched = sscanf(InString, "%*s %d %d %s", &numtables, &length, dstDepthS);
436:            sscanf(dstDepthS, "%d%c", &dstDepth, &floatC, 1);
438:            sscanf(dstDepthS, "%d%c", &dstDepth, &floatC);

OpenColorIO/fileformats/ctf/CTFReaderHelper.cpp
598:        // (e.g. "10.5@") but it does not cause the sscanf to fail.
757:        if (0 == sscanf(versionValue, "%d", &fver))

OpenColorIO/fileformats/ctf/CTFTransform.cpp
78:    sscanf(versionString.c_str(), "%d.%d.%d",

%d should be pretty safe, not sure about %s (probably not).

Fixes some instances of #297 occurring.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ enkore (9d2bbd2, fb136ae, 9ef151a)
• ✅ Patrick Hodoul (b142bf1)

[enkore]

I'm not sure if this is intended behavior, or the old comment just meant to note the parser would accept this, while no file should look like this. Let me know.

[Dithermaster]

FWIW, the way we fixed this in our apps was to save the locale, set the locale, call OCIO to do its work, then restore the locale. We wrapped all the locale stuff in a C++ class that can be used as an automatic (the ctor does the pre steps and the dtor does the post steps). The implementation uses different APIs on Windows compared to macOS/Linux. You could do something similar here.

[hodoulp]

Hi @enkore you need to register to EasyCLA.

[enkore]

Done

[hodoulp]

Thanks @enkore for taking the time to think about the locale problem and proposing a solution.

The pieces of code you changed are critical ones directly impacting the read performance of the spi files. So, I took the time to check any performance penalty. Unfortunately, the spi3d reader is now 3-4 time slower.

In order to have the numbers, you can:

1. modify src/apps/ocioperf/main.cpp by adding at line 269

Measure m("Create the processor from transform", 1);
m.resume();

2. export OCIO=/Users/Patrick/dev/OpenColorIO-Configs/aces_1.0.3/config.ocio
3. ./src/apps/ocioperf/ocioperf --image /Users/Patrick/dev/aces_dev/images/ACES/syntheticChart.01.exr --transform /Users/Patrick/dev/OpenColorIO-Configs/aces_1.0.3/luts/LMT_Shaper.ACES_1.0_to_0.1_emulation.spi3d

So, I cannot accept the changes unless you find another solution without performance impact.

Note: There is no performance impact on the spi1d reader.

[hodoulp]

There is a major performance hit on the spi3d reader.

[enkore]

Thanks for looking at this. I looked at the performance side of things. master branch is for me around ~175 ms for that step, this change brought that to ~7xx ms. I made a few changes and brought it back down to about ~210 ms. That might be more acceptable. About ~90 % of the time in SPI3D read is now spent in stringstream>>(float/int).

Aside from perf there is also the question of correctness. I didn't look at the ParseUtils before, so I didn't notice they just directly use istringstream, which depends on the C++ locale. So while a simple setlocale call (unfortunately a number of libraries do this) won't break istringstream, setting the C++ locale will. Fortunately, most software doesn't use C++ locales, so that's less of a problem.

[hodoulp]

Hi @enkore Sorry for that delay without any answer but the 'feature complete' milestone and Siggraph consume all my time. I should have some time next week.

[hodoulp]

@enkore Thanks for updating the pull request but I still find a major performance hit when loading a 3D LUT.
On my macOS, the numbers are from 355 ms (master code) to 745 ms (pull request code) using the test case explained above. So, you clearly improved the performance but that's still two times slower.

By design replacing a sscanf() method by several string manipulations is unlikely to have the same performance.

Anyway thank you for investigating such a challenging problem.

[michdolan]

Hi @enkore . As @hodoulp mentioned, thanks for investigating this problem. Just checking in to see if you would like to continue this investigation? Or if you would like to close the pull request?

There was some discussion in a TSC meeting recently regarding locale handling and I think the determination was that we should read all LUT files with a locked locale, since they have format specifications, but better handle locale when reading a OCIO config file. You can see the discussion notes here: https://github.com/AcademySoftwareFoundation/OpenColorIO/blob/d5de7101288c656f50b525d4c9b6f06b9b8c0d6c/ASWF/meetings/tsc/2021-02-08.md

@hodoulp may have more insight into where this is at now that OCIO v2 has been released.

[hodoulp]

@enkore @michdolan As this issue hit us hard few weeks ago, I think to put some time on it. The design will be to follow the recommandation discussed at the TSC meeting i.e. a fixed en locale for number parsing only. But I still need to create the GitHub issue to clearly document the approach.

@enkore let us know if you would like to continue your work.

[enkore]

Sorry for kinda disappearing here; kinda forgot about it and for my usage I just settled on enforcing C locale. I think this PR is a dead-end, I'm closing it.