Skip to content

Commit

Permalink
Adsk Contrib - Optimizer should detect pair inverses before combining…
Browse files Browse the repository at this point in the history 
… multiple op pairs (AcademySoftwareFoundation#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]>
(cherry picked from commit 1890b7d)
Signed-off-by: Doug Walker <[email protected]>
  • Loading branch information
@doug-walker
doug-walker committed Dec 12, 2024
1 parent 047834b commit f8e28c7
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 28 deletions.
4 changes: 3 additions & 1 deletion src/OpenColorIO/Config.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
37 changes: 29 additions & 8 deletions src/OpenColorIO/OpOptimizers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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.

Expand All @@ -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
{
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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());
Expand Down
96 changes: 77 additions & 19 deletions tests/cpu/OpOptimizers_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}

Expand All @@ -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<const OCIO::RangeOpData>(op->data());
OCIO_REQUIRE_ASSERT(range);
}

OCIO_ADD_TEST(OpOptimizers, non_optimizable)
{
OCIO::OpRcPtrVec ops;
Expand Down Expand Up @@ -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());
Expand All @@ -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(), "<GammaOp>");

CompareRender(ops, optOps_noIdentity, __LINE__, 5e-5f);

OCIO_CHECK_NO_THROW(optOps.finalize());
OCIO_CHECK_NO_THROW(optOps.optimize(OCIO::OPTIMIZATION_DEFAULT));

Expand All @@ -1038,6 +1080,10 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp_identity)
auto gamma1 = std::make_shared<OCIO::GammaOpData>(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>(OCIO::GammaOpData::BASIC_FWD,
Expand All @@ -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(), "<RangeOp>");
OCIO_REQUIRE_EQUAL(optOps.size(), 1);
OCIO_CHECK_EQUAL(optOps[0]->getInfo(), "<GammaOp>");
}
{
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(), "<RangeOp>");
}

// Now do the same test with MONCURVE rather than BASIC style.

ops.clear();
optOps.clear();

params1 = { 2., 0.5 };
params2 = { 2., 0.6 };
Expand All @@ -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());
Expand Down
10 changes: 10 additions & 0 deletions tests/cpu/transforms/DisplayViewTransform_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit f8e28c7

Please sign in to comment.