I fixed some cppcheck and clang static analyzer warnings, but others need someone that knows the code:

The enclosing 'for' loop goes to m_NDims (the ivar), should be _nDims (the param)?

src/metaImage.cxx:584:18: Assigned value is garbage or undefined
src/metaImage.cxx:590:25: Assigned value is garbage or undefined

These may be dead code, or something is failing to use those results...

src/metaImage.cxx:1205:5: Value stored to 'posend' is never read
src/metaImage.cxx:2908:9: Value stored to 'fileImageDim' is never read
src/metaImage.cxx:3044:9: Value stored to 'maxV' is never read

redundantAssignment,src/metaImage.cxx:3051,style,Variable 'maxV' is reassigned a value before the old one has been used.
unreadVariable,src/metaImage.cxx:1205,style,Variable 'posend' is assigned a value that is never used.
unreadVariable,src/metaImage.cxx:1290,style,Variable 'elementDataFileName' is assigned a value that is never used.

I encountered this bug while converting a microscopy image to an mha file.

The SimpleITK code to reproduce the problem is:

fname_test = "test.mha"
img_test = sitk.Image(10,11,12, sitk.sitkUInt8)
img_test.SetMetaData("Color", "000 1.000")
sitk.WriteImage(img_test,fname_test)
try:
    sitk.ReadImage(fname_test)
except Exception as e:
     print(e)

The error from SimpleITK/ITK is not useful either:

Exception thrown in SimpleITK ReadImage: /scratch/blowekamp/build/SimpleITK/ITK/Modules/IO/Meta/src/itkMetaImageIO.cxx:94:
itk::ERROR: MetaImageIO(0x244ec70): File cannot be read: test.mha for reading.

I also encountered the issue with having a "Origin" metadata dictionary entry too.

When building and installing this MetaIO library stand-alone, with CMAKE_INSTALL_PREFIX=/opt/metaio on macOS, the installed files are:

/opt/metaio
├── include
│   ├── itkMetaIO
│   │   ├── itkThirdParty.h
│   │   ├── itk_zlib.h
│   │   └── metaIOConfig.h
│   ├── localMetaConfiguration.h
│   ├── metaArray.h
│   ├── metaArrow.h
│   ├── metaBlob.h
│   ├── metaCommand.h
│   ├── metaContour.h
│   ├── metaDTITube.h
│   ├── metaEllipse.h
│   ├── metaEvent.h
│   ├── metaFEMObject.h
│   ├── metaForm.h
│   ├── metaGaussian.h
│   ├── metaGroup.h
│   ├── metaIOConfig.h
│   ├── metaITKUtils.h
│   ├── metaImage.h
│   ├── metaImageTypes.h
│   ├── metaImageUtils.h
│   ├── metaLandmark.h
│   ├── metaLine.h
│   ├── metaMesh.h
│   ├── metaObject.h
│   ├── metaOutput.h
│   ├── metaScene.h
│   ├── metaSurface.h
│   ├── metaTransform.h
│   ├── metaTube.h
│   ├── metaTubeGraph.h
│   ├── metaTypes.h
│   ├── metaUtils.h
│   └── metaVesselTube.h
└── lib
    └── libITKMetaIO.a

The localMetaConfiguration.h contains the statement #include "itk_zlib.h", but this header file is in subdirectory itkMetaIO, requiring the addition of this include path to the list of include search paths. It seems to me that the install paths of the header files are inconsistent.

For comparison, when build and installed as part of ITK with CMAKE_INSTALL_PREFIX=/opt/itk, all header files of MetaIO end up in /opt/itk/include/ITK-$major.$minor/. Given this, I would expect that all header files of the stand-alone MetaIO library would end up in /opt/metaio/include/itkMetaIO instead of the top-level include directory.

Alternatively, the itkMetaIO subdirectory could be dropped from the stand-alone installation, given the itk file name prefixes and redundant itkMetaIO/metaIOConfig.h?

1.2.3 was released in 2005 : http://madler.net/pipermail/zlib-announce_madler.net/2005/000004.html

Is some special procedure required to update it, or do we just change the files?

src/metaImage.cxx line 2837

Code:
return false;
return false; // <-- identical
}

As originally posted about in https://discourse.slicer.org/t/failures-saving-a-volume-to-path-with-sign/25657 and more specifically in
https://discourse.slicer.org/t/failures-saving-a-volume-to-path-with-sign/25657/6 thanks to @lassoan:

MetaIO supports One-Slice-Per-File Data Formats 1 and it uses the % character to specify the slice number in the filename format string.

Due to the special meaning of the % character, currently MetaIO does not allow using this chracter in the path. This limitation should either be clearly documented or the implementation should be made more sophisticated (to somehow differentiate the % that refers to per-slice filename generation from simple % occurrences in the path; or by allowing completely disabling this extremely rarely used one-slice-per-file feature).

As such, % used in the basename could possibly still be treated with the special meaning for the filename format string for One-Slice-Per-File functionality however, % used elsewhere in the filepath should be supported. Last resort would be to document this limitation.

The below code uses 3D Slicer to show that the mhd file is written for a volume output filepath that contains a "%", but the corresponding data file (.zraw) is not written.

import os
import SampleData

volume_node = SampleData.SampleDataLogic().downloadMRHead()

output_filepath = os.path.join(os.getenv("USERPROFILE"), "Downloads", "2%concentration", "MyVolume.mhd")
slicer.util.saveNode(volume_node, output_filepath)
print(f"{output_filepath} exists?: {os.path.exists(output_filepath)}")
raw_filepath = os.path.splitext(output_filepath)[0] + '.zraw'  # default Slicer saves it compressed hence zraw and not raw
print(f"{raw_filepath} exists?: {os.path.exists(raw_filepath)}")

There are a large number of memory leaks detected on the ITK dashboard related to MetaIO. Here is a sample:

{
   
   Memcheck:Leak
   match-leak-kinds: definite
   fun:_Znwm
   fun:_ZN9MetaImageC1Ev
   fun:_ZN3itk11MetaImageIOC1Ev
   fun:_ZN3itk11MetaImageIO3NewEv
   fun:_ZN3itk20CreateObjectFunctionINS_11MetaImageIOEE12CreateObjectEv
   fun:_ZN3itk17ObjectFactoryBase15CreateAllObjectEPKc
   fun:_ZN3itk17ObjectFactoryBase17CreateAllInstanceEPKc
   fun:_ZN3itk14ImageIOFactory13CreateImageIOEPKcNS_11CommonEnums10IOFileModeE
   fun:_ZN3itk15ImageFileReaderINS_5ImageIhLj2EEENS_25DefaultConvertPixelTraitsIhEEE25GenerateOutputInformationEv
   fun:_ZN3itk13ProcessObject23UpdateOutputInformationEv
   fun:_ZN3itk9ImageBaseILj2EE23UpdateOutputInformationEv
   fun:_ZN3itk10DataObject6UpdateEv
   fun:_ZN3itk13ProcessObject6UpdateEv
   fun:_Z35itkIsolatedWatershedImageFilterTestiPPc
   fun:main
}
{
   
   Memcheck:Leak
   match-leak-kinds: definite
   fun:_Znwm
   fun:_ZN9MetaImageC1Ev
   fun:_ZN3itk11MetaImageIOC1Ev
   fun:_ZN3itk11MetaImageIO3NewEv
   fun:_ZN3itk20CreateObjectFunctionINS_11MetaImageIOEE12CreateObjectEv
   fun:_ZN3itk17ObjectFactoryBase15CreateAllObjectEPKc
   fun:_ZN3itk17ObjectFactoryBase17CreateAllInstanceEPKc
   fun:_ZN3itk14ImageIOFactory13CreateImageIOEPKcNS_11CommonEnums10IOFileModeE
   fun:_ZN3itk15ImageFileWriterINS_5ImageIhLj2EEEE5WriteEv
   fun:_ZN3itk15ImageFileWriterINS_5ImageIhLj2EEEE6UpdateEv
   fun:_Z35itkIsolatedWatershedImageFilterTestiPPc
   fun:main
}
{
   
   Memcheck:Leak
   match-leak-kinds: definite
   fun:_Znwm
   fun:_ZN9MetaImageC1Ev
   fun:_ZN3itk11MetaImageIOC1Ev
   fun:_ZN3itk11MetaImageIO3NewEv
   fun:_ZN3itk20CreateObjectFunctionINS_11MetaImageIOEE12CreateObjectEv
   fun:_ZN3itk17ObjectFactoryBase15CreateAllObjectEPKc
   fun:_ZN3itk17ObjectFactoryBase17CreateAllInstanceEPKc
   fun:_ZN3itk14ImageIOFactory13CreateImageIOEPKcNS_11CommonEnums10IOFileModeE
   fun:_Z13HashTestImagePKcRKSt6vectorISsSaISsEE
   fun:main
}
{
   
   Memcheck:Leak
   match-leak-kinds: definite
   fun:_Znwm
   fun:_ZN9MetaImageC1Ev
   fun:_ZN3itk11MetaImageIOC1Ev
   fun:_ZN3itk11MetaImageIO3NewEv
   fun:_ZN3itk20CreateObjectFunctionINS_11MetaImageIOEE12CreateObjectEv
   fun:_ZN3itk17ObjectFactoryBase15CreateAllObjectEPKc
   fun:_ZN3itk17ObjectFactoryBase17CreateAllInstanceEPKc
   fun:_ZN3itk14ImageIOFactory13CreateImageIOEPKcNS_11CommonEnums10IOFileModeE
   fun:_ZN3itk15ImageFileReaderINS_11VectorImageIhLj6EEENS_25DefaultConvertPixelTraitsIhEEE25GenerateOutputInformationEv
   fun:_ZN3itk13ProcessObject23UpdateOutputInformationEv
   fun:_ZN3itk13ProcessObject27UpdateLargestPossibleRegionEv
   fun:_Z11ComputeHashIN3itk11VectorImageIhLj6EEEESsPKc
   fun:_Z13HashTestImagePKcRKSt6vectorISsSaISsEE
   fun:main
}

The documentation seems somewhat sparing (see my comments here). One thing I most want to know is whether the ObjectType field is guaranteed to come first: if so, then I can use it for magic bytes in Julia's general "figure out what type of file this is" library.

When I tried to change dicom into mhd and raw, I used
SimpleITK.WriteImage(sitk_img, os.path.join(SaveRawDicom, name + ".mhd")),
but an error occured. That is "MetaImage: M_WriteElementsData: file stream is fail after write".
How can I solve this problem?
Thank you so much!

A change (336c99d) in the cast that gets done to the CompressedDataSize field (see https://github.com/Kitware/MetaIO/blob/master/src/metaObject.cxx#L1490, probably done to address signedness mismatch) leads to problems loading files >2GB.
There can be a bad_alloc exception thrown when trying to decompress (raw) files larger 2GB (2^31) due to m_CompressedDataSize having a negative value.

Now that VTK & ITK do, can MetaIO require C++11 at this point?

Especially small things like use of 'override' and 'nullptr'.

Those 2 could be done automatically with clang-tidy (I volunteer).

It would allow me to enable additional warnings on my VTK dashboards...

Some time ago the METAIO_STREAM macro was removed from MetaIO and the new code, which uses std::ifstream/ofstream , breaks unicode compliance in VTK.
See https://gitlab.kitware.com/vtk/vtk/-/merge_requests/6716#note_908828

VTK uses vtksys::ifstream/ofstream to support utf8 path and filenames across all platforms, so please reinstate the macro to allow VTK to function correctly with non-English characters on Windows.

clang trunk in r312750 learned to warn about tautological comparison of an unsigned value with signed zero:

http://llvmweekly.org/issue/193

https://reviews.llvm.org/rL312750

and now on one of my VTK dashboards there is 1 and only 1 such warning:

/Users/builder/external/VTK/Utilities/MetaIO/vtkmetaio/metaUtils.cxx:241:12: warning: comparison of unsigned enum expression >= 0 is always true [-Wtautological-unsigned-zero-compare]
  if(_vType>=0 && _vType<=MET_NUM_VALUE_TYPES)
     ~~~~~~^ ~
1 warning generated.

Instead of a pull request, I thought I'd open an issue, because I'm not sure how you'd like to fix this. The entire check shouldn't be necessary because an enum can't hold a value outside its range. Shall we just remove it? Also, maybe I need more coffee, but shouldn't that <= be < ?

MetaScene

MetaIO was created not only to store images and isolated objects (meshes, masks, arrows, etc), but to also arrange and physically couple them into a scene.

In a MetaScene, a metaObject can be assigned to be a child of another metaObject, and each object has an object-to-parent transform that also applies to all its children - forming a transform tree that allows subtrees to be rotated/translated, etc in space.

Object-to-Parent Transforms...except metaImages...

For every metaObject, the object-to-parent transform is stored in the TransformMatrix, Offset, and CenterOfRotation variables. However, for metaImages, there is also the transform that maps i,j,k space of voxels to physical space. Originally MetaIO stored that i,j,k to physical space transform in the TransformMatrix, offset, and CenterOfRotation variables - and the object-to-parent transform wasn't supported, and so subtrees that had an image as their subroot couldn't be rotated, translated, etc and intervening GroupSpatial objects had to be inserted into the trees to provide the functionality.

Proposal

I am proposing to introduce ElementDirection and ElementOrigin to the metaImage class definition (note that metaObject class already has an ElementSpacing variable which is unused in all but the Image class). ElementDirection, ElementOrigin, and ElementSpacing will mimic the definitions of Direction, Origin, and Spacing of an itkImage. They will store the transforms from i,j,k to physical space, so that TransformMatrix, Offset, and CenterOfRotation can be used (as intended) for the object-to-parent transform.

Backward compatibility

Backward compatibility is maintained as follows:

1. If ElementDirection, ElementOrigin, and/or Spacing are not specified (as currently/originally done in MetaIO files), then the TransformMatrix, Offset, CenterOfRotation, and ElementSpacing variables are treated as originally/currently defined: as i,j,k to physical space transform specifications. This applies for these variables individually: so if ElementDirection alone isn't provided, then TransformMatrix is used instead.

2. If one or more of ElementDirection and/or ElementOrigin is provided, then the new definition is used for those specific variables. For example, if ElementDirection is specified, then it is used for the i,j,k transform and TransformMatrix is used for the object-to-parent transform when the image is part of a metaScene.

The good and the bad

• All existing metaScene and metaImage (.mhd/.raw and .mha) files will still work as they do now when read by new code after these changes.
• When an stand-alone metaImage file is written using the new code, we propose to default to using the old (current) format so that old programs can read them correctly.
• Only when metaScenes are written and they include a metaImage, then that embedded metaImage will use the new format and the metaScene will support object-to-parent transforms for all metaObjects.
• Broken Backward Compatibility will happen when an old program reads a new metaScene file - the old/current MetaIO code will report (provide message, not an error) that ElementDirection, ElementOrigin, and Spacing are defined in the file, but they are undefined in the program.
  • very few programs currently read/write metaScenes, so this isolate break in backward compatibility will have limited impact.
  • These changes will produce a more consistent definition and behavior for metaScenes, so the break is worth it.
  • We will also introduce a versioning scheme for metaObjects, so that file format updates can be handled / flagged correctly in the future.

Thoughts?
@thewtex
@hjmjohnson
@zivy
@N-Dekker

Is ITK ready if I make a PR which adds override to the headers or does it need to be conditionalized?

Cc: @thewtex

Looking at the .mhd documentation, I see no explicit mention of any assumed encoding (or a setting for it).

Yet when ElementDataFile points to a filename containing special characters, the character encoding is highly relevant for MetaIO to find the actual file. In my experiments (utilizing the MetaIO included in ITK), on Windows, an encoding of cp1252 is assumed, while on Linux, an encoding of utf-8 is expected. This means that when the filename given under ElementDataFile contains special characters, a separate .mhd file is required for Linux and Windows (and potentially more for other platforms I have not tested). What is thus required to make this consistent (and .mhd files with special characters transferrable between platforms), in my opinion, is to implement one of two options:

1. That .mhd files are required to have a specific encoding (utf-8 seems to be the logic choice), or
2. To have a separate entry specifying the encoding of the .mhd file

Or am I missing something here, is there an encoding specification somewhere already?

Initial work in dzenanz@60f7c69.

This issue is first reported here: SimpleITK/SimpleITK#2082

It appears that MetaIO does not sanitize the input to write from UNICODE, and can write some UNICODE strings. However, MetaIO is not able to read the same UNICODE string it has written.

Example output from above:

Expected equality of these values:
  img2.GetMetaData("test2")
    Which is: "des caract\xC3\xA8res sp\xC3\xA9" "ci"
    As Text: "des caractères spéci"
  value2
    Which is: "des caract\xC3\xA8res sp\xC3\xA9" "ci\xC3\xB6"
    As Text: "des caractères spéciö"

The Sample header file:

ObjectType = Image
NDims = 3
BinaryData = True
BinaryDataByteOrderMSB = False
CompressedData = False
TransformMatrix = 1 0 0 0 1 0 0 0 1
Offset = 0 0 0
CenterOfRotation = 0 0 0
AnatomicalOrientation = RAI
ElementSpacing = 1 1 1
test1 = des caractères spéciaux
test2 = des caractères spéciö
DimSize = 2 2 2
ElementType = MET_DOUBLE
ElementDataFile = LOCAL