Arguably, this is one of the most important pieces of Slicer, since absolutely every user wants their DICOM series to be loaded correctly. We should really strive for the scalar volume loader to be very robust!
Arguably, DCMTK is a more widely used, more frequently updated, and stronger project as compared to GDCM. Switching to DCMTK IO is simple. A recent example demonstrates how GDCM is the culprit in failing to parse user data: ERROR with DCE MRI loading in DICOM Browser.
Please respond here if you know of a good reason to stick to GDCMImageIO for scalar volumes!
I don’t know a good reason why GDCM is used and I agree that it would be much better to use DCMTK.
There may be differences in what mechanisms are implemented in GDCM and DCMTK for addressing some inconsistencies/common problems in DICOM images (what fields are used for storing image spacing, dealing with multiframe volumes, etc.), so some regressions are quite likely, but we should be able to address them when they come up. Maybe we should create a stable release before we switch?
It’s purely a legacy thing, and I agree we should use the DCMTK code wherever possible. I had worked on replacing that GDCM code a couple times, but I realized that it includes a lot of workarounds for special cases that aren’t really documented anywhere. So as @lassoan says we are likely to see regressions on corner cases. We won’t know how many until we try.
It’s a common issue with DICOM software that debugging is done on data that cannot be shared publically. To the extent possible I’d like to see us create a comprehensive collection of examples of the data we do support so we can perform regression testing.
Also, we could expose a user option to use the GDCM code as a fallback.
@pieper do you mind putting the pointer to those special cases that are important for reading scalar volumes here for completeness?
I am talking specifically about the scalar volume reader here. The individual files have already been sorted, it has already been confirmed they belong to the same volume, and have consistent spacing by the time that reader is called. I am fine if I am proven naive, but I thought all that reader needs to do is to assemble a volume, no?
As an aside, one level up, in the ArchetypeImageSeriesReader, I see this (8 years old by Xiaodong!) code https://github.com/Slicer/Slicer/blame/master/Libs/vtkITK/vtkITKArchetypeImageSeriesReader.cxx#L390, which on the surface is intended to do something similar to what MultiVolumePlugin is doing. Is it exercised anywhere really? Is this something that later was reimplemented in DICOM to NRRD converter, but was never cleaned up from the core?
I’m not arguing with you Andrey, and given that we now have significant
experience on the topic I’m sure that as Andras suggested and I agreed we
should try it out and fix any issues we run into.
You are right that the DICOMScalarVolumePlugin does a lot of consistency
checking in an attempt to be sure the Archetype reader has the best chance
of success. The reason I still used Archetype reader is so that it can
deal with the encodings, pixel representations, lookup tables, etc.
As I also mentioned, the GDCM-based code does not clearly document which
things, so I can’t give you a direct pointers to everything. But it
doesn’t take long looking at the ITK GDCM code to find all kinds of
heuristics and workaround for various non-standard data files (I pasted a
few quick examples below).
// There isn’t a definitive way to check for DICOM files;
// In general this should be relatively safe to assume
// Stupid file: CT-MONO2-8-abdo.dcm
// The spacing is something like that: [0.2\0\0.200000]
// I would need to throw an expection that VM is not compatible
The archetype reader was useful because it allowed a saved scene to refer to existing DICOM files. However, this does not work anymore (since the change that forces the file name to be the same as node name on save).
I think we should also consider David Gobbi’s vtk-DICOM package. It is the most comprehensive open-soruce DICOM image reading/writing solution - it can read multiframe, cine, multidimensional, compressed, gantry-tilt images and it can also write several image IODs (see more info here and here). It’s small, clean, simple, supported and continuously improved by David.
The only downside I can see is that for some applications, like in dcmqi, we might end up duplicating some of the slice-to-volume conversion logic in something closer to native DCMTK without the VTK dependency.
I think to expedite things and help the user, I should add an alternative scalar volume reader to the multivolume importer plugin, label its output loadable as “DCMTK scalar volume”, and use it in the multivolume either as the primary, or as a fallback when the first-order scalar volume plugin fails.
almost all of the tests are passing when I tested on linux (from the list below, most of the failing tests failed because I killed Slicer app manually, as the test was running for very long time, often without any indication of what it was doing, some of them keep doing something or showed non-responsive app window after 10 minutes and up)
We thought about this a bit more and discussed the implementation plan further with @pieper.
The following issues related to the initially considered approach of introducing a new C++ CLI that would read a DICOM series using DCMTKImageIO have been identified:
there will be a need to re-implement the code that determines the scalar type from DICOM - this has to be done before the ITK reader is instantiated!
since in the general case we cannot assume that the directory containing DICOM files does not include files from other series, and there is a limit on the maximum length of a command line in Windows, we would need to work around this by either copying files into a temp location, or communicating the list of files via a file
Based on this, and further analysis of the archetype reder code, an alternative approach has been developed:
By using this approach, we will not change any of the behavior in the application or its reliance on GDCM, unless GDCM fails. As such, we will be able to introduce the fix addressing the user need relatively easily, and without waiting for the next release, and without confusing the user by introducing a new type of plugin for scalar volumes.
If you have any concerns or suggestions, please respond. We plan to proceed with the implementation of the proposed approach very soon.
May this could be added to ITK … in that case a list of readers could be returned. @thewtex Do you think that make sense or should Slicer team proceed as described ?
or communicating the list of files via a file
This is a common practice. Within build system, such files are called “response” files
@jcfr thanks for the response. Do you have any specific concerns with the proposed plan?
I think the issue of improving ITK is an important, but a separate one. I would prefer not to wait for the ITK improvements to be developed, tested and integrated to resolve this specific issue, unless there are really good reasons.
I like the proposed solution of implementing this by making vtkITKArchetypeImageSeriesScalarReader smarter.
I have just a small comment: it would be a bit nicer to make the DICOM reader selector a string attribute (PreferredDICOMReader, PreferredDICOMIOMethod, PreferredIOClass, …), instead of a Boolean flag (UseGDCMImageIO), as in the future we will probably want to have more options (for example, different modes for using DCMTK or GDCM, auto-select between different readers, use other toolkits) without changing the interface.
It would be useful to have a way to load using DCMTK, even when GDCM returns something. For this, the DICOMScalarVolumePlugin could always offer two loadables for a series: a GDCM-based and a DCMTK-based. By default, if GDCM can interpret the image, the GDCM-based loadable would have higher confidence (so it would be selected by default).
Yes, I think it is a idea to have the DICOM image type code available in ITK without duplication, but it should not be a blocker to move forward. Could an issue please be created here:
I agree with @lasson that we should rename the methods for selecting which toolkit to use. I also like the idea of offering the two methods as alternative loadables so the user can pick which to use in Advanced mode of the DICOM browser.
I’m also thinking it could make sense to write a test that checks out Matheiu’s repository (beefed up with any additional examples) and then runs both readers and compares the results. I wouldn’t want to make this a Nightly test, but it would make sense to be able to run it when the code changes.
I believe it captures most (all?) of the agreed upon approaches discussed here.
TL;DR People should not notice anything different for now, since GDCM will still be the default reader, but if it fails to load correctly DCMTK will be used as a backup. There’s now a settings panel for DICOM where people can choose to use DCMTK if they want to experiment.
Any deprecation schedule, to avoid the conversation in two years about “can’t remove GDCM OR DCMTK because it might break someone’s volume”?
By the way, when I looked at the original issue that prompted this, I noticed that some slices of the sample data did in fact have thickness set to 0 – so GDCM wasn’t lying. I believe it worked in DCMTK because they don’t look at that tag at all (and then Slicer recalculates the spacing from IPP anyway).
I think this is a long-term issue for the reasons I mentioned above - we don’t have a good way of knowing the impact of any particular change because we cannot test it with users’ actual data. For the self test I implemented code to load with both methods and compare the results. I was thinking to make that a selectable option so that we could start experimenting about when one fails but the other works.
Realistically what we need is a very large regression testing suite of data that somehow defines the data that we do support. This would allow changes to be made on at least some solid footing.
Unfortunately I don’t think there are any easy answers…
This is a common practice. Within build system, such files are called 