Skip to content

Commit ac94d7b

Browse files
doug-walkerdoug-walker
authored
Adsk Contrb - Fix pass-thru exponent composition issue (#2154)
* Fix gamma composition issue Signed-off-by: Doug Walker <[email protected]> * Adjust docs build settings to fix CI Signed-off-by: Doug Walker <[email protected]> * Adjust docs build settings to fix CI Signed-off-by: Doug Walker <[email protected]> * Revert CI change Signed-off-by: Doug Walker <[email protected]> * Minor comment tweak Signed-off-by: Doug Walker <[email protected]> --------- Signed-off-by: Doug Walker <[email protected]>
1 parent 36b7595 commit ac94d7b

File tree

5 files changed

+194
-17
lines changed

5 files changed

+194
-17
lines changed

‎include/OpenColorIO/OpenColorIO.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -870,7 +870,7 @@ class OCIOEXPORT Config
870870
const char * getDefaultView(const char * display, const char * colorspaceName) const;
871871

872872
/**
873-
* Return the number of views attached to the display including the number of
873+
* Return the number of active views attached to the display including the number of
874874
* shared views if any. Return 0 if display does not exist.
875875
*/
876876
int getNumViews(const char * display) const;

‎src/OpenColorIO/ops/gamma/GammaOpData.cpp‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -659,20 +659,24 @@ namespace
659659
GammaOpData::Style CombineBasicStyles(GammaOpData::Style a, GammaOpData::Style b)
660660
{
661661
// This function assumes that mayCompose was called on the inputs and returned true.
662-
// The logic here is only valid for that situation.
662+
// The logic here is only valid for that situation. There is no intent to preserve
663+
// the direction, a forward style is always returned.
663664
if (a == GammaOpData::BASIC_FWD || a == GammaOpData::BASIC_REV ||
664665
b == GammaOpData::BASIC_FWD || b == GammaOpData::BASIC_REV)
665666
{
667+
// If either a or b is a BASIC style, that is the combined style since the
668+
// BASIC style clamps negatives, so the combination must also clamp.
666669
return GammaOpData::BASIC_FWD;
667670
}
668671
else if (a == GammaOpData::BASIC_MIRROR_FWD || a == GammaOpData::BASIC_MIRROR_REV)
669672
{
670-
// b BASIC_MIRROR.
673+
// Neither a or b is a BASIC style, so it may only be MIRROR or PASS_THRU, but
674+
// mayCompose will not allow b to be PASS_THRU in this case, both are MIRROR.
671675
return GammaOpData::BASIC_MIRROR_FWD;
672676
}
673-
else // a is BASIC_PASS_THRU (ensured by mayCompose).
677+
else
674678
{
675-
// b BASIC_PASS_THRU.
679+
// Both a and b are BASIC_PASS_THRU as a consequence of mayCompose being true.
676680
return GammaOpData::BASIC_PASS_THRU_FWD;
677681
}
678682
}
@@ -715,7 +719,7 @@ GammaOpDataRcPtr GammaOpData::compose(const GammaOpData & B) const
715719
double b1 = getBlueParams()[0];
716720
double a1 = getAlphaParams()[0];
717721

718-
if (styleA == BASIC_REV || styleA == BASIC_MIRROR_REV || styleA == BASIC_PASS_THRU_FWD)
722+
if (styleA == BASIC_REV || styleA == BASIC_MIRROR_REV || styleA == BASIC_PASS_THRU_REV)
719723
{
720724
r1 = 1. / r1;
721725
g1 = 1. / g1;
@@ -727,7 +731,7 @@ GammaOpDataRcPtr GammaOpData::compose(const GammaOpData & B) const
727731
double g2 = B.getGreenParams()[0];
728732
double b2 = B.getBlueParams()[0];
729733
double a2 = B.getAlphaParams()[0];
730-
if (styleB == BASIC_REV || styleB == BASIC_MIRROR_REV || styleB == BASIC_PASS_THRU_FWD)
734+
if (styleB == BASIC_REV || styleB == BASIC_MIRROR_REV || styleB == BASIC_PASS_THRU_REV)
731735
{
732736
r2 = 1. / r2;
733737
g2 = 1. / g2;
@@ -747,6 +751,7 @@ GammaOpDataRcPtr GammaOpData::compose(const GammaOpData & B) const
747751
RoundAround1(bOut);
748752
RoundAround1(aOut);
749753

754+
// NB: This always returns a forward style.
750755
Style style = CombineBasicStyles(styleA, styleB);
751756

752757
// By convention, we try to keep the gamma parameter > 1.

‎tests/cpu/OpOptimizers_tests.cpp‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ void CompareRender(OCIO::OpRcPtrVec & ops1, OCIO::OpRcPtrVec & ops2,
4949

5050
for (const auto & op : ops1)
5151
{
52+
// NB: This hard-codes OPTIMIZATION_FAST_LOG_EXP_POW to off, see Op.h.
5253
op->apply(&img1[0], &img1[0], nbPixels);
5354
}
5455

@@ -1070,6 +1071,70 @@ OCIO_ADD_TEST(OpOptimizers, gamma_comp)
10701071
CompareRender(ops, optOps, __LINE__, 1e-4f, true);
10711072
}
10721073

1074+
OCIO_ADD_TEST(OpOptimizers, gamma_comp_test2)
1075+
{
1076+
// This transform has a pair of gammas separated by a pair of matrices that
1077+
// compose into an identity matrix and get optimized out. Then the gammas
1078+
// get composed into a non-identity gamma. Finally the exponent is inverted
1079+
// (to follow the convention of keeping it > 1) and the direction is inverted.
1080+
1081+
const std::string fileName("gamma_comp_test2.ctf");
1082+
OCIO::OpRcPtrVec ops;
1083+
OCIO::ContextRcPtr context = OCIO::Context::Create();
1084+
OCIO_CHECK_NO_THROW(OCIO::BuildOpsTest(ops, fileName, context,
1085+
OCIO::TRANSFORM_DIR_FORWARD));
1086+
1087+
// First one is the file no op.
1088+
OCIO_CHECK_EQUAL(ops.size(), 5);
1089+
1090+
// Remove no ops & finalize for computation.
1091+
OCIO_CHECK_NO_THROW(ops.finalize());
1092+
OCIO_CHECK_NO_THROW(ops.optimize(OCIO::OPTIMIZATION_NONE));
1093+
1094+
OCIO_CHECK_EQUAL(ops.size(), 4);
1095+
1096+
OCIO::OpRcPtrVec optOps = ops.clone();
1097+
OCIO::OpRcPtrVec optOps_noComp = ops.clone();
1098+
1099+
OCIO_CHECK_EQUAL(optOps_noComp.size(), 4);
1100+
OCIO_CHECK_NO_THROW(optOps_noComp.finalize());
1101+
// NB: The op->apply function used here hard-codes OPTIMIZATION_FAST_LOG_EXP_POW to off.
1102+
OCIO_CHECK_NO_THROW(optOps_noComp.optimize(AllBut(OCIO::OPTIMIZATION_COMP_GAMMA)));
1103+
OCIO_CHECK_EQUAL(optOps_noComp.size(), 2);
1104+
OCIO_CHECK_EQUAL(optOps_noComp[0]->getInfo(), "<GammaOp>");
1105+
OCIO_CHECK_EQUAL(optOps_noComp[1]->getInfo(), "<GammaOp>");
1106+
1107+
// Due to rounding error in the two 3x3 matrix multiplies with much larger values, the
1108+
// 1.52e-4 input value is off by 60% going into the second gamma (see ociochecklut -s).
1109+
// Therefore the optOps_noComp and optOps are actually more accurate than ops here.
1110+
CompareRender(ops, optOps_noComp, __LINE__, 1e-4f);
1111+
1112+
OCIO_CHECK_NO_THROW(optOps.finalize());
1113+
OCIO_CHECK_NO_THROW(optOps.optimize(OCIO::OPTIMIZATION_DEFAULT));
1114+
1115+
// Now check that the optimized transform renders the same as the original.
1116+
CompareRender(ops, optOps, __LINE__, 1e-4f);
1117+
1118+
// Check the op is as expected.
1119+
OCIO::GroupTransformRcPtr group = OCIO::GroupTransform::Create();
1120+
OCIO_REQUIRE_EQUAL(optOps.size(), 1);
1121+
OCIO::ConstOpRcPtr op(optOps[0]);
1122+
OCIO::CreateGammaTransform(group, op);
1123+
OCIO_REQUIRE_EQUAL(group->getNumTransforms(), 1);
1124+
auto transform = group->getTransform(0);
1125+
OCIO_REQUIRE_ASSERT(transform);
1126+
auto gTransform = OCIO_DYNAMIC_POINTER_CAST<OCIO::ExponentTransform>(transform);
1127+
OCIO_REQUIRE_ASSERT(gTransform);
1128+
OCIO_CHECK_EQUAL(gTransform->getNegativeStyle(), OCIO::NEGATIVE_PASS_THRU);
1129+
OCIO_CHECK_EQUAL(gTransform->getDirection(), OCIO::TRANSFORM_DIR_INVERSE);
1130+
double vals[4];
1131+
gTransform->getValue(vals);
1132+
OCIO_CHECK_CLOSE(vals[0], 2.2/1.8, 1e-6f);
1133+
OCIO_CHECK_CLOSE(vals[1], 2.2/1.8, 1e-6f);
1134+
OCIO_CHECK_CLOSE(vals[2], 2.2/1.8, 1e-6f);
1135+
OCIO_CHECK_EQUAL(vals[3], 1.);
1136+
}
1137+
10731138
OCIO_ADD_TEST(OpOptimizers, gamma_comp_identity)
10741139
{
10751140
OCIO::OpRcPtrVec ops;

‎tests/cpu/ops/gamma/GammaOpData_tests.cpp‎

Lines changed: 85 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -579,7 +579,8 @@ void CheckGammaCompose(OCIO::GammaOpData::Style style1,
579579
OCIO::GammaOpData::Style style2,
580580
const OCIO::GammaOpData::Params & params2,
581581
OCIO::GammaOpData::Style refStyle,
582-
const OCIO::GammaOpData::Params & refParams)
582+
const OCIO::GammaOpData::Params & refParams,
583+
unsigned line)
583584
{
584585
static const OCIO::GammaOpData::Params paramsA = { 1. };
585586

@@ -591,12 +592,12 @@ void CheckGammaCompose(OCIO::GammaOpData::Style style1,
591592

592593
const OCIO::GammaOpDataRcPtr g3 = g1.compose(g2);
593594

594-
OCIO_CHECK_EQUAL(g3->getStyle(), refStyle);
595+
OCIO_CHECK_EQUAL_FROM(g3->getStyle(), refStyle, line);
595596

596-
OCIO_CHECK_ASSERT(g3->getRedParams() == refParams);
597-
OCIO_CHECK_ASSERT(g3->getGreenParams() == refParams);
598-
OCIO_CHECK_ASSERT(g3->getBlueParams() == refParams);
599-
OCIO_CHECK_ASSERT(g3->getAlphaParams() == paramsA);
597+
OCIO_CHECK_ASSERT_FROM(g3->getRedParams() == refParams, line);
598+
OCIO_CHECK_ASSERT_FROM(g3->getGreenParams() == refParams, line);
599+
OCIO_CHECK_ASSERT_FROM(g3->getBlueParams() == refParams, line);
600+
OCIO_CHECK_ASSERT_FROM(g3->getAlphaParams() == paramsA, line);
600601
}
601602

602603
};
@@ -610,7 +611,8 @@ OCIO_ADD_TEST(GammaOpData, compose)
610611

611612
CheckGammaCompose(OCIO::GammaOpData::BASIC_FWD, params1,
612613
OCIO::GammaOpData::BASIC_FWD, params2,
613-
OCIO::GammaOpData::BASIC_FWD, refParams);
614+
OCIO::GammaOpData::BASIC_FWD, refParams,
615+
__LINE__);
614616
}
615617

616618
{
@@ -620,7 +622,8 @@ OCIO_ADD_TEST(GammaOpData, compose)
620622

621623
CheckGammaCompose(OCIO::GammaOpData::BASIC_REV, params1,
622624
OCIO::GammaOpData::BASIC_REV, params2,
623-
OCIO::GammaOpData::BASIC_REV, refParams);
625+
OCIO::GammaOpData::BASIC_REV, refParams,
626+
__LINE__);
624627
}
625628

626629
{
@@ -630,7 +633,8 @@ OCIO_ADD_TEST(GammaOpData, compose)
630633

631634
CheckGammaCompose(OCIO::GammaOpData::BASIC_REV, params1,
632635
OCIO::GammaOpData::BASIC_FWD, params2,
633-
OCIO::GammaOpData::BASIC_REV, refParams);
636+
OCIO::GammaOpData::BASIC_REV, refParams,
637+
__LINE__);
634638
}
635639

636640
{
@@ -640,7 +644,78 @@ OCIO_ADD_TEST(GammaOpData, compose)
640644

641645
CheckGammaCompose(OCIO::GammaOpData::BASIC_REV, params1,
642646
OCIO::GammaOpData::BASIC_FWD, params2,
643-
OCIO::GammaOpData::BASIC_FWD, refParams);
647+
OCIO::GammaOpData::BASIC_FWD, refParams,
648+
__LINE__);
649+
}
650+
651+
{
652+
const OCIO::GammaOpData::Params params1 = { 2. };
653+
const OCIO::GammaOpData::Params params2 = { 4. };
654+
const OCIO::GammaOpData::Params refParams = { 2. };
655+
656+
CheckGammaCompose(OCIO::GammaOpData::BASIC_FWD, params1,
657+
OCIO::GammaOpData::BASIC_REV, params2,
658+
OCIO::GammaOpData::BASIC_REV, refParams,
659+
__LINE__);
660+
}
661+
662+
{
663+
const OCIO::GammaOpData::Params params1 = { 2. };
664+
const OCIO::GammaOpData::Params params2 = { 4. };
665+
const OCIO::GammaOpData::Params refParams = { 2. };
666+
667+
CheckGammaCompose(OCIO::GammaOpData::BASIC_PASS_THRU_FWD, params1,
668+
OCIO::GammaOpData::BASIC_PASS_THRU_REV, params2,
669+
OCIO::GammaOpData::BASIC_PASS_THRU_REV, refParams,
670+
__LINE__);
671+
}
672+
673+
{
674+
const OCIO::GammaOpData::Params params1 = { 2. };
675+
const OCIO::GammaOpData::Params params2 = { 4. };
676+
const OCIO::GammaOpData::Params refParams = { 2. };
677+
678+
CheckGammaCompose(OCIO::GammaOpData::BASIC_MIRROR_FWD, params1,
679+
OCIO::GammaOpData::BASIC_MIRROR_REV, params2,
680+
OCIO::GammaOpData::BASIC_MIRROR_REV, refParams,
681+
__LINE__);
682+
}
683+
684+
{
685+
const OCIO::GammaOpData::Params params1 = { 2. };
686+
const OCIO::GammaOpData::Params params2 = { 4. };
687+
const OCIO::GammaOpData::Params refParams = { 2. };
688+
689+
CheckGammaCompose(OCIO::GammaOpData::BASIC_MIRROR_FWD, params1,
690+
OCIO::GammaOpData::BASIC_REV, params2,
691+
OCIO::GammaOpData::BASIC_REV, refParams,
692+
__LINE__);
693+
}
694+
695+
{
696+
const OCIO::GammaOpData::Params params1 = { 2. };
697+
const OCIO::GammaOpData::Params params2 = { 4. };
698+
const OCIO::GammaOpData::Params refParams = { 2. };
699+
700+
CheckGammaCompose(OCIO::GammaOpData::BASIC_PASS_THRU_FWD, params1,
701+
OCIO::GammaOpData::BASIC_REV, params2,
702+
OCIO::GammaOpData::BASIC_REV, refParams,
703+
__LINE__);
704+
}
705+
706+
{
707+
const OCIO::GammaOpData::Params params1 = { 4. };
708+
OCIO::GammaOpData::Params paramsA = { 1. };
709+
OCIO::GammaOpData g1(OCIO::GammaOpData::BASIC_MIRROR_FWD,
710+
params1, params1, params1, paramsA);
711+
712+
OCIO::GammaOpData::Params params2 = { 2. };
713+
OCIO::GammaOpData g2(OCIO::GammaOpData::BASIC_PASS_THRU_FWD,
714+
params2, params2, params2, paramsA);
715+
716+
OCIO_CHECK_THROW_WHAT(g1.compose(g2),
717+
OCIO::Exception,
718+
"GammaOp can only be combined with some GammaOps");
644719
}
645720

646721
{

‎tests/data/files/gamma_comp_test2.ctf‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<ProcessList id="none" version="2.0">
3+
4+
<Description>Transform should optimize into a basicPassThruRev with gamma=2.2/1.8</Description>
5+
6+
<Gamma inBitDepth="16f" outBitDepth="16f" style="basicPassThruFwd">
7+
<GammaParams gamma="1.8" />
8+
</Gamma>
9+
10+
<Matrix inBitDepth="16f" outBitDepth="16f">
11+
<Description>Rec.709 to XYZ</Description>
12+
<Array dim="3 3 3">
13+
0.412390799266 0.357584339384 0.180480788402
14+
0.212639005872 0.715168678768 0.072192315361
15+
0.019330818716 0.119194779795 0.95053215225
16+
</Array>
17+
</Matrix>
18+
19+
<Matrix inBitDepth="16f" outBitDepth="16f">
20+
<Description>XYZ to Rec.709</Description>
21+
<Array dim="3 3 3">
22+
3.240969941905 -1.53738317757 -0.498610760293
23+
-0.969243636281 1.875967501508 0.041555057407
24+
0.055630079697 -0.203976958889 1.056971514243
25+
</Array>
26+
</Matrix>
27+
28+
<Gamma inBitDepth="16f" outBitDepth="16f" style="basicPassThruRev">
29+
<GammaParams gamma="2.2" />
30+
</Gamma>
31+
32+
</ProcessList>

0 commit comments

Comments
 (0)