Improve support for non-compliant DICOM and edge cases

[lnogueir]

I was playing around with the pydicom test files and noticed that parsing (with SkipProcessingPixelDataValue() option enabled) the following files result in errors:

• 1. ExplVR_BigEndNoMeta.dcm (addressed in Parse: If transfer syntax is missing, attempt to infer it by peeking next 100 bytes. #330)
• 2. ExplVR_LitEndNoMeta.dcm (addressed in Parse: If transfer syntax is missing, attempt to infer it by peeking next 100 bytes. #330)
• 3. MR_truncated.dcm
• 4. UN_sequence.dcm
• 5. meta_missing_tsyntax.dcm
• 6. nested_priv_SQ.dcm (addressed in Read elements with VR=UN and undefined VL as SQ #331)
• 7. no_meta.dcm
• 8. no_meta_group_length.dcm (addressable by a ParseOption already)

These files were parseable by dcm4che.

For most of them, the issue is related to missing some or all meta tags.
Ideally, if the meta tags aren't included, we should ignore and try parsing the first data element. And we can guess the transfer syntax by peeking the first tag and VR of the first element. We try [implicit LE, explicit BE, explicit LE], whichever one decodes a known tag and VR is likely the correct transfer syntax and we should use that for the rest of the dataset.

[suyashkumar]

Thanks @lnogueir! I agree that we can attempt some enhanced fallback behavior, particularly if that's standard practice in other parsers. Our current fallback behavior for no transfer syntax found in the Metadata is to proceed with little endian implicit but it shouldn't be too difficult to try a couple different transfer syntaxes speculatively.

[suyashkumar]

For missing metadata group length we do hav a flag for that, but can consider automatic fallback behavior:

dicom/read.go

Line 184 in 65259e5

if !r.opts.allowMissingMetaElementGroupLength {

I think originally the idea was the safest option is to force callers to be as explicit as possible about what loosened restrictions they may want in case there are any safety issues with fallback behavior or in case the fact the dicom is not compliant is something the user would want raised to them (in case that led to other concerns). That being said, recently I've been thinking it's more reasonable to just have more automatic fallbacks, at least ones that are "standard" in the industry for dicom non-compliance.

[suyashkumar]

Going through these in a little more detail, and adding some more notes:

1. Easy fix (have working locally)
2. Easy fix (have working locally)
3. This appears to parse fine, but iiuc the problem is the PixelData is intentionally truncated? So the vl says 8192 but there aren't that many bytes left in the dicom. We treat this as an error, which seems reasonable imo but we can discuss more.
4. Bug/implementation needed with UN unknown sequences
5. Needs more investigation, seems related to the UN sequences / tags possibly similar to 4.
6. Also seems possible related to UN sequences / tag handling
7. Same as 4/5/6 at first glance?
8. Should parse fine with allowMissingMetaElementGroupLength but need to check

[suyashkumar]

The draft changes in #330 address 1 and 2.

[lnogueir]

For missing metadata group length we do hav a flag for that, but can consider automatic fallback behavior:

dicom/read.go

Line 184 in 65259e5

if !r.opts.allowMissingMetaElementGroupLength {

I think originally the idea was the safest option is to force callers to be as explicit as possible about what loosened restrictions they may want in case there are any safety issues with fallback behavior or in case the fact the dicom is not compliant is something the user would want raised to them (in case that led to other concerns). That being said, recently I've been thinking it's more reasonable to just have more automatic fallbacks, at least ones that are "standard" in the industry for dicom non-compliance.

I agree. I think it's better to have these automatic fallbacks and just log warnings if non-compliant things are found. That's the behavior most libraries choose I believe.

[suyashkumar]

Also, the changes in #331 address 6 (with SkipPixelData). They almost address 4 but some other changes are still needed for that.

[suyashkumar]

Edited the original comment to update current status, link to PRs, etc.

[lnogueir]

I noticed that the following files (also from pydicom) do not error, but drop values:

1. rtdose_rle.dcm
2. rtdose_rle_1frame.dcm (seems to be the same issue as the file above)

The issue seems to be that they change the transfer syntax when writing the SQ dataset. The top level dataset is encoded with explicit little endian, but the dataset of the Referenced RT Plan Sequence is encoded with implicit little endian.

These were the pydicom issues that handled that: pydicom/pydicom#1035, pydicom/pydicom#1067