From 58565ca4eba683e85b7f0b48c0216b0678a4c93b Mon Sep 17 00:00:00 2001 From: Doug Walker Date: Sat, 3 May 2025 19:45:46 -0400 Subject: [PATCH 1/5] Fix gamma composition issue Signed-off-by: Doug Walker --- src/OpenColorIO/ops/gamma/GammaOpData.cpp | 17 ++-- tests/cpu/OpOptimizers_tests.cpp | 65 ++++++++++++++++ tests/cpu/ops/gamma/GammaOpData_tests.cpp | 95 ++++++++++++++++++++--- tests/data/files/gamma_comp_test2.ctf | 32 ++++++++ 4 files changed, 193 insertions(+), 16 deletions(-) create mode 100644 tests/data/files/gamma_comp_test2.ctf diff --git a/src/OpenColorIO/ops/gamma/GammaOpData.cpp b/src/OpenColorIO/ops/gamma/GammaOpData.cpp index 482a5b79d7..baf0bc7288 100644 --- a/src/OpenColorIO/ops/gamma/GammaOpData.cpp +++ b/src/OpenColorIO/ops/gamma/GammaOpData.cpp @@ -659,20 +659,24 @@ namespace GammaOpData::Style CombineBasicStyles(GammaOpData::Style a, GammaOpData::Style b) { // This function assumes that mayCompose was called on the inputs and returned true. - // The logic here is only valid for that situation. + // The logic here is only valid for that situation. There is no intent to preserve + // the direction, a forward style is always returned. if (a == GammaOpData::BASIC_FWD || a == GammaOpData::BASIC_REV || b == GammaOpData::BASIC_FWD || b == GammaOpData::BASIC_REV) { + // If either a or b is a BASIC style, that is the combined style since the + // BASIC style clamps negatives, so the combination must also clamp. return GammaOpData::BASIC_FWD; } else if (a == GammaOpData::BASIC_MIRROR_FWD || a == GammaOpData::BASIC_MIRROR_REV) { - // b BASIC_MIRROR. + // Neither a or b is a BASIC style, so it may only be MIRROR or PASS_THRU, but + // mayCompose will not allow b to be PASS_THRU in this case, both are MIRROR. return GammaOpData::BASIC_MIRROR_FWD; } - else // a is BASIC_PASS_THRU (ensured by mayCompose). + else { - // b BASIC_PASS_THRU. + // Both a and b are BASIC_PASS_THRU as a consequence of mayCompose being true. return GammaOpData::BASIC_PASS_THRU_FWD; } } @@ -715,7 +719,7 @@ GammaOpDataRcPtr GammaOpData::compose(const GammaOpData & B) const double b1 = getBlueParams()[0]; double a1 = getAlphaParams()[0]; - if (styleA == BASIC_REV || styleA == BASIC_MIRROR_REV || styleA == BASIC_PASS_THRU_FWD) + if (styleA == BASIC_REV || styleA == BASIC_MIRROR_REV || styleA == BASIC_PASS_THRU_REV) { r1 = 1. / r1; g1 = 1. / g1; @@ -727,7 +731,7 @@ GammaOpDataRcPtr GammaOpData::compose(const GammaOpData & B) const double g2 = B.getGreenParams()[0]; double b2 = B.getBlueParams()[0]; double a2 = B.getAlphaParams()[0]; - if (styleB == BASIC_REV || styleB == BASIC_MIRROR_REV || styleB == BASIC_PASS_THRU_FWD) + if (styleB == BASIC_REV || styleB == BASIC_MIRROR_REV || styleB == BASIC_PASS_THRU_REV) { r2 = 1. / r2; g2 = 1. / g2; @@ -747,6 +751,7 @@ GammaOpDataRcPtr GammaOpData::compose(const GammaOpData & B) const RoundAround1(bOut); RoundAround1(aOut); + // NB: This always returns a forward style. Style style = CombineBasicStyles(styleA, styleB); // By convention, we try to keep the gamma parameter > 1. diff --git a/tests/cpu/OpOptimizers_tests.cpp b/tests/cpu/OpOptimizers_tests.cpp index 6806ccba09..bffb1b6ad1 100644 --- a/tests/cpu/OpOptimizers_tests.cpp +++ b/tests/cpu/OpOptimizers_tests.cpp @@ -49,6 +49,7 @@ void CompareRender(OCIO::OpRcPtrVec & ops1, OCIO::OpRcPtrVec & ops2, for (const auto & op : ops1) { + // NB: This hard-codes OPTIMIZATION_FAST_LOG_EXP_POW to off, see Op.h. op->apply(&img1[0], &img1[0], nbPixels); } @@ -1070,6 +1071,70 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp) CompareRender(ops, optOps, __LINE__, 1e-4f, true); } +OCIO_ADD_TEST(OpOptimizers, gamma_comp_test2) +{ + // This transform has a pair of gammas separated by a pair of matrices that + // compose into an identity matrix and get optimized out. Then the gammas + // get composed into a non-identity gamma. Finally the exponent is inverted + // (to follow the convention of keeping it > 1) and the direction is inverted. + + const std::string fileName("gamma_comp_test2.ctf"); + OCIO::OpRcPtrVec ops; + OCIO::ContextRcPtr context = OCIO::Context::Create(); + OCIO_CHECK_NO_THROW(OCIO::BuildOpsTest(ops, fileName, context, + OCIO::TRANSFORM_DIR_FORWARD)); + + // First one is the file no op. + OCIO_CHECK_EQUAL(ops.size(), 5); + + // Remove no ops & finalize for computation. + OCIO_CHECK_NO_THROW(ops.finalize()); + OCIO_CHECK_NO_THROW(ops.optimize(OCIO::OPTIMIZATION_NONE)); + + OCIO_CHECK_EQUAL(ops.size(), 4); + + OCIO::OpRcPtrVec optOps = ops.clone(); + OCIO::OpRcPtrVec optOps_noComp = ops.clone(); + + OCIO_CHECK_EQUAL(optOps_noComp.size(), 4); + OCIO_CHECK_NO_THROW(optOps_noComp.finalize()); + // NB: The op->apply function used here hard-codes OPTIMIZATION_FAST_LOG_EXP_POW to off. + OCIO_CHECK_NO_THROW(optOps_noComp.optimize(AllBut(OCIO::OPTIMIZATION_COMP_GAMMA))); + OCIO_CHECK_EQUAL(optOps_noComp.size(), 2); + OCIO_CHECK_EQUAL(optOps_noComp[0]->getInfo(), ""); + OCIO_CHECK_EQUAL(optOps_noComp[1]->getInfo(), ""); + + // Due to rounding error in the two 3x3 matrix multiplies with much larger values, the + // 1.52e-4 input value is off by 60% going into the second gamma (see ociochecklut -s). + // Therefore the optOps_noComp and optOps are actually more accurate than ops here. + CompareRender(ops, optOps_noComp, __LINE__, 1e-4f); + + OCIO_CHECK_NO_THROW(optOps.finalize()); + OCIO_CHECK_NO_THROW(optOps.optimize(OCIO::OPTIMIZATION_DEFAULT)); + + // Now check that the optimized transform renders the same as the original. + CompareRender(ops, optOps, __LINE__, 1e-4f); + + // Check the op is as expected. + OCIO::GroupTransformRcPtr group = OCIO::GroupTransform::Create(); + OCIO_REQUIRE_EQUAL(optOps.size(), 1); + OCIO::ConstOpRcPtr op(optOps[0]); + OCIO::CreateGammaTransform(group, op); + OCIO_REQUIRE_EQUAL(group->getNumTransforms(), 1); + auto transform = group->getTransform(0); + OCIO_REQUIRE_ASSERT(transform); + auto gTransform = OCIO_DYNAMIC_POINTER_CAST(transform); + OCIO_REQUIRE_ASSERT(gTransform); + OCIO_CHECK_EQUAL(gTransform->getNegativeStyle(), OCIO::NEGATIVE_PASS_THRU); + OCIO_CHECK_EQUAL(gTransform->getDirection(), OCIO::TRANSFORM_DIR_INVERSE); + double vals[4]; + gTransform->getValue(vals); + OCIO_CHECK_CLOSE(vals[0], 2.2/1.8, 1e-6f); + OCIO_CHECK_CLOSE(vals[1], 2.2/1.8, 1e-6f); + OCIO_CHECK_CLOSE(vals[2], 2.2/1.8, 1e-6f); + OCIO_CHECK_EQUAL(vals[3], 1.); +} + OCIO_ADD_TEST(OpOptimizers, gamma_comp_identity) { OCIO::OpRcPtrVec ops; diff --git a/tests/cpu/ops/gamma/GammaOpData_tests.cpp b/tests/cpu/ops/gamma/GammaOpData_tests.cpp index e243b9fc3d..24beb6a280 100644 --- a/tests/cpu/ops/gamma/GammaOpData_tests.cpp +++ b/tests/cpu/ops/gamma/GammaOpData_tests.cpp @@ -579,7 +579,8 @@ void CheckGammaCompose(OCIO::GammaOpData::Style style1, OCIO::GammaOpData::Style style2, const OCIO::GammaOpData::Params & params2, OCIO::GammaOpData::Style refStyle, - const OCIO::GammaOpData::Params & refParams) + const OCIO::GammaOpData::Params & refParams, + unsigned line) { static const OCIO::GammaOpData::Params paramsA = { 1. }; @@ -591,12 +592,12 @@ void CheckGammaCompose(OCIO::GammaOpData::Style style1, const OCIO::GammaOpDataRcPtr g3 = g1.compose(g2); - OCIO_CHECK_EQUAL(g3->getStyle(), refStyle); + OCIO_CHECK_EQUAL_FROM(g3->getStyle(), refStyle, line); - OCIO_CHECK_ASSERT(g3->getRedParams() == refParams); - OCIO_CHECK_ASSERT(g3->getGreenParams() == refParams); - OCIO_CHECK_ASSERT(g3->getBlueParams() == refParams); - OCIO_CHECK_ASSERT(g3->getAlphaParams() == paramsA); + OCIO_CHECK_ASSERT_FROM(g3->getRedParams() == refParams, line); + OCIO_CHECK_ASSERT_FROM(g3->getGreenParams() == refParams, line); + OCIO_CHECK_ASSERT_FROM(g3->getBlueParams() == refParams, line); + OCIO_CHECK_ASSERT_FROM(g3->getAlphaParams() == paramsA, line); } }; @@ -610,7 +611,8 @@ OCIO_ADD_TEST(GammaOpData, compose) CheckGammaCompose(OCIO::GammaOpData::BASIC_FWD, params1, OCIO::GammaOpData::BASIC_FWD, params2, - OCIO::GammaOpData::BASIC_FWD, refParams); + OCIO::GammaOpData::BASIC_FWD, refParams, + __LINE__); } { @@ -620,7 +622,8 @@ OCIO_ADD_TEST(GammaOpData, compose) CheckGammaCompose(OCIO::GammaOpData::BASIC_REV, params1, OCIO::GammaOpData::BASIC_REV, params2, - OCIO::GammaOpData::BASIC_REV, refParams); + OCIO::GammaOpData::BASIC_REV, refParams, + __LINE__); } { @@ -630,7 +633,8 @@ OCIO_ADD_TEST(GammaOpData, compose) CheckGammaCompose(OCIO::GammaOpData::BASIC_REV, params1, OCIO::GammaOpData::BASIC_FWD, params2, - OCIO::GammaOpData::BASIC_REV, refParams); + OCIO::GammaOpData::BASIC_REV, refParams, + __LINE__); } { @@ -640,7 +644,78 @@ OCIO_ADD_TEST(GammaOpData, compose) CheckGammaCompose(OCIO::GammaOpData::BASIC_REV, params1, OCIO::GammaOpData::BASIC_FWD, params2, - OCIO::GammaOpData::BASIC_FWD, refParams); + OCIO::GammaOpData::BASIC_FWD, refParams, + __LINE__); + } + + { + const OCIO::GammaOpData::Params params1 = { 2. }; + const OCIO::GammaOpData::Params params2 = { 4. }; + const OCIO::GammaOpData::Params refParams = { 2. }; + + CheckGammaCompose(OCIO::GammaOpData::BASIC_FWD, params1, + OCIO::GammaOpData::BASIC_REV, params2, + OCIO::GammaOpData::BASIC_REV, refParams, + __LINE__); + } + + { + const OCIO::GammaOpData::Params params1 = { 2. }; + const OCIO::GammaOpData::Params params2 = { 4. }; + const OCIO::GammaOpData::Params refParams = { 2. }; + + CheckGammaCompose(OCIO::GammaOpData::BASIC_PASS_THRU_FWD, params1, + OCIO::GammaOpData::BASIC_PASS_THRU_REV, params2, + OCIO::GammaOpData::BASIC_PASS_THRU_REV, refParams, + __LINE__); + } + + { + const OCIO::GammaOpData::Params params1 = { 2. }; + const OCIO::GammaOpData::Params params2 = { 4. }; + const OCIO::GammaOpData::Params refParams = { 2. }; + + CheckGammaCompose(OCIO::GammaOpData::BASIC_MIRROR_FWD, params1, + OCIO::GammaOpData::BASIC_MIRROR_REV, params2, + OCIO::GammaOpData::BASIC_MIRROR_REV, refParams, + __LINE__); + } + + { + const OCIO::GammaOpData::Params params1 = { 2. }; + const OCIO::GammaOpData::Params params2 = { 4. }; + const OCIO::GammaOpData::Params refParams = { 2. }; + + CheckGammaCompose(OCIO::GammaOpData::BASIC_MIRROR_FWD, params1, + OCIO::GammaOpData::BASIC_REV, params2, + OCIO::GammaOpData::BASIC_REV, refParams, + __LINE__); + } + + { + const OCIO::GammaOpData::Params params1 = { 2. }; + const OCIO::GammaOpData::Params params2 = { 4. }; + const OCIO::GammaOpData::Params refParams = { 2. }; + + CheckGammaCompose(OCIO::GammaOpData::BASIC_PASS_THRU_FWD, params1, + OCIO::GammaOpData::BASIC_REV, params2, + OCIO::GammaOpData::BASIC_REV, refParams, + __LINE__); + } + + { + const OCIO::GammaOpData::Params params1 = { 4. }; + OCIO::GammaOpData::Params paramsA = { 1. }; + OCIO::GammaOpData g1(OCIO::GammaOpData::BASIC_MIRROR_FWD, + params1, params1, params1, paramsA); + + OCIO::GammaOpData::Params params2 = { 2. }; + OCIO::GammaOpData g2(OCIO::GammaOpData::BASIC_PASS_THRU_FWD, + params2, params2, params2, paramsA); + + OCIO_CHECK_THROW_WHAT(g1.compose(g2), + OCIO::Exception, + "GammaOp can only be combined with some GammaOps"); } { diff --git a/tests/data/files/gamma_comp_test2.ctf b/tests/data/files/gamma_comp_test2.ctf new file mode 100644 index 0000000000..08e225088b --- /dev/null +++ b/tests/data/files/gamma_comp_test2.ctf @@ -0,0 +1,32 @@ + + + + Transform should optimize into a basicPassThruRev with gamma=2.2/1.8 + + + + + + + Rec.709 to XYZ + +0.412390799266 0.357584339384 0.180480788402 +0.212639005872 0.715168678768 0.072192315361 +0.019330818716 0.119194779795 0.95053215225 + + + + + XYZ to Rec.709 + + 3.240969941905 -1.53738317757 -0.498610760293 +-0.969243636281 1.875967501508 0.041555057407 + 0.055630079697 -0.203976958889 1.056971514243 + + + + + + + + From d1d4e2dd8373c9da3c14decc82aab934fa81a3e2 Mon Sep 17 00:00:00 2001 From: Doug Walker Date: Sat, 3 May 2025 20:36:06 -0400 Subject: [PATCH 2/5] Adjust docs build settings to fix CI Signed-off-by: Doug Walker --- .github/workflows/ci_workflow.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci_workflow.yml b/.github/workflows/ci_workflow.yml index 07db553ea4..e0e152e27b 100644 --- a/.github/workflows/ci_workflow.yml +++ b/.github/workflows/ci_workflow.yml @@ -489,7 +489,7 @@ jobs: arch-type: "x86_64" build-type: Release build-shared: 'ON' - build-docs: 'ON' + build-docs: 'OFF' build-openfx: 'OFF' use-simd: 'OFF' use-oiio: 'OFF' @@ -624,7 +624,7 @@ jobs: test-rosetta: "OFF" build-type: Release build-shared: 'ON' - build-docs: 'OFF' + build-docs: 'ON' build-openfx: 'OFF' use-simd: 'ON' use-oiio: 'OFF' From bbd3368e12f4027a9bd9d41dc4e9bb081ab60fd3 Mon Sep 17 00:00:00 2001 From: Doug Walker Date: Sat, 3 May 2025 21:17:59 -0400 Subject: [PATCH 3/5] Adjust docs build settings to fix CI Signed-off-by: Doug Walker --- .github/workflows/ci_workflow.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci_workflow.yml b/.github/workflows/ci_workflow.yml index e0e152e27b..c9485e8467 100644 --- a/.github/workflows/ci_workflow.yml +++ b/.github/workflows/ci_workflow.yml @@ -624,7 +624,7 @@ jobs: test-rosetta: "OFF" build-type: Release build-shared: 'ON' - build-docs: 'ON' + build-docs: 'OFF' build-openfx: 'OFF' use-simd: 'ON' use-oiio: 'OFF' From a9568362158a52811ceabba17887733a89a33db3 Mon Sep 17 00:00:00 2001 From: Doug Walker Date: Sat, 17 May 2025 14:16:18 -0400 Subject: [PATCH 4/5] Revert CI change Signed-off-by: Doug Walker --- .github/workflows/ci_workflow.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci_workflow.yml b/.github/workflows/ci_workflow.yml index c9485e8467..07db553ea4 100644 --- a/.github/workflows/ci_workflow.yml +++ b/.github/workflows/ci_workflow.yml @@ -489,7 +489,7 @@ jobs: arch-type: "x86_64" build-type: Release build-shared: 'ON' - build-docs: 'OFF' + build-docs: 'ON' build-openfx: 'OFF' use-simd: 'OFF' use-oiio: 'OFF' From 49af1f1a6b3ee0ef71b1f18cdb5f2c0f5529b829 Mon Sep 17 00:00:00 2001 From: Doug Walker Date: Sat, 17 May 2025 14:44:11 -0400 Subject: [PATCH 5/5] Minor comment tweak Signed-off-by: Doug Walker --- include/OpenColorIO/OpenColorIO.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/OpenColorIO/OpenColorIO.h b/include/OpenColorIO/OpenColorIO.h index b7183ce77f..c5e499ff78 100644 --- a/include/OpenColorIO/OpenColorIO.h +++ b/include/OpenColorIO/OpenColorIO.h @@ -870,7 +870,7 @@ class OCIOEXPORT Config const char * getDefaultView(const char * display, const char * colorspaceName) const; /** - * Return the number of views attached to the display including the number of + * Return the number of active views attached to the display including the number of * shared views if any. Return 0 if display does not exist. */ int getNumViews(const char * display) const;