OHIF / image-JPEG2000

30 stars 21 forks source link

Error decoding 16 bits images #2

Closed gama closed 9 years ago

gama commented 9 years ago

I had issues decoding a 16 bits DICOM image encoded using JPEG2000. After some debugging, I saw that the values from the storedPixelData array had negative values. Investigating further, I saw that JPEG2000 was storing the pixels in an Int16Array instead of a Uint16Array (which is used for raw images).

The following patch seems to work for me:

diff --git a/dist/jpx.js b/dist/jpx.js
index 14da7c9..e0095b7
--- a/dist/jpx.js
+++ b/dist/jpx.js
@@ -1383,7 +1383,7 @@ var JpxImage = (function JpxImageClosure() {
         transformedTiles[c] = transformTile(context, tile, c);
       }
       var tile0 = transformedTiles[0];
-      var out = new Int16Array(tile0.items.length * componentsCount);
+      var out = new Uint16Array(tile0.items.length * componentsCount);
       var result = {
         left: tile0.left,
         top: tile0.top,
diff --git a/dist/jpx.min.js b/dist/jpx.min.js
jpambrun commented 9 years ago

Negative values are common in medical images. Therefore, the signed integer is required. I suspect that the issue is somewhere else. The image, the slope and intercept settings or the max/min values of cornerstone may be incorrect.

On Fri, Jun 12, 2015, 13:05 gama notifications@github.com wrote:

I had issues decoding a 16 bits DICOM image encoded using JPEG2000. After some debugging, I saw that the values from the storedPixelData array had negative values. Investigating further, I saw that JPEG2000 was storing the pixels in an Int16Array instead of a Uint16Array (which is used for raw images).

The following patch seems to work for me:

diff --git a/dist/jpx.js b/dist/jpx.js index 14da7c9..e0095b7--- a/dist/jpx.js+++ b/dist/jpx.js@@ -1383,7 +1383,7 @@ var JpxImage = (function JpxImageClosure() { transformedTiles[c] = transformTile(context, tile, c); } var tile0 = transformedTiles[0];- var out = new Int16Array(tile0.items.length * componentsCount);+ var out = new Uint16Array(tile0.items.length * componentsCount); var result = { left: tile0.left, top: tile0.top,diff --git a/dist/jpx.min.js b/dist/jpx.min.js

— Reply to this email directly or view it on GitHub https://github.com/OHIF/image-JPEG2000/issues/2.

gama commented 9 years ago

From what I could grasp from cornerstone's WADO Image Loader source code, when the pixel representation tag (0028, 0103) is 0 (unsigned) it uses an Uint16Array; and when it is 1 (signed) it uses a Int16Array. This seems to work fine with the uncompressed images I'm using -- they all have "pixelRepresentation" set as "unsigned" (and, for the record, the rescale slope is "1", rescale intercept is "0", window center is "32768" and window width is "65536"). When I converted these images to JPEG2000 Lossless, none of these tags were modified.

image-JPEG2000, however, always uses an Int16Array, but it's obviously impossible to store unsigned data using such a data type. Do you know if the JPEG2000 spec hardcodes the pixel representation for 16 bit images as "signed"? I tried to search for it but the only documents I found (from http://www.jpeg.org/jpeg2000/) are kind of expensive... :-(

If that's not the case, shouldn't image-JPEG2000 have some logic to handle the case where the pixel representation is "unsigned"?

jpambrun commented 9 years ago

I understand. In that case you are right, an unsigned file with a range exceeding 15 bits will fail. I just never seen a medical image with such a high dynamic range. Can you share the image? Could I add it to the test cases?

On Fri, Jun 12, 2015, 17:07 gama notifications@github.com wrote:

From what I could grasp from cornerstone's WADO Image Loader source code, when the pixel representation tag (0028, 0103) is 0 (unsigned) it uses an Uint16Array; and when it is 1 (signed) it uses a Int16Array. This seems to work fine with the uncompressed images I'm using -- they all have "pixelRepresentation" set as "unsigned" (and, for the record, the rescale slope is "1", rescale intercept is "0", window center is "32768" and window width is "65536"). When I converted these images to JPEG2000 Lossless, none of these tags were modified.

image-JPEG2000, however, always uses an Int16Array, but it's obviously impossible to store unsigned data using such a data type. Do you know if the JPEG2000 spec hardcodes the pixel representation for 16 bit images as "signed"? I tried to search for it but the only documents I found (from http://www.jpeg.org/jpeg2000/) are kind of expensive... :-(

If that's not the case, shouldn't image-JPEG2000 have some logic to handle the case where the pixel representation is "unsigned"?

— Reply to this email directly or view it on GitHub https://github.com/OHIF/image-JPEG2000/issues/2#issuecomment-111619228.

jpambrun commented 9 years ago

Also, this decoder was developed by someone else with a PDF viewer in mind. It is not 100% feature complete nor 100% standard compliant, but I will try to fix bugs and add missing featured related diagnostic imaging.

On Fri, Jun 12, 2015, 17:25 JF Pambrun jf.pambrun@gmail.com wrote:

I understand. In that case you are right, an unsigned file with a range exceeding 15 bits will fail. I just never seen a medical image with such a high dynamic range. Can you share the image? Could I add it to the test cases?

On Fri, Jun 12, 2015, 17:07 gama notifications@github.com wrote:

From what I could grasp from cornerstone's WADO Image Loader source code, when the pixel representation tag (0028, 0103) is 0 (unsigned) it uses an Uint16Array; and when it is 1 (signed) it uses a Int16Array. This seems to work fine with the uncompressed images I'm using -- they all have "pixelRepresentation" set as "unsigned" (and, for the record, the rescale slope is "1", rescale intercept is "0", window center is "32768" and window width is "65536"). When I converted these images to JPEG2000 Lossless, none of these tags were modified.

image-JPEG2000, however, always uses an Int16Array, but it's obviously impossible to store unsigned data using such a data type. Do you know if the JPEG2000 spec hardcodes the pixel representation for 16 bit images as "signed"? I tried to search for it but the only documents I found (from http://www.jpeg.org/jpeg2000/) are kind of expensive... :-(

If that's not the case, shouldn't image-JPEG2000 have some logic to handle the case where the pixel representation is "unsigned"?

— Reply to this email directly or view it on GitHub https://github.com/OHIF/image-JPEG2000/issues/2#issuecomment-111619228.

gama commented 9 years ago

Can you share the image? Could I add it to the test cases?

I'll ask permission from the company that shared the image with us and will get back to you asap.

gama commented 9 years ago

All clear. Here's a couple of sample images: https://drive.google.com/open?id=0B8m9t3sArEbwZEstY0VaRTRfQkE&authuser=2 https://drive.google.com/open?id=0B8m9t3sArEbwUGZoaUtibGtQcUk&authuser=2

Let me know if you need the uncompressed versions as well.

jpambrun commented 9 years ago

That was an easy fix. Thanks for reporting and thanks for providing the source material for a new test case.

gama commented 9 years ago

Easy fix?? Wow, I don't even want know what you call a hard one... ;-) Thanks a lot!

jpambrun commented 9 years ago

Well, you did all the work and pointed to the solution. From my perspective, it was easy..

gama commented 9 years ago

@jpambrun, the following image still has the same issue:

https://drive.google.com/open?id=0B8m9t3sArEbwdHJtdDNCY3dBZ1E

In theory, the only difference is that this one was compressed using lossy JPEG 2000 (while the other two were compressed with lossless JPEG 2000).

jpambrun commented 9 years ago

This is not the same issue. When compared with Matlab's JPEG 2000 decoder, almost every pixels appears incorrectly reconstructed. This is illustrated in the attached figure where black represents perfect reconstruction and white represents a difference of 1000. Pixel close to 0 and 65,535 can roll over resulting in very obvious distortions that looked like the previous issue.

2015-07-06-104922_1601x1225_scrot

jpambrun commented 9 years ago

I have created a new issue #5.