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

Asdk Contrib - OCIO-146 Abs Paths through proxy #27

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+62 −51
Open

Asdk Contrib - OCIO-146 Abs Paths through proxy #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7eb3d4f
Ticket #2111
cozdas Jan 9, 2025
dcfdeff
- Changing the logic so that for abs paths we first try the configPro…
cozdas Jan 11, 2025
23b3e4d
- Ah! make_unique is a c++14 feature and we support C++11. I wonder w…
cozdas Jan 11, 2025
9bf4f93
- Minor cleanup
cozdas Jan 13, 2025
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
18 changes: 12 additions & 6 deletions src/OpenColorIO/PathUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,20 +88,26 @@ std::string GetFastFileHash(const std::string & filename, const Context & contex
fileHashResultPtr->ready = true;

std::string h = "";
if (!context.getConfigIOProxy())
if (context.getConfigIOProxy())
{
// Default case.
h = g_hashFunction(filename);
// Case for when ConfigIOProxy is used (callbacks mechanism).
h = context.getConfigIOProxy()->getFastLutFileHash(filename.c_str());

// For absolute paths, if the proxy does not provide a hash, try the file system.
if (h.empty() && pystring::os::path::isabs(filename))
{
h = g_hashFunction(filename);
}
}
else
{
// Case for when ConfigIOProxy is used (callbacks mechanism).
h = context.getConfigIOProxy()->getFastLutFileHash(filename.c_str());
// Default case
h = g_hashFunction(filename);
}

fileHashResultPtr->hash = h;
}

hash = fileHashResultPtr->hash;
}

Expand Down
72 changes: 27 additions & 45 deletions src/OpenColorIO/transforms/FileTransform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,35 +192,33 @@ std::unique_ptr<std::istream> getLutData(
{
if (config.getConfigIOProxy())
{
std::vector<uint8_t> buffer = config.getConfigIOProxy()->getLutData(filepath.c_str());
std::stringstream ss;
ss.write(reinterpret_cast<const char*>(buffer.data()), buffer.size());

return std::unique_ptr<std::stringstream>(
new std::stringstream(std::move(ss))
);
}

// Default behavior. Return file stream.
return std::unique_ptr<std::ifstream>(
new std::ifstream(Platform::filenameToUTF(filepath).c_str(), mode)
);
}
std::vector<uint8_t> buffer;
// Try to open through proxy.
try
{
buffer = config.getConfigIOProxy()->getLutData(filepath.c_str());
}
catch (const std::exception&)
{
// If the path is absolute, we'll try the file system, but otherwise
// nothing to do.
if (!pystring::os::path::isabs(filepath))
throw;
}

// Close stream returned by getLutData
void closeLutStream(const Config & config, const std::istream & istream)
{
// No-op when it is using ConfigIOProxy since getLutData returns a vector<uint8_t>.
if (config.getConfigIOProxy() == nullptr)
{
// The std::istream is coming from a std::ifstream.
// Pointer cast to ifstream and then close it.
std::ifstream * pIfStream = (std::ifstream *) &istream;
if (pIfStream->is_open())
// If the buffer is empty, we'll try the file system for abs paths.
Copy link
Author

Choose a reason for hiding this comment

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

If the file is missing in the archive, it doesn't throw but return an empty buffer. So the fallback kicks in both when the file is missing and getLutData() throws.

if (!buffer.empty() || !pystring::os::path::isabs(filepath))
{
pIfStream->close();
auto pss = std::unique_ptr<std::stringstream>(new std::stringstream);
pss->write(reinterpret_cast<const char*>(buffer.data()), buffer.size());

return pss;
}
}
Copy link
Author

@cozdas cozdas Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This function is not needed. istream is a RAII class and we wrap them with unique_pointers, which are also RAII. All will close on destruction. RAII also handles the actual object type behind the base class, we don't need to know the details.


// Default behavior. Return file stream.
return std::unique_ptr<std::istream>(new std::ifstream(
Platform::filenameToUTF(filepath), mode));
}

bool CollectContextVariables(const Config &,
Expand Down Expand Up @@ -635,9 +633,8 @@ void LoadFileUncached(FileFormat * & returnFormat,
filepath,
tryFormat->isBinary() ? std::ios_base::binary : std::ios_base::in
);
auto & filestream = *pStream;

if (!filestream.good())
Comment on lines -638 to -640
Copy link
Author

@cozdas cozdas Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Dangerous! If getLutData returns nullptr, this will crash / throw. I'm adding checks against null pointer and using the pointer instead of casting to a reference.

if (!pStream || !pStream->good())
{
std::ostringstream os;
os << "The specified FileTransform srcfile, '";
Expand All @@ -647,7 +644,7 @@ void LoadFileUncached(FileFormat * & returnFormat,
throw Exception(os.str().c_str());
}

CachedFileRcPtr cachedFile = tryFormat->read(filestream, filepath, interp);
CachedFileRcPtr cachedFile = tryFormat->read(*pStream, filepath, interp);

if(IsDebugLoggingEnabled())
{
Expand All @@ -660,17 +657,10 @@ void LoadFileUncached(FileFormat * & returnFormat,
returnFormat = tryFormat;
returnCachedFile = cachedFile;

closeLutStream(config, filestream);

return;
}
catch(std::exception & e)
{
if (pStream)
{
closeLutStream(config, *pStream);
}

primaryErrorText += " '";
primaryErrorText += tryFormat->getName();
primaryErrorText += "' failed with: ";
Expand Down Expand Up @@ -712,9 +702,8 @@ void LoadFileUncached(FileFormat * & returnFormat,
filepath,
altFormat->isBinary() ? std::ios_base::binary : std::ios_base::in
);
auto& filestream = *pStream;

if (!filestream.good())
if (!pStream || !pStream->good())
{
std::ostringstream os;
os << "The specified FileTransform srcfile, '";
Expand All @@ -725,7 +714,7 @@ void LoadFileUncached(FileFormat * & returnFormat,
throw Exception(os.str().c_str());
}

cachedFile = altFormat->read(filestream, filepath, interp);
cachedFile = altFormat->read(*pStream, filepath, interp);

if(IsDebugLoggingEnabled())
{
Expand All @@ -738,17 +727,10 @@ void LoadFileUncached(FileFormat * & returnFormat,
returnFormat = altFormat;
returnCachedFile = cachedFile;

closeLutStream(config, filestream);

return;
}
catch(std::exception & e)
{
if (pStream)
{
closeLutStream(config, *pStream);
}

if(IsDebugLoggingEnabled())
{
std::ostringstream os;
Expand Down
23 changes: 23 additions & 0 deletions tests/cpu/OCIOZArchive_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,29 @@ OCIO_ADD_TEST(OCIOZArchive, context_test_for_search_paths_and_filetransform_sour
mtx->getMatrix(mat);
OCIO_CHECK_EQUAL(mat[0], 4.);
}

{
// File transform source is an absolute path, not in the archive.
const std::string filePath =
OCIO::GetTestFilesDir() + "/matrix_example4x4.ctf";
OCIO::FileTransformRcPtr transform = OCIO::FileTransform::Create();
transform->setSrc(filePath.c_str());
OCIO::ConstProcessorRcPtr processor = cfg->getProcessor(transform);
OCIO::ConstTransformRcPtr tr =
processor->createGroupTransform()->getTransform(0);
auto mtx = OCIO::DynamicPtrCast<const OCIO::MatrixTransform>(tr);
OCIO_REQUIRE_ASSERT(mtx);
mtx->getMatrix(mat);
OCIO_CHECK_EQUAL(mat[0], 3.24);
}

{
// File transform source is an abs path but doesn't exist anywhere.
const std::string filePath = OCIO::GetTestFilesDir() + "/missing.ctf";
OCIO::FileTransformRcPtr transform = OCIO::FileTransform::Create();
transform->setSrc(filePath.c_str());
OCIO_CHECK_THROW(cfg->getProcessor(transform), OCIO::Exception);
}
};

testPaths(cfgWindowsArchive, ctxWindowsArchive);
Expand Down
Loading