Update metaio release 5.4 - #6836
Conversation
Code extracted from:
https://github.com/Kitware/MetaIO.git
at commit 1755773c8d678c5c9a57999c8ec373703134c0e2 (master).
# By MetaIO Maintainers * upstream-MetaIO: MetaIO 2026-09-04 (1755773c)
github-actions
Bot
added
type:Infrastructure
|
@dzenanz There are more changes than I was hoping there were. |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # ifdef metaio_EXPORTS | ||
| # define METAIO_EXPORT __declspec(dllexport) | ||
| # define METAIO_EXTERN | ||
| # else | ||
| # define METAIO_EXPORT __declspec(dllimport) |
There was a problem hiding this comment.
In Windows shared builds, ITKMetaIO receives the target definition ITKMetaIO_EXPORTS, but this branch checks metaio_EXPORTS. The condition is therefore false while compiling the library itself, causing its public declarations to use __declspec(dllimport) instead of __declspec(dllexport). This prevents the shared library from exporting its API correctly; test ITKMetaIO_EXPORTS here or configure the target export definition consistently.
Knowledge Base Used: Module build and distribution system
Artifacts
Windows export macro reproduction
- The script preprocesses the header with the ITK shared-library target definitions and compares the current and corrected export macro expansions.
Current export macro expansion
- The current header expands the public API macro to a DLL import declaration while compiling the library.
Corrected export macro expansion
- Changing the tested definition to the ITKMetaIO target definition expands the public API macro to a DLL export declaration.
ITKMetaIO target definition inspection
- The inspected CMake configuration shows that the ITK build target is named ITKMetaIO, establishing the expected target export definition.
| if (m_NDims < 10) | ||
| { | ||
| m_ElementSpacing[i] = 1; | ||
| m_AnatomicalOrientation[i] = MET_ORIENTATION_UNKNOWN; | ||
| for (int i = 0; i < m_NDims; i++) | ||
| { | ||
| m_ElementSpacing[i] = 1; | ||
| m_TransformMatrix[i * m_NDims + i] = 1; | ||
| m_AnatomicalOrientation[i] = MET_ORIENTATION_UNKNOWN; | ||
| } | ||
| } |
There was a problem hiding this comment.
Exactly 10 dimensions are supported, but this condition skips identity initialization at that boundary. Constructing a 10-dimensional object, or reading one without a TransformMatrix, leaves its transform diagonal as zero instead of the default identity matrix. This is a non-blocking metadata correctness concern, but it produces invalid spatial metadata for supported 10D objects.
| if (m_NDims < 10) | |
| { | |
| m_ElementSpacing[i] = 1; | |
| m_AnatomicalOrientation[i] = MET_ORIENTATION_UNKNOWN; | |
| for (int i = 0; i < m_NDims; i++) | |
| { | |
| m_ElementSpacing[i] = 1; | |
| m_TransformMatrix[i * m_NDims + i] = 1; | |
| m_AnatomicalOrientation[i] = MET_ORIENTATION_UNKNOWN; | |
| } | |
| } | |
| for (int i = 0; i < m_NDims; i++) | |
| { | |
| m_ElementSpacing[i] = 1; | |
| m_TransformMatrix[i * m_NDims + i] = 1; | |
| m_AnatomicalOrientation[i] = MET_ORIENTATION_UNKNOWN; | |
| } |
Artifacts
- The C++ test constructs a 10-D MetaObject and reads a minimal 10-D header without TransformMatrix, checking every diagonal value; it directly exercises the claimed paths.
- The shell command compiles the authored test against current source and a one-condition corrected source copy, then runs both; it produces an executable before-and-after comparison.
Current-source 10D transform result
- The executed current-source test exits 1 and records zeros for all ten construction and read-path diagonal entries; it confirms the reported defect.
Corrected 10D transform result
- The executed one-condition corrected-source test exits 0 and records ones for all ten construction and read-path diagonal entries; it confirms the proposed boundary fix resolves the defect.
| if (objectType.find("Tube") != std::string::npos || ((objectType.empty()) && !strcmp(suf, "tre"))) | ||
| { | ||
| char * subtype = MET_ReadSubType(*m_ReadStream); | ||
| if (!strncmp(subtype, "Vessel", 6)) | ||
| const std::string subtype = MET_ReadSubType(*m_ReadStream); | ||
| if (subtype.find("Vessel") != std::string::npos) | ||
| { | ||
| auto * vesseltube = new MetaVesselTube(); | ||
| vesseltube->APIVersion(m_APIVersion); | ||
| vesseltube->SetEvent(m_Event); | ||
| vesseltube->ReadStream(m_NDims, m_ReadStream); | ||
| m_ObjectList.push_back(vesseltube); | ||
| } | ||
| else if (!strncmp(subtype, "DTI", 3)) | ||
| else if (subtype.find("DTI") != std::string::npos) | ||
| { | ||
| auto * dtitube = new MetaDTITube(); | ||
| dtitube->APIVersion(m_APIVersion); | ||
| dtitube->SetEvent(m_Event); | ||
| dtitube->ReadStream(m_NDims, m_ReadStream); | ||
| m_ObjectList.push_back(dtitube); | ||
| } | ||
| else | ||
| { | ||
| auto * tube = new MetaTube(); | ||
| tube->APIVersion(m_APIVersion); | ||
| tube->SetEvent(m_Event); | ||
| tube->ReadStream(m_NDims, m_ReadStream); | ||
| m_ObjectList.push_back(tube); | ||
| } | ||
| delete[] subtype; | ||
| } | ||
|
|
||
| else if (!strncmp(objectType.c_str(), "Transform", 9)) | ||
| else if (objectType.find("Transform") != std::string::npos) | ||
| { | ||
| auto * transform = new MetaTransform(); | ||
| transform->APIVersion(m_APIVersion); | ||
| transform->SetEvent(m_Event); | ||
| transform->ReadStream(m_NDims, m_ReadStream); | ||
| m_ObjectList.push_back(transform); | ||
| } |
There was a problem hiding this comment.
The generic Tube and Transform substring checks run before the TubeGraph and AffineTransform cases. TubeGraph is therefore constructed as MetaTube, and AffineTransform as MetaTransform, so callers receive objects of the wrong class from otherwise valid scenes. Check specific types first or restore exact or prefix matching.
| unsigned int m_FileFormatVersion; | ||
| unsigned int m_APIVersion; |
There was a problem hiding this comment.
The filename constructor calls Read() before initializing these new scalar members. The read path sets the file-format version but never assigns m_APIVersion, leaving it indeterminate after a successful read; later version-dependent behavior can therefore branch unpredictably. Give both members in-class defaults, or initialize them before reading.
| unsigned int m_FileFormatVersion; | |
| unsigned int m_APIVersion; | |
| unsigned int m_FileFormatVersion{}; | |
| unsigned int m_APIVersion{}; |
Artifacts
Filename constructor version reproduction
- The C++ reproduction constructs MetaObject from a valid file after patterned initialization and records both version values.
- The script builds and runs the version reproduction against the current implementation and an initialized comparison copy.
Current filename-constructor version result
- The current implementation reports a successful read while retaining the patterned API-version value.
Initialized filename-constructor version result
- Initializing both version members before reading produces the deterministic API-version value of zero.
| @@ -46,9 +49,10 @@ MetaObject::MetaObject() | |||
| MetaObject::MetaObject(const char * _fileName) | |||
There was a problem hiding this comment.
- Bug
- The filename constructor calls Read() before initializing the newly added version members. M_Read() resets m_FileFormatVersion, but no read path assigns m_APIVersion. A successfully read object therefore retains an indeterminate API version, so deprecated-API checks can branch unpredictably.
- Cause
- m_FileFormatVersion and m_APIVersion have no in-class initializer; MetaObject(const char *) does not assign either before Read(), and M_Read() only assigns m_FileFormatVersion.
- Fix
- Initialize both members before MetaObject::Read(_fileName), or add in-class default initializers. m_APIVersion needs an explicit default because no file field supplies it.
|
Agreed. Let's close this, and merge the small patch from #6834. |
|
Abandoning in favor of just the minimal fix. |
2 participants
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.