2021-10-18 - OpenColorIO - Academy Software Foundation Wiki

Doug: Need to make sure this supports current build matrix and that license is acceptable. Other things besides whether it solves the localization bug.

Patrick: Started as discussion about localization. This library addresses this and gives an important performance improvement. Localization is fixed, in that lib is independent of locale. Float is always expecting a point. Focus on LUT files because original defect was about LUTs, but there are many other float use cases in OCIO.

Mark B: Any time you add a dependency, making a contract to keep up with that dependency, and making build more complicated. Easier for header only lib, but would likely be pulling from upstream. Adding dependency is the last thing you want to do to solve a problem. We're using standard lib functions, but we could swap those out and solve the problem without the dependency. If the performance benefit is strong though, perhaps it should be considered.

Doug: Agree. Also worth mentioning this introduces change of behavior. This is optimized version of feature introduced in C++17 (from_chars). Follows documentation of that feature. As a side effect, it no longer allows a leading "+" sign in front of numbers. The CLF spec does allow that based on XML specification. It's an edge case, but not sure we can just use this as-is. Would have to decide per-LUT-format what to use this for.

Patrick: Investigated and "+" sign is easy fix with patch. No impact in terms of performance.

Doug: Upstream is not going to accept this as a PR since it is not part of C++17 spec.

Patrick: Correct. We would have to patch it. Take the include file only. Only supports decimals, not hex.

Doug: Hex is not in CLF spec. Don't think that's essential to support.

Patrick: Goal was to fix locale. It does fix it. This could have been done by fixing our code. It's nice to have, though only impacts initial read of LUT file, which is cached.

Michael: Has performance improvement been verified?

Patrick: Yes, improved LUT read performance considerably.

Doug: This locale issue has come up a number of times before. Some hardcode locale to resolve it, which would also fix it. Would that be a better solution?

Mark B: Think fixing the parsers is better. Locale for C/C++ is problematic. Better to find fix without changing locale.

Patrick: We need to preserve the locale, but also parse.

Michael: Would accepting this reduce effort to solve problem another way?

Mark B: In theory, shouldn't be difficult. In practice, it's probably harder than it sounds.

Patrick: Tried to use stream to fix it before, but major impact on performance.

Michael: Could we wait for C++17 to be minimum supported version? Will be a while.

Kevin: Is the loss of leading "+" a loss of functionality? Do we want to solve this with a external lib? Is this the right one? It almost is the right approach, but if we need leading "+", it's unfortunate, but would say no to it. What are the alternatives? How important is the issue? How many users have this issue?

Mark B: Quite a few users, mostly though integration in another piece of software. Other question is, not difficult to replace code, but is the degraded performance that bad if it's just for parsing LUT files. Is the performance aspect of this change important to pursue?

Kevin: Also, do we have a symmetric function for writing floats out to LUT files as well?

Mark B: Good point.

Patrick: Agree, would be same problem.

Michael: Agree that writing out LUTs is important feature to consider too.

Rémi: If we compile with C++17, can we avoid the need to introduce library?

Patrick: Yes, but lack of "+" support may be problem.

Doug: Hex support should be ok to omit. Was originally some discussion around it, but never made it into formal specification. The "+" is in the CLF spec. We could change that with the CLF working group, if that's the direction that C++ parsing is going.

Kevin: Only time I've used leading "+" was for matrices, for clarity.

Mark B: Should be able to catch that in our own code while parsing.

Patrick: What we're currently doing with the proposed patch.

Mark B: Can move the "+" handling from patch to OCIO side, which would allow supporting this or C++17 feature.

Rémi: If we we're to introduce C++17 feature, would still need to add feature to handle other language versions for consistency. Could be limited time support.

Michael: Could code it to allow standard feature when C++17 is minimum version, but always use fast_float in meantime.

Mark B: Also should talk about symbol hiding. When using header only libs, can be problematic.

Patrick: Good point. Need to check that.

TODO: Patrick will follow up with amyspark on TSC discussion. Group will think more on proposed change, with goal of making a decision at the next meeting in two weeks.

Patrick: Do we need formal license approval?

Doug: At a company, would have legal partner review license.

Kevin: This has two licenses. How do we note which we're using?

Mark B: Usually in LICENSE.md or THIRD-PARTY.md file with full license text.

Patrick: Approval should happen before merge.

TODO: Michael will reach out to LF for license review/scan.