October 18, 2021

Host: Michael Dolan

Attendees:

  • Rémi Achard (TSC) - DNEG

  • Mark Boorer (TSC) - Industrial Light & Magic

  • Mei Chu (TSC) - Sony Pictures Imageworks

  • Sean Cooper (TSC ACES TAC Rep) - ARRI

  • Michael Dolan (TSC Chair) - Epic Games

  • Patrick Hodoul (TSC) - Autodesk

  • John Mertic - Academy Software Foundation / Linux Foundation

  • Carol Payne (TSC) - Netflix

  • Mark Titchener (TSC) - Foundry

  • Carl Rand (TSC) - Weta Digital

  • Doug Walker (TSC Chief Architect) - Autodesk

  • Kevin Wheatley (TSC) - Framestore

OCIO TSC Meeting Notes

  • PR #1496:
    • 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.
  • Update third-party libraries for RB-2.1:
    • Patrick: Previously agreed to upgrade third party libs. Discovered issue in find module. Are we ok updating expat and yaml-cpp versions?
    • Michael: Need to decide if we are ok with bumping minimum supported version? Or if we should fix our custom CMake find mechanism? It currently uses same version for minimum supported version and default version when using OCIO_INSTALL_EXT_PACKAGES. Should ideally create macro to take a minimum and default version separately, but is an involved change.
    • Patrick: Tried to bump version last year, but created conflict and had to use specific version. Understand need to support range for other packages in VFX platform, but for internal libs, may not be as critical? We could limit to specific version, but not sure if that's ok for linux ecosystem.
    • Rémi: Package maintainers often want to use latest, so one reason to support latest version.
    • Patrick: Are we ok with imposing a version for internal dependencies (expat, yaml-cpp)? We can then know that they work.
    • No objections raised by group. Other dependencies like Imath will be handled differently.
  • Changing default branch name from master to main:
    • Michael: Looked into GH support for this change. Similar to when we moved GH organizations, platform supports default branch name change and will handle PRs, links, gh-pages, etc. Alternative is to create a second branch and gradually retire current one.
    • Mark B: Don't see a problem with utilizing GH tools, since it is the main upstream. 
    • TODO: Michael will bring up topic at TAC meeting to get other projects thoughts.
  • Items for next meeting agenda:
    • Finalize discussion on PR #1496
    • Follow up on default branch name change
  • No labels