Enet4 / nifti-rs

Rust implementation of the NIfTI-1 format
Apache License 2.0
40 stars 11 forks source link

Data writing is always done in native byte order regardless of header #34

Closed Enet4 closed 5 years ago

Enet4 commented 5 years ago

If the target's endianness is Big Endian, the voxel data will be stored in the system's native order, while the header is stored in little endian. This will make the header/volume pair inconsistent, leading to garbage voxel data.

This can be reproduced by building and testing for the mips64-unknown-linux-gnuabi64 target.

cross test --features ndarray_volumes --target mips64-unknown-linux-gnuabi64
running 5 tests
test tests::test_c_writing ... FAILED
test tests::test_fortran_writing ... FAILED
test tests::test_header_slope_inter ... FAILED
test tests::test_write_3d_rgb ... ok
test tests::test_write_4d_rgb ... ok

failures:

---- tests::test_c_writing stdout ----
thread 'tests::test_c_writing' panicked at 'assertion failed: read_nifti.all_close(&arr, 1e-10)', tests/writer.rs:74:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- tests::test_fortran_writing stdout ----
thread 'tests::test_fortran_writing' panicked at 'assertion failed: read_nifti.all_close(&arr, 1e-10)', tests/writer.rs:74:9

---- tests::test_header_slope_inter stdout ----
thread 'tests::test_header_slope_inter' panicked at 'assertion failed: read_nifti.all_close(&transformed_data, 1e-10)', tests/writer.rs:142:9

failures:
    tests::test_c_writing
    tests::test_fortran_writing
    tests::test_header_slope_inter

test result: FAILED. 2 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out

The best way to fix this is to write the header in exactly the same endianness as the header.endianness, and then write the volume data in the same byte order. We can still use write_all for each slice if we first reverse the bytes of each voxel value in case of a non-native order.

I will be adjusting our Travis CI job matrix to also cover big endian systems, as we really have a fair amount of code that depends on the target system's byte order (while taking away some other less important jobs).

nilgoyette commented 5 years ago

I consider header.endianness as a general indication on the endianness of the file

My premises may be false. I though you didn't rely in header.endianness when reading an image but it seems you do in src/object.rs. It seems to me that we can do "wathever we want" when writing but not when reading.

Enet4 commented 5 years ago

The crate was initially focused on reading NIfTI files. And indeed, the Endianness value was retrieved based on whether dim[0] is in the expected range. We're using this check as the ground truth of the file, so the data has to agree with it. The specification itself states that "The byte order of the data arrays is presumed to be the same as the byte order of the header (which is determined by examining dim[0])".

When it comes to writing however, this matter no longer comes to play, since the object could have been built from scratch. So I figured we could use header.endianness both ways: (1) keep the endianness of the file being read; as well as (2) an indicator of the endianness to assume when writing the object to a file.