Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(color): Fix RGB inconsistencies #829

Merged
merged 12 commits into from
May 28, 2024
Merged

fix(color): Fix RGB inconsistencies #829

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0084489
Fix RGB inconsistencies
rodrigobasilio2022 Oct 14, 2023
0409f4a
Add color space conversion check
rodrigobasilio2022 Oct 16, 2023
d4739ff
Merge branch 'main' of https://github.com/rodrigobasilio2022/cornerst…
rodrigobasilio2022 Oct 16, 2023
7810290
refactor(comments) : refactor comments in code
rodrigobasilio2022 Oct 17, 2023
02b2117
refactor(code): change requirements YBR_FULL_422
rodrigobasilio2022 Oct 17, 2023
b6c8838
Merge branch 'main' of https://github.com/rodrigobasilio2022/cornerst…
rodrigobasilio2022 Oct 18, 2023
f133ace
fix(color): fix bug in color example
rodrigobasilio2022 Oct 18, 2023
c252514
fix bug in image loader example
rodrigobasilio2022 Oct 18, 2023
274c609
Refactoring function
rodrigobasilio2022 Oct 18, 2023
30304fb
Merge branch 'main' of github.com:cornerstonejs/cornerstone3D into pr…
sedghi May 21, 2024
a60d973
fix: correct typo in color space conversion function
sedghi May 21, 2024
bf9a8c2
refactor: Refactor color space conversion functions
sedghi May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/dicomImageLoader/examples/dicomImageLoaderWADOURI/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import {
setTitleAndDescription,
} from '../../../../utils/demo/helpers';

import { cache } from '@cornerstonejs/core';

// This is for debugging purposes
console.warn(
'Click on index.ts to open source code for this example --------->'
Expand All @@ -45,6 +47,7 @@ addToggleButtonToToolbar({
defaultToggle: false,
onClick(toggle) {
toggle ? setUseCPURendering(true) : setUseCPURendering(false);
cache.purgeCache();
},
});

Expand All @@ -53,6 +56,7 @@ addToggleButtonToToolbar({
defaultToggle: false,
onClick(toggle) {
toggle ? setPreferSizeOverAccuracy(true) : setPreferSizeOverAccuracy(false);
cache.purgeCache();
},
});

Expand Down
6 changes: 4 additions & 2 deletions packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertRGBColorByPixel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ export default function (
useRGBA: boolean
): void {
if (imageFrame === undefined) {
throw new Error('decodeRGB: rgbBuffer must not be undefined');
throw new Error('decodeRGB: rgbBuffer must be defined');
}
if (imageFrame.length % 3 !== 0) {
throw new Error('decodeRGB: rgbBuffer length must be divisible by 3');
throw new Error(
`decodeRGB: rgbBuffer length ${imageFrame.length} must be divisible by 3`
);
}

const numPixels = imageFrame.length / 3;
Expand Down
6 changes: 4 additions & 2 deletions packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertRGBColorByPlane.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ export default function (
useRGBA: boolean
): void {
if (imageFrame === undefined) {
throw new Error('decodeRGB: rgbBuffer must not be undefined');
throw new Error('decodeRGB: rgbBuffer must be defined');
}
if (imageFrame.length % 3 !== 0) {
throw new Error('decodeRGB: rgbBuffer length must be divisible by 3');
throw new Error(
`decodeRGB: rgbBuffer length ${imageFrame.length} must be divisible by 3`
);
}

const numPixels = imageFrame.length / 3;
Expand Down
6 changes: 4 additions & 2 deletions packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertYBRFull422ByPixel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new Error('decodeRGB: ybrBuffer length must be divisble by 3');
throw new Error(
`convertYBRFull422ByPixel: ybrBuffer length ${imageFrame.length} must be divisible by 2`
);
}

const numPixels = imageFrame.length / 2;
Expand Down
6 changes: 4 additions & 2 deletions packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertYBRFullByPixel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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('convertYBRFullByPixel: ybrBuffer must be defined');
}
if (imageFrame.length % 3 !== 0) {
throw new Error('decodeRGB: ybrBuffer length must be divisble by 3');
throw new Error(
`convertYBRFullByPixel: ybrBuffer length ${imageFrame.length} must be divisible by 3`
);
}

const numPixels = imageFrame.length / 3;
Expand Down
6 changes: 4 additions & 2 deletions packages/dicomImageLoader/src/imageLoader/colorSpaceConverters/convertYBRFullByPlane.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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('convertYBRFullByPlane: ybrBuffer must be defined');
}
if (imageFrame.length % 3 !== 0) {
throw new Error('decodeRGB: ybrBuffer length must be divisble by 3');
throw new Error(
`convertYBRFullByPlane: ybrBuffer length ${imageFrame.length} must be divisible by 3`
);
}

const numPixels = imageFrame.length / 3;
Expand Down
9 changes: 6 additions & 3 deletions packages/dicomImageLoader/src/imageLoader/createImage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import {
PixelDataTypedArray,
} from '../types';
import convertColorSpace from './convertColorSpace';
import isColorConversionRequirementsFulfilled from './isColorConversionRequirementsFulfilled';
import decodeImageFrame from './decodeImageFrame';
import getImageFrame from './getImageFrame';
import getScalingParameters from './getScalingParameters';
import { getOptions } from './internal/options';
import isColorImageFn from '../shared/isColorImage';
import isJPEGBaseline8BitColor from './isJPEGBaseline8BitColor';

/**
* When using typical decompressors to decompress compressed color images,
Expand Down Expand Up @@ -228,9 +228,12 @@ function createImage(
cornerstone.metaData.get('modalityLutModule', imageId) || {};
const sopCommonModule: MetadataSopCommonModule =
cornerstone.metaData.get('sopCommonModule', imageId) || {};
const { rows, columns } = imageFrame;
if (isColorImage) {
const { rows, columns } = imageFrame;
if (TRANSFER_SYNTAX_USING_PHOTOMETRIC_COLOR[transferSyntax]) {
if (
TRANSFER_SYNTAX_USING_PHOTOMETRIC_COLOR[transferSyntax] &&
isColorConversionRequirementsFulfilled(imageFrame, useRGBA)
) {
canvas.height = imageFrame.rows;
canvas.width = imageFrame.columns;
const context = canvas.getContext('2d');
Expand Down
24 changes: 24 additions & 0 deletions packages/dicomImageLoader/src/imageLoader/isColorConversionRequirementsFulfilled.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* This function checks color space conversion data requirements before
* apply them. This function was created to solve problems like the one
sedghi marked this conversation as resolved.
Show resolved Hide resolved
* discussed in here https://discourse.orthanc-server.org/t/orthanc-convert-ybr-to-rgb-but-does-not-change-metadata/3533/17
sedghi marked this conversation as resolved.
Show resolved Hide resolved
* In this case, Orthanc server convertes the pixel data from YBR to RGB, but maintain
* the photometricInterpretation dicom tag in YBR
* @param imageFrame
* @param RGBA
* @returns
*/
export default function isColorConversionRequirementsFulfilled(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wayfarer3130 Can you please look at this function? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will limit the checks to only YBR_FULL_422

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

imageFrame,
RGBA
) {
if (imageFrame === undefined) {
return false;
}
const { rows, columns } = imageFrame;
if (imageFrame.photometricInterpretation === 'YBR_FULL_422') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@sedghi sedghi Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return imageFrame.pixelDataLength === 2 * rows * columns && rows % 2 === 0;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true;
}
}