Opened 6 years ago

Closed 6 years ago

#4239 closed defect (fixed)

[PATCH] JP2KAK creates incorrect files on big-endian platforms

Reported by: jeepingben Owned by: warmerdam
Priority: normal Milestone: 1.9.0
Component: GDAL_Raster Version: unspecified
Severity: normal Keywords:
Cc:

Description

When creating a jp2 file using jp2kak in gdal on a big-endian platform, the box identifiers ('uuid', 'asoc', ' xml') are stored backwards in the file ('diuu', 'cosa', 'lmx '). This seems to be due to an unconditional byte-swap in GDALJP2Box::SetType?()

Attachments (3)

ticket_4239.patch (1.5 KB) - added by Even Rouault 6 years ago.
byte-sparc.jp2 (2.9 KB) - added by jeepingben 6 years ago.
Tif converted to JP2 on sparc
byte-solx86.jp2 (2.9 KB) - added by jeepingben 6 years ago.
Tif converted to jp2 on solaris-x86

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 years ago by Even Rouault

Actually the whole byte-swapping around szBoxType seem to be overly complicated, both in SetType?() and CreateAsocBox?(). It is not sane that SetType?() / GetType?() does not round-trip. It would be more appropriate that the user code of GetType?() does the byte-swapping if it wants to use the string as a numeric value, so JP2KAKWriteBox() likely needs to do CPL_MSBPTR32() on the result to preserve correct behaviour on little-endian platforms.

Could you try the attached patch and report ? I don't have access to Kakadu, so it would be nice if you could test the patch on a little-endian platform too to see if it hasn't broken it.

Changed 6 years ago by Even Rouault

Attachment: ticket_4239.patch added

comment:2 Changed 6 years ago by jeepingben

Thanks rouault.

I applied the patch to gdal-1.7.2 and it worked correctly for solaris-10 sparc and solaris-10 x86.

comment:3 Changed 6 years ago by Even Rouault

Summary: JP2KAK creates incorrect files on big-endian platforms[PATCH] JP2KAK creates incorrect files on big-endian platforms

jeepingben, before applying and as I cannot test myself, I'd like to be sure of the extent of testing you did. I'd suggest doing the follwing :

  • generate a jp2 file with JP2 boxes on the sparc and read it on the sparc and x86
  • generate a jp2 file with JP2 boxes on the x86 and read it on the sparc and x86

That way we are sure to have full interoperability between both endianness. Could you report the result of those tests? Could you also attach the produced files (as small as possible) to the ticket ? I'd like to be sure I can also read them with other jpeg2000 (non kakadu based) drivers. You can use as a source a small image like http://trac.osgeo.org/gdal/export/23074/trunk/autotest/gcore/data/byte.tif

Thanks

Changed 6 years ago by jeepingben

Attachment: byte-sparc.jp2 added

Tif converted to JP2 on sparc

Changed 6 years ago by jeepingben

Attachment: byte-solx86.jp2 added

Tif converted to jp2 on solaris-x86

comment:4 Changed 6 years ago by jeepingben

I've attached the 2 files. These were passed through the product I am working on which means they were converted to NITF, then to JPEG2000. Previously I tested by looking in an editor to be sure the box delimeters were written correctly. I also checked the autotests of the product I am working on and that showed that the jp2 images created with your patch were producing valid images on either endian.

Thanks again for the patch, Ben

comment:5 Changed 6 years ago by Even Rouault

Component: defaultGDAL_Raster
Milestone: 1.9.0
Resolution: fixed
Status: newclosed

Thanks for the reports. I also add to fix the ECW driver similarly to JP2KAK. I've looked at other potential users of GDALJP2Box::GetType?() in GDAL code and ECW and JP2KAK showed up as the only users, so that should be all for now. Anyway, this impact on drivers show clearly that it is definitely not material for a stable branch.

r23076 /trunk/gdal/ (3 files in 3 dirs): GDALJP2Box::SetType?() : remove byte-swapping so that SetType?()/GetType?() correctly round-trips. Consequently, use CPL_MSBPTR32() on GetType?() result in JP2KAK and ECW drivers. (#4239)

Note: See TracTickets for help on using tickets.