BUG: DCMTK reader wrongly rejects file with preamble

[thewtex]

A previous optimization didn't take into account the preamble of a dicom file. This adds a check.

Fixes #4108

[thewtex]

Great suggestions, @N-Dekker , added.

[N-Dekker]

Fixes #4801

Are you sure it's issue 4801? I cannot find it 🤷

[jhlegarreta]

Can a minimal test be added if an appropriate dataset can be found, please?

[thewtex]

@pieper do you have a test file?

[issakomi]

Can a minimal test be added if an appropriate dataset can be found, please?

Almost every DICOM file has the "preamble". There are old ACR-NEMA files without preamble / prefix / header (0x0002 group).
Strictly speaking, the preamble consists of 128 (zero) bytes at the beginning (reserved), and the subsequent 4 bytes "DICM" are called the prefix.
https://dicom.nema.org/medical/dicom/current/output/chtml/part10/chapter_7.html

Here is one without preamble and prefix, starts with the header (0x0002 group), specially for testing purposes. Most DICOM viewers can open it. It is also 'dciodvfy' clean.
https://data.kitware.com/#item/65efb5f18353a45974ec399b

Edit:
Here is one tiny and 'dciodvfy' clean file with correct preamble/prefix, just for simplicity, if a dedicated test is required. But again, every modern DICOM file should have this
https://data.kitware.com/#item/65efb9518353a45974ec399e

[issakomi]

Fixes #4801

Are you sure it's issue 4801? I cannot find it 🤷

#4108

[thewtex]

@issakomi awesome, thank you! I added tests accordingly for both GDCM, DCMTK.

[issakomi]

Please give me some time for more test. I am not sure that we have tested DCMTKIO CanReadFile. It seems to reject no_preamble.dcm. The name of the PR is a little bit not precise. The DCMTKIO reader doesn't reject files with preamble. DCMTKIO CanReadFile returns false for usual and valid files with preamble, but it is usually not invoked at all, only if a user calls it explicitly.

[issakomi]

Here is test with CanReadFile and readers for both DCMTKIO and GDCMIO.

---

r@deb2:~/tmp/test11/build$ ./test ../data/preamble.dcm 

DCMTKIO CanReadFile(../data/preamble.dcm): false

GDCMIO CanReadFile(../data/preamble.dcm): true

Failure for DCMTK CanReadFile with preamble will be fixed by this PR

---

r@deb2:~/tmp/test11/build$ ./test ../data/no_preamble.dcm 

DCMTKIO CanReadFile(../data/no_preamble.dcm): false

GDCMIO CanReadFile(../data/no_preamble.dcm): false

Not sure that it is important, no_preamble.dcm is synthetic, but both readers work with it.

---

r@deb2:~/tmp/test11/build$ ./test ../data/SIEMENS_GBS_III-16-ACR_NEMA_1.acr 
No DICOM magic number found, but the file appears to be DICOM without a preamble.
Proceeding without caution.
DCMTKIO CanReadFile(../data/SIEMENS_GBS_III-16-ACR_NEMA_1.acr): false
DCMTKIO reader failed
ITK ERROR: DCMTKImageIO(0x564451eb2bf0): Missing Image Data in ../data/SIEMENS_GBS_III-16-ACR_NEMA_1.acr
No DICOM magic number found, but the file appears to be DICOM without a preamble.
Proceeding without caution.
GDCMIO CanReadFile(../data/SIEMENS_GBS_III-16-ACR_NEMA_1.acr): true

SIEMENS_GBS_III-16-ACR_NEMA_1.acr is real ACR-NEMA file, DCMTKIO reader fails (and CanReadFile too).

---

#include "itkImage.h"
#include "itkDCMTKImageIO.h"
#include "itkGDCMImageIO.h"
#include "itkImageFileReader.h"
#include "itkImageFileWriter.h"
#include <iostream>

int main(int argc, char ** argv)
{
  if (argc < 2)
  {
    std::cout << "Filename is required" << std::endl;
  }
  {
    using ImageType = itk::Image<short, 2>;
    using ImageIOType = itk::DCMTKImageIO;
    auto io = ImageIOType::New();

    ////////////////////////////////////////////////////////////////////////
    //
    // Bug https://github.com/InsightSoftwareConsortium/ITK/issues/4108
    //
    try
    {
      const bool r = io->CanReadFile(argv[1]);
      std::cout << "\nDCMTKIO CanReadFile(" << argv[1] << "): " << (r ? "true" : "false") << std::endl;
    }
    catch (const itk::ExceptionObject & e)
    {
      std::cout << e.GetDescription() << std::endl;
    }
    //
    //
    ////////////////////////////////////////////////////////////////////////

    itk::ImageFileReader<ImageType>::Pointer reader = itk::ImageFileReader<ImageType>::New();
    reader->SetImageIO(io);
    reader->SetFileName(argv[1]);
    try
    {
      reader->Update();
    }
    catch (const itk::ExceptionObject & e)
    {
      std::cout << "DCMTKIO reader failed\n" << e.GetDescription() << std::endl;
    }
  }
  //
  {
    using ImageType = itk::Image<short, 2>;
    using ImageIOType = itk::GDCMImageIO;
    auto io = ImageIOType::New();

    ////////////////////////////////////////////////////////////////////////
    //
    //
    try
    {
      const bool r = io->CanReadFile(argv[1]);
      std::cout << "\nGDCMIO CanReadFile(" << argv[1] << "): " << (r ? "true" : "false") << std::endl;
    }
    catch (const itk::ExceptionObject & e)
    {
      std::cout << e.GetDescription() << std::endl;
    }
    //
    //
    ////////////////////////////////////////////////////////////////////////

    itk::ImageFileReader<ImageType>::Pointer reader = itk::ImageFileReader<ImageType>::New();
    reader->SetImageIO(io);
    reader->SetFileName(argv[1]);
    try
    {
      reader->Update();
    }
    catch (const itk::ExceptionObject & e)
    {
      std::cout << "GDCMIO reader failed\n" << e.GetDescription() << std::endl;
    }
  }
  return 0;
}

Edit: updated image type to short, but it doesn't change anything, just for correctness (ACR-NEMA test file is 16 bits allocated)

[issakomi]

Approved for clarity. Despite of issues reported above, the PR is a step in the right direction.

[thewtex]

@issakomi thank you for helping us progress forward in the right direction on this!!

I added a few more tests that improve our coverage based on your remarks.

[thewtex]

/azp run ITK.macOS