Add support for multiple Output ICC profile types #6

diricxbart · 2025-06-22T07:39:49Z

The default Output ICC profile is still "sRGB" (backwards compatibility).
Additionally, a second wide-gamut Output ICC profile "Display-P3" has been added.
This profile uses a D65 white point, a sRGB transfer function, the DCI-P3 primaries and is widely supported by modern browsers.

The default Output ICC profile is still "sRGB" (backwards compatibility). Additionally, a second wide-gamut Output ICC profile "Display-P3" has been added. This profile uses a D65 white point, a sRGB transfer function, the DCI-P3 primaries and is widely supported by modern browsers.

The original implementation assumes the display used to view the WSI images is using the sRGB color space. Now we first check if the display color space is sRGB or Display-P3 (a more recent wide-gamut color space). Based on this dicomicc is configured to either transform colors to the sRGB color space or to the Display-P3 color space. Prerequisite: approval of Pull Request to add support for Display-P3 in libdicomicc (ImagingDataCommons/libdicomicc#6)

diricxbart · 2025-06-22T19:58:51Z

I'm not sure if this failing macOS build and test has something to do with my changes, or if in general this test is broken?

install to a system dir does not work on mac

diricxbart · 2025-07-10T16:46:41Z

Just wandering if the macos version shouldn't also use sudo make install like the Linux version, now it's not using sudo...

diricxbart · 2025-07-31T07:30:46Z

I added a change which 'might' fix the build issue on macos (which is unrelated to the other functional changes in this PR), however, a maintainer needs to approve running the github action to see if this resolves the issue.
@fedorov could you please have a look at this?

diricxbart · 2025-08-04T08:05:30Z

Hi @fedorov,
My latest change fixed the pre-existing build failure, I assume this PR is now ready to merge?

fedorov · 2025-08-05T02:49:04Z

wasm/src/ColorManager.hpp

  ColorManager(FrameInfo frameInfo,
-               const val &iccProfile) {
+               const val &iccProfile,
+               int outputType = 0 /* 0: sRGB (default), 1: Display-P3 */) {


Why not use the enum constants here instead of 0 and 1?

fedorov · 2025-08-05T02:53:36Z

@diricxbart I appreciate your contribution, but I have to say I am not sure how to proceed. The developer of this library who had expertise in the subject moved on, and I am not sure who can review this PR appropriately. I do not have the needed expertise I am afraid.

@dclunie do you think you could take a look at the new profile added?

@igoroctaviano how can we test and confirm this does not cause a regression in slim?

dclunie · 2025-08-05T14:03:42Z

FYI, the addition of this new color space is not (quite) yet standardized in DICOM - see CP 2474, which @diricxbart initiated and is currently out for letter ballot.

I would suggest that ADOBERGB and ROMMRGB also be included as alternatives, not just DISPLAYP3, and I too prefer meaningful string-named enums rather than opaque numeric values.

I haven't looked at the code so I can't comment on how this new parameter is being used (fed to the color management system to actually command that the color conversion from PCS to the defined color space be performed).

… + Add support for Adobe RGB (1998) and ROMM RGB

diricxbart · 2025-08-07T16:23:15Z

@fedorov My latest commit should handle your feedback on using the enum constant.

@dclunie My latest commit now also additionally adds support for Adobe RGB (1998) and ROMM RGB.

@fedorov To validate the new functionality, I took the following approach. The main thing this PR adds is a selectable output ICC profile, instead of a fixed sRGB profile. The key thing to validate here is, I think, the correctness of the 3 new ICC profiles which are created. The approach I took here was twofold: (1) read the spec for each of these colour spaces (see links I added as documentation in the code) and (2) compare against existing ICC profiles in this repository. I each time used the v4 ICC profiles, using the parametric curve approach: DisplayP3-v4.icc, ProPhoto-v4.icc (which is an alias for ROMM RGB) and AdobeCompat-v4.icc.

Next I used the ICC Profile Inspector and compared the different tags (wtpt, chad, rXYZ, gXYZ, bXYZ, rTRC, gTRC and bTRC). I can conclude that for Display-P3, Adobe RGB (1998) and ROMM RGB there's a perfect match...

Please find here some additional background info on why the type 4 parametric curve was selected instead of the original type 5 (see this commit), assuring black and white remain unchanged by the transfer curve.

… white remain unchanged by the transfer function. See saucecontrol/Compact-ICC-Profiles#22 (comment) for more info

diricxbart · 2025-09-04T20:27:28Z

Hi @fedorov , anything else I can help with to get this change approved?

fedorov · 2025-09-04T20:41:45Z

I am sorry!

@dclunie do you have any other requests?

Copilot

Pull Request Overview

This PR adds support for multiple output ICC profile types to extend the color management capabilities beyond the default sRGB. The default behavior remains unchanged for backward compatibility.

Introduces an enum DcmIccOutputType with four color profile options: sRGB, Display-P3, Adobe RGB, and ROMM RGB
Adds a new function dcm_icc_transform_create_for_output that accepts an output type parameter
Updates the ColorManager constructor to accept an optional output type parameter with sRGB as default

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
wasm/src/jslib.cpp	Exposes the new DcmIccOutputType enum to JavaScript and updates ColorManager constructor to accept output type parameter
wasm/src/ColorManager.hpp	Modifies constructor to accept optional output type parameter and uses new create function
src/dicomicc.h	Defines DcmIccOutputType enum and declares new create function with output type support
src/dicomicc.c	Implements profile creation functions for each color space and adds backward-compatible wrapper
package.json	Bumps version from 0.1.0 to 0.2.0
CMakeLists.txt	Updates minor version number from 1 to 2
.github/workflows/run_unit_tests.yml	Updates cmake configuration with install prefix

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

package.json

src/dicomicc.c

fedorov · 2025-09-04T20:44:43Z

@diricxbart mac build is failing!

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

diricxbart · 2025-09-04T21:53:33Z

@fedorov I processed the copilot review remarks and implemented an attempt to fix the failing macos build as I think it indicates that cmake is already installed in that image and doesn't have to be installed again (not 100% sure though)

diricxbart mentioned this pull request Jun 22, 2025

Add support for dynamically configuring the dicomicc iccOutputType ImagingDataCommons/dicom-microscopy-viewer#186

Open

change install location

37790cd

install to a system dir does not work on mac

Attempt to fix unit test build failure on macos

3885fff

fedorov reviewed Aug 5, 2025

View reviewed changes

Handle PR feedback: use enum value instead of hardcoded integer value…

db22f94

… + Add support for Adobe RGB (1998) and ROMM RGB

diricxbart mentioned this pull request Aug 7, 2025

ProPhoto RGB (ROMM RGB) uses incorrect parametric curve saucecontrol/Compact-ICC-Profiles#22

Closed

Use ROMM RGB without 'measurement correction', guaranteeing black and…

d4e3e51

… white remain unchanged by the transfer function. See saucecontrol/Compact-ICC-Profiles#22 (comment) for more info

fedorov requested a review from Copilot September 4, 2025 20:43

Copilot AI reviewed Sep 4, 2025

View reviewed changes

package.json Show resolved Hide resolved

src/dicomicc.c Outdated Show resolved Hide resolved

diricxbart changed the title ~~Add suport for multiple Output ICC profile types~~ Add support for multiple Output ICC profile types Sep 4, 2025

diricxbart and others added 2 commits September 4, 2025 23:29

Fix typo in comments based on copilot review

35dd17a

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

macos-latest already has cmake installed, no need to install it again

6d9b675

fedorov merged commit 7c8f993 into ImagingDataCommons:master Sep 5, 2025
2 checks passed

rflamand mentioned this pull request Dec 11, 2025

Update ColorManager constructor with proper type #9

Open

Add support for multiple Output ICC profile types #6

Add support for multiple Output ICC profile types #6

Uh oh!

Conversation

diricxbart commented Jun 22, 2025

Uh oh!

diricxbart commented Jun 22, 2025

Uh oh!

diricxbart commented Jul 10, 2025

Uh oh!

diricxbart commented Jul 31, 2025

Uh oh!

diricxbart commented Aug 4, 2025

Uh oh!

fedorov Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

fedorov commented Aug 5, 2025

Uh oh!

dclunie commented Aug 5, 2025

Uh oh!

diricxbart commented Aug 7, 2025 • edited by fedorov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diricxbart commented Sep 4, 2025

Uh oh!

fedorov commented Sep 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

fedorov commented Sep 4, 2025

Uh oh!

diricxbart commented Sep 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

diricxbart commented Aug 7, 2025 •

edited by fedorov

Loading