Skip to content

Commit ecdc10c

Browse files
hodoulphodoulp
authored
Adsk Contrib - Fix the processor creation using a DisplayViewTransform with a no-op look (#1460) (#1470)
* Adsk Contrib - Fix the processor creation using a DisplayViewTransform with a no-op look Signed-off-by: Patrick Hodoul <[email protected]> * Remove a wrong comment Signed-off-by: Patrick Hodoul <[email protected]>
1 parent 8dc5f6e commit ecdc10c

File tree

4 files changed

+133
-37
lines changed

4 files changed

+133
-37
lines changed

‎src/OpenColorIO/transforms/LookTransform.cpp‎

Lines changed: 38 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -195,10 +195,15 @@ std::ostream& operator<< (std::ostream& os, const LookTransform& t)
195195
namespace
196196
{
197197

198-
void RunLookTokens(OpRcPtrVec & ops,
199-
ConstColorSpaceRcPtr & currentColorSpace,
198+
// The method gets the list of ops from the list of looks.
199+
//
200+
// It returns the list of ops (which could be empty if looks are all no-ops), and the final process
201+
// color space after applying the complete look transformation.
202+
//
203+
void RunLookTokens(OpRcPtrVec & ops, // [in/out]
204+
ConstColorSpaceRcPtr & currentColorSpace, // [in/out]
200205
bool skipColorSpaceConversion,
201-
const Config& config,
206+
const Config & config,
202207
const ConstContextRcPtr & context,
203208
const LookParseResult::Tokens & lookTokens)
204209
{
@@ -235,8 +240,6 @@ void RunLookTokens(OpRcPtrVec & ops,
235240
throw Exception(os.str().c_str());
236241
}
237242

238-
// Put the new ops into a temp array, to see if it's a no-op
239-
// If it is a no-op, dont bother doing the colorspace conversion.
240243
OpRcPtrVec tmpOps;
241244

242245
switch (lookTokens[i].dir)
@@ -269,34 +272,33 @@ void RunLookTokens(OpRcPtrVec & ops,
269272
}
270273
}
271274

272-
if (!tmpOps.isNoOp())
275+
ConstColorSpaceRcPtr processColorSpace = config.getColorSpace(look->getProcessSpace());
276+
if(!processColorSpace)
273277
{
274-
ConstColorSpaceRcPtr processColorSpace = config.getColorSpace(look->getProcessSpace());
275-
if(!processColorSpace)
276-
{
277-
std::ostringstream os;
278-
os << "RunLookTokens error. ";
279-
os << "The specified look, '" << lookTokens[i].name;
280-
os << "', requires processing in the ColorSpace, '";
281-
os << look->getProcessSpace() << "' which is not defined.";
282-
throw Exception(os.str().c_str());
283-
}
284-
if (!currentColorSpace)
285-
{
286-
currentColorSpace = processColorSpace;
287-
}
288-
// If current color space is already the process space skip the conversion.
289-
if (!skipColorSpaceConversion &&
290-
currentColorSpace.get() != processColorSpace.get())
291-
{
292-
// Default behavior is to bypass data color space.
293-
BuildColorSpaceOps(ops, config, context,
294-
currentColorSpace, processColorSpace, true);
295-
currentColorSpace = processColorSpace;
296-
}
278+
std::ostringstream os;
279+
os << "RunLookTokens error. ";
280+
os << "The specified look, '" << lookTokens[i].name;
281+
os << "', requires processing in the ColorSpace, '";
282+
os << look->getProcessSpace() << "' which is not defined.";
283+
throw Exception(os.str().c_str());
284+
}
297285

298-
ops += tmpOps;
286+
if (!currentColorSpace)
287+
{
288+
currentColorSpace = processColorSpace;
299289
}
290+
291+
// If current color space is already the process space skip the conversion.
292+
if (!skipColorSpaceConversion &&
293+
currentColorSpace.get() != processColorSpace.get())
294+
{
295+
// Default behavior is to bypass data color space.
296+
BuildColorSpaceOps(ops, config, context,
297+
currentColorSpace, processColorSpace, true);
298+
currentColorSpace = processColorSpace;
299+
}
300+
301+
ops += tmpOps;
300302
}
301303
}
302304

@@ -369,12 +371,12 @@ void BuildLookOps(OpRcPtrVec & ops,
369371

370372
if(options.empty())
371373
{
372-
// Do nothing
374+
// Do nothing.
373375
}
374376
else if(options.size() == 1)
375377
{
376-
// As an optimization, if we only have a single look option,
377-
// just push back onto the final location
378+
// As an optimization, if we only have a single look option, just push back onto the
379+
// final location.
378380
RunLookTokens(ops,
379381
currentColorSpace,
380382
skipColorSpaceConversion,
@@ -384,9 +386,9 @@ void BuildLookOps(OpRcPtrVec & ops,
384386
}
385387
else
386388
{
387-
// If we have multiple look options, try each one in order,
388-
// and if we can create the ops without a missing file exception,
389-
// push back it's results and return.
389+
// If we have multiple look options, try each one in order, and if we can create the ops
390+
// without a missing file exception, push back it's results and return i.e., take the
391+
// first successully created look.
390392

391393
bool success = false;
392394
std::ostringstream os;

‎tests/cpu/Config_tests.cpp‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8344,3 +8344,96 @@ search_path: "./"
83448344
OCIO_CHECK_EQUAL((uint32_t)outCol2[2], 128);
83458345
}
83468346
}
8347+
8348+
OCIO_ADD_TEST(Config, look_is_noop)
8349+
{
8350+
// Test that the processor creation from a color space to a (dislay, view) pair succeeds even
8351+
// if the look transformation is a 'no-op'.
8352+
8353+
{
8354+
static constexpr char CONFIG[]{ R"(ocio_profile_version: 1
8355+
roles:
8356+
scene_linear: cs
8357+
8358+
displays:
8359+
disp1:
8360+
- !<View>
8361+
name: view1
8362+
colorspace: cs
8363+
looks: cdl
8364+
8365+
looks:
8366+
- !<Look>
8367+
name: cdl
8368+
process_space: cs
8369+
transform: !<CDLTransform> {}
8370+
8371+
colorspaces:
8372+
- !<ColorSpace>
8373+
name: cs
8374+
)" };
8375+
8376+
std::istringstream iss;
8377+
iss.str(CONFIG);
8378+
8379+
OCIO::ConstConfigRcPtr config;
8380+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
8381+
OCIO_CHECK_NO_THROW(config->validate());
8382+
8383+
OCIO::ConstProcessorRcPtr proc;
8384+
8385+
OCIO_CHECK_NO_THROW(proc = config->getProcessor("cs", "disp1", "view1", OCIO::TRANSFORM_DIR_FORWARD));
8386+
OCIO_CHECK_ASSERT(proc->isNoOp());
8387+
8388+
OCIO_CHECK_NO_THROW(proc = config->getProcessor("cs", "disp1", "view1", OCIO::TRANSFORM_DIR_INVERSE));
8389+
OCIO_CHECK_ASSERT(proc->isNoOp());
8390+
}
8391+
8392+
{
8393+
static constexpr char CONFIG[]{ R"(ocio_profile_version: 1
8394+
roles:
8395+
scene_linear: cs
8396+
8397+
displays:
8398+
disp1:
8399+
- !<View>
8400+
name: view1
8401+
colorspace: cs
8402+
looks: cdl
8403+
8404+
looks:
8405+
- !<Look>
8406+
name: cdl
8407+
process_space: cs1
8408+
transform: !<CDLTransform> {}
8409+
8410+
colorspaces:
8411+
- !<ColorSpace>
8412+
name: cs
8413+
- !<ColorSpace>
8414+
name: cs1
8415+
from_reference: !<CDLTransform> {offset: [0.3, 0.3, 0.3]}
8416+
)" };
8417+
8418+
std::istringstream iss;
8419+
iss.str(CONFIG);
8420+
8421+
OCIO::ConstConfigRcPtr config;
8422+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
8423+
OCIO_CHECK_NO_THROW(config->validate());
8424+
8425+
OCIO::ConstProcessorRcPtr proc;
8426+
8427+
OCIO_CHECK_NO_THROW(proc = config->getProcessor("cs", "disp1", "view1", OCIO::TRANSFORM_DIR_FORWARD));
8428+
// Because the look process space is not a no-op.
8429+
OCIO_CHECK_ASSERT(!proc->isNoOp());
8430+
OCIO_CHECK_NO_THROW(proc = proc->getOptimizedProcessor(OCIO::OPTIMIZATION_DEFAULT));
8431+
// Because the look process space forward and inverse ops are then optimized.
8432+
OCIO_CHECK_ASSERT(proc->isNoOp());
8433+
8434+
OCIO_CHECK_NO_THROW(proc = config->getProcessor("cs", "disp1", "view1", OCIO::TRANSFORM_DIR_INVERSE));
8435+
OCIO_CHECK_ASSERT(!proc->isNoOp());
8436+
OCIO_CHECK_NO_THROW(proc = proc->getOptimizedProcessor(OCIO::OPTIMIZATION_DEFAULT));
8437+
OCIO_CHECK_ASSERT(proc->isNoOp());
8438+
}
8439+
}

‎tests/cpu/apphelpers/DisplayViewHelpers_tests.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ OCIO_ADD_TEST(DisplayViewHelpers, basic)
116116
"lin_1", "DISP_1", "VIEW_5",
117117
OCIO::ConstMatrixTransformRcPtr(),
118118
OCIO::TRANSFORM_DIR_FORWARD));
119+
OCIO_REQUIRE_ASSERT(processor);
119120

120121
OCIO::GroupTransformRcPtr groupTransform;
121122
OCIO_CHECK_NO_THROW(groupTransform = processor->createGroupTransform());

‎tests/testutils/UnitTest.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ int UnitTestMain(int argc, const char ** argv)
137137
name.resize(maxCharToDisplay);
138138
}
139139

140-
std::cerr << "[" << std::right << std::setw(3)
140+
std::cerr << "[" << std::right << std::setw(4)
141141
<< (index+1) << "/" << numTests << "] ["
142142
<< std::left << std::setw(maxCharToDisplay+1)
143143
<< name << "] - "

0 commit comments

Comments
 (0)