Skip to content

Adsk Contrib - Optimizer should detect pair inverses before combining multiple op pairs #2104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
doug-walker merged 4 commits into from
Dec 10, 2024
Merged

Adsk Contrib - Optimizer should detect pair inverses before combining multiple op pairs #2104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61f1b9f
Fix opt combine issue
doug-walker Dec 8, 2024
7775e71
Improve gamma test
doug-walker Dec 9, 2024
67860a2
Add check for DisplayP3-HDR built-in
doug-walker Dec 9, 2024
0c1d458
Merge branch 'main' into bug/walker/opt_combine
doug-walker Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/OpenColorIO/Config.cpp
View file
Open in desktop
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") )
Copy link
Collaborator Author

@doug-walker doug-walker Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one-liner is unrelated to the optimization issue. It's just a validation check I noticed was missing related to the new built-in that Thomas added recently.

Copy link
Collaborator

@remia remia Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since OCIO configs don't have patch release versions, we should in theory avoid new builtins for patch versions. But given that 2.4.0 was meant as a developer preview, I guess it's fine in this case?

Copy link
Collaborator Author

@doug-walker doug-walker Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed. There will be configs that pass this validation but would not work in a 2.4.0 library, which is not ideal, but probably acceptable in this case.

)
{
std::ostringstream os;
Expand Down
37 changes: 29 additions & 8 deletions src/OpenColorIO/OpOptimizers.cpp
View file
Open in desktop
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
Open in desktop
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);
Copy link
Collaborator Author

@doug-walker doug-walker Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the test assumed all the gamma ops would be combined into a single identity, but that is no longer true. They now get removed as a pair of inverses before being combined a second time into an identity. The end result of the optimization is the same, so this part of the test is not needed. (Gamma composition is already tested in GammaOpData_tests.cpp, so it is not necessary to test that again here.)


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
Open in desktop
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
Loading