Fail to find the image element in a DICOM file with private Sequence/SIEMENS tags

[Zaid-Safadi]

I have a DICOM file that is generated by SIEMENS with many private elements that the parser fails to read/find the image element.

The file is tested on multiple other desktop parsers (DICOM Editors, MicroDicom, fo-dicom...) and it is being read successfully and the image renders without issues.

[Zaid-Safadi]

Tracked the issue and it is caused by the parser finding a private sequence element (in an implicit VR DS) and trying to read and parse the elements inside it.

However the sequence is not following the DICOM standards (it is private after all) causing the parser to calculate the wrong length and skipping over the rest of the file.

The solution would be for the DICOM parser to ignore trying to parse the private implicit VR sequence (which is the behavior of all the other libraries) and skip it. The reason the library thinks it should read it because it has a sequence detection logic:
https://github.com/cornerstonejs/dicomParser/blob/master/src/readDicomElementImplicit.js#L15-L25

const isSequence = (element, byteStream, vrCallback) => {
  // if a data dictionary callback was provided, use that to verify that the element is a sequence.
  if (typeof vrCallback !== 'undefined') {
    return (vrCallback(element.tag) === 'SQ');
  }

  if ((byteStream.position + 4) <= byteStream.byteArray.length) {
    const nextTag = readTag(byteStream);

    byteStream.seek(-4);

    // Item start tag (fffe,e000) or sequence delimiter (i.e. end of sequence) tag (0fffe,e0dd)
    // These are the tags that could potentially be found directly after a sequence start tag (the delimiter
    // is found in the case of an empty sequence). This is not 100% safe because a non-sequence item
    // could have data that has these bytes, but this is how to do it without a data dictionary.
    return (nextTag === 'xfffee000') || (nextTag === 'xfffee0dd');
  }

[yagni]

The problem with this PR is that the DICOM is nonstandard, not that we should never read private tags. The DICOM standard even says that private tags should follow the normal DICOM element format. If you need this, I'd recommend forking this repo and adding it in your fork.

[Zaid-Safadi]

thanks for the feedback @yagni
I would agree with you except that for this case, we are talking about an implicit VR for a private element that technically the toolkit should have no idea how to read it other than as binary.

It is only because the code implement a hack to detect sequences -since it doesn't depend on/use a data dictionary- that it was able to figure out that this is a sequence element. The code even has this comment in the method to address similar cases:

     //This is not 100% safe because a non-sequence item
    // could have data that has these bytes, but this is how to do it without a data dictionary.

Finally, all the viewer/toolkits I tested with produce the same behavior. It reads the implicit VR sequence element as binary (because it doesn't even know it is a sequence).

So back to your point, yes private elements should follow the DICOM standard, for this case, as far as the toolkit is concerned, this is an element with binary data.

[yagni]

I'm assuming that the private sequence element has an explicit length; otherwise the other parsers wouldn't be able to properly skip over it. If that's true, you should be able get the same effect without a PR by passing a vrCallback that returns 'OB' when passed your element's tag.

[Zaid-Safadi]

@yagni do you have a use case than needs to read implicit VR private sequence where this change would break things for you?

The behavior in this PR aligns with the DICOM standard interpretation and with all the other viewers/toolkits so it should be the "general" solution and a fix for a "bug".

The callback option might be used for someone who wants to identify and read the private tag not the other way around.

Finally, if for any reason we need to preserve the old functionality then it could be done through adding a configuration option when initializing the parser. However, I am not sure this is needed at this point.

[yagni]

This actually wouldn't affect my use of dicomParser at all; I'm just concerned about nonstandard behavior creeping into this library. dicomParser has historically been used to parse DICOM standard-compliant files. We all know there are plenty of deviations from the standard in the wild, but the creators of this library have pushed users to handle those things within their own application code, not as PRs here. For example, in my own work I have a layer where I preprocess some nonconformities before passing the data to dicomParser.

My main problem is that I don't see anything in the DICOM standard about treating private elements as binary; in fact, everything I see mentions private data elements should be as readable as standard data elements. Admittedly, the standard is so big that I could have easily missed it--can you point me to the section you're referring to?

Either way, I don't have the ability approve or reject PRs so a maintainer is going to have to weigh in on which way they'd like to go.

[Zaid-Safadi]

@yagni with all due respect, I would encourage you to research this yourself before having a strong opinion about it. You seem to be confused as in this case, the private element creator can encode their element in anyway they like if the VR is unknown. Just because the DICOM parser thinks this is a sequence it doesn't mean it is.

[yagni]

Apologies, I stand corrected. I see it's mentioned in section 6.2.2 of Part 5 of the standard, where it talks about the UN VR. I didn't realize that private tags were assumed to be UN when implicitly encoded, but that makes sense as there's no way to know an implicit element's VR without a dictionary (i.e. vrCallback here).

That being said, it may be better to skip the sequence peek within isSequence if the element is private rather than the entire isSequence function. That way, if the user wants to parse their private elements they can provide a vrCallback that returns the VRs for the ones they want to read.

[chafey]

I believe the standards compliant behavior for private elements with known lengths is to skip over the value and not parse it. The current library does not do this - it attempts to parse it which is causing this issue. I am OK with making this change as it is inline with the design intent for the library to be strictly standards compliant. It is a change in behavior that some people may depend up thought so it will be need to be done as part of a major version release.

[xtfer]

@Zaid-Safadi Did your commit on the 29th resolve this issue? I'm trying to trace a related issue in the OHIF viewer.