BUG: Fix loading of DICOM files with no preamble

[cpinter]

The fix consists of two parts:

• In DCMTK and GDCM ImageIO there are two duplicate functions called readNoPreambleDicom. Its implementation was faulty, as it did not consider the two bytes that were already read but rejected as it was not a VR (value representation). These two bytes are now reused for the length, which is now correctly read, which confirms that the order of information in the DICOM header is correct. This function was fixed both places
• In DCMTK file reader CanReadFile function the loadFile call confirms that the file is valid despite not having a preamble, in which case it returns an EC_FileMetaInfoHeaderMissing value instead of an EC_Normal. This value needs to be accepted as well

This change can be tested on the datasets mentioned in this SlicerRT issue: SlicerRt/SlicerRT#49
The datasets in question are: aw-4.4-foot (CT, scout), corvus-6.2.2-phantom (CT, dose, RT images), xio-4.33.02-phantom-chest, xio-4.60.00-phantom-chest/t50, (CT, dose), xio-4.60.00-* (CT, dose, RT image), xio-4.64-phantom-prostate-beams-* (CT, dose)
Which can be found here: https://pocus.cs.queensu.ca/#collection/5cc8811601d4930406c40465 or here https://github.com/SlicerRt/SlicerRtData/tree/master

PR Checklist

• 🚫 Makes breaking changes
• 🚫 Makes design changes
• 🚫 Adds the License notice to new files.
• 🚫 Adds Python wrapping.
• 🚫 Adds tests and baseline comparison (quantitative).
• 🚫 Adds test data.
• 🚫 Adds Examples to ITKExamples
• 🚫 Adds Documentation.

Refer to the ITK Software Guide for
further development details if necessary.

[cpinter]

@hjmjohnson Please hang on, maybe I overrode previous changes. Sorry!

[cpinter]

I thought I checked all files, but apparently not. Fixed now!

[hjmjohnson]

@cpinter Thank you. Is there any chance a test could be added? Is one of these data sets suitable for creating a test of this code? That would ensure that future changes would also be able to read those files.

[dzenanz]

Please keep in mind support for both big endian and little endian.

As Hans said, you seem to have the data, so it would be nice if you added the smallest such image as a read test. That is not a blocker, though.

[hjmjohnson]

@cpinter @pieper @lassoan THANK YOU for investigatting this issue. I think it is 90% there. The two outstanding issues that need to be addressed to get this into ITK robustly for the future:

• Reference test to validate correct results into the future (not strictly required, but it sure would be nice so that we don't revisit this issue in the future).
• address concerns about big/little endedness

[cpinter]

Thank you! I intend to take care of these tomorrow.

[cpinter]

1. I tried loading both liettle and big endian files, and they were loaded fine.
2. I'm adding very simple tests that load an 8KB test file with DCMTK and GDCM. What is the proper way to add data files so that it appears in ExternalData?

[cpinter]

The tests work, but I still don't know how to add ExternalData. I tried to follow this
https://itk.org/Wiki/ITK/Git/Develop/Data#ExternalData
but the "CMake will move the original file" step did not happen. I cannot see anything in my Kitware Girder (yes, I added the API key). Help would be appreciated, thanks!

[dzenanz]

The two test can be merged, and have the instantiated IO depend on a command-line argument.

[dzenanz]

The link you looked at is outdated. New content is here:
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Documentation/Data.md

[dzenanz]

... and a more direct link:
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Documentation/UploadBinaryData.md

[cpinter]

The two test can be merged, and have the instantiated IO depend on a command-line argument.

I would rather not spend more time on this than absolutely necessary, as I have several projects waiting for me. The tests cover the necessary cases, so this change would be only cosmetic (and I'm not sure where the common test should go, since I wouldn't put a GDCM test in DCMTK project or vice versa). Please let me know if it's OK like this.

[dzenanz]

Uh, I didn't realize they need to be in different subprojects. Keep it as-is then.

[cpinter]

I added and tested the download of the external data input file (8.3K dcm file) for these tests. I did a clean build and the tests pass. I updated the PR, I think it should be good to go.
Thanks for the fast reviews!

[dzenanz]

Awesome!

[lassoan]

@dzenanz, it would be nice if you could add a big red warning to https://itk.org/Wiki/ITK/Git/Develop/Data#ExternalData that instructions are outdated and add a link to the current instructions.

You could even replace the entire text of the page with links to relevant sections of current documentation because the page right now is not useful but potentially misleading for developers (and old content can be always retrieved from the page's history).

[cpinter]

There is a warning on the top but I missed it so probably something bigger and redder would be useful, I agree :)

[dzenanz]

... replace the entire text of the page with links ...

Done!

[lassoan]

Perfect, thank you!

[cpinter]

Excellent, thanks a lot!