Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIfTI C library is outdated with broken byte swapping within newer GCC #5

Open
schuhschuh opened this issue Apr 1, 2015 · 13 comments
Labels

Comments

@schuhschuh
Copy link
Member

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).

https://gcc.gnu.org/ml/gcc-help/2015-04/msg00005.html

@schuhschuh schuhschuh added the bug label Apr 1, 2015
@kevin-keraudren
Copy link
Contributor

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.

@schuhschuh
Copy link
Member Author

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.

@schuhschuh
Copy link
Member Author

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?

@drkarim
Copy link

drkarim commented Mar 28, 2017

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?

@ghisvail
Copy link
Member

IRTK is deprecated in favour of MIRTK, which should include a more robust handling of NifTI I/O.

@drkarim
Copy link

drkarim commented Mar 28, 2017

The only problem is that I have a lot of legacy code written that depends on IRTK.

@ghisvail
Copy link
Member

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.

@drkarim
Copy link

drkarim commented Mar 29, 2017

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.

@ghisvail
Copy link
Member

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.

@schuhschuh
Copy link
Member Author

@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.

@drkarim
Copy link

drkarim commented Mar 29, 2017

@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?

screen shot 2017-03-29 at 11 27 31

info_ct3_edt

@ghisvail
Copy link
Member

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.

@ghisvail
Copy link
Member

Never mind, I saw Andreas kick-started the portage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants