diff --git a/src/OpenColorIO/Config.cpp b/src/OpenColorIO/Config.cpp index 3bde3abb9..a4cf7c5fe 100644 --- a/src/OpenColorIO/Config.cpp +++ b/src/OpenColorIO/Config.cpp @@ -5268,7 +5268,9 @@ void Config::Impl::checkVersionConsistency(ConstTransformRcPtr & transform) cons || 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-500nit-REC2020-D60-in-REC2020-D65_2.0") || 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-1000nit-REC2020-D60-in-REC2020-D65_2.0") || 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-2000nit-REC2020-D60-in-REC2020-D65_2.0") - || 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-4000nit-REC2020-D60-in-REC2020-D65_2.0") ) + || 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-4000nit-REC2020-D60-in-REC2020-D65_2.0") + // NB: This one was added in OCIO 2.4.1. + || 0 == Platform::Strcasecmp(blt->getStyle(), "DISPLAY - CIE-XYZ-D65_to_DisplayP3-HDR") ) ) { std::ostringstream os; diff --git a/src/OpenColorIO/OpOptimizers.cpp b/src/OpenColorIO/OpOptimizers.cpp index 7788bfb16..58b395d5c 100755 --- a/src/OpenColorIO/OpOptimizers.cpp +++ b/src/OpenColorIO/OpOptimizers.cpp @@ -68,7 +68,7 @@ bool IsCombineEnabled(OpData::Type type, OptimizationFlags flags) (type == OpData::RangeType && HasFlag(flags, OPTIMIZATION_COMP_RANGE)); } -constexpr int MAX_OPTIMIZATION_PASSES = 8; +constexpr int MAX_OPTIMIZATION_PASSES = 80; int RemoveNoOpTypes(OpRcPtrVec & opVec) { @@ -317,8 +317,10 @@ int CombineOps(OpRcPtrVec & opVec, OptimizationFlags oFlags) op1->combineWith(tmpops, op2); FinalizeOps(tmpops); - // tmpops may have any number of ops in it. (0, 1, 2, ...) - // (size 0 would occur only if the combination results in a no-op). + // The tmpops may have any number of ops in it: (0, 1, 2, ...). + // (Size 0 would occur only if the combination results in a no-op, + // for example, a pair of matrices that compose into a no-op are + // returned as empty rather than as an identity matrix.) // // No matter the number, we need to swap them in for the original ops. @@ -336,6 +338,15 @@ int CombineOps(OpRcPtrVec & opVec, OptimizationFlags oFlags) // We've done something so increment the count! ++count; + + // Break, since combining ops is less desirable than other optimization options. + // For example, it is preferable to remove a pair of ops using RemoveInverseOps + // rather than combining them. Consider this example: + // Lut1D A --> Matrix B --> Matrix C --> Lut1D Ainv + // If Matrix B & C are not pair inverses but do combine into an identity, then + // CombineOps would compose Lut1D A & Ainv, into a new Lut1D rather than + // allowing another round of optimization which would remove them as inverses. + break; } else { @@ -629,7 +640,7 @@ void OpRcPtrVec::optimize(OptimizationFlags oFlags) os << "**" << std::endl; os << "Optimized "; os << originalSize << "->" << finalSize << ", 1 pass, "; - os << total_nooptype << " noop types removed\n"; + os << total_nooptype << " no-op types removed\n"; os << SerializeOpVec(*this, 4); LogDebug(os.str()); } @@ -664,11 +675,21 @@ void OpRcPtrVec::optimize(OptimizationFlags oFlags) while (passes <= MAX_OPTIMIZATION_PASSES) { + // Remove all ops for which isNoOp is true, including identity matrices. int noops = optimizeIdentity ? RemoveNoOps(*this) : 0; + + // Replace all complex ops with simpler ops (e.g., a CDL which only scales with a matrix). // Note this might increase the number of ops. int replacedOps = replaceOps ? ReplaceOps(*this) : 0; + + // Replace all complex identities with simpler ops (e.g., an identity Lut1D with a range). int identityops = ReplaceIdentityOps(*this, oFlags); + + // Remove all adjacent pairs of ops that are inverses of each other. int inverseops = RemoveInverseOps(*this, oFlags); + + // Combine a pair of ops, for example multiply two adjacent Matrix ops. + // (Combines at most one pair on each iteration.) int combines = CombineOps(*this, oFlags); if (noops + identityops + inverseops + combines == 0) @@ -721,12 +742,12 @@ void OpRcPtrVec::optimize(OptimizationFlags oFlags) os << "Optimized "; os << originalSize << "->" << finalSize << ", "; os << passes << " passes, "; - os << total_nooptype << " noop types removed, "; - os << total_noops << " noops removed, "; + os << total_nooptype << " no-op types removed, "; + os << total_noops << " no-ops removed, "; os << total_replacedops << " ops replaced, "; os << total_identityops << " identity ops replaced, "; - os << total_inverseops << " inverse ops removed, "; - os << total_combines << " ops combines, "; + os << total_inverseops << " inverse op pairs removed, "; + os << total_combines << " ops combined, "; os << total_inverses << " ops inverted\n"; os << SerializeOpVec(*this, 4); LogDebug(os.str()); diff --git a/tests/cpu/OpOptimizers_tests.cpp b/tests/cpu/OpOptimizers_tests.cpp index 6f606fd6e..6806ccba0 100644 --- a/tests/cpu/OpOptimizers_tests.cpp +++ b/tests/cpu/OpOptimizers_tests.cpp @@ -311,6 +311,8 @@ OCIO_ADD_TEST(OpOptimizers, combine_ops) OCIO::CombineOps(ops, AllBut(OCIO::OPTIMIZATION_COMP_MATRIX)); OCIO_CHECK_EQUAL(ops.size(), 3); OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL); + // CombineOps removes at most one pair on each call, repeat to combine all pairs. + OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL); OCIO_CHECK_EQUAL(ops.size(), 1); } @@ -351,6 +353,10 @@ OCIO_ADD_TEST(OpOptimizers, combine_ops) OCIO::CombineOps(ops, AllBut(OCIO::OPTIMIZATION_COMP_MATRIX)); OCIO_CHECK_EQUAL(ops.size(), 5); OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL); + // CombineOps removes at most one pair on each call, repeat to combine all pairs. + OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL); + OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL); + OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL); OCIO_CHECK_EQUAL(ops.size(), 1); } @@ -366,10 +372,57 @@ OCIO_ADD_TEST(OpOptimizers, combine_ops) OCIO::CombineOps(ops, AllBut(OCIO::OPTIMIZATION_COMP_MATRIX)); OCIO_CHECK_EQUAL(ops.size(), 4); OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL); + // CombineOps removes at most one pair on each call, repeat to combine all pairs. + OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL); OCIO_CHECK_EQUAL(ops.size(), 0); } } +OCIO_ADD_TEST(OpOptimizers, prefer_pair_inverse_over_combine) +{ + // When a pair of forward / inverse LUTs with non 0 to 1 domain are used + // as process space for a Look (eg. CDL), the Optimizer tries to combine + // them when the Look results in a no-op. Here we make sure this result + // in an appropriate clamp instead of a new half-domain LUT resulting + // from the naive composition of the two LUTs. + + OCIO::OpRcPtrVec ops; + + // This spi1d uses "From -1.0 2.0", so the forward direction would become a + // Matrix to do the scaling followed by a Lut1D, and the inverse is a Lut1D + // followed by a Matrix. Note that although the matrices compose into an identity, + // they are both forward direction and *not* pair inverses of each other. + const std::string fileName("lut1d_4.spi1d"); + OCIO::ContextRcPtr context = OCIO::Context::Create(); + OCIO_CHECK_NO_THROW(OCIO::BuildOpsTest(ops, fileName, context, + OCIO::TRANSFORM_DIR_INVERSE)); + + // The default negativeStyle is basicPassThruFwd, hence this op will be + // removed as a no-op on the first optimization pass. + const double exp_null[4] = {1.0, 1.0, 1.0, 1.0}; + OCIO::CreateExponentOp(ops, exp_null, OCIO::TRANSFORM_DIR_FORWARD); + + OCIO_CHECK_NO_THROW(OCIO::BuildOpsTest(ops, fileName, context, + OCIO::TRANSFORM_DIR_FORWARD)); + + OCIO_CHECK_NO_THROW(ops.finalize()); + OCIO_CHECK_NO_THROW(ops.optimize(OCIO::OPTIMIZATION_ALL)); + // The starting list of ops is this: + // FileNoOp --> Lut1D --> Matrix --> Gamma --> FileNoOp --> Matrix --> Lut1D + // This becomes the following on the first pass after no-ops are removed: + // Lut1D --> Matrix --> Matrix --> Lut1D + // The matrices are combined and removed on the first pass, leaving this: + // Lut1D --> Lut1D + // Second pass: the LUTs are identified as a pair of inverses and replaced with a Range: + // Range + + OCIO_CHECK_EQUAL(ops.size(), 1); + OCIO::ConstOpRcPtr op = ops[0]; + OCIO_REQUIRE_ASSERT(op); + auto range = OCIO_DYNAMIC_POINTER_CAST(op->data()); + OCIO_REQUIRE_ASSERT(range); +} + OCIO_ADD_TEST(OpOptimizers, non_optimizable) { OCIO::OpRcPtrVec ops; @@ -993,7 +1046,6 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp) OCIO::OpRcPtrVec optOps = ops.clone(); OCIO::OpRcPtrVec optOps_noComp = ops.clone(); - OCIO::OpRcPtrVec optOps_noIdentity = ops.clone(); OCIO_CHECK_EQUAL(optOps_noComp.size(), 4); OCIO_CHECK_NO_THROW(optOps_noComp.finalize()); @@ -1006,16 +1058,6 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp) CompareRender(ops, optOps_noComp, __LINE__, 1e-6f); - OCIO_CHECK_EQUAL(optOps_noIdentity.size(), 4); - OCIO_CHECK_NO_THROW(optOps_noIdentity.finalize()); - OCIO_CHECK_NO_THROW(optOps_noIdentity.optimize(AllBut(OCIO::OPTIMIZATION_IDENTITY_GAMMA))); - // Identity matrix is removed and gammas are combined into an identity gamma - // but it is not removed. - OCIO_CHECK_EQUAL(optOps_noIdentity.size(), 1); - OCIO_CHECK_EQUAL(optOps_noIdentity[0]->getInfo(), ""); - - CompareRender(ops, optOps_noIdentity, __LINE__, 5e-5f); - OCIO_CHECK_NO_THROW(optOps.finalize()); OCIO_CHECK_NO_THROW(optOps.optimize(OCIO::OPTIMIZATION_DEFAULT)); @@ -1038,6 +1080,10 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp_identity) auto gamma1 = std::make_shared(OCIO::GammaOpData::BASIC_FWD, params1, params1, params1, paramsA); + // Note that gamma2 is not a pair inverse of gamma1, it is another FWD gamma where the + // parameter is an inverse. Therefore it won't get replaced as a pair inverse, it must + // be composed into an identity, which may then be replaced. Since the BASIC_FWD style + // clamps negatives, it is replaced with a Range. OCIO::GammaOpData::Params params2 = { 1. / 0.45 }; auto gamma2 = std::make_shared(OCIO::GammaOpData::BASIC_FWD, @@ -1049,17 +1095,29 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp_identity) OCIO_CHECK_NO_THROW(ops.finalize()); OCIO_CHECK_EQUAL(ops.size(), 2); - OCIO::OpRcPtrVec optOps = ops.clone(); + { + OCIO::OpRcPtrVec optOps = ops.clone(); - // BASIC gamma are composed resulting into identity, that get optimized as a range. - OCIO_CHECK_NO_THROW(optOps.finalize()); - OCIO_CHECK_NO_THROW(optOps.optimize(OCIO::OPTIMIZATION_DEFAULT)); + OCIO_CHECK_NO_THROW(optOps.finalize()); + OCIO_CHECK_NO_THROW(optOps.optimize(AllBut(OCIO::OPTIMIZATION_IDENTITY_GAMMA))); - OCIO_REQUIRE_EQUAL(optOps.size(), 1); - OCIO_CHECK_EQUAL(optOps[0]->getInfo(), ""); + OCIO_REQUIRE_EQUAL(optOps.size(), 1); + OCIO_CHECK_EQUAL(optOps[0]->getInfo(), ""); + } + { + OCIO::OpRcPtrVec optOps = ops.clone(); + + // BASIC gammas are composed resulting in an identity, that get optimized as a range. + OCIO_CHECK_NO_THROW(optOps.finalize()); + OCIO_CHECK_NO_THROW(optOps.optimize(OCIO::OPTIMIZATION_DEFAULT)); + + OCIO_REQUIRE_EQUAL(optOps.size(), 1); + OCIO_CHECK_EQUAL(optOps[0]->getInfo(), ""); + } + + // Now do the same test with MONCURVE rather than BASIC style. ops.clear(); - optOps.clear(); params1 = { 2., 0.5 }; params2 = { 2., 0.6 }; @@ -1075,7 +1133,7 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp_identity) OCIO_CHECK_NO_THROW(ops.finalize()); OCIO_CHECK_EQUAL(ops.size(), 2); - optOps = ops.clone(); + OCIO::OpRcPtrVec optOps = ops.clone(); // MONCURVE composition is not supported yet. OCIO_CHECK_NO_THROW(optOps.finalize()); diff --git a/tests/cpu/transforms/DisplayViewTransform_tests.cpp b/tests/cpu/transforms/DisplayViewTransform_tests.cpp index 50ae88ec1..082ea3ee0 100644 --- a/tests/cpu/transforms/DisplayViewTransform_tests.cpp +++ b/tests/cpu/transforms/DisplayViewTransform_tests.cpp @@ -1109,6 +1109,16 @@ ocio_profile_version: 2 dt->setDisplay("display"); dt->setView("view"); + // This results in the following conversion: + // displayCSIn to scene_ref offset= -0.15 0.15 0.15 0.05 + // scene_ref to display_ref offset= -0.2 -0.2 -0.4 -0 + // display_ref to displayCSProcess (look process_space) offset= -0.1 -0.1 -0.1 -0 + // apply look offset= 0.1 0.2 0.3 0 + // displayCSProcess to display_ref offset= 0.1 0.1 0.1 0 + // apply display_vt offset= -0.3 -0.1 -0.1 -0 + // display_ref to displayCSOut offset= -0.25 -0.15 -0.35 -0 + // Total transformation: offset= -0.8 -0.1 -0.4 0.05 + OCIO::ConstProcessorRcPtr proc; OCIO_CHECK_NO_THROW(proc = config->getProcessor(dt)); // Remove the no-ops, since they are useless here.