Reduce pystring usages by meimchu · Pull Request #2313 · AcademySoftwareFoundation/OpenColorIO

meimchu · 2026-05-19T01:49:08Z

First step of pystring removal by improving StringUtils (#2256)
StringUtils have been updated to include some pystring functionalities. Focused on replacing:

pystring::split()
pystring::strip()
pystring::replace()
pystring::mul()
pystring::startswith()
pystring::endswith()

These usages have also been replaced throughout the modules that use these as well. Only pystring::os usages should be left now.

Additional changes of note:

Temporarily changed ci_workflow.yml. Additional explanation below.
Added an additional include path for OpenFX build.

@KevinJW please take a look when you have a moment! Thank you.

meimchu · 2026-05-19T01:52:50Z

I have temporarily removed a couple of combinations of our CICD Linux tests here to at least allow more tests to run. Since one tests from the OS category can stop the other jobs from running.

There is an on-going discussions in Slack regarding some Linux combinations and the recent aswf/ci-ocio:2023 docker image update. That is not the focus of this PR so I have temporarily removed it as a distraction.

I will not merge this request without a conclusion is reached in that regards.

The initial message from Jean-Francois Panisset:

See my comments in #wg-ci, it does seem like the update of the aswf/ci-ocio:2023 image from 2023.2 to 2023.3 has "changed something", and now the self build of OpenEXR 3.4 (the version from 2025) is breaking since that makes use of C++ 20 features.

Quick workaround would be to pin this build to use the aswf/ci-ocio:2023.2 image which should restore the previous behavior, but more generally it would be interesting to figure out "what does building latest OCIO with VFX 2025 OpenEXR in a VFX 2023 environment" really mean, and is that something we need / want to support.

KevinJW · 2026-06-03T08:49:33Z

+    if (n <= 0) return "";
+    if (n == 1) return str;
+
+    std::ostringstream os;


I wonder if something like the following might be 'better'/faster

std::string repeat_fast(const std::string& input, std::size_t n) { if (n == 0) { return {}; } if (n == 1) { return input; } std::string result; const auto input_size = input.size(); result.reserve(input_size * n); result = input; std::size_t current = 1; while (current * 2 <= n) { result += result; current *= 2; } const auto remaining = n - current; if (remaining > 0) { result += std::string_view(result.data(), input_size * remaining); } return result; }

Edited because I nerd sniped myself and ran some performance tests
And again avoid an extra allocation

might want to call the function Repeat or Repeat_n rather than Multiply.

Note that reserve() can raise an exception if we would go over std::string::max_size() but likely we would run out of memory before that happens.

What a wonderful suggestion. I just want to speak briefly on what I've done so far to get your eyes on it.

Implemented your function above as Repeat(). Made some small changes for syntax consistency.

I attempted to put a max_limit check as you've pointed out prior to reserve(). I chose not to throw an exception but I'm open to suggestions. It is a new behaviour compared to the old pystring::mul.

I have replaced the single StringUtils::Multiply() usage found in the code base (which was using pystring::mul() before).

I did do a quick benchmark testing between Multiply() and Repeat(). I got a string of 23 chars in length and repeated it 1000 times. The difference is significant and I like the pointer arithmetic in this implementation. I'm happy to remove Multiply() completely in favour of Repeat() if that is the desired decision.

Multiply() Total Time: 388.173 ms Multiply() Avg Time: 0.0388174 ms/iter Repeat() Total Time: 4.99192 ms Repeat() Avg Time: 0.000499192 ms/iter

KevinJW · 2026-06-03T08:51:14Z

-// In place replace the 'search' substring by the 'replace' string in 'str'.
-inline bool ReplaceInPlace(std::string & subject, const std::string & search, const std::string & replace)
+// In place replace the 'search' substring by the 'replace' string in 'subject'. Limited by 'count'.
+inline bool ReplaceInPlace(std::string & subject, const std::string & search, const std::string & replace, int count)


count should possibly be a size_t ? if we really need signed values then C++20 has std::ssize_t or we could use std::ptrdiff_t

Thank you for this suggestion. I've replaced it with size_t as you've suggested to make it consistent with the other size_t usages. As well, I've removed a signed integer comparison ReplaceInPlace() accordingly.

KevinJW · 2026-06-03T08:51:29Z


-    size_t pos =  0;
+    size_t pos = 0;
+    int iter = 0;


More size_t replacement as suggested!

KevinJW · 2026-06-03T08:51:53Z

+}
+
+// Replace the 'search' substring by the 'replace' string in 'subject'. Limited by 'count'.
+inline std::string Replace(const std::string & subject, const std::string & search, const std::string & replace, int count)


More size_t replacement as suggested!

KevinJW · 2026-06-04T08:48:58Z

+    // Check for overflow if n is greater than maximum repeatable amount via string max_size.
+    // New behaviour compared to pystring::mul and StringUtil::Multiply.
+    // limits.h says size_t max size is 18446744073709551615.
+    if (n > str.max_size() / str_size) { return {}; }


Ah I didn't mean you needed to detect this case, I don't think you need this. if you remove it then the reserve() call below will deal with it. With this check you could think of it as silently "failing".

Removed and let it YOLO! When I was testing this, I did mistakenly let it ran and it filled up my swap pretty fast. Also of note is that this is a private function only used in Op's std::string SerializeOpVec(const OpRcPtrVec & ops, int indent=0);. I have reservations about that function's indent parameter as int. As SerializeOpVec is internal and infrequently used, I have switched this to size_t to remove negative integer possibility. If a contributor decided to use Repeat() or SerializeOpVec() with a negative number though, then they're going to have a bad time!

KevinJW · 2026-06-04T08:51:12Z

+inline std::string Repeat(const std::string & str, size_t n)
+{
+    // Early exit and match pystring::mul behaviour.
+    if (n == 0) { return {}; }


I've not benchmarked it but it is possible that under some circumstances we might want to check || str.empty() to catch the case of appending an empty string many times. though the extra test probably slows down the function when n is small.

I agree with you that doing an early check on str.empty() for early return makes sense. I ran a benchmark on both Multiply() and Repeat() with an empty string input and 10,000 repeats. Both Multiply() and Repeat() will return itself and Repeat() just barely beat out Multiply(). I think at this point, I feel pretty good about just removing Multiply() in favour of Repeat() if you agree with it.

Multiply() Total Time: 0.129125 ms Multiply() Avg Time: 1.29125e-05 ms/iter Repeat() Total Time: 0.119125 ms Repeat() Avg Time: 1.19125e-05 ms/iter

Copilot

Pull request overview

This PR is a first step toward removing the pystring dependency by expanding StringUtils to cover several pystring string-manipulation helpers and replacing those usages across the codebase. It also refactors GPU test infrastructure to support multiple GPU backends and introduces an experimental DirectX 12 backend (plus supporting CMake and CI wiring).

Changes:

Add/extend StringUtils helpers (trim-by-char-set, bounded replace, repeat/multiply) and replace several pystring::* call sites.
Introduce a GraphicalApp abstraction and add a DirectX 12 GPU test backend (plus GPU test harness refactor and tolerance updates).
Add CMake support for DirectX-Headers + DXC runtime discovery/copying and update docs/CI configuration accordingly.

Reviewed changes

Copilot reviewed 37 out of 40 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
vendor/openfx/OCIOUtils.h	Add `StringUtils` include for OpenFX integration.
vendor/openfx/OCIOUtils.cpp	Replace `pystring` helpers with `StringUtils` equivalents.
vendor/openfx/CMakeLists.txt	Add `${PROJECT_SOURCE_DIR}/src/` include path for `utils/StringUtils.h`.
tests/utils/StringUtils_tests.cpp	Expand unit tests to cover new trim/replace/repeat behavior.
tests/gpu/MatrixOp_test.cpp	Adjust epsilon to accommodate DirectX rounding differences.
tests/gpu/GPUUnitTest.cpp	Refactor to use `GraphicalApp` and add DirectX/Vulkan backend selection.
tests/gpu/FixedFunctionOp_test.cpp	Relax thresholds to account for DirectX precision differences.
tests/gpu/CMakeLists.txt	Split GPU tests per backend; add DXC runtime copying for DirectX.
tests/cpu/Op_tests.cpp	Add indentation-prefix assertion for `SerializeOpVec`.
src/utils/StringUtils.h	Add trim-by-char-set overloads, bounded replace, multiply/repeat.
src/OpenColorIO/transforms/FileTransform.cpp	Replace `pystring::replace` with `StringUtils::Replace`.
src/OpenColorIO/Op.h	Change `SerializeOpVec` indent type to `size_t`.
src/OpenColorIO/Op.cpp	Remove `pystring` usage; use `StringUtils::Repeat` for indentation.
src/OpenColorIO/OCIOZArchive.cpp	Replace `pystring::lstrip` with `StringUtils::LeftTrim`.
src/OpenColorIO/fileformats/FileFormatIridasLook.cpp	Replace `pystring::strip` with `StringUtils::Trim`.
src/OpenColorIO/Config.cpp	Replace `pystring::split/startswith` with `StringUtils` equivalents.
src/libutils/oglapphelpers/vulkanapp.h	Make `VulkanApp` derive from `GraphicalApp` and override API.
src/libutils/oglapphelpers/vulkanapp.cpp	Update Vulkan app to use `GraphicalApp` verbosity and buffer API.
src/libutils/oglapphelpers/oglapp.h	Make `OglApp` derive from `GraphicalApp`; rename Screen/Headless classes.
src/libutils/oglapphelpers/oglapp.cpp	Implement `OglApp::CreateApp`; rename APIs; adjust GLUT include workaround.
src/libutils/oglapphelpers/metalapp.mm	Update inheritance and verbosity calls to match refactor.
src/libutils/oglapphelpers/metalapp.h	Update base class name references after refactor.
src/libutils/oglapphelpers/hlsl.h	Add `HLSLBuilder` for DirectX LUT upload/allocation.
src/libutils/oglapphelpers/hlsl.cpp	Implement DirectX LUT texture upload + SRV creation.
src/libutils/oglapphelpers/graphicalapp.h	Add new abstract GPU-app interface shared by OGL/Vulkan/DX.
src/libutils/oglapphelpers/graphicalapp.cpp	Implement factory selection for `GraphicalApp::CreateApp`.
src/libutils/oglapphelpers/dxutils.h	Add D3D12 helper utilities (HRESULT, row pitch alignment).
src/libutils/oglapphelpers/dxapp.h	Add DirectX 12 `GraphicalApp` implementation declaration.
src/libutils/oglapphelpers/dxapp.cpp	Implement DirectX 12 backend for GPU tests (DXC compile, render, readback).
src/libutils/oglapphelpers/CMakeLists.txt	Build oglapphelpers conditionally for GL/DX/Vulkan; add DirectX-Headers dep.
src/apps/ociodisplay/main.cpp	Switch to `GraphicalApp` pointer type and updated method names.
src/apps/ocioconvert/main.cpp	Use `GraphicalApp::CreateApp` and updated buffer/verbose methods.
src/apps/ociochecklut/main.cpp	Use `GraphicalApp` API and updated method names.
share/cmake/utils/LocateDXCompilerRuntime.cmake	Locate/copy DXC runtime DLLs and report version for test runtime.
share/cmake/modules/install/InstallDirectX-Headers.cmake	Fetch/install DirectX-Headers when missing.
share/cmake/modules/FindDirectX-Headers.cmake	Find DirectX-Headers via config or header search; provide target fallback.
SECURITY.md	Move CVE history section and add new CVE entry.
docs/quick_start/installation.rst	Document new `OCIO_DIRECTX_ENABLED` / `OCIO_VULKAN_ENABLED` options.
CMakeLists.txt	Add `OCIO_DIRECTX_ENABLED` (Windows-only) and `OCIO_VULKAN_ENABLED` options.
.github/workflows/ci_workflow.yml	Allow container-tag override; disable fail-fast for matrix runs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+inline std::string LeftTrim(std::string str, const std::string & prefix)
+{
+    size_t first_good_char = str.find_first_not_of(prefix);
+    if (first_good_char == std::string::npos) { return str; }
+    str.erase(0, first_good_char);
+    return str;
+}


+inline std::string RightTrim(std::string str, const std::string & suffix)
+{
+    size_t last_good_char = str.find_last_not_of(suffix);
+    if (last_good_char == std::string::npos) { return str; }
+    str.erase(last_good_char + 1);
+    return str;
+}


+    windowClass.style = CS_HREDRAW | CS_VREDRAW;
+    windowClass.lpfnWndProc = WindowProc;
+    windowClass.hInstance = NULL;
+    windowClass.hCursor = LoadCursor(NULL, IDC_ARROW);
+    windowClass.lpszClassName = winTitle;


+        windowRect.right - windowRect.left,
+        windowRect.bottom - windowRect.top,
+        NULL,        // We have no parent window.
+        NULL,        // We aren't using menus.
+        NULL,
+        NULL);


+        NULL,
+        NULL);
+
+    ShowWindow(m_hwnd, SW_RESTORE);
+


 	# Add folders for glut and glew DLLs.
 	set(extra_dirs "${GLUT_INCLUDE_DIR}/../bin\\;${GLEW_INCLUDE_DIRS}/../bin\\")


cozdas · 2026-06-24T04:31:43Z

Looks like commits
57a94a8 (DX12)
907a643 (Linux container)
f705d2e (CVE)
are replayed (cherry picked?) into the PR branch with different commit ID's and that confuses the PR diff.

I think rebasing your branch to main and force-pushing your branch should clear the duplicate changes and remove the extra changes listed here.

Signed-off-by: Mei Chu <meimchu@gmail.com>

meimchu · 2026-07-03T23:45:12Z

Looks like commits 57a94a8 (DX12) 907a643 (Linux container) f705d2e (CVE) are replayed (cherry picked?) into the PR branch with different commit ID's and that confuses the PR diff.

I think rebasing your branch to main and force-pushing your branch should clear the duplicate changes and remove the extra changes listed here.

Thanks for your feedback! I have done the rebase as you've suggested. I did pull in a bunch of changes from other commits the last time I rebased to get an updated CICD workflow (it was busted for a while). I wonder if there has been recent changes again to CICD that's causing different issues again?

Docs building seems to be failing in the last few actions/jobs the last couple of days.
I'm not entirely sure why, but C++20 builds are mostly failing now for linux platform. I see errors in there about linker with opencolorio_v2_3 namespace... while linking ociolutimage? I am not sure why this is happening and don't believe I've done anything that would impact ociolutimage like this. Any help regarding this would be much appreciated!

meimchu · 2026-07-03T23:58:57Z

https://github.com/AcademySoftwareFoundation/OpenColorIO/actions/runs/28687774279/job/85083494812?pr=2313#step:2:258

In the initializing containers step, I see ASWF_OPENCOLORIO_VERSION=2.3.2. I wonder if that is a hint of what's causing this issue? I also see it later doing a git sparse-checkout, so maybe there is some clobbering going on there?

meimchu marked this pull request as ready for review May 22, 2026 04:23

meimchu force-pushed the feature/reduce_pystring_usages branch from ba9f569 to d24be22 Compare May 27, 2026 07:18

meimchu mentioned this pull request May 29, 2026

Proposal to remove pystring dependency #2256

Open

KevinJW reviewed Jun 3, 2026

View reviewed changes

KevinJW reviewed Jun 4, 2026

View reviewed changes

cozdas requested a review from Copilot June 24, 2026 03:17

Copilot started reviewing on behalf of cozdas June 24, 2026 03:18 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

meimchu added 19 commits July 3, 2026 16:03

Replaced split(), startswith() and endswith() pystring usages

230853f

Signed-off-by: Mei Chu <meimchu@gmail.com>

Add utils as include path for openfx build

fc23cca

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix silly char issue

ad324ce

Signed-off-by: Mei Chu <meimchu@gmail.com>

Update StringUtils Trim functionalities.

e906f5a

Signed-off-by: Mei Chu <meimchu@gmail.com>

Add tests and Replace

e57ce09

Signed-off-by: Mei Chu <meimchu@gmail.com>

Add more Replace tests.

4d6a859

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix test

c7505f1

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix test

3339fa4

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix replace test

68957b4

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix replace test

8694cf7

Signed-off-by: Mei Chu <meimchu@gmail.com>

More replace test fix

a0c02ec

Signed-off-by: Mei Chu <meimchu@gmail.com>

Replace pystring::replace in FileTransform

6a9f8bf

Signed-off-by: Mei Chu <meimchu@gmail.com>

Test changed cicd

c277336

Signed-off-by: Mei Chu <meimchu@gmail.com>

Update CICD test

25f596e

Signed-off-by: Mei Chu <meimchu@gmail.com>

Integrate StringUtils::Multiply

160c1ec

Signed-off-by: Mei Chu <meimchu@gmail.com>

Small syntax update.

f39f593

Signed-off-by: Mei Chu <meimchu@gmail.com>

Revert workflow.yml to get ready for real fix.

9417f9b

Signed-off-by: Mei Chu <meimchu@gmail.com>

Test changed cicd

f41b7eb

Signed-off-by: Mei Chu <meimchu@gmail.com>

Update CICD test

75ba8e1

Signed-off-by: Mei Chu <meimchu@gmail.com>

meimchu added 14 commits July 3, 2026 16:07

Revert workflow.yml to get ready for real fix.

e84b2b4

Signed-off-by: Mei Chu <meimchu@gmail.com>

Address notes to use size_t instead of int for count parameter.

d2d5368

Signed-off-by: Mei Chu <meimchu@gmail.com>

Add Repeat() function to prepare to replace Multiply().

92e37dd

Signed-off-by: Mei Chu <meimchu@gmail.com>

Use Repeat() in Op::SerializeOpVec. Add more tests

0ef5d09

Signed-off-by: Mei Chu <meimchu@gmail.com>

Remove the limit test for Repeat.

ddd0f48

Signed-off-by: Mei Chu <meimchu@gmail.com>

Update Repeat and Multiply accordingly.

c226670

Signed-off-by: Mei Chu <meimchu@gmail.com>

Add SerializeOpVec output prefix check.

6be11f5

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix test

42a38f5

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix test

2048eb9

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix test

e488851

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix test

355aa32

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix test

ac7647c

Signed-off-by: Mei Chu <meimchu@gmail.com>

Fix test

27fd8bd

Signed-off-by: Mei Chu <meimchu@gmail.com>

Op::SerializeOpVec switch to size_t

989ac0e

Signed-off-by: Mei Chu <meimchu@gmail.com>

meimchu force-pushed the feature/reduce_pystring_usages branch from 49a3222 to 989ac0e Compare July 3, 2026 23:09

Not changing ci_workflow.yml

b9d9493

Signed-off-by: Mei Chu <meimchu@gmail.com>

		# Add folders for glut and glew DLLs.
		set(extra_dirs "${GLUT_INCLUDE_DIR}/../bin\\;${GLEW_INCLUDE_DIRS}/../bin\\")

Uh oh!

Conversation

meimchu commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meimchu commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinJW Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meimchu Jun 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

cozdas commented Jun 24, 2026

Uh oh!

meimchu commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meimchu commented Jul 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

meimchu commented May 19, 2026 •

edited

Loading

meimchu commented May 19, 2026 •

edited

Loading

KevinJW Jun 3, 2026 •

edited

Loading

meimchu Jun 6, 2026 •

edited

Loading

meimchu commented Jul 3, 2026 •

edited

Loading