fix(color): Fix RGB inconsistencies #829

rodrigobasilio2022 · 2023-10-14T00:17:16Z

Context

This PR aims to fix the problem with RGB inconsistencies in loading specific color dicom images

Changes & Results

Testing

Checklist

PR

[] My Pull Request title is descriptive, accurate and follows the
semantic-release format and guidelines.

Code

[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the yarn build:update-api to update the API documentation, and have
committed the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)

Public Documentation Updates

[] The documentation page has been updated as necessary for any public API
additions or removals.

Tested Environment

[] "OS:
[] "Node version:
[] "Browser:

netlify · 2023-10-14T00:17:22Z

✅ Deploy Preview for cornerstone-3d-docs ready!

Name	Link
🔨 Latest commit	`bf9a8c2`
🔍 Latest deploy log	https://app.netlify.com/sites/cornerstone-3d-docs/deploys/6655f6995682bd00084701ae
😎 Deploy Preview	https://deploy-preview-829--cornerstone-3d-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

sedghi · 2023-10-16T15:16:51Z

This part of the code is very tricky to touch. Please explain the logic behind your addition

rodrigobasilio2022 · 2023-10-16T15:52:52Z

This part of the code is very tricky to touch. Please explain the logic behind your addition

As stated in the lines 21-28 of the same file, the purpose of the code is to transform from RGBA -> RGB, as the main job was already been done by the decompressors. So if the data is already in RGB (3 bytes per voxel), then there is no need to go further in the code. Also the code in here

cornerstone3D/packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertYBRFull422ByPixel.ts

Lines 8 to 14 in b33eac9

    
           if (imageFrame === undefined) { 
        
             throw new Error('decodeRGB: ybrBuffer must not be undefined'); 
        
           } 
        
           if (imageFrame.length % 2 !== 0) { 
        
             throw new Error('decodeRGB: ybrBuffer length must be divisble by 3'); 
        
           }

didnt make much sense (wrong divisor indication, function name and check as it didnt take into account the number of elements...)

sedghi · 2023-10-16T16:05:38Z

I'm pretty certain this breaks a lot of other color images. It is not just RGBA -> RGB, it also handles color space conversions

if (TRANSFER_SYNTAX_USING_PHOTOMETRIC_COLOR[transferSyntax]) {
          canvas.height = imageFrame.rows;
          canvas.width = imageFrame.columns;
          const context = canvas.getContext('2d');
          const imageData = context.createImageData(
            imageFrame.columns,
            imageFrame.rows
          );
          convertColorSpace(imageFrame, imageData.data, useRGBA);
          imageFrame.imageData = imageData;
          imageFrame.pixelData = imageData.data;
          imageFrame.pixelDataLength = imageData.data.length;
        }

rodrigobasilio2022 · 2023-10-16T16:44:16Z

I'm pretty certain this breaks a lot of other color images. It is not just RGBA -> RGB, it also handles color space conversions

if (TRANSFER_SYNTAX_USING_PHOTOMETRIC_COLOR[transferSyntax]) {
          canvas.height = imageFrame.rows;
          canvas.width = imageFrame.columns;
          const context = canvas.getContext('2d');
          const imageData = context.createImageData(
            imageFrame.columns,
            imageFrame.rows
          );
          convertColorSpace(imageFrame, imageData.data, useRGBA);
          imageFrame.imageData = imageData;
          imageFrame.pixelData = imageData.data;
          imageFrame.pixelDataLength = imageData.data.length;
        }

I see. But we need to make sure which conversions were completely handled by decompression. This particular color space, ybr full 422 was completely handled by the decompression and doesn't need any further processing

sedghi · 2023-10-16T16:47:09Z

Yeah i know exactly what you are talking about. I tried to find the correct condition, but everytime I tried something it failed something else. @wayfarer3130 maybe he has an idea of what is the best way to identify if something comes out of the decompresser as RGB

rodrigobasilio2022 · 2023-10-16T17:12:51Z

Yeah i know exactly what you are talking about. I tried to find the correct condition, but everytime I tried something it failed something else. @wayfarer3130 maybe he has an idea of what is the best way to identify if something comes out of the decompresser as RGB

What if for the moment we get the requirements for each conversion and only allow if the input is correct: for example ybr_full_422 expects two bytes data. What if we check that and if the requirements does match we allow proceed and if not we return the data as is? The only extepcting two byte data is the YBR_FULL_422, all others expects three byte data

…one3D-beta into fix/RGBinconsistencies

rodrigobasilio2022 · 2023-10-16T18:03:59Z

Adde a new function that checks the requirements for color space convertions. It convers only one color space conversion for now, the YBR_FULL_422, as it tests if the voxel byte size is equal to 2.

packages/dicomImageLoader/src/imageLoader/convertColorSpace.ts

packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertYBRFullByPixel.ts

packages/dicomImageLoader/src/imageLoader/convertColorSpace.ts

rodrigobasilio2022 · 2023-10-17T12:42:20Z

Changed as suggested

sedghi · 2023-10-17T15:37:55Z

Testing on https://www.cornerstonejs.org/live-examples/dicomimageloaderwadouri

These two are broken which is not in main right now

…one3D-beta into fix/RGBinconsistencies

rodrigobasilio2022 · 2023-10-18T09:51:10Z

Fixed the issue!!!

packages/dicomImageLoader/src/imageLoader/isColorConversionRequirementsFulfilled.ts

sedghi · 2023-10-18T12:18:55Z

packages/dicomImageLoader/src/imageLoader/isColorConversionRequirementsFulfilled.ts

+ * @param RGBA
+ * @returns
+ */
+export default function isColorConversionRequirementsFulfilled(


@wayfarer3130 Can you please look at this function? Thanks

I will limit the checks to only YBR_FULL_422

Limitting the checks to the types ending in _420 or _422 is sufficient, but I would also add an override to allow forcing the issue to return RGB if a flag is set and it starts with YBR - that will then automagically include things like:
YBR_ICT, YBR_RCT, YBR_PARTIAL_422
etc.

There are a few other retired PMI values such as HSV and CMYK, but I doubt you will ever see those.

rodrigobasilio2022 · 2023-10-18T12:38:42Z

Changed as suggested

sedghi · 2024-02-15T14:37:44Z

I don't think we need this after this PR can someone confirm? OHIF/Viewers#3883

wayfarer3130 · 2024-02-15T14:43:07Z

packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertYBRFull422ByPixel.ts

@@ -6,10 +6,12 @@ export default function (
  useRGBA: boolean
 ): void {
  if (imageFrame === undefined) {
-    throw new Error('decodeRGB: ybrBuffer must not be undefined');
+    throw new Error('convertYBRFull422ByPixel: ybrBuffer must be defined');
  }
  if (imageFrame.length % 2 !== 0) {


Please add a comment here that the length is divisible by 2 because for 4 pixels (a 2x2 array), there are 4 y and 2 b and 2 r values, so 4+2+2 / 4 = 2 bytes per pixel

packages/dicomImageLoader/src/imageLoader/isColorConversionRequirementsFulfilled.ts

wayfarer3130 · 2024-02-15T14:45:28Z

packages/dicomImageLoader/src/imageLoader/isColorConversionRequirementsFulfilled.ts

+    return false;
+  }
+  const { rows, columns } = imageFrame;
+  if (imageFrame.photometricInterpretation === 'YBR_FULL_422') {


You can check endsWith 422 or endsWIth 420 to support all the test values we can check.

as we talked to Bill
the formula should be edited to

for ybr_Full_422

ybr y ybr y ybr .... (ybr or y) y y y y y ........ y .. .. .. (last row) ybr y ybr y ybr y ... (ybr or y) OR y y y y y y

-> 3 * ceil(cols/2) + florr(cols/2) * ceil(rows/2) + cols * floor(rows/2)

number of ybr y ybr y ybr y elements

ybr full 420

ybr y ybr y ybr y ybr y ybr y ybr y

for ybr_420
(3 * ceilt(cols/2) + floor (col/2)) * rows

if it is /3 -> convert as RGB

as we talked to Bill the formula should be edited to

for ybr_Full_422

ybr y ybr y ybr .... (ybr or y) y y y y y ........ y .. .. .. (last row) ybr y ybr y ybr y ... (ybr or y) OR y y y y y y

-> 3 * ceil(cols/2) + florr(cols/2) * ceil(rows/2) + cols * floor(rows/2)

That is:
(3 * ceil(cols/2) + floor(cols/2)) * ceil(rows/2) + cols * floor(rows/2)

Note the parenthesis around the first expression. That breaks down into:
There are ceil(rows/2) YBR containing rows (think about 1,2,3 rows, having 1 1 and 2 YBR containing rows)
There are floor(rows/2) NON YBR containing rows. Those have length cols as they are just y containing.
For each YBR row, there are ceil(cols/2) YBR elements - think about hte same thing as the rows, but for columns - the first, third, fifth etc column has YBR. Each YBR has 3 elements (Y b and r)
Then, for each YBR row, there are floor(cols/2) Y element only.

wayfarer3130 · 2024-02-15T14:47:27Z

packages/dicomImageLoader/src/imageLoader/isColorConversionRequirementsFulfilled.ts

+  const { rows, columns } = imageFrame;
+  if (imageFrame.photometricInterpretation === 'YBR_FULL_422') {
+    return imageFrame.pixelDataLength === 2 * rows * columns && rows % 2 === 0;
+  } else {


Add a settable flag allowing coercion to RGB if the response is uncompressed, and the original is YBR of some flavour. That flag could go in cornerstone settings, and maybe be triggered based on the URL prefix (eg url.startsWith), or a regular expression match.

Can you please add more to this? I don't understand

If the photometric interpretation is YBR_FULL_422, BUT the length is 3rowscolumns, then assume it is the configurable value of either RGB or YBR_FULL. That is invalid data, but is common enough that we see it regularly as a "bug" fix request.
To decide whether RGB or YBR_FULL, check the datasource configuration - something like:
UncompressedColorEncoding=YBR_FULL | RGB (default is RGB)
and perhaps
ForceColorEncoding=AllUncompressed | All | WrongLength (default is WrongLength)

Then, have the above function return the PMI to use for the given image.

const {ForceColorEncoding = WrongLength, UncompressedColorEncoding = RGB} = dataSourceConfiguration;

if( pixel length === 3 * rows * columns && pmi ==='YBR_FULL_422 && forceColorEncoding===WrongLength ) {
return UncompressedColorEncoding

indicating that this image needs decompression handling, in the specified color encoding.

wayfarer3130 · 2024-02-15T14:59:28Z

I don't think we need this after this PR can someone confirm? OHIF/Viewers#3883

Unfortunately, the back end can still decide to reply in uncompressed, and they can still be invalid, so yes, I think this is a good idea still. It is technically wrong, but practically is the right thing to do.

…/rodrigobasilio2022/fix/RGBinconsistencies

Fix RGB inconsistencies

0084489

rodrigobasilio2022 added 2 commits October 16, 2023 14:57

Add color space conversion check

0409f4a

Merge branch 'main' of https://github.com/rodrigobasilio2022/cornerst…

d4739ff

…one3D-beta into fix/RGBinconsistencies

sedghi reviewed Oct 16, 2023

View reviewed changes

packages/dicomImageLoader/src/imageLoader/convertColorSpace.ts Outdated Show resolved Hide resolved

wayfarer3130 reviewed Oct 17, 2023

View reviewed changes

packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertYBRFullByPixel.ts Outdated Show resolved Hide resolved

wayfarer3130 reviewed Oct 17, 2023

View reviewed changes

packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertYBRFullByPixel.ts Outdated Show resolved Hide resolved

refactor(comments) : refactor comments in code

7810290

wayfarer3130 reviewed Oct 17, 2023

View reviewed changes

packages/dicomImageLoader/src/imageLoader/convertColorSpace.ts Outdated Show resolved Hide resolved

wayfarer3130 reviewed Oct 17, 2023

View reviewed changes

packages/dicomImageLoader/src/imageLoader/convertColorSpace.ts Outdated Show resolved Hide resolved

refactor(code): change requirements YBR_FULL_422

02b2117

rodrigobasilio2022 added 2 commits October 18, 2023 06:34

Merge branch 'main' of https://github.com/rodrigobasilio2022/cornerst…

b6c8838

…one3D-beta into fix/RGBinconsistencies

fix(color): fix bug in color example

f133ace

fix bug in image loader example

c252514

sedghi reviewed Oct 18, 2023

View reviewed changes

packages/dicomImageLoader/src/imageLoader/isColorConversionRequirementsFulfilled.ts Outdated Show resolved Hide resolved

sedghi reviewed Oct 18, 2023

View reviewed changes

Refactoring function

274c609

sedghi mentioned this pull request Jan 8, 2024

feat(transferSyntax): prefer server transcoded transfer syntax for all images OHIF/Viewers#3883

Merged

wayfarer3130 reviewed Feb 15, 2024

View reviewed changes

packages/dicomImageLoader/src/imageLoader/isColorConversionRequirementsFulfilled.ts Outdated Show resolved Hide resolved

wayfarer3130 reviewed Feb 15, 2024

View reviewed changes

packages/dicomImageLoader/src/imageLoader/isColorConversionRequirementsFulfilled.ts Outdated Show resolved Hide resolved

wayfarer3130 reviewed Feb 15, 2024

View reviewed changes

sedghi mentioned this pull request Feb 15, 2024

Study with Photometric Interpretation YBR_FULL_422 are "green" OHIF/Viewers#2887

Closed

sedghi added 3 commits May 21, 2024 16:34

Merge branch 'main' of github.com:cornerstonejs/cornerstone3D into pr…

30304fb

…/rodrigobasilio2022/fix/RGBinconsistencies

fix: correct typo in color space conversion function

a60d973

refactor: Refactor color space conversion functions

bf9a8c2

wayfarer3130 approved these changes May 28, 2024

View reviewed changes

sedghi merged commit 2fc22bc into cornerstonejs:main May 28, 2024
10 checks passed

This was referenced May 29, 2024

fix(ultrasound): Upgrade cornerstone3D version to resolve coloring issues OHIF/Viewers#4181

Merged

Iteration Plan for 3.9 OHIF/Viewers#4094

Open

igoroctaviano mentioned this pull request Oct 23, 2024

[Bug] Regression handling RGB in XC modality OHIF/Viewers#4382

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(color): Fix RGB inconsistencies #829

fix(color): Fix RGB inconsistencies #829

rodrigobasilio2022 commented Oct 14, 2023

netlify bot commented Oct 14, 2023 •

edited

Loading

sedghi commented Oct 16, 2023

rodrigobasilio2022 commented Oct 16, 2023 •

edited

Loading

sedghi commented Oct 16, 2023

rodrigobasilio2022 commented Oct 16, 2023

sedghi commented Oct 16, 2023

rodrigobasilio2022 commented Oct 16, 2023

rodrigobasilio2022 commented Oct 16, 2023

rodrigobasilio2022 commented Oct 17, 2023

sedghi commented Oct 17, 2023

rodrigobasilio2022 commented Oct 18, 2023

sedghi Oct 18, 2023

rodrigobasilio2022 Oct 18, 2023

wayfarer3130 Feb 15, 2024

rodrigobasilio2022 commented Oct 18, 2023

sedghi commented Feb 15, 2024

wayfarer3130 Feb 15, 2024

wayfarer3130 Feb 15, 2024

sedghi Mar 20, 2024 •

edited

Loading

wayfarer3130 Mar 22, 2024

wayfarer3130 Feb 15, 2024

sedghi Feb 15, 2024

wayfarer3130 Mar 22, 2024

wayfarer3130 commented Feb 15, 2024

fix(color): Fix RGB inconsistencies #829

fix(color): Fix RGB inconsistencies #829

Conversation

rodrigobasilio2022 commented Oct 14, 2023

Context

Changes & Results

Testing

Checklist

PR

Code

Public Documentation Updates

Tested Environment

netlify bot commented Oct 14, 2023 • edited Loading

✅ Deploy Preview for cornerstone-3d-docs ready!

sedghi commented Oct 16, 2023

rodrigobasilio2022 commented Oct 16, 2023 • edited Loading

sedghi commented Oct 16, 2023

rodrigobasilio2022 commented Oct 16, 2023

sedghi commented Oct 16, 2023

rodrigobasilio2022 commented Oct 16, 2023

rodrigobasilio2022 commented Oct 16, 2023

rodrigobasilio2022 commented Oct 17, 2023

sedghi commented Oct 17, 2023

rodrigobasilio2022 commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigobasilio2022 commented Oct 18, 2023

sedghi commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sedghi Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wayfarer3130 commented Feb 15, 2024

netlify bot commented Oct 14, 2023 •

edited

Loading

rodrigobasilio2022 commented Oct 16, 2023 •

edited

Loading

sedghi Mar 20, 2024 •

edited

Loading