Skip to content

OCIOZ archive feature (#1627)#1696

Merged
doug-walker merged 9 commits into
AcademySoftwareFoundation:mainfrom
autodesk-forks:adsk_contrib/ocioz_archive_config
Oct 25, 2022
Merged

OCIOZ archive feature (#1627)#1696
doug-walker merged 9 commits into
AcademySoftwareFoundation:mainfrom
autodesk-forks:adsk_contrib/ocioz_archive_config

Conversation

@cedrik-fuoco-adsk
Copy link
Copy Markdown
Contributor

@cedrik-fuoco-adsk cedrik-fuoco-adsk commented Sep 30, 2022

This PR is the implementation of the OCIOZ archive config feature (#1627).

  • The OCIO environment variable can now be set to an OCIOZ archive and an archive will work in CreateFromFile.
    e.g. export OCIO=/home/user/myconfig/myarchived_config.ocioz

  • A new abstract class called ConfigIOProxy has been added to the Public API.

    This abstract class allows a client application to implement a derived class that can provide a different
    way of getting the config, the LUTs, and the existence of the LUTs.

    The three methods are :

    getConfigData:
    Return the config
    
    getLutData:
    Return the LUT
    
    getFastLutFileHash:
    Check for the existence of a LUT and return the hash. It must return the hash because
    OCIO cannot use the default hash computation here since the LUT might not be from the filesystem.
    
  • CreateFromConfgIOProxy has been added to the Public API which allows creation of a config from a derived instance of ConfigIOProxy.

  • isArchivable has been added to the Public API which tell you if the current config is archivable or not.

    • A config is archivable based on the following requirements:
      • The working directory is set and it is an absolute path.
      • Search paths and FileTransform source have no absolute paths.
      • All LUTs must be inside the working directory on the filesystem.
      • A search path or a FileTransfrom source must not start with a context variable.
  • A new app called "ocioarchive" provides the following:

    • Archive a config using the $OCIO environment variable
    • Archive a config by specifying the config file (e.g. myconfig.ocio)
    • Extract an OCIOZ archive
    • List the contents of an OCIOZ archive
  • ociocheck:

    • Prints the config CacheID in the miscellaneous section
    • Prints whether the config is archivable.
  • Minizip-ng (handles the ZIP format) and ZLIB (handles compression) have been added as dependencies to OCIO.

Signed-off-by: Cedrik Fuoco cedrik.fuoco@autodesk.com

Signed-off-by: Cedrik Fuoco <cedrik.fuoco@autodesk.com>
@amyspark
Copy link
Copy Markdown
Contributor

amyspark commented Oct 2, 2022

Hi @cedrik-fuoco-adsk ! I take this should fix #1580 too?

Signed-off-by: Cedrik Fuoco <cedrik.fuoco@autodesk.com>
… in order to respect C++ idiom and letting compiler optimize with NRVO and Copy elision

- Improving how cmake find zlib (renamed getZLIB to Findzlib)
- Make sure that ZLIB is statically linked on all platform (it wasn't on windows)
- Fix issue with Python Unit test when using Python 2 (problem discovered on OSX)
- Change how OpenEXR finds ZLIB by using our custom Findzlib
- Patching Minizip-ng makefile to change cmake minimum version

Signed-off-by: Cedrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk
Copy link
Copy Markdown
Contributor Author

Hi @cedrik-fuoco-adsk ! I take this should fix #1580 too?

Hi @amyspark,

I would say that it should potentially fix issue #1580 based on the information on the issue.

Unfortunately, I don't have the setup or environment to test that.

Comment thread share/cmake/patches/PatchMinizip-ngCMakelists.cmake Outdated
Copy link
Copy Markdown
Collaborator

@remia remia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @cedrik-fuoco-adsk, appreciate all the unit tests.

Comment thread include/OpenColorIO/OpenColorIO.h Outdated
Comment thread share/cmake/modules/GetZLIB.cmake
Comment thread src/OpenColorIO/Config.cpp Outdated
Comment thread src/OpenColorIO/Config.cpp
Comment thread src/OpenColorIO/Config.cpp Outdated
Comment thread tests/python/OCIOZArchiveTest.py Outdated
Comment thread src/OpenColorIO/OCIOZArchive.cpp Outdated
Comment thread src/OpenColorIO/OCIOZArchive.cpp Outdated
Comment thread src/OpenColorIO/OCIOZArchive.cpp Outdated
Comment thread share/cmake/patches/PatchMinizip-ngCMakelists.cmake Outdated
@doug-walker
Copy link
Copy Markdown
Collaborator

Thank you for the thorough review Remi! We will push another commit to address your points.

…on needs to happend.

- Responding to Remi's comments.
  Notable changes:
  - Comments typo and styles fix.
  - Unit tests fixes.
  - Removing the usage of std::ifstream in getLutData and getConfig since it is not needed at all - It was slowing down getLutData.
  - Implicitly converting python list to std::vector<uint8_t> (for getLutData)
  - When archiving a config, the file won't added (overwridden) multiple times if the file matches multiple supported format.

Signed-off-by: Cedrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk
Copy link
Copy Markdown
Contributor Author

cedrik-fuoco-adsk commented Oct 12, 2022

Thanks @remia for all the comments!

The last commit should address all of your comments except the side note on splitting Config_tests.cpp.

Note that the Linux CI Build are still failing because of the issue with the CMake version.

- In ConfigIOProxy Python Unit test, changed getLutData to return a bytearray instead of a list. I think this makes more sense and it works with Python 2 and 3.

Signed-off-by: Cedrik Fuoco <cedrik.fuoco@autodesk.com>
@doug-walker
Copy link
Copy Markdown
Collaborator

One more commit is in progress to update a few comments.

Signed-off-by: Cedrik Fuoco <cedrik.fuoco@autodesk.com>
@remia
Copy link
Copy Markdown
Collaborator

remia commented Oct 24, 2022

Here is a proposed fix for the VFX 2019 Linux builds, minizip-ng should compile successfully, there might be a follow up issue due to zlib build location using lib instead of lib64 (update the CMake module to fix that) from what I saw in my local Docker container.

--- a/.github/workflows/ci_workflow.yml
+++ b/.github/workflows/ci_workflow.yml
@@ -252,6 +252,10 @@ jobs:
     steps:
       - name: Checkout
         uses: actions/checkout@v2
+      # minizip-ng requires CMake 3.13+ but VFX2019 image ships with 3.12
+      - name: Upgrade VFX2019 CMake
+        run: pip install cmake==3.13.3
+        if: matrix.vfx-cy == '2019'
       - name: Install docs env
         run: share/ci/scripts/linux/yum/install_docs_env.sh
         if: matrix.build-docs == 'ON'

…inux (matrix.vfx-cy == '2019').

Signed-off-by: Cedrik Fuoco <cedrik.fuoco@autodesk.com>
@remia
Copy link
Copy Markdown
Collaborator

remia commented Oct 25, 2022

I assume the remaining error can be addressed doing something similar to what we do for expat here?

@cedrik-fuoco-adsk
Copy link
Copy Markdown
Contributor Author

cedrik-fuoco-adsk commented Oct 25, 2022

I assume the remaining error can be addressed doing something similar to what we do for expat here?

The lib prefix is ok. It seems like it doesn't use CMAKE_INSTALL_LIBDIR even though it is set to "lib64". It still installs it into "ext/dist/lib" instead of "ext/dist/lib64"

EDIT: Should be all fixed now!

- Removing unused CMAKE variables from ZLIB_CMAKE_ARGS

Signed-off-by: Cedrik Fuoco <cedrik.fuoco@autodesk.com>
@doug-walker doug-walker merged commit f767cad into AcademySoftwareFoundation:main Oct 25, 2022
@doug-walker doug-walker deleted the adsk_contrib/ocioz_archive_config branch November 11, 2024 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants