Skip to content

Commit 1890b7d

Browse files
doug-walkerdoug-walker
authored
Adsk Contrib - Optimizer should detect pair inverses before combining multiple op pairs (#2104)
* Fix opt combine issue Signed-off-by: Doug Walker <[email protected]> * Improve gamma test Signed-off-by: Doug Walker <[email protected]> * Add check for DisplayP3-HDR built-in Signed-off-by: Doug Walker <[email protected]> --------- Signed-off-by: Doug Walker <[email protected]>
1 parent 3132579 commit 1890b7d

File tree

4 files changed

+119
-28
lines changed

4 files changed

+119
-28
lines changed

‎src/OpenColorIO/Config.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5268,7 +5268,9 @@ void Config::Impl::checkVersionConsistency(ConstTransformRcPtr & transform) cons
52685268
|| 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-500nit-REC2020-D60-in-REC2020-D65_2.0")
52695269
|| 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-1000nit-REC2020-D60-in-REC2020-D65_2.0")
52705270
|| 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-2000nit-REC2020-D60-in-REC2020-D65_2.0")
5271-
|| 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-4000nit-REC2020-D60-in-REC2020-D65_2.0") )
5271+
|| 0 == Platform::Strcasecmp(blt->getStyle(), "ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - HDR-4000nit-REC2020-D60-in-REC2020-D65_2.0")
5272+
// NB: This one was added in OCIO 2.4.1.
5273+
|| 0 == Platform::Strcasecmp(blt->getStyle(), "DISPLAY - CIE-XYZ-D65_to_DisplayP3-HDR") )
52725274
)
52735275
{
52745276
std::ostringstream os;

‎src/OpenColorIO/OpOptimizers.cpp‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ bool IsCombineEnabled(OpData::Type type, OptimizationFlags flags)
6868
(type == OpData::RangeType && HasFlag(flags, OPTIMIZATION_COMP_RANGE));
6969
}
7070

71-
constexpr int MAX_OPTIMIZATION_PASSES = 8;
71+
constexpr int MAX_OPTIMIZATION_PASSES = 80;
7272

7373
int RemoveNoOpTypes(OpRcPtrVec & opVec)
7474
{
@@ -317,8 +317,10 @@ int CombineOps(OpRcPtrVec & opVec, OptimizationFlags oFlags)
317317
op1->combineWith(tmpops, op2);
318318
FinalizeOps(tmpops);
319319

320-
// tmpops may have any number of ops in it. (0, 1, 2, ...)
321-
// (size 0 would occur only if the combination results in a no-op).
320+
// The tmpops may have any number of ops in it: (0, 1, 2, ...).
321+
// (Size 0 would occur only if the combination results in a no-op,
322+
// for example, a pair of matrices that compose into a no-op are
323+
// returned as empty rather than as an identity matrix.)
322324
//
323325
// No matter the number, we need to swap them in for the original ops.
324326

@@ -336,6 +338,15 @@ int CombineOps(OpRcPtrVec & opVec, OptimizationFlags oFlags)
336338

337339
// We've done something so increment the count!
338340
++count;
341+
342+
// Break, since combining ops is less desirable than other optimization options.
343+
// For example, it is preferable to remove a pair of ops using RemoveInverseOps
344+
// rather than combining them. Consider this example:
345+
// Lut1D A --> Matrix B --> Matrix C --> Lut1D Ainv
346+
// If Matrix B & C are not pair inverses but do combine into an identity, then
347+
// CombineOps would compose Lut1D A & Ainv, into a new Lut1D rather than
348+
// allowing another round of optimization which would remove them as inverses.
349+
break;
339350
}
340351
else
341352
{
@@ -629,7 +640,7 @@ void OpRcPtrVec::optimize(OptimizationFlags oFlags)
629640
os << "**" << std::endl;
630641
os << "Optimized ";
631642
os << originalSize << "->" << finalSize << ", 1 pass, ";
632-
os << total_nooptype << " noop types removed\n";
643+
os << total_nooptype << " no-op types removed\n";
633644
os << SerializeOpVec(*this, 4);
634645
LogDebug(os.str());
635646
}
@@ -664,11 +675,21 @@ void OpRcPtrVec::optimize(OptimizationFlags oFlags)
664675

665676
while (passes <= MAX_OPTIMIZATION_PASSES)
666677
{
678+
// Remove all ops for which isNoOp is true, including identity matrices.
667679
int noops = optimizeIdentity ? RemoveNoOps(*this) : 0;
680+
681+
// Replace all complex ops with simpler ops (e.g., a CDL which only scales with a matrix).
668682
// Note this might increase the number of ops.
669683
int replacedOps = replaceOps ? ReplaceOps(*this) : 0;
684+
685+
// Replace all complex identities with simpler ops (e.g., an identity Lut1D with a range).
670686
int identityops = ReplaceIdentityOps(*this, oFlags);
687+
688+
// Remove all adjacent pairs of ops that are inverses of each other.
671689
int inverseops = RemoveInverseOps(*this, oFlags);
690+
691+
// Combine a pair of ops, for example multiply two adjacent Matrix ops.
692+
// (Combines at most one pair on each iteration.)
672693
int combines = CombineOps(*this, oFlags);
673694

674695
if (noops + identityops + inverseops + combines == 0)
@@ -721,12 +742,12 @@ void OpRcPtrVec::optimize(OptimizationFlags oFlags)
721742
os << "Optimized ";
722743
os << originalSize << "->" << finalSize << ", ";
723744
os << passes << " passes, ";
724-
os << total_nooptype << " noop types removed, ";
725-
os << total_noops << " noops removed, ";
745+
os << total_nooptype << " no-op types removed, ";
746+
os << total_noops << " no-ops removed, ";
726747
os << total_replacedops << " ops replaced, ";
727748
os << total_identityops << " identity ops replaced, ";
728-
os << total_inverseops << " inverse ops removed, ";
729-
os << total_combines << " ops combines, ";
749+
os << total_inverseops << " inverse op pairs removed, ";
750+
os << total_combines << " ops combined, ";
730751
os << total_inverses << " ops inverted\n";
731752
os << SerializeOpVec(*this, 4);
732753
LogDebug(os.str());

‎tests/cpu/OpOptimizers_tests.cpp‎

Lines changed: 77 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,8 @@ OCIO_ADD_TEST(OpOptimizers, combine_ops)
311311
OCIO::CombineOps(ops, AllBut(OCIO::OPTIMIZATION_COMP_MATRIX));
312312
OCIO_CHECK_EQUAL(ops.size(), 3);
313313
OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL);
314+
// CombineOps removes at most one pair on each call, repeat to combine all pairs.
315+
OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL);
314316
OCIO_CHECK_EQUAL(ops.size(), 1);
315317
}
316318

@@ -351,6 +353,10 @@ OCIO_ADD_TEST(OpOptimizers, combine_ops)
351353
OCIO::CombineOps(ops, AllBut(OCIO::OPTIMIZATION_COMP_MATRIX));
352354
OCIO_CHECK_EQUAL(ops.size(), 5);
353355
OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL);
356+
// CombineOps removes at most one pair on each call, repeat to combine all pairs.
357+
OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL);
358+
OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL);
359+
OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL);
354360
OCIO_CHECK_EQUAL(ops.size(), 1);
355361
}
356362

@@ -366,10 +372,57 @@ OCIO_ADD_TEST(OpOptimizers, combine_ops)
366372
OCIO::CombineOps(ops, AllBut(OCIO::OPTIMIZATION_COMP_MATRIX));
367373
OCIO_CHECK_EQUAL(ops.size(), 4);
368374
OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL);
375+
// CombineOps removes at most one pair on each call, repeat to combine all pairs.
376+
OCIO::CombineOps(ops, OCIO::OPTIMIZATION_ALL);
369377
OCIO_CHECK_EQUAL(ops.size(), 0);
370378
}
371379
}
372380

381+
OCIO_ADD_TEST(OpOptimizers, prefer_pair_inverse_over_combine)
382+
{
383+
// When a pair of forward / inverse LUTs with non 0 to 1 domain are used
384+
// as process space for a Look (eg. CDL), the Optimizer tries to combine
385+
// them when the Look results in a no-op. Here we make sure this result
386+
// in an appropriate clamp instead of a new half-domain LUT resulting
387+
// from the naive composition of the two LUTs.
388+
389+
OCIO::OpRcPtrVec ops;
390+
391+
// This spi1d uses "From -1.0 2.0", so the forward direction would become a
392+
// Matrix to do the scaling followed by a Lut1D, and the inverse is a Lut1D
393+
// followed by a Matrix. Note that although the matrices compose into an identity,
394+
// they are both forward direction and *not* pair inverses of each other.
395+
const std::string fileName("lut1d_4.spi1d");
396+
OCIO::ContextRcPtr context = OCIO::Context::Create();
397+
OCIO_CHECK_NO_THROW(OCIO::BuildOpsTest(ops, fileName, context,
398+
OCIO::TRANSFORM_DIR_INVERSE));
399+
400+
// The default negativeStyle is basicPassThruFwd, hence this op will be
401+
// removed as a no-op on the first optimization pass.
402+
const double exp_null[4] = {1.0, 1.0, 1.0, 1.0};
403+
OCIO::CreateExponentOp(ops, exp_null, OCIO::TRANSFORM_DIR_FORWARD);
404+
405+
OCIO_CHECK_NO_THROW(OCIO::BuildOpsTest(ops, fileName, context,
406+
OCIO::TRANSFORM_DIR_FORWARD));
407+
408+
OCIO_CHECK_NO_THROW(ops.finalize());
409+
OCIO_CHECK_NO_THROW(ops.optimize(OCIO::OPTIMIZATION_ALL));
410+
// The starting list of ops is this:
411+
// FileNoOp --> Lut1D --> Matrix --> Gamma --> FileNoOp --> Matrix --> Lut1D
412+
// This becomes the following on the first pass after no-ops are removed:
413+
// Lut1D --> Matrix --> Matrix --> Lut1D
414+
// The matrices are combined and removed on the first pass, leaving this:
415+
// Lut1D --> Lut1D
416+
// Second pass: the LUTs are identified as a pair of inverses and replaced with a Range:
417+
// Range
418+
419+
OCIO_CHECK_EQUAL(ops.size(), 1);
420+
OCIO::ConstOpRcPtr op = ops[0];
421+
OCIO_REQUIRE_ASSERT(op);
422+
auto range = OCIO_DYNAMIC_POINTER_CAST<const OCIO::RangeOpData>(op->data());
423+
OCIO_REQUIRE_ASSERT(range);
424+
}
425+
373426
OCIO_ADD_TEST(OpOptimizers, non_optimizable)
374427
{
375428
OCIO::OpRcPtrVec ops;
@@ -993,7 +1046,6 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp)
9931046

9941047
OCIO::OpRcPtrVec optOps = ops.clone();
9951048
OCIO::OpRcPtrVec optOps_noComp = ops.clone();
996-
OCIO::OpRcPtrVec optOps_noIdentity = ops.clone();
9971049

9981050
OCIO_CHECK_EQUAL(optOps_noComp.size(), 4);
9991051
OCIO_CHECK_NO_THROW(optOps_noComp.finalize());
@@ -1006,16 +1058,6 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp)
10061058

10071059
CompareRender(ops, optOps_noComp, __LINE__, 1e-6f);
10081060

1009-
OCIO_CHECK_EQUAL(optOps_noIdentity.size(), 4);
1010-
OCIO_CHECK_NO_THROW(optOps_noIdentity.finalize());
1011-
OCIO_CHECK_NO_THROW(optOps_noIdentity.optimize(AllBut(OCIO::OPTIMIZATION_IDENTITY_GAMMA)));
1012-
// Identity matrix is removed and gammas are combined into an identity gamma
1013-
// but it is not removed.
1014-
OCIO_CHECK_EQUAL(optOps_noIdentity.size(), 1);
1015-
OCIO_CHECK_EQUAL(optOps_noIdentity[0]->getInfo(), "<GammaOp>");
1016-
1017-
CompareRender(ops, optOps_noIdentity, __LINE__, 5e-5f);
1018-
10191061
OCIO_CHECK_NO_THROW(optOps.finalize());
10201062
OCIO_CHECK_NO_THROW(optOps.optimize(OCIO::OPTIMIZATION_DEFAULT));
10211063

@@ -1038,6 +1080,10 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp_identity)
10381080
auto gamma1 = std::make_shared<OCIO::GammaOpData>(OCIO::GammaOpData::BASIC_FWD,
10391081
params1, params1, params1, paramsA);
10401082

1083+
// Note that gamma2 is not a pair inverse of gamma1, it is another FWD gamma where the
1084+
// parameter is an inverse. Therefore it won't get replaced as a pair inverse, it must
1085+
// be composed into an identity, which may then be replaced. Since the BASIC_FWD style
1086+
// clamps negatives, it is replaced with a Range.
10411087
OCIO::GammaOpData::Params params2 = { 1. / 0.45 };
10421088

10431089
auto gamma2 = std::make_shared<OCIO::GammaOpData>(OCIO::GammaOpData::BASIC_FWD,
@@ -1049,17 +1095,29 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp_identity)
10491095
OCIO_CHECK_NO_THROW(ops.finalize());
10501096
OCIO_CHECK_EQUAL(ops.size(), 2);
10511097

1052-
OCIO::OpRcPtrVec optOps = ops.clone();
1098+
{
1099+
OCIO::OpRcPtrVec optOps = ops.clone();
10531100

1054-
// BASIC gamma are composed resulting into identity, that get optimized as a range.
1055-
OCIO_CHECK_NO_THROW(optOps.finalize());
1056-
OCIO_CHECK_NO_THROW(optOps.optimize(OCIO::OPTIMIZATION_DEFAULT));
1101+
OCIO_CHECK_NO_THROW(optOps.finalize());
1102+
OCIO_CHECK_NO_THROW(optOps.optimize(AllBut(OCIO::OPTIMIZATION_IDENTITY_GAMMA)));
10571103

1058-
OCIO_REQUIRE_EQUAL(optOps.size(), 1);
1059-
OCIO_CHECK_EQUAL(optOps[0]->getInfo(), "<RangeOp>");
1104+
OCIO_REQUIRE_EQUAL(optOps.size(), 1);
1105+
OCIO_CHECK_EQUAL(optOps[0]->getInfo(), "<GammaOp>");
1106+
}
1107+
{
1108+
OCIO::OpRcPtrVec optOps = ops.clone();
1109+
1110+
// BASIC gammas are composed resulting in an identity, that get optimized as a range.
1111+
OCIO_CHECK_NO_THROW(optOps.finalize());
1112+
OCIO_CHECK_NO_THROW(optOps.optimize(OCIO::OPTIMIZATION_DEFAULT));
1113+
1114+
OCIO_REQUIRE_EQUAL(optOps.size(), 1);
1115+
OCIO_CHECK_EQUAL(optOps[0]->getInfo(), "<RangeOp>");
1116+
}
1117+
1118+
// Now do the same test with MONCURVE rather than BASIC style.
10601119

10611120
ops.clear();
1062-
optOps.clear();
10631121

10641122
params1 = { 2., 0.5 };
10651123
params2 = { 2., 0.6 };
@@ -1075,7 +1133,7 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp_identity)
10751133
OCIO_CHECK_NO_THROW(ops.finalize());
10761134
OCIO_CHECK_EQUAL(ops.size(), 2);
10771135

1078-
optOps = ops.clone();
1136+
OCIO::OpRcPtrVec optOps = ops.clone();
10791137

10801138
// MONCURVE composition is not supported yet.
10811139
OCIO_CHECK_NO_THROW(optOps.finalize());

‎tests/cpu/transforms/DisplayViewTransform_tests.cpp‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,6 +1109,16 @@ ocio_profile_version: 2
11091109
dt->setDisplay("display");
11101110
dt->setView("view");
11111111

1112+
// This results in the following conversion:
1113+
// displayCSIn to scene_ref offset= -0.15 0.15 0.15 0.05
1114+
// scene_ref to display_ref offset= -0.2 -0.2 -0.4 -0
1115+
// display_ref to displayCSProcess (look process_space) offset= -0.1 -0.1 -0.1 -0
1116+
// apply look offset= 0.1 0.2 0.3 0
1117+
// displayCSProcess to display_ref offset= 0.1 0.1 0.1 0
1118+
// apply display_vt offset= -0.3 -0.1 -0.1 -0
1119+
// display_ref to displayCSOut offset= -0.25 -0.15 -0.35 -0
1120+
// Total transformation: offset= -0.8 -0.1 -0.4 0.05
1121+
11121122
OCIO::ConstProcessorRcPtr proc;
11131123
OCIO_CHECK_NO_THROW(proc = config->getProcessor(dt));
11141124
// Remove the no-ops, since they are useless here.

0 commit comments

Comments
 (0)