Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DRAFT: ocioarchive enhancements #1931

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

DRAFT: ocioarchive enhancements #1931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b796a6f
Add function to return list of all external files in a config
KevinJW Jan 9, 2024
4869a87
Add basic Archive flags for configuring archiving
KevinJW Jan 15, 2024
a115736
Add global comment to archives to record OCIO version used to create …
KevinJW Jan 16, 2024
54121ce
Add Basic minimal archive functionallity
KevinJW Jan 16, 2024
c727983
Add support for environment variables in minimal configuration archives
KevinJW Jan 22, 2024
8671def
Add minimal code to make the python tests pass
KevinJW Jan 22, 2024
ec3e761
Add minimal boolean to lambda captures for MSVC
KevinJW Jan 23, 2024
94a0221
Rework lambda to pass MSVC and GCC
KevinJW Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/api/constants.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ These environmental variables are used by the OpenColorIO library
Ex: OCIO_OPTIMIZATION_FLAGS="20479" or "0x4FFF" for
OPTIMIZATION_LOSSLESS.

.. data:: PyOpenColorIO.OCIO_ARCHIVE_FLAGS_ENVVAR

The envvar 'OCIO_ARCHIVE_FLAGS' provides a way to modify the parameters
used to create an ocioz archive. Remove the variable or set the value
to empty to not use it. Set the value of the variable to the desired
parameters as either an integer or hexadecimal value.
Ex: OCIO_ARCHIVE_FLAGS="256" or "0x0100" for ARCHIVE_FLAGS_MINIMAL.

.. group-tab:: C++

.. doxygengroup:: VarsEnvvar
Expand Down
11 changes: 9 additions & 2 deletions include/OpenColorIO/OpenColorIO.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <string>
#include <fstream>
#include <vector>
#include <set>

#include "OpenColorABI.h"
#include "OpenColorTypes.h"
Expand Down Expand Up @@ -1503,7 +1504,7 @@ class OCIOEXPORT Config
*
* \return bool Archivable if true.
*/
bool isArchivable() const;
bool isArchivable(bool minimal) const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this accept the Flag enum instead, could make it more future proof? Adding it as an overload might be cleaner for current users of isArchivable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I don't like the use of Flags as they can hide the actual meaning so I guess I defaulted to a single boolean, that said I'd prefer not using multiple bool's in APIs, so if we needed other parameters in the future then I'd suggest wanting to wrap the boolean in a stronger type to avoid calling function(true, false, true, ...) but if others want Flags That is simple enough

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with you, although to me, even one boolean is not very explicit here, when you see isArchivable(true) somewhere randomly in the code. Flags are already used in other places of the API like for the Processor queries so that might be more consistent.


/**
* \brief Archive the config and its LUTs into the specified output stream.
Expand All @@ -1521,15 +1522,21 @@ class OCIOEXPORT Config
* trying to resolve all the FileTransforms in the Config to specific files is because of the
* goal to allow context variables to continue to work.
*
* Archiving a minimal Config will try resolve these file references using the current context.
*
* If a Config is created with CreateFromStream, CreateFromFile with an OCIOZ archive, or
* CreateFromConfigIOProxy, it cannot be archived unless the working directory is manually set
* to a directory that contains any necessary LUT files.
*
* The provided output stream must be closed by the caller, if necessary (e.g., an ofstream).
*
* \param ostream The output stream to write to.
* \param flags Flags top control archive creation
*/
void archive(std::ostream & ostream) const;
void archive(std::ostream & ostream, ArchiveFlags flags) const;

//TODO: document
void GetAllFileReferences(std::set<std::string> & files) const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want this in the public API or is it only for OCIOZArchive need? If the latter, there might be a way to move it to ConfigUtils.h instead. If the former, I'd vote to return a vector instead of set in the spirit of reducing STL headers included here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was one of my thoughts, I would think other uses for the list of files would be interesting and thus a public function may be needed.

Note that the existing code uses them as a set rather than a vector as this then makes each entry unique as it is obviously possible to use the same external file in multiple transforms in a config file (we use the same placeholder file for some of our configuration files). If we want to turn that into a vector for a public API then I guess that is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally it seems this new method is not strictly needed as you can implement it with the existing public API (if I'm not mistaking), but I'm not opposed to adding it if you think there is a strong benefit to have it.

Yes I think the use of set is appropriate for the implementation but not strictly required in the public API, might be nitpicking but seems slightly cleaner!


Config(const Config &) = delete;
Config& operator= (const Config &) = delete;
Expand Down
31 changes: 31 additions & 0 deletions include/OpenColorIO/OpenColorTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,15 @@ extern OCIOEXPORT const char * OCIO_INACTIVE_COLORSPACES_ENVVAR;
*/
extern OCIOEXPORT const char * OCIO_OPTIMIZATION_FLAGS_ENVVAR;

/**
* The envvar 'OCIO_OPTIMIZATION_FLAGS' provides a way to force a given
* optimization level. Remove the variable or set the value to empty to
* not use it. Set the value of the variable to the desired optimization
* level as either an integer or hexadecimal value.
* Ex: OCIO_OPTIMIZATION_FLAGS="1" or "0x01" for ARCHIVE_FLAGS_MINIMAL.
*/
extern OCIOEXPORT const char * OCIO_ARCHIVE_FLAGS_ENVVAR;

/**
* The envvar 'OCIO_USER_CATEGORIES' allows the end-user to filter color spaces shown by
* applications. Only color spaces that include at least one of the supplied categories will be
Expand Down Expand Up @@ -945,6 +954,28 @@ extern OCIOEXPORT const char * OCIO_CONFIG_DEFAULT_NAME;
extern OCIOEXPORT const char * OCIO_CONFIG_DEFAULT_FILE_EXT;
extern OCIOEXPORT const char * OCIO_CONFIG_ARCHIVE_FILE_EXT;

//!cpp:type:: Enum to control the behavior of archiving a :cpp:class:`Config`.
//
// TODO: extend docs
// TODO: Python bindings
// TODO: environment variable?
//
enum ArchiveFlags : unsigned long
{
ARCHIVE_FLAGS_NONE = 0x0000,

ARCHIVE_FLAGS_FAST_COMPRESSION = 0x02, // Compression level
ARCHIVE_FLAGS_NORMAL_COMPRESSION = 0x06, // Compression level
ARCHIVE_FLAGS_BEST_COMPRESSION = 0x09, // Compression level
ARCHIVE_FLAGS_COMPRESSION_MASK = 0x0F, // Compression level mask

ARCHIVE_FLAGS_MINIMAL = 0x0100, // Generate a Minimal Archive

ARCHIVE_FLAGS_DEFAULT = (ARCHIVE_FLAGS_NONE | ARCHIVE_FLAGS_BEST_COMPRESSION),

ARCHIVE_FLAGS_SAFEST = (ARCHIVE_FLAGS_NONE | ARCHIVE_FLAGS_BEST_COMPRESSION) // TODO: name is poor
};

// Built-in config feature
// URI Prefix
extern OCIOEXPORT const char * OCIO_BUILTIN_URI_PREFIX;
Expand Down
55 changes: 27 additions & 28 deletions src/OpenColorIO/Config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const char * OCIO_ACTIVE_DISPLAYS_ENVVAR = "OCIO_ACTIVE_DISPLAYS";
const char * OCIO_ACTIVE_VIEWS_ENVVAR = "OCIO_ACTIVE_VIEWS";
const char * OCIO_INACTIVE_COLORSPACES_ENVVAR = "OCIO_INACTIVE_COLORSPACES";
const char * OCIO_OPTIMIZATION_FLAGS_ENVVAR = "OCIO_OPTIMIZATION_FLAGS";
const char * OCIO_ARCHIVE_FLAGS_ENVVAR = "OCIO_ARCHIVE_FLAGS";
const char * OCIO_USER_CATEGORIES_ENVVAR = "OCIO_USER_CATEGORIES";

// Default filename (with extension) of a config and archived config.
Expand Down Expand Up @@ -1103,6 +1104,16 @@ class Config::Impl
// That should never happen.
return -1;
}

void GetAllFileReferences(std::set<std::string> & files) const
{
ConstTransformVec allTransforms;
this->getAllInternalTransforms(allTransforms);
for(const auto & transform : allTransforms)
{
GetFileReferences(files, transform);
}
}
};


Expand Down Expand Up @@ -1956,14 +1967,8 @@ void Config::validate() const
///// Resolve all file Transforms using context variables.

{
ConstTransformVec allTransforms;
getImpl()->getAllInternalTransforms(allTransforms);

std::set<std::string> files;
for (const auto & transform : allTransforms)
{
GetFileReferences(files, transform);
}
getImpl()->GetAllFileReferences(files);

// Check that at least one of the search paths can be resolved into a valid path.
// Note that a search path without context variable(s) always correctly resolves.
Expand Down Expand Up @@ -4890,14 +4895,8 @@ const char * Config::getCacheID(const ConstContextRcPtr & context) const
{
std::ostringstream filehash;

ConstTransformVec allTransforms;
getImpl()->getAllInternalTransforms(allTransforms);

std::set<std::string> files;
for(const auto & transform : allTransforms)
{
GetFileReferences(files, transform);
}
getImpl()->GetAllFileReferences(files);

for(const auto & iter : files)
{
Expand Down Expand Up @@ -5473,7 +5472,7 @@ ConfigIOProxyRcPtr Config::getConfigIOProxy() const
return getImpl()->m_context->getConfigIOProxy();
}

bool Config::isArchivable() const
bool Config::isArchivable(bool minimal) const
{
ConstContextRcPtr context = getCurrentContext();

Expand All @@ -5486,7 +5485,7 @@ bool Config::isArchivable() const
}

// Utility lambda to check the following criteria.
auto validatePathForArchiving = [](const std::string & path)
auto validatePathForArchiving = [&minimal](const std::string & path)
{
// Using the normalized path.
const std::string normPath = pystring::os::path::normpath(path);
Expand All @@ -5496,9 +5495,10 @@ bool Config::isArchivable() const
// 2) Path may not start with double dot ".." (going above working directory).
pystring::startswith(normPath, "..") ||
// 3) A context variable may not be located at the start of the path.
(ContainsContextVariables(path) &&
(StringUtils::Find(path, "$") == 0 ||
StringUtils::Find(path, "%") == 0)))
(ContainsContextVariables(path) && //TODO: if we can resolve context, do so and validate path to file
(!minimal &&
(StringUtils::Find(path, "$") == 0 ||
StringUtils::Find(path, "%") == 0))))
{
return false;
}
Expand All @@ -5525,14 +5525,8 @@ bool Config::isArchivable() const
/////////////////////////////////
// FileTransform verification. //
/////////////////////////////////
ConstTransformVec allTransforms;
getImpl()->getAllInternalTransforms(allTransforms);

std::set<std::string> files;
for(const auto & transform : allTransforms)
{
GetFileReferences(files, transform);
}
getImpl()->GetAllFileReferences(files);

// Check that FileTransform sources are not absolute nor have context variables outside of
// config working directory.
Expand All @@ -5548,10 +5542,15 @@ bool Config::isArchivable() const
return true;
}

void Config::archive(std::ostream & ostream) const
void Config::archive(std::ostream & ostream, ArchiveFlags flags) const
{
// Using utility functions in OCIOZArchive.cpp.
archiveConfig(ostream, *this, getCurrentContext()->getWorkingDir());
archiveConfig(ostream, *this, getCurrentContext()->getWorkingDir(), flags);
}

void Config::GetAllFileReferences(std::set<std::string> & files) const
{
return getImpl()->GetAllFileReferences(files);
}

} // namespace OCIO_NAMESPACE
Loading
Loading