Consolidate CIELab↔RGB conversion on DCMTK's IODCIELabUtil and drop dcmqi's ColorUtilities #563

Summary

dcmqi currently carries two independent CIELab↔sRGB implementations and uses both, in different code paths. I propose we consolidate on DCMTK's IODCIELabUtil and delete dcmqi's ColorUtilities.

Beyond the redundancy, the investigation that prompted this turned up a concrete correctness problem: the dcmqi implementation does not round-trip, while DCMTK's does exactly. Details and measurements below.

Current situation

1. dcmqi's own implementation — include/dcmqi/ColorUtilities.h + libsrc/ColorUtilities.cpp, a C++ port of @dclunie's PixelMed ColorUtilities (the Java source is kept in util/ColorUtilities.java). Used on the primary conversion paths:

• libsrc/Itk2DicomConverter.cpp#L323 — getIntegerScaledCIELabPCSFromSRGB(), writing Recommended Display CIELab Value per segment
• libsrc/Dicom2ItkConverterBase.cpp#L269 — getSRGBFromIntegerScaledCIELabPCS(), reading the color back out into the JSON metadata

2. DCMTK's IODCIELabUtil (dcmtk/dcmiod/cielabutil.h). Used in exactly one place:

• libsrc/Bin2Label.cpp#L704 — IODCIELabUtil::dicomLab2RGB(), building the Palette Color LUT for binary→labelmap conversion in PALETTE color model

So a SEG converted to labelmap/PALETTE gets its colors through DCMTK, while the same colors written on the way in, and read back into JSON, go through dcmqi's code. Two code paths, two implementations, same DICOM attribute.

dcmtk/dcmiod/cielabutil.h is additionally #included in Dicom2ItkConverterLabel.cpp#L7 and Dicom2ItkConverterBin.cpp#L8, but neither file calls anything from it — dead includes.

How we got here

ColorUtilities was introduced in ab6a207 (Dec 2020), "ENH: switch to CIELab conversion from @dclunie", with the rationale:

round-trip conversion fails using DCMTK routines

That diagnosis appears to have been mistaken. The DCMTK call sites that were replaced looked like this:

double rgbDouble[3] = {double(rgb[0])/256., double(rgb[1])/256., double(rgb[2])/256.};
IODCIELabUtil::rgb2DicomLab(cielab[0], cielab[1], cielab[2], rgbDouble[0], rgbDouble[1], rgbDouble[2]);
// ...
IODCIELabUtil::dicomLab2RGB(rgb[0], rgb[1], rgb[2], ciedcm[0], ciedcm[1], ciedcm[2]);
rgb[0] = unsigned(rgb[0]*256);

IODCIELabUtil expects RGB normalised to 0..1, i.e. division by 255, not 256 — and the return path truncates with unsigned(...) rather than rounding. That scaling error and truncation, in dcmqi's own glue code, is what broke the round trip. DCMTK's conversion math was not at fault.

Measurements

Exhaustive sweep over all 16,777,216 RGB triples, round-tripping sRGB → DICOM-scaled CIELab → sRGB, linking the real ColorUtilities.cpp and the real libdcmiod (DCMTK 3.6.8):

The third row reproduces the 2020 observation and confirms its cause: an off-by-one on 13% of values, entirely attributable to the /256 + truncation glue, and bounded at 1 unit. Used correctly, DCMTK recovers every one of the 16.7M triples exactly.

Forward agreement between the two implementations is otherwise fine — max difference 2/65535 in the DICOM-scaled CIELab values, from DCMTK carrying the sRGB matrix to ~19 significant digits vs the 4-digit easyrgb.com constants in ColorUtilities. Both use the D65 white point and the same piecewise gamma, so there is no colorimetric disagreement to reconcile, only precision.

Root cause of the dcmqi failures

libsrc/ColorUtilities.cpp#L89-L95, in getCIEXYZFromLAB():

if (var_Y_pow3 > 0.008856) var_Y = var_Y_pow3;
else                       var_Y = ( var_Y - 16 / 116 ) / 7.787;   // 16 / 116 == 0

16 / 116 is integer division and evaluates to 0 instead of 0.1379. The forward function getCIELabFromXYZ() 20 lines earlier correctly writes 16.0 / 116; only the inverse is affected, so the linear (low-luminance) branch of Lab→XYZ is wrong and dark colors break.

This is a porting bug, not inherited: the Java original writes 16d / 116 (util/ColorUtilities.java#L111-L117) and is correct. The d suffix was lost in translation to C++.

Changing those three lines to 16.0 / 116 takes dcmqi's round-trip error to 0/16,777,216 — confirming this is the sole cause.

User-visible effect

Round-tripping a SEG through dcmqi (itkimage2segimage then segimage2itkimage), the RGB reported in the output JSON:

Black comes back as dark grey, and dark red picks up a blue cast. Note also that getSRGBFromCIEXYZPCS() does no clamping, so out-of-range results such as -89 are possible; since these feed SegmentAttributes::setRecommendedDisplayRGBValue(const unsigned&, ...), a negative value is reinterpreted as 4294967207.

The DICOM written by dcmqi is fine — the encoding path is only affected at the 2/65535 precision level. It is the decoding path, and therefore the JSON metadata dcmqi reports, that is wrong.

Proposal

Consolidate on DCMTK's IODCIELabUtil and remove dcmqi's ColorUtilities:

1. Replace the two ColorUtilities call sites with IODCIELabUtil, scaling RGB by 255.0 and rounding (lround) on the way back, with clamping to 0..255.
2. Optionally keep a small dcmqi-internal helper for the int/unsigned ↔ double marshalling, so the 0..255-int convention used throughout dcmqi is expressed in one place rather than at each call site.
3. Delete include/dcmqi/ColorUtilities.h, libsrc/ColorUtilities.cpp and their entries in libsrc/CMakeLists.txt.
4. Drop the dead cielabutil.h includes in Dicom2ItkConverterLabel.cpp and Dicom2ItkConverterBin.cpp, or make them real once the call sites move.
5. Add a round-trip regression test over the RGB cube (or a representative sample including the dark-color cases above) so this cannot regress again.
6. Decide what to do with util/ColorUtilities.java and util/TestColorConversions_SRGB_CIELabPCS.java — presumably remove them alongside, as they exist only as the reference for the C++ port.

Rationale for standardising on DCMTK rather than fixing ColorUtilities in place: dcmqi already depends on DCMTK, IODCIELabUtil is exact over the full RGB cube, it carries higher-precision matrix constants, it is the implementation that DCMTK's own dcmseg/dcmiod use for the same attribute, and one of our code paths (Bin2Label.cpp) is already on it. Keeping a second implementation in dcmqi means maintaining colorimetry code that isn't our core concern.

Worth noting that fixing the 16 / 116 bug is a one-line-times-three change and could go in first as a quick correctness fix if we want the bug closed before the larger consolidation lands.

Backwards-compatibility note

Any comparison baselines that captured RGB values produced by the current (incorrect) decoding path will change. Test data with dark segment colors is where this will show up.

Measurements above were produced against 0ed03e9 with DCMTK 3.6.8, linking the actual ColorUtilities.cpp and libdcmiod — happy to attach the test harness if useful.