NIfTI C library is outdated with broken byte swapping within newer GCC
schuhschuh opened this issue · 13 comments
The NIfTI-1 C library source code included with the IRTK is outdated. I ran in particular into an issue that the byte swapping was not working when the library is compiled with optimization using GCC >=4.7. It turns out these problems have been fixed in the most recent 2.0 release version (this particular problem was possibly fixed in v1.1 from 2008 already).
Hi @schuhschuh , do you know if this issue is fixed in IRTK 2.0?
When trying out the new IRTK 2.0 through Python, I got:
img = irtk.imread("brain.nii")
irtkFileNIFTIToImage: Data type 1 not supported, trying signed short data type
and this messed up image header:
{ 'dim': array([256, 256, 192, 1], dtype=int32),
'orientation': array([[ inf, nan, nan],
[ -1., 0., 0.],
[ 0., -1., 0.]]),
'origin': array([ 0., -1., 1., 1.]),
'pixelSize': array([ 0., 1., 1., 1.]) }
CMake says my system is little endian (Mac OSX if that's related), if I force the big endian flag, I get:
Modules/Image/src/irtkFileNIFTIToImage.cc:197:29: error: use of undeclared
identifier 'MSB_FIRST'
if (hdr.nim->byteorder == MSB_FIRST) {
^
1 error generated.
Bu if I remove the lines https://github.com/BioMedIA/IRTK/blob/master/Modules/Image/src/irtkFileNIFTIToImage.cc#L192 :
#ifndef WORDS_BIGENDIAN
if (hdr.nim->byteorder == 1) { //LSB_FIRST) {
this->_Swapped = !this->_Swapped;
}
#else
if (hdr.nim->byteorder == MSB_FIRST) {
this->_Swapped = !this->_Swapped;
}
#endif
Then I get the correct header.
{ 'dim': array([256, 256, 192, 1], dtype=int32),
'orientation': array([[-1., 0., 0.],
[ 0., -1., 0.],
[ 0., 0., 1.]]),
'origin': array([-127.5, -127.5, 95.5, 0. ]),
'pixelSize': array([ 1., 1., 1., 1.]) }
Now that IRTK 2.0 uses NiftiClib instead of shipping its own version of nifti reading code, I do not know if it still corresponds to the bug you reported but I'm curious to hear from your experience.
Hi @kevin-keraudren, regarding the issue I reported here, it will not affect you with the new IRTK when on your system is a recent enough NiftiCLib installed (at least v1.1, better 2.0).
Regarding the byte swapping issue within the IRTK code itself, I have had no issue so far with my private IRTK copy (no particular CMake refactoring). I suggest you report this issue and your findings in the issue tracker of the new IRTK so @ghisvail can have a closer look.
Given that in the code you remove to "fix" the issue the _Swapped flag is simply inverted (which must be the case, otherwise removing the code wouldn't do anything), it might as well be that this flag is now incorrectly initialised in irtkCifstream.cc.
The other issue you report seems that MSB_FIRST is not defined. A look at nifti1_io.h reveals that this header defines this macro for internal use only by the corresponding nifti1_io.cc. It may well be that we have to define it ourselves in irtkNIFTI.h, e.g., as IRTK_MSB_FIRST and IRTK_LSB_FIRST. You can also note that in the #ifndef branch, LSB_FIRST is not used, but instead expanded already. Probably for the same reason.
P.S.: I think the IRTK so far actually never defined WORDS_BIGENDIAN... so the added endianness check might be the reason for your issue.
EDIT: The image you are trying to read, was it written by an (old) IRTK tool?
I am having trouble reading NIFTII files with this IRTK and I get an error like this:
irtkFileNIFTIToImage: Data type 1065353216 not supported, trying signed short data type
It seemed to me that it might related to this issue. The header is not read correctly, thus resulting in not finding the correct data type for the image?
I included the NifTI 2.0 library but that did not fix. I am running on MacOS Sierra 10.12 with gcc version 4.2.1. Any pointers on how this issue could be fixed?
IRTK is deprecated in favour of MIRTK, which should include a more robust handling of NifTI I/O.
The only problem is that I have a lot of legacy code written that depends on IRTK.
Do you known on which platform your NifTI files have been generated from? Also can you successfully load them on papaya?
This would tell us whether they are well formed, and which endianness they are potentially in.
These were mostly generated under Windows 7. They successfully work when loaded on papaya.
It is happening to pretty much every NifTI file I am trying to load with IRTK.
These were mostly generated under Windows 7.
So, big endian.
They successfully work when loaded on papaya.
Confirming the files are well-formed.
It is happening to pretty much every NifTI file I am trying to load with IRTK.
Could you try running IRTK's info executable on this file and paste the results or the error raised.
FYI, I am looking for something obvious to fix. If the problem lies deeper in the code, then I will most likely not investigate further as IRTK is no longer actively maintained. Most of our software has been ported to MIRTK, which we actively maintain and support.
@drkarim If you send a list of IRTK binaries that you are using, I can compile a brief overview of corresponding MIRTK commands or we'll see if one you need is missing. In case you mostly need nreg, you'd save lots of time waiting for results switching to MIRTK. From a comparison I've done based on OASIS / Neuromorphometrics labels, nreg runs on average in 403 min whereas MIRTK run on average in only 3.6 min with 8 cores, achieving the same if not higher Dice overlap.
As I see you're at King's, @ericspod has recently updated Eidolon to use MIRTK.
@ghisvail The info command manages to read the header, but it is not quite correct (black window). The correct header is shown by running info from my working IRTK installation (purple window). It seems that the bytes are not read properly?
I suspect @drkarim is using IRTK as a library. Could you confirm @drkarim ? If that's the case, then porting the code to MIRTK might be a bit more involved. If you are just using the IRTK executables, please consider @schuhschuh's suggestion to port your process calls over to MIRTK
Regardless, if this were a global endianness issue, the output of info would affect all fields which are larger than a byte in memory, i.e. pretty much all of them. Here only the voxel type is incorrect, and then wrongly assigned to short as a result of the failure.
Never mind, I saw Andreas kick-started the portage.