Use openjpeg instead of nanojpeg if found

[ignatenkobrain]

It will make possible to package dcm2niix in Fedora.

[neurolabusc]

Is there a specific problem with nanojpeg and Fedora? As I note, I include nanojpeg and this can convert classic JPEG-compressed DICOMs. I regularly compile dcm2niix on the Fedora-based CentOS and have never seen a problem. The user can also choose to compile to the Jasper or OpenJPEG libraries to add JPEG 2000 support, though the executable size grows a lot. In my experience JPEG 2000 is VERY rare in medical imaging. It is lossy. While the classic JPEG can lead to visible artifacts, the artifacts from JPEG 2000 can be subtle but are often precisely in the textures radiologists use to identify pathologies.

[ignatenkobrain]

There is problem with bundling libraries. I already included openjpeg for building, but it still uses nanojpeg for one function and I want to get rid of it.

[neurolabusc]

OK, fine. Are you confident that your changes will not cause problems for Debian based builds?

-c

[ignatenkobrain]

I will add some if statements into cmake and it will work fine.

[ignatenkobrain]

So please prepare replacement of current nanojpeg code using openjpeg and I will prepare cmake stuff and you will able to use nanojpeg or openjpeg ( I will use for official package second).

[neurolabusc]

Hello
I tried to pipe classic JPEG files to OpenJPEG instead of nanoJPEG. However, OpenJPEG reports that it can not read the header. I think this is because OpenJPEG only supports JPEG-2000 not the older traditional JPEG required by DICOM transfer syntax 1.2.840.10008.1.2.4.50. Therefore, I think the correct approach is to use nanoJPEG for classic JPEG and if requested at compile time add OpenJPEG for J2K. I have built this on CentOS and OSX and never had a conflict from including both OpenJPEG and NanoJPEG.

http://www.openjpeg.org
"OpenJPEG is an open-source JPEG 2000 codec"
see also
http://www.openjpeg.org/doxygen/openjpeg_8h.html#a1d857738cef754699ffb79ddff48efbf
for supported codecs

[neurolabusc]

I also note you have been working on the rather ancient debian branch, rather than the current master branch. Can I suggest you try out the current master branch? As listed on the home page, compiling with both ujpeg (for classic JPEG) and OpenJPEG (for JPEG2000) is pretty easy. If you want to tune make files for Fedora and the master branch I would be happy to include them. In any case, here is a typical (clang) compilation to include OpenJPEG:

g++ -O3 -dead_strip -I. main_console.cpp nii_dicom.cpp nifti1_io_core.cpp nii_ortho.cpp nii_dicom_batch.cpp jpg_0XC3.cpp ujpeg.cpp -o dcm2niix -I/usr/local/lib /usr/local/lib/libopenjp2.a

[ghisvail]

Has this been effectively solved in the end? Is there a way to select the system jpeg libraries instead of the vendorized nanojpeg for the build?

[ignatenkobrain]

I think I used http://pkgs.fedoraproject.org/cgit/rpms/dcm2niix.git/tree/0001-allow-nifti-openjpeg2-be-system.patch

[ghisvail]

Looks like your patch unbundles nanojpeg, miniz and the nifti wrapper code in favour of linking with openjpeg2, zlib and the system nifticlib, am I correct?

[ignatenkobrain]

@ghisvail yes, and as far as I can see it's merged in this repository, so I think master branch should have it (if it was not reverted)

[ghisvail]

I don't think it has been merged in master. Otherwise, cmake/Modules/FindNIFTI.cmake should be present according to your patch, but it is not.

Perhaps an improvement over your patch would be to declare corresponding use flags (USE_SYSTEM_JPEG, USE_SYSTEM_ZLIB and USE_SYSTEM_NIFTI), to allow distros like Fedora and Debian to selectively disable the bundled dependencies but still support the current build path?

[ignatenkobrain]

it definitely was merged in #6. Weird thing is that I can find in git-log my commit, but I can't see any commits which would remove it....

[ignatenkobrain]

@neurolabusc Any ideas?

[ghisvail]

Well, the license stuff were merged indeed. Did you submit a PR for the unbundling?

[ignatenkobrain]

@ghisvail I updated comment, it's actually #6, not #7.

6916539: Sat, 5 Dec 2015 15:53:43 +0100 allow nifti, openjpeg2 be system (Igor Gnatenko)

[ghisvail]

FYI, the reason I bumped this issue up is because I am also interested in using less of the vendorized code for the Debian package, at the very least for the JPEG dependency (for which we get security problems very often).

[ignatenkobrain]

@ghisvail yeah, I missed updates for Fedora package for long time, so I didn't notice. but some comment from @neurolabusc would be very useful.. The cmake/Modules just disappeared somehow.

[neurolabusc]

I would be happy to support patches for the master branch. As I recall, the patches were for a Debian branch that I do not support.

1.) I did enable using zlib instead of Miniz, this is documented in the readme.md ("ZLIB BUILD") section.

2.) As I noted, openjpeg2 is not a replacement for nanojpeg. The former deals with JPEG2000 while the latter deals with classic JPEG. The submitted patch would break if you tried to convert an image converted in the classic JPEG format. DICOM images can be encoded for either classic or 2000 formats. Therefore, you can build with openjpeg (see the "JPEG2000 BUILD" section of the readme.md), but it does not remove the dependency for nanojpeg. Therefore, if you want to remove the dependency on nanojpeg you could either drop support for classic jpeg images altogether (with some new compiler directive) or add a compiler directive that choses between nanojpeg and libjpeg.

3.) You could certainly update the patch to support nifticlib instead of nifti. I would suggest you follow the same style as the miniz -> zlib (e.g. update code and readme.md to support and describe a -myDisableNiftiInbuilt function). As I recall, on some systems/compilers had issues with the definition "DT_UNKNOWN" which is why I renamed it "DT_UNKNOWN_DT" This is also mentioned here. So please validate any patch you submit for this extensively.

[ghisvail]

1.) I did enable using zlib instead of Miniz, this is documented in the readme.md ("ZLIB BUILD") section.

It is a good start, but it would be better to have it integrated in CMake, hence my proposal of adding a configuration flag to choose whether to use the embedded miniz.c or the system zlib.

As I noted, openjpeg2 is not a replacement for nanojpeg. The former deals with JPEG2000 while the latter deals with classic JPEG.

What about jpeg-turbo then, which is the default "classic" jpeg implementation on Debian? Is it the same for Fedora @ignatenkobrain?

3.) You could certainly update the patch to support nifticlib instead of nifti.

Indeed, although the vendorization of Nifti in the dcm2niix code base is more convoluted than I expected it to be.

"DT_UNKNOWN" which is why I renamed it "DT_UNKNOWN_DT"

Yes DT_UNKNOWN can potentially conflict with macros from other libs. A better solution would be to use the NIFTI_ prefixed macros. I believe all DT_ macros have corresponding NIFTI_ ones.

[neurolabusc]

1. Sure, feel free to update the CMake file to handle this.

2. I just updated the master to include the myDisableClassicJPEG directive. This removes the dependency on NanoJPEG but also drops support for these lossy-compressed images. You could further modify this to support jpeg-turbo, lib-jpeg, etc. I do not have those libraries installed, and am not sure that how you call them. You can find sample images online, which I list here
   https://www.nitrc.org/plugins/mwiki/index.php/dcm2nii:MainPage#Sample_DICOM_Images
   with a good place to start being here
   http://www.barre.nom.fr/medical/samples/index.html

3. The DICOM standard is pretty complex, and I wanted code that compiled on multiple operating systems (Linux, macOS, Windows, minGW), with multiple compilers (gcc, llvm/clang, microsoft), with minimal dependencies. I am happy to consider any ideas for simplification, but validating across all of these contingencies is time consuming.

4. I think they all have NIFTI aliases EXCEPT the problematic UNKNOWN.

[neurolabusc]

I did take a look at the code, and I think it is not a good idea to use nifticlib instead of the inbuilt functions. My unit nifti1_io_core uses some mathematical routines from the nifticlib unit nifti1_io, but it defines a number of additional mathematical operations and structures as well as removing all of the input/output routines of nifti1_io. My unit is 17 kb, where the nifti1_io is 270 kb. Further, my unit does not require the znzlib dependency. In other words, we use a few core mathematical functions from the huge library. The routines we use are public domain, mature, stable and small. In addition, we include a number of additional mathematical functions. Perhaps to remove confusion, we should rename nifti1_io_core to nifti1_math, as it is a small collection of mathematical functions with no input/output functions.

[ghisvail]

I think it is not a good idea to use nifticlib instead of the inbuilt functions [...]

Our motivations for not using the vendorized code in the packaged version of dcm2niix have nothing to do with size or performance matters. C-libraires gets patched for vulnerabilities reasonably often and dynamically linking to them significantly help with the dispatch of security updates.

What @ignatenkobrain and myself are investigating here is a way to support both your code path using the vendored dependencies and an alternative which could make the most of the packaged system libraries.

It does not have to be one or the other, that's what I am saying.

[neurolabusc]

As I noted, the code from nifticlib is a couple of small, mature math routines and the names of the structures. These functions are not security risks. I do think you could break my unit up into two: one with the copies from nifticlib and another with the extensions I added, but I do think you will hurt the readability and maintainability of the code base: all of those functions and structures belong together.

I think your goal is laudable for things like JPEG decompression, where there are concerns regarding buffer overflow and other IO issues. However, I do not use any of the nifticlib input/output routines that would raise these concerns.

[neurolabusc]

1.) I did update the CMake file, so it will build to zlib instead of miniz if it detects the zlib package. Please test this out and confirm it works.

2.) I do think a developer could replace nanoJPEG with jpeg-turbo or libjpeg. The libjpeg API looks pretty cumbersome, but the turbojpeg API looks pretty promising:
http://stackoverflow.com/questions/9094691/examples-or-tutorials-of-using-libjpeg-turbos-turbojpeg
If someone with these libraries installed can develop an alternative to the nii_loadImgJPEG50 function, I would be happy to add it to the branch.

[ghisvail]

Just to be clear, my earlier comment was only meant to explicit our motivations and make sure we were all on the same page.

From a pragmatic standpoint, I completely agree with you that the vendorized subset of nifticlib is much less of a concern compared to jpeg and zlib.

it will build to zlib instead of miniz if it detects the zlib package

👍

I do think a developer could replace nanoJPEG with jpeg-turbo or libjpeg

That would be the plan.

develop an alternative to the nii_loadImgJPEG50 function

Thanks for the pointer.

[neurolabusc]

1.) I have updated the code to optionally use libjpeg-turbo instead of nanojpeg. This is described in the USING LIBJPEG-TURBO TO DECODE CLASSIC JPEG section of the readme.md file.

2.) I am not very familiar with cmake files, so I suggest you update that to use the myTurboJPEG directive if it can find the required packages (similar to the changes I made to the cmake file for zlib instead of miniz support).

3.) The code also contains a couple of verbose printf comments for use while testing this - we can remove those before a general release.

4.) Once this is done, I think we can close this issue.

[ghisvail]

I have updated the code to optionally use libjpeg-turbo instead of nanojpeg.

I'll look it up. Thanks for investigating.

use the myTurboJPEG directive if it can find the required packages

I can provide a patch for the CMake integration once 1) is validated.

[ghisvail]

I guess this issue can be closed now. The build system now supports compilation and linkage with the system zlib and turbojpeg.