Bigendian: opj_compress + opj_decompress fails #518

manisandro · 2015-06-24T23:44:49Z

On bigendian, converting some image to j2k and decompressing it again results in failure, i.e.:

$ wget http://people.sc.fsu.edu/~jburkardt/data/ppma/star_field.ascii.ppm

$ opj2_compress -i star_field.ascii.ppm -o test.j2k -q 30,40,50
[INFO] tile number 1 / 1
Generated outfile test.j2k

$ opj2_decompress -i test.j2k -o test.ppm
[INFO] Start to read j2k main header (0).
[ERROR] Prevent buffer overflow (x1: 671090818, y1: 671090818)
[ERROR] Marker handler function failed to read the marker segment
ERROR -> opj_decompress: failed to read the header

A quick scan through the codebase reveals for instance in convert.c portions of code such as

            union { signed short val; signed char vals[2]; } uc16;
            mask = (1 << image->comps[compno].prec) - 1;
            ptr = image->comps[compno].data;
            for (line = 0; line < h; line++) {
                for(row = 0; row < w; row++)    {
                    curr = *ptr;
                    if(curr > 32767 ) curr = 32767; else if( curr < -32768) curr = -32768;
                    uc16.val = (signed short)(curr & mask);
                    res = fwrite(uc16.vals, 1, 2, rawFile);
                    if( res < 2 ) {
                        fprintf(stderr, "failed to write 2 byte for %s\n", outfile);
                        goto fin;
                    }
                    ptr++;
                }
            }

which likely produce wrong results on bigendian.

The text was updated successfully, but these errors were encountered:

sharkcz · 2015-06-25T11:00:24Z

Let me know you will need access to a big endian machine (ppc64)

szukw000 · 2015-06-25T16:50:52Z

On Wed, 24 Jun 2015 16:44:50 -0700, Sandro Mani wrote:

On bigendian, converting some image to j2k and decompressing
it again results in failure, i.e.:

A long time ago I sent a patch for PPC64 to [email protected]
for this BIGENDIAN bug. This patch has been ignored.

Here it is again. Ignore it once more. Or apply it.

winfried

--- openjpeg-2.x-trunk-r3006-1/src/lib/openjp2/cio.c.orig 2015-05-31 23:37:08.000000000 +0000
+++ openjpeg-2.x-trunk-r3006-1/src/lib/openjp2/cio.c 2015-06-01 01:54:50.503990204 +0000
@@ -46,11 +46,11 @@

void opj_write_bytes_BE (OPJ_BYTE * p_buffer, OPJ_UINT32 p_value, OPJ_UINT32 p_nb_bytes)
{

const OPJ_BYTE * l_data_ptr = ((const OPJ_BYTE *) &p_value) + p_nb_bytes;
const OPJ_BYTE * l_data_ptr = ((const OPJ_BYTE *) &p_value);

assert(p_nb_bytes > 0 && p_nb_bytes <= sizeof(OPJ_UINT32));

memcpy(p_buffer,l_data_ptr,p_nb_bytes);
memcpy(p_buffer,l_data_ptr+sizeof(OPJ_UINT32)-p_nb_bytes,p_nb_bytes);
}

void opj_write_bytes_LE (OPJ_BYTE * p_buffer, OPJ_UINT32 p_value, OPJ_UINT32 p_nb_bytes)
@@ -72,7 +72,7 @@
assert(p_nb_bytes > 0 && p_nb_bytes <= sizeof(OPJ_UINT32));

*p_value = 0;

memcpy(l_data_ptr+4-p_nb_bytes,p_buffer,p_nb_bytes);
memcpy(l_data_ptr+sizeof(OPJ_UINT32)-p_nb_bytes,p_buffer,p_nb_bytes);
}

void opj_read_bytes_LE(const OPJ_BYTE * p_buffer, OPJ_UINT32 * p_value, OPJ_UINT32 p_nb_bytes)

manisandro · 2015-06-25T20:34:18Z

Thanks, that patch works. I suppose the best chance of getting it committed is by creating a pull request. Do you want to do so or shall I?

szukw000 · 2015-06-25T20:38:23Z

manisandro, please do it yourself. Good luck.

winfried

szukw000 · 2015-06-26T22:15:48Z

manisandro, here is a supplement for #518.

The file 'data//input/conformance/file6.jp2' does not have 16-Bit but 12-Bit. I have found
that this TIFF file is very dark without the following change:

--- openjpeg/src/bin/jp2/convert.c.orig 2015-06-11 03:03:10.000000000 +0000
+++ openjpeg/src/bin/jp2/convert.c 2015-06-26 23:45:52.126991261 +0000
@@ -1603,7 +1603,7 @@

 if(bps > 8 && bps < 16)
 {

   ushift = 16 - bps; dshift = bps - ushift;

   ushift = 16 - bps; dshift = (bps - ushift)>>1;
 bps = 16; force16 = 1;

}

I suppose there should be another change for 'imagetotif()' in case the infile is
GRAY or GRAYA (one or two channels). But I do not have an example file.

Please excuse that I insert the change here. I do not know how to send a fix for
GITHUB.

winfried

szukw000 · 2015-06-26T22:21:04Z

Something has gone wrong here:

--- openjpeg/src/bin/jp2/convert.c.orig 2015-06-11 03:03:10.000000000 +0000
+++ openjpeg/src/bin/jp2/convert.c 2015-06-26 23:45:52.126991261 +0000
@@ -1603,7 +1603,7 @@

 if(bps > 8 && bps < 16)
 {

   ushift = 16 - bps; dshift = bps - ushift;

   ushift = 16 - bps; dshift = (bps - ushift)>>1;
 bps = 16; force16 = 1;

}

winfried

manisandro · 2015-06-26T22:26:40Z

In case you are interested in how to work with github:

Fork the project, as you have already done
Clone the project locally

git clone [email protected]:szukw000/openjpeg.git
Checkout a new branch where you will make the changes (my_branch here is some name you can choose)

git checkout -b my_branch
Make the changes
Commit the changes

git commit -am "my commit message"
Push your branch with changes to github

git push -u origin my_branch
On github, on the page of your openjpeg fork (https://github.com/szukw000/openjpeg), you should see a message bar mentioning your just-pushed branch, with a button "Pull request". Click on that one and you can file a pull request with your changes. You might have to select your just uploaded branch if that message bar does not appear.

Hope this helps.

szukw000 · 2015-06-27T01:14:44Z

No, this did not help.

I meant your 'Fix opj_write_bytes_BE' patch.

winfried

szukw000 · 2015-06-27T17:16:11Z

manisandro, I have now found a GRAY TIFF image with 16-Bit. What follows is the complete patch.

winfried

--- openjpeg/src/bin/jp2/convert.c.orig 2015-06-11 03:03:10.000000000 +0200
+++ openjpeg/src/bin/jp2/convert.c 2015-06-27 18:52:56.739592290 +0200
@@ -1603,7 +1603,7 @@

 if(bps > 8 && bps < 16)
 {

   ushift = 16 - bps; dshift = bps - ushift;

   ushift = 16 - bps; dshift = (bps - ushift)>>1;
 bps = 16; force16 = 1;

}

@@ -1775,7 +1775,9 @@
if(r > 65535) r = 65535; else if(r < 0) r = 0;
if(g > 65535) g = 65535; else if(g < 0) g = 0;

if(b > 65535) b = 65535; else if(b < 0) b = 0;

+#ifdef OPJ_BIG_ENDIAN

```
  r = swap16(r); g = swap16(g); b = swap16(b);
```
+#endif
dat8[i+0] = (unsigned char)r;/LSB/
dat8[i+1] = (unsigned char)(r >> 8);/MSB/
dat8[i+2] = (unsigned char)g;
@@ -1785,6 +1787,9 @@
if(has_alpha)
{
if(a > 65535) a = 65535; else if(a < 0) a = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  a = swap16(a);
```
+#endif
dat8[i+6] = (unsigned char)a;
dat8[i+7] = (unsigned char)(a >> 8);
}
@@ -1825,7 +1830,9 @@
if(r > 65535) r = 65535; else if(r < 0) r = 0;
if(g > 65535) g = 65535; else if(g < 0) g = 0;
if(b > 65535) b = 65535; else if(b < 0) b = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  r = swap16(r); g = swap16(g); b = swap16(b);
```
+#endif
dat8[i+0] = (unsigned char) r;/LSB/
if(i+1 < ssize) dat8[i+1] = (unsigned char)(r >> 8);else break;/MSB/
if(i+2 < ssize) dat8[i+2] = (unsigned char) g; else break;
@@ -1836,6 +1843,9 @@
if(has_alpha)
{
if(a > 65535) a = 65535; else if(a < 0) a = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  a = swap16(a);
```
+#endif
if(i+6 < ssize) dat8[i+6] = (unsigned char)a; else break;
if(i+7 < ssize) dat8[i+7] = (unsigned char)(a >> 8); else break;
}
@@ -1948,11 +1958,17 @@
if(has_alpha) a = (a<<ushift) + (a>>dshift);
}
if(r > 65535) r = 65535; else if(r < 0) r = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  r = swap16(r);
```
+#endif
dat8[i+0] = (unsigned char)r;/LSB/
dat8[i+1] = (unsigned char)(r >> 8);/MSB/
if(has_alpha)
{
if(a > 65535) a = 65535; else if(a < 0) a = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  a = swap16(a);
```
+#endif
dat8[i+2] = (unsigned char)a;
dat8[i+3] = (unsigned char)(a >> 8);
}
@@ -2251,6 +2267,7 @@
numcomps = 1 + has_alpha;
color_space = OPJ_CLRSPC_GRAY;

+fprintf(stderr,"%s:%d:\n\ttiftoimage numcomps(%d)\n",FILE,LINE,numcomps);
for(j = 0; j < numcomps; ++j)
{
cmptparm[j].prec = tiBps;
@@ -2292,6 +2309,7 @@
unsigned char *dat8;
tsize_t i, ssize;
int step;

       unsigned short v;

     ssize = TIFFReadEncodedStrip(tif, strip, buf, strip_size);
     dat8 = (unsigned char*)buf;

@@ -2304,9 +2322,19 @@
{
if(index < imgsize)
{

                   image->comps[0].data[index] = ( dat8[i+1] << 8 ) | dat8[i+0];

                  v = ( dat8[i+1] << 8 ) | dat8[i+0];

+#ifdef OPJ_BIG_ENDIAN

```
                  v = swap16(v);
```
+#endif

                   image->comps[0].data[index] = v;
                 if(has_alpha)

                       image->comps[1].data[index] = ( dat8[i+3] << 8 ) | dat8[i+2];

```
                 {
```

                  v = ( dat8[i+3] << 8 ) | dat8[i+2];

+#ifdef OPJ_BIG_ENDIAN

```
                  v = swap16(v);
```
+#endif

                  image->comps[1].data[index] = v;

szukw000 · 2015-06-27T17:18:41Z

This is a mess. I give up.

winfried

szukw000 · 2015-06-27T17:47:53Z

@masandro, here is another attempt.

winfried

--- openjpeg/src/bin/jp2/convert.c.orig 2015-06-11 03:03:10.000000000 +0200
+++ openjpeg/src/bin/jp2/convert.c 2015-06-27 18:52:56.739592290 +0200
@@ -1603,7 +1603,7 @@

 if(bps > 8 && bps < 16)
 {

   ushift = 16 - bps; dshift = bps - ushift;

   ushift = 16 - bps; dshift = (bps - ushift)>>1;
 bps = 16; force16 = 1;

}

@@ -1775,7 +1775,9 @@
if(r > 65535) r = 65535; else if(r < 0) r = 0;
if(g > 65535) g = 65535; else if(g < 0) g = 0;

if(b > 65535) b = 65535; else if(b < 0) b = 0;

+#ifdef OPJ_BIG_ENDIAN

```
  r = swap16(r); g = swap16(g); b = swap16(b);
```
+#endif
dat8[i+0] = (unsigned char)r;/LSB/
dat8[i+1] = (unsigned char)(r >> 8);/MSB/
dat8[i+2] = (unsigned char)g;
@@ -1785,6 +1787,9 @@
if(has_alpha)
{
if(a > 65535) a = 65535; else if(a < 0) a = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  a = swap16(a);
```
+#endif
dat8[i+6] = (unsigned char)a;
dat8[i+7] = (unsigned char)(a >> 8);
}
@@ -1825,7 +1830,9 @@
if(r > 65535) r = 65535; else if(r < 0) r = 0;
if(g > 65535) g = 65535; else if(g < 0) g = 0;
if(b > 65535) b = 65535; else if(b < 0) b = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  r = swap16(r); g = swap16(g); b = swap16(b);
```
+#endif
dat8[i+0] = (unsigned char) r;/LSB/
if(i+1 < ssize) dat8[i+1] = (unsigned char)(r >> 8);else break;/MSB/
if(i+2 < ssize) dat8[i+2] = (unsigned char) g; else break;
@@ -1836,6 +1843,9 @@
if(has_alpha)
{
if(a > 65535) a = 65535; else if(a < 0) a = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  a = swap16(a);
```
+#endif
if(i+6 < ssize) dat8[i+6] = (unsigned char)a; else break;
if(i+7 < ssize) dat8[i+7] = (unsigned char)(a >> 8); else break;
}
@@ -1948,11 +1958,17 @@
if(has_alpha) a = (a<<ushift) + (a>>dshift);
}
if(r > 65535) r = 65535; else if(r < 0) r = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  r = swap16(r);
```
+#endif
dat8[i+0] = (unsigned char)r;/LSB/
dat8[i+1] = (unsigned char)(r >> 8);/MSB/
if(has_alpha)
{
if(a > 65535) a = 65535; else if(a < 0) a = 0;
+#ifdef OPJ_BIG_ENDIAN
```
  a = swap16(a);
```
+#endif
dat8[i+2] = (unsigned char)a;
dat8[i+3] = (unsigned char)(a >> 8);
}
@@ -2251,6 +2267,7 @@
numcomps = 1 + has_alpha;
color_space = OPJ_CLRSPC_GRAY;

+fprintf(stderr,"%s:%d:\n\ttiftoimage numcomps(%d)\n",FILE,LINE,numcomps);
for(j = 0; j < numcomps; ++j)
{
cmptparm[j].prec = tiBps;
@@ -2292,6 +2309,7 @@
unsigned char *dat8;
tsize_t i, ssize;
int step;

       unsigned short v;

     ssize = TIFFReadEncodedStrip(tif, strip, buf, strip_size);
     dat8 = (unsigned char*)buf;

@@ -2304,9 +2322,19 @@
{
if(index < imgsize)
{

                   image->comps[0].data[index] = ( dat8[i+1] << 8 ) | dat8[i+0];

                  v = ( dat8[i+1] << 8 ) | dat8[i+0];

+#ifdef OPJ_BIG_ENDIAN

```
                  v = swap16(v);
```
+#endif

                   image->comps[0].data[index] = v;
                 if(has_alpha)

                       image->comps[1].data[index] = ( dat8[i+3] << 8 ) | dat8[i+2];

```
                 {
```

                  v = ( dat8[i+3] << 8 ) | dat8[i+2];

+#ifdef OPJ_BIG_ENDIAN

```
                  v = swap16(v);  
```
+#endif

                  image->comps[1].data[index] = v;

manisandro · 2015-06-27T17:50:13Z

Just to clarify, I'm not involved in openjpeg development, so I'm not really in the position to review these patches. I'd encourage you to file pull requests, it really is pretty simple.

detonin · 2015-07-03T15:59:33Z

@szukw000 PR is indeed the preferred way to submit patches (pasting them here does not work as comments here support mardown language). There is a learning curve indeed but it's really worthwile. Thanks.

mayeut · 2015-07-17T22:40:25Z

@szukw000, @manisandro, could you test the master branch following #535 merge ? It should correct the big endianness issue for TIFF conversion.

⚠️ I only modified imagetotif & tiftoimage

mayeut · 2015-07-17T22:54:32Z

Related to PR #521

szukw000 · 2015-07-18T11:03:36Z

@mayeut, testing your changes in master-openjpeg-2015-07-18
on LINUX and PPC64 succeded.

I have made a small change for tiftoimage().

BUT: you have forgotten to apply the patch in #518.

The difference can bee seen in

http://home.arcor.de/szukw000/cio.txt.zip

winfried

--- converttif.c.orig 2015-07-18 02:11:45.902539565 +0000
+++ converttif.c 2015-07-18 04:38:05.481044832 +0000
@@ -796,7 +796,7 @@
opj_image_cmptparm_t cmptparm[4]; /* RGBA */
opj_image_t *image = NULL;
int has_alpha = 0;

unsigned short tiBps, tiPhoto, tiSf, tiSpp, tiPC;
unsigned short tiBps, tiPhoto, tiSf, tiSpp, tiPC, fails;
unsigned int tiWidth, tiHeight;
OPJ_BOOL is_cinema = OPJ_IS_CINEMA(parameters->rsiz);
tif_Xto32s cvtTifTo32s = NULL;
@@ -824,17 +824,20 @@
TIFFGetField(tif, TIFFTAG_PLANARCONFIG, &tiPC);
w= (int)tiWidth;
h= (int)tiHeight;

fails = 0;
if((tiBps > 16U) || ((tiBps != 1U) && (tiBps & 1U))) tiBps = 0U;
if(tiPhoto != PHOTOMETRIC_MINISBLACK && tiPhoto != PHOTOMETRIC_RGB) tiPhoto = 0;
if((tiBps > 16U) || ((tiBps != 1U) && (tiBps & 1U))) fails |= 1;

if((tiPhoto != PHOTOMETRIC_MINISBLACK)
&& (tiPhoto != PHOTOMETRIC_RGB)) fails |= 1;
if( !tiBps || !tiPhoto)
if(fails)
{
```
  if( !tiBps)
```

  if((tiBps > 16U) || ((tiBps != 1U) && (tiBps & 1U)))
    fprintf(stderr,"tiftoimage: Bits=%d, Only 1, 2, 4, 6, 8, 10, 12, 14 and 16 bits implemented\n",tiBps);

```
  else
```
```
      if( !tiPhoto)
```

          fprintf(stderr,"tiftoimage: Bad color format %d.\n\tOnly RGB(A)"

  if(tiPhoto != PHOTOMETRIC_MINISBLACK && tiPhoto != PHOTOMETRIC_RGB)

      fprintf(stderr,"tiftoimage: Bad color format %d.\n\tOnly RGB(A)"
                        " and GRAY(A) has been implemented\n",(int) tiPhoto);

fprintf(stderr,"\tAborting\n");

mayeut · 2015-07-20T05:26:44Z

@szukw000 ,

I updated the code to show proper error as suggested.
For the other patch, please see comments of @detonin on PR #521

szukw000 mentioned this issue Jun 27, 2015

Added OPJ_BIG_ENDIAN code in src/bin/jp2/convert.c for TIFF images with 16-Bit #524

Closed

mayeut added the bug label Jul 6, 2015

mayeut mentioned this issue Jul 17, 2015

Fix opj_write_bytes_BE (#518) #521

Merged

szukw000 mentioned this issue Jul 20, 2015

Update TIFF conversion to support more bit depth. #535

Merged

mayeut closed this as completed in 8c4afef Jul 25, 2015

mayeut added this to the OPJ v2.1.1 milestone Aug 27, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bigendian: opj_compress + opj_decompress fails #518

Bigendian: opj_compress + opj_decompress fails #518

manisandro commented Jun 24, 2015

sharkcz commented Jun 25, 2015

szukw000 commented Jun 25, 2015

manisandro commented Jun 25, 2015

szukw000 commented Jun 25, 2015

szukw000 commented Jun 26, 2015

szukw000 commented Jun 26, 2015

manisandro commented Jun 26, 2015

szukw000 commented Jun 27, 2015

szukw000 commented Jun 27, 2015

+#endif
dat8[i+6] = (unsigned char)a;
dat8[i+7] = (unsigned char)(a >> 8);
}
@@ -1825,7 +1830,9 @@
if(r > 65535) r = 65535; else if(r < 0) r = 0;
if(g > 65535) g = 65535; else if(g < 0) g = 0;
if(b > 65535) b = 65535; else if(b < 0) b = 0;

szukw000 commented Jun 27, 2015

szukw000 commented Jun 27, 2015

+#endif
dat8[i+6] = (unsigned char)a;
dat8[i+7] = (unsigned char)(a >> 8);
}
@@ -1825,7 +1830,9 @@
if(r > 65535) r = 65535; else if(r < 0) r = 0;
if(g > 65535) g = 65535; else if(g < 0) g = 0;
if(b > 65535) b = 65535; else if(b < 0) b = 0;

manisandro commented Jun 27, 2015

detonin commented Jul 3, 2015

mayeut commented Jul 17, 2015

mayeut commented Jul 17, 2015

szukw000 commented Jul 18, 2015

mayeut commented Jul 20, 2015

Bigendian: opj_compress + opj_decompress fails #518

Bigendian: opj_compress + opj_decompress fails #518

Comments

manisandro commented Jun 24, 2015

sharkcz commented Jun 25, 2015

szukw000 commented Jun 25, 2015

manisandro commented Jun 25, 2015

szukw000 commented Jun 25, 2015

szukw000 commented Jun 26, 2015

szukw000 commented Jun 26, 2015

Something has gone wrong here:

manisandro commented Jun 26, 2015

szukw000 commented Jun 27, 2015

szukw000 commented Jun 27, 2015

winfried

if(b > 65535) b = 65535; else if(b < 0) b = 0;

+#endif dat8[i+6] = (unsigned char)a; dat8[i+7] = (unsigned char)(a >> 8); } @@ -1825,7 +1830,9 @@ if(r > 65535) r = 65535; else if(r < 0) r = 0; if(g > 65535) g = 65535; else if(g < 0) g = 0; if(b > 65535) b = 65535; else if(b < 0) b = 0;

szukw000 commented Jun 27, 2015

szukw000 commented Jun 27, 2015

if(b > 65535) b = 65535; else if(b < 0) b = 0;

+#endif dat8[i+6] = (unsigned char)a; dat8[i+7] = (unsigned char)(a >> 8); } @@ -1825,7 +1830,9 @@ if(r > 65535) r = 65535; else if(r < 0) r = 0; if(g > 65535) g = 65535; else if(g < 0) g = 0; if(b > 65535) b = 65535; else if(b < 0) b = 0;

manisandro commented Jun 27, 2015

detonin commented Jul 3, 2015

mayeut commented Jul 17, 2015

mayeut commented Jul 17, 2015

szukw000 commented Jul 18, 2015

mayeut commented Jul 20, 2015

+#endif
dat8[i+6] = (unsigned char)a;
dat8[i+7] = (unsigned char)(a >> 8);
}
@@ -1825,7 +1830,9 @@
if(r > 65535) r = 65535; else if(r < 0) r = 0;
if(g > 65535) g = 65535; else if(g < 0) g = 0;
if(b > 65535) b = 65535; else if(b < 0) b = 0;

+#endif
dat8[i+6] = (unsigned char)a;
dat8[i+7] = (unsigned char)(a >> 8);
}
@@ -1825,7 +1830,9 @@
if(r > 65535) r = 65535; else if(r < 0) r = 0;
if(g > 65535) g = 65535; else if(g < 0) g = 0;
if(b > 65535) b = 65535; else if(b < 0) b = 0;