[WIP] Color management using ICC profiles

[hackermd]

Describe the changes

Adds support for color management using ICC Profiles (see #1244).

Tasks

• Unit tests added that reproduce the issue or prove feature is working
• Fix or feature added
• Code typed and mypy shows no errors
• Documentation updated (if relevant)
  • No warnings during build
  • Preview link (CircleCI -> Artifacts -> doc/_build/html/index.html)
• Unit tests passing and overall coverage the same or better

[pep8speaks]

Hello @hackermd! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file pydicom/color.py:

Line 69:80: E501 line too long (83 > 79 characters)

Comment last updated at 2021-07-22 00:35:16 UTC

[hackermd]

Note that this feature requires Pillow, which is an optional dependency.

[codecov]

Codecov Report

Merging #1446 (9cb072a) into master (4aed913) will decrease coverage by 0.03%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master    #1446      +/-   ##
==========================================
- Coverage   97.42%   97.38%   -0.04%     
==========================================
  Files          66       67       +1     
  Lines       10185    10225      +40     
==========================================
+ Hits         9923     9958      +35     
- Misses        262      267       +5     

Impacted Files	Coverage Δ
pydicom/color.py	87.50% <87.50%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4aed913...9cb072a. Read the comment docs.

[scaramallion]

I know almost nothing about ICC profiles, but is relative colorimetric correct? Standard has:

The following constraints on the encoding of the ICC Profile are recommended:
The Rendering Intent should be Perceptual.

And that's only a recommendation, not a requirement.

[hackermd]

Also not sure what intent would be best suited for the library. We may want to add a parameter so that the caller can change the behavior if necessary. Thoughts?

[hackermd]

@dclunie do you have any thoughts on this?

[scaramallion]

Does this need to be a class in a new module? I was thinking of adding it as a function to the pixel utilities instead:

def apply_icc_profile(
    arr: "np.ndarray",
    icc_profile: Optional[bytes] = None,
    transform: Optional["PIL.ImageCms.CmsTransform"] = None
) -> "np.ndarray":
    """Return `arr` with an applied ICC profile.

    .. versionadded:: 2.3

    .. warning::

        ``apply_icc_profile()`` requires `Pillow
        <https://pillow.readthedocs.io/en/stable/>`_ and
        `NumPy<https://numpy.org/>`_

    Parameters
    ----------
    arr : numpy.ndarray
        An :class:`~numpy.ndarray` containing single or multi-framed RGB pixel
        data.
    icc_profile : bytes, optional
        The ICC profile to apply to `arr`, as given by (0028,2000) *ICC
        Profile*. Required if `cms_profile` is not supplied. Only sRGB is
        supported as an output color space.
    transform : PIL.ImageCms.CmsTransform, optional
        An existing transform to apply to `arr` rather than creating one
        from `icc_profile`. Required if `icc_profile` is not supplied.
    """
    if not HAVE_PIL or not HAVE_NP:
        raise ImportError("apply_icc_profile() requires Pillow and NumPy")

    if icc_profile is None and transform is None:
        raise ValueError(
            "One of 'icc_profile' or 'transform' is required"
        )

    if icc_profile and transform:
        raise ValueError(
            "Only one of 'icc_profile' or 'transform' may be used"
        )

    if arr.ndim not in (3, 4) or arr.shape[-1] != 3:
        raise ValueError(
            "'arr' has incorrect dimensions for RGB pixel data"
        )

    if icc_profile:
        transform = ImageCms.buildTransform(
            inputProfile=ImageCms.ImageCmsProfile(BytesIO(icc_profile)),
            outputProfile=ImageCms.createProfile("sRGB"),
            inMode="RGB",  # according to PS3.3 C.11.15.1.1
            outMode="RGB",
        )

    if arr.ndim == 3:
        im = Image.fromarray(arr)
        ImageCms.applyTransform(im, transform, inPlace=True)
        return np.asarray(im)

    for idx, frame in enumerate(arr):
        im = Image.fromarray(arr)
        ImageCms.applyTransform(im, transform, inPlace=True)
        arr[idx] = np.asarray(im)

    return arr

[scaramallion]

Maybe add a note somewhere that sRGB is the only supported color space

[dclunie]

sRGB is a very limited color space and its use as an intermediary may result in out of gamut colors being inappropriately handled - how is your color management implementation going to take advantage of a calibrated monitor (especially a "wide gamut" monitor), or in any case, with a gamut wider than sRGB supports (not to mention the quantization issues caused by using 8 bit per channel intermediary spaces for transformations that are not linear)?

I appreciate that anything beyond an sRGB working space in HTML5 Canvas may be a bit beyond the state of the art at the moment, but if this is not handled properly, images in browsers may display poorly compared to thick clients; see also https://lists.w3.org/Archives/Public/public-colorweb/2021Dec/att-0002/2021-Conversion_Overview.pdf.

You might want to consult with some color experts from ICC, W3C and FDA.

[hackermd]

Thanks for your feedback @dclunie. For the Python library, my focus has been on image analysis (for which sRGB may be ok?) rather than display use cases. However, I agree that we should implement this in a way that would work for a variety of use cases.

@scaramallion we could just add an optional parameter output_profile to allow the caller to provide the output ICC profile as well.

[hackermd]

Since the caller can already provide the transform object, we may indeed just need to document the default behaviour.

[hackermd]

Does this need to be a class in a new module? I was thinking of adding it as a function to the pixel utilities instead:

I would prefer a "functor" instead of a function. For multi-frame images with a large number of frames, we don't want to build the transform object over and over again for every frame. It would be nice to just create an instance of a class and repeatedly use that instance.

This function allows for that, too, since the transform object can be passed as an argument. However, it thereby exposes implementation details (Pillow type), which I was trying to avoid.

[scaramallion]

For multi-frame images with a large number of frames, we don't want to build the transform object over and over again for every frame.

I did include a way to re-use the transform for a multi-frame array (apart from supplying the transform parameter). buildTransform() is kinda expensive...

I don't think I'm too fussed about the "PIL.ImageCms.CmsTransform" transform parameter. Anyone that knows enough to be using the parameter would be comfortably creating one, and for everyone else a function provides an easy way to apply the color profile.

🤷

[hackermd]

For multi-frame images with a large number of frames, we don't want to build the transform object over and over again for every frame.

I did include a way to re-use the transform for a multi-frame array (apart from supplying the transform parameter). buildTransform() is kinda expensive...

I don't think I'm too fussed about the "PIL.ImageCms.CmsTransform" transform parameter. Anyone that knows enough to be using the parameter would be comfortably creating one, and for everyone else a function provides an easy way to apply the color profile.

🤷

I would personally prefer:

class ColorRenderingIntent(enum):

    PERCEPTUAL = 'perceptual'
    RELATIVE_COLORIMETRIC = 'relative_colorimetric'
    ABSOLUTE_COLORIMETRIC = 'absolute_colorimetric'


class ColorTransformer:

    def __init__(
        self,
        icc_input_profile: bytes,
        icc_output_profile: Optional[bytes] = None,
        rendering_intent: ColorRenderingIntent = ColorRenderingIntent.PERCEPTUAL
    ):
        pass

    def __call__(self, image: numpy.ndarray) -> numpy.ndarray:
        pass

color_transform = ColorTransformer(icc_input_profile)
for frame in selected_frames:
    corrected_frame = color_transform(frame)

Since Pillow is an optional dependency, I would avoid exposing the Pillow type via the public API. This will allow us to change the implementation if necessary without breaking backwards compatibility.

The callable "functor" style is also widely used by other Python libaries (e.g., torchvision.transforms).