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

Fix issue #114 - Fail to find the image element in a DICOM file with private Sequence tags #115

Merged
merged 3 commits into from Feb 29, 2020

Conversation

Zaid-Safadi
Copy link
Contributor

The PR fix and prevent the behavior of the parser "trying to detect and parse private sequences as they not necessarily follow the DICOM standard" causing the parser state to be corrupt.

@coveralls
Copy link

coveralls commented Jun 9, 2019

Coverage Status

Coverage increased (+0.8%) to 52.759% when pulling cc8fdcd on issue-114 into 374ad3d on master.

yagni added a commit to yagni/dicomParser that referenced this pull request Aug 1, 2019
…rectly peeked rather than skipping over them.
@yagni
Copy link
Collaborator

yagni commented Nov 6, 2019

@Zaid-Safadi Do you have any sample DICOM or test cases we can incorporate around this PR?

yagni added a commit to yagni/dicomParser that referenced this pull request Nov 21, 2019
…rectly peeked rather than skipping over them.

(cherry picked from commit c827f86)
@Zaid-Safadi
Copy link
Contributor Author

Hey @yagni , unfortunately I can't share the sample I have. I can't get the file anonymized because of the private elements that are needed for this.

@malaterre
Copy link
Contributor

@Zaid-Safadi Here is my 2cent on the issue. Looking at your patch, here is my understanding:

  1. You want to parse the entire DICOM DataSet (including Private attributes, and in this case a Private sequence)
  2. The cornerstone library properly implement CP-246 for Sequence with VR=UN (and decode nested DataElement using Implicit Little Endian Transfer Syntax)

Now in your case you seem to have found a case where the Private Sequence is declared with VR=UN, but the encoding of the nested Data Element is Explicit Little Endian. If this is the case, this is illegal (per DICOM standard).

To convince yourself (since you cannot share your DICOM DataSet), here are two ways to check:

[Let's consider that the private sequence described above is at tag XXXX,YYYYY]

Solution 1:

  1. Use dcmdump to check it can decode the private sequence,
  2. Using dcmdump you should see the nested attributes underneath tag XXXX,YYYYY
  3. If you cannot see the attributes underneath tag XXXX,YYYYY, this mean you found a private sequence that is missing in your dicom.dic file (on my system: /usr/share/libdcmtk14/dicom.dic). If so update this file so that the tag is properly declared as Sequence.
  4. Start item (1) again, now you should see the nested attributes underneath tag XXXX,YYYYY
    Finally. If the above does not work, but dcmdump using --disable-cp246 does work for you, there you know you are dealing with an illegal DICOM encoded DataSet.

Solution 2:

  1. Make sure gdcmdump can dump attributes nested underneath tag XXXX,YYYYY, if not solution How do I use dicomParser in node? #2 wont work.
  2. Use gdcmconv to convert your original DICOM DataSet into another one. Eg:
    gdcmconv original.dcm clean.dcm from your UNIX/cmd prompt.
  3. Check the two files are identical. Eg: md5sum original.dcm clean.dcm should be enough.
  4. If they are not identical and suddenly dcmdump can now handle the private sequence, this means that the bug may be related to invalid CP-246 handling on your DataSet.

Pay attention above gdcmdump != dcmdump.

In any case you should be able to reproduce the bug with a public DICOM dataset containing Sequence, there is nothing special about Private Tag in the DICOM standard (at least for the encoding of the Data Element).

@Zaid-Safadi
Copy link
Contributor Author

@malaterre I’m afraid your understanding of the issue is not accurate.

Simply, the parser is trying to parse a private element in an implicit VR DS because it wrongly assumes it’s a sequence when it may or may not be.

According to the standard the parser should just skip over the element all together which what this fix does.

@Zaid-Safadi Zaid-Safadi merged commit c7caa59 into master Feb 29, 2020
@dannyrb
Copy link
Member

dannyrb commented Feb 29, 2020

@Zaid-Safadi! I didn't realize you had merge rights. Thanks for wrapping up this issue 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants