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.

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

GDCM ignores image geometry in secondary captures #4109

Closed
pieper opened this issue Jul 14, 2023 · 9 comments
Closed

GDCM ignores image geometry in secondary captures #4109

pieper opened this issue Jul 14, 2023 · 9 comments
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances

Comments

@pieper
Copy link
Contributor

pieper commented Jul 14, 2023

Description

Secondary capture images can optionally have ImageOrientationPatient and PixelSpacing fields, but GDCM currently forces the orientation to identity and looks for NominalScannedPixelSpacing instead of PixelSpacing, so otherwise valid images are loaded incorrectly.

There is more information in the discussion at this PR:

Slicer/Slicer#7089

Steps to Reproduce

The data available from the idc-open-data set on aws is the Visible Human Project male dataset converted to dicom by @dclunie.

You can get one file with this command:

aws s3 cp --no-sign-request s3://idc-open-data/4aaf9181-fb6a-4a4c-bf49-d1eb9ed4a385/fbc89e1c-29c4-48cf-89fd-53b7c1dc1488.dcm .

Or the whole dataset using this command (about 13 gigabytes).

s5cmd --no-sign-request --endpoint-url https://s3.amazonaws.com cp 's3://idc-open-data/4aaf9181-fb6a-4a4c-bf49-d1eb9ed4a385/*' .

Expected behavior

The index to physical transform should be correctly populated.

Actual behavior

The direction cosines are explicitly set of identity:

https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/GDCM/src/gdcm/Source/MediaStorageAndFileFormat/gdcmImageHelper.cxx#L731-L740

The PixelSpacing is ignored, and the NominalScannedPixelSpacing is looked for, but is typically absent.

https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/GDCM/src/gdcm/Source/MediaStorageAndFileFormat/gdcmImageHelper.cxx#L1265-L1273

Reproducibility

Always

Versions

Tested on current main branch, but I suspect the code has been around for decades.

Environment

Any

Additional Information

I'll probably fix this, although it may not be a high priority if we fix #4108 instead.

@pieper pieper added the type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances label Jul 14, 2023
pieper added a commit to ITK-fork/ITK that referenced this issue Jul 15, 2023
Secondary capture images can have pixel spacing and image
orientation, but the GDCM code explicitly ignored these values
(whereas DCMTK code read them).

Per David Clunie (@dclunie) the editor of the standard,
it was clarified in CP-586 that PixelSpacing is the correct
place to store the best known value (e.g. one that has
been corrected for distortion) while other fields like the
NominalScannedPixelSpacing, which GDCM used before this
commit, if stored at all should have the uncorrected values.

For these reasons this commit makes the default behavior of
GDCM for SC match what it would do if SC images were MR or CT
in terms of handling the image geometry, which is a better
default than the previos behavior.

https://dicom.nema.org/medical/dicom/Final/cp586_ft.pdf

Fixes InsightSoftwareConsortium#4109
@dclunie
Copy link

dclunie commented Jul 26, 2023

@issakomi I have asked @malaterre to reopen and implement the referenced PR, and uploaded to the PR a copy of a proposed DICOM CP specifying the behavior.

@issakomi
Copy link
Member

Thank you very much for your attention to the problem.

@malaterre
Copy link
Member

As stated elsewhere, you already have a mechanism in place directly in ITK to handle any extension you like:

As stated also elsewhere, you could be totally backward compatible with all the existing tools out there, if only you had used the right SOP Class : 1.2.840.10008.5.1.4.1.1.7.4 [Multi-frame True Color Secondary Capture Image Storage]

@malaterre
Copy link
Member

Secondary capture images can optionally have ImageOrientationPatient and PixelSpacing fields, but GDCM currently forces the orientation to identity and looks for NominalScannedPixelSpacing instead of PixelSpacing, so otherwise valid images are loaded incorrectly.

This is what I read with my version of dciodvfy(*). Just implement your private extended SOP Class directly in ITK.

(*)

$ dciodvfy 000198cd-cd88-4760-97d1-21bbea047fff.dcm
[...]
Warning - Attribute is not present in standard DICOM IOD - (0x0018,0x0088) DS Spacing Between Slices
Warning - Attribute is not present in standard DICOM IOD - (0x0020,0x0032) DS Image Position (Patient)
Warning - Attribute is not present in standard DICOM IOD - (0x0020,0x0037) DS Image Orientation (Patient)
Warning - Attribute is not present in standard DICOM IOD - (0x0020,0x0052) UI Frame of Reference UID
Warning - Attribute is not present in standard DICOM IOD - (0x0020,0x1040) LO Position Reference Indicator
Warning - Attribute is not present in standard DICOM IOD - (0x0020,0x1041) DS Slice Location

@malaterre
Copy link
Member

One last rant before I stop on the subject. This PR requires first a CP to the current DICOM 2023c edition:

@dclunie
Copy link

dclunie commented Jul 27, 2023

Since @malaterre has decided not to be helpful, we will need to work around his recalcitrance in ITK as he suggests.

@malaterre
Copy link
Member

Since @malaterre has decided not to be helpful, we will need to work around his recalcitrance in ITK as he suggests.

Yeah right. 20 years in maintaining GDCM summarized because I disagree with you.

@dclunie
Copy link

dclunie commented Jul 27, 2023

Your long history of valuable contributions is not undermined by one deviation.

thewtex added a commit to thewtex/ITK that referenced this issue Mar 15, 2024
Add patches to GDCM for reading pixel spacing, image orientation patient,
image position patient, from DICOM secondary capture datasets in:

  malaterre/GDCM#167

which are based on GDCM `release`.

xref:

- malaterre/GDCM#158
- InsightSoftwareConsortium#4109
- Slicer/Slicer#7089
- InsightSoftwareConsortium#4111
thewtex added a commit to thewtex/ITK that referenced this issue Mar 19, 2024
This is using a Visible Male RGB slice secondary capture DICOM created by David Clunie as an input,
described in Issue InsightSoftwareConsortium#4109.
thewtex added a commit to thewtex/ITK that referenced this issue Mar 25, 2024
This is using a Visible Male RGB slice secondary capture DICOM created by David Clunie as an input,
described in Issue InsightSoftwareConsortium#4109.
@thewtex
Copy link
Member

thewtex commented Apr 22, 2024

Closed via #4521

@thewtex thewtex closed this as completed Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances
Projects
None yet
5 participants