Patch to read GML geo metadata and other associated data for inclusio… #1110

HWiman · 2018-03-22T12:38:14Z

Referring to Issue #1109, this is the requested pull request.

We need the GML meta data stored in some JP2 files as described here.

There was no reader for ASOC, LBL or XML boxes so I have added that (v2.3.0). I hope you will consider adding these changes to future versions of openjpeg.Please let me know if I can assist in getting this into openjpeg.

The patches add a new opj_jp2_asoc_t type. An opj_jp2 decoder has numasoc number of such instances in asoc. For retrieval on client side, these data are copied to opj_codestream_info_v2_t for direct access and can be dumped using opj_dump_associated_data( opj_codestream_info_v2_t cstr_info, FILE *output_stream ).

…n in openjpeg

HWiman · 2018-03-22T12:50:27Z

Sample image file with GML XML data as defined in the GML standard.
T33VWD_20170127T102301_B10.zip

Result of opj_dump -i T33VWD_20170127T102301_B10.jp2

dump.txt

rouault · 2018-03-22T12:44:25Z

src/lib/openjp2/j2k.c

@@ -10442,6 +10442,9 @@ opj_codestream_info_v2_t* j2k_get_cstr_info(opj_j2k_t* p_j2k)
    if (!cstr_info) {
        return NULL;
    }
+
+	cstr_info->nbasoc = 0;


Indentation issue. Please build with cmake -DWITH_ASTYLE=ON and run scripts/prepare-commit.sh

I just did that, but with no effect. Is the prepare-commit.sh Windows-compatible?

Hum probably not... Maybe under Cygwin ? Anyway on already committed code, this has no effect. You can use scripts/astyle.sh your.c to fix an existing committed file

I did not find an exact match of astyle in Code::Blocks. The closest I got was Kernighan&Ritchie but had to make small edits in source files manually. I guess the
build system checks if this will pass?

Latest push clearly changed everything. My attempt with astyle failed completely.

I am afraid I need your assistance to format the committed six files since it can not be done on Windows. I compiled on a remote Linux machine WITH_ASTYLE, but it does not have C++11 and I am not authorized to install gcc. Can you please run astyle.sh on the commited files?

OK, I've run astyle. Please pull from rouault:associated_data_support to get rouault@713e131

rouault · 2018-03-22T12:45:34Z

src/lib/openjp2/jp2.c

+    assert(jp2 != 00);
+    assert(p_header_data != 00);
+    assert(p_manager != 00);
+


you should check p_header_size is at least 8 bytes long here

rouault · 2018-03-22T12:47:15Z

src/lib/openjp2/jp2.c

+    asoc = &(jp2->asoc[jp2->numasoc-1]);
+    asoc->level = jp2->numasoc-1; /* TODO: This is not correct if a parent asoc contains multiple child asocs! */
+    asoc->label_length = asoc_size;
+    asoc->label = opj_malloc(asoc_size);


would be safer for users to allocate asoc_size + 1 and nul terminate asoc->label[asoc_size]

rouault · 2018-03-22T12:47:37Z

src/lib/openjp2/jp2.c

+    asoc = &(jp2->asoc[jp2->numasoc-1]);
+    asoc->level = jp2->numasoc-1; /* TODO: This is not correct if a parent asoc contains multiple child asocs! */
+    asoc->label_length = asoc_size;
+    asoc->label = opj_malloc(asoc_size);


you should check asoc->label is not null

asoc->label is always uninitialized since it is created a few lines above.

rouault · 2018-03-22T12:48:20Z

src/lib/openjp2/jp2.c

+    p_header_data += asoc_size;
+    p_header_size -= asoc_size;
+
+    opj_read_bytes(p_header_data,&asoc_tag,4);


ypi sjpimd check that there's at least 4 remaining bytes

rouault · 2018-03-22T12:50:46Z

src/lib/openjp2/jp2.c

+        /* Copy the unknown data for external handling.
+        NOTE: This is not tested, but does the same as if an XML tag was found.*/
+          asoc->xml_len = p_header_size;
+          asoc->xml_buf  = opj_malloc(p_header_size);


same as above

rouault · 2018-03-22T12:51:38Z

src/lib/openjp2/jp2.c

+        to_asoc->level = asoc->level;
+        to_asoc->label_length = asoc->label_length;
+        to_asoc->xml_len = asoc->xml_len;
+        to_asoc->label = opj_malloc( to_asoc->label_length );


null pointer checks needed

rouault · 2018-03-22T12:52:32Z

src/lib/openjp2/openjpeg.c

@@ -1049,7 +1093,12 @@ opj_stream_t* OPJ_CALLCONV opj_stream_create_file_stream(

    return l_stream;
 }
-
+
+OPJ_OFF_T opj_stream_skip_api(opj_stream_t * p_stream, OPJ_OFF_T p_size)


this is unrelated to this pull request AFAICS. Better put that in another one

Indeed, sorry! Some files (like NITF) encapsulate JP2 in a file, so we need to offset the start of file reading. I did not find any way to do that so added this method.

This is a bit self-promotion, but you are probably aware of the GDAL library (http://gdal.org/) that takes care of all this geo stuff already and deals with NITF.

Yes, that´s an impressing and complete library! We do all our software in Java so it would require a lot of JNI bindings and we have implemented almost all image and sensor support we need already, this GML parsing being one of few missing parts.

rouault · 2018-03-22T12:52:56Z

src/lib/openjp2/openjpeg.h

@@ -1314,9 +1345,6 @@ OPJ_API OPJ_BOOL OPJ_CALLCONV opj_setup_decoder(opj_codec_t *p_codec,
 * number, or "ALL_CPUS". If OPJ_NUM_THREADS is set and this function is called,
 * this function will override the behaviour of the environment variable.
 *
- * Currently this function must be called after opj_setup_decoder() and


why has this comment been removed ?

Unintentional mistake. Re-inserted.

rouault · 2018-03-22T12:55:44Z

src/lib/openjp2/openjpeg.h

@@ -943,6 +959,12 @@ typedef struct opj_codestream_info_v2 {
    /** information regarding tiles inside image */
    opj_tile_info_v2_t *tile_info; /* FIXME not used for the moment */

+  /** Number of associated data boxes*/
+  OPJ_UINT32 nbasoc;


technically GMLJP2 boxes are JP2 level info, not codestream-level, so it is a bit weird to put them there. @detonin any better idea of a more appropriate structure ?

Commented dump asoc function.

HWiman · 2018-03-23T07:49:51Z

I get fails that I do not understand:

The following tests FAILED:
1401 - NR-DEC-issue226.j2k-74-decode (Failed)
1402 - NR-DEC-issue226.j2k-74-decode-md5 (Failed)
and:
1012.2
Compiler: clang-3.8 C++ OPJ_CI_CC=clang-3.8 OPJ_CI_CXX=clang-3.8 OPJ_CI_CHECK_STYLE=1 OPJ_CI_SKIP_TESTS=1 5 min 4 sec
1012.11
Compiler: g++-4.8 C++ OPJ_CI_CC=gcc-4.8 OPJ_CI_CXX=g++-4.8 OPJ_CI_ABI_CHECK=1 2 min 32 sec

What action should I take for these?

HWiman · 2018-03-23T07:51:27Z

Except from fails and the potential move of associated data from code stream structure, I think the review is finished. Thanks a lot for taking the time!

rouault · 2018-04-07T17:03:55Z

@HWiman Can you pull from from rouault:associated_data_support to get rouault/openjpeg@713e131 which should fix indentation issues ?

Run astyle

HWiman · 2018-04-11T09:52:32Z

Accepted pull request from @rouault .

rouault · 2018-04-11T11:31:44Z

@detonin Are you OK with the addition of the new members at the bottom of opj_codestream_info_v2_t ?

+    /** Number of associated data boxes*/
+    OPJ_UINT32 nbasoc;
+
+    /** Associated data, e.g. GML geoinformation */
+    opj_jp2_asoc_t *asoc_info;
+

I had a semantic observation that GMLJP2 boxes are at the JP2 level and not the codestream one, but I'm not sure of a better place where that could be stored ?
Or perhaps add a dedicated function opj_get_jp2_asoc_boxes() ? On the reflexion that might be the best solution. @HWiman would you be willing to do such change ?

HWiman · 2018-04-11T13:25:15Z

@rouault I can change to a opj_get_jp2_asoc_boxes() function if that is preferred. I'll wait for @detonin and your decision, though.

detonin · 2018-04-11T14:39:38Z

@rouault Indeed this kind of geoinformation needs to be at JP2 level and certainly not in the J2K codestream. The dedicated function you propose seems to be a good option. I guess it means that we will have to bump the minor version in the next release (as we add function in the API).

HWiman · 2018-04-12T06:17:50Z

There is an unused opj_jp2_metadata struct. Would you prefer that I put the asoc boxes there and implement the declared opj_get_jp2_metadata or would you prefer that I create a specific OPJ_API opj_jp2_asoc_t* OPJ_CALLCONV opj_get_jp2_asoc_boxes( opj_codec_t *p_codec, OPJ_UINT32 *num_asoc );?

rouault · 2018-04-18T13:59:52Z

I'm fine with you implementing opj_get_jp2_metadata() and using opj_jp2_metadata. Could you add a comment in the doc of the opj_jp2_metadata struct to mention that it should never be allocated by calling code, and only used as the return of opj_get_jp2_metadata() (so we can add fields without breaking the ABI). There will probably be a need for a cleanup function to release allocated memory ?

- Fixed bug in allocation of asoc data (length was set one byte larger than allocated)

…penjpeg into associated_data_support

HWiman · 2018-05-28T12:54:07Z

Hi, I finally implemented and pushed changes which put asoc data in the opj_jp2_metadata struct as agreed. Possibly the code needs to be reformatted again? If so, can I ask you, Evan, to do it for me like last time since I do not have the tools? I´ll merged it into my branch according to your last message when you´re done, like last time: @HWiman <https://github.com/HWiman> Can you pull from from rouault:associated_data_support to get rouault/openjpeg@713e131 <rouault@713e131> which should fix indentation issues ? Cheers, Håkan

…

On Wed, Apr 18, 2018 at 3:59 PM, Even Rouault ***@***.***> wrote: I'm fine with you implementing opj_get_jp2_metadata() and using opj_jp2_metadata. Could you add a comment in the doc of the opj_jp2_metadata struct to mention that it should never be allocated by calling code, and only used as the return of opj_get_jp2_metadata() (so we can add fields without breaking the ABI). There will probably be a need for a cleanup function to release allocated memory ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1110 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Aj1kMAPR9GC3v8gIFPu5juXxiXd6D9v1ks5tp0bagaJpZM4S2-IT> .

rouault · 2018-06-16T12:35:28Z

@HWiman I've run astyle on your changes and committed the reformat in rouault@14fd14e
I'm wnodering why they are failures on NR-DEC-issue226.j2k-74-decode test specifically on Windows builds though

rouault · 2018-06-16T13:19:13Z

I've also fixed memory leaks per rouault@e984dfc

rouault · 2018-08-03T18:49:17Z

@HWiman Can you pull my changes from https://github.com/rouault/openjpeg/tree/associated_data_support to update this PR ?

emiax · 2019-10-04T12:38:25Z

Hi @HWiman and @rouault,
This seems like a great addition to the openjpeg, and I'd love to use it in our open source project. I see that there has not been any update here since Aug 2018. Are there any plans to continue this work? Is there anything I can do to help? Thank you

Patch to read GML geo metadata and other associated data for inclusio…

53df2bf

…n in openjpeg

rouault requested changes Mar 22, 2018

View reviewed changes

HWiman added 5 commits March 22, 2018 13:56

Example how to dump associated data, if there is any.

2c8e3b4

Fixes after code review by rouault.

dfcb751

Fixed indentation using astyle=Kernighan&Ritchie

64a8e4e

Eoln set to LF only (not CR-LF)

ca773e4

Reinserted comment that was "lost in translation"

4dc8a20

Commented dump asoc function.

Run astyle

713e131

Merge pull request #1 from rouault/associated_data_support

ae71bb8

Run astyle

HWiman added 3 commits May 28, 2018 13:52

- Moved ASOC data to opj_jp2_metadata_t

34801b4

- Fixed bug in allocation of asoc data (length was set one byte larger than allocated)

Merge branch 'associated_data_support' of https://github.com/HWiman/o…

ec4aec1

…penjpeg into associated_data_support

- Always return metadata struct, but set asoc to null iff j2k.

58c19b6

radarhere mentioned this pull request Apr 6, 2024

Enhancement request: JPEG2000 w/ metadata python-pillow/Pillow#3048

Open

Patch to read GML geo metadata and other associated data for inclusio… #1110

Are you sure you want to change the base?

Patch to read GML geo metadata and other associated data for inclusio… #1110

Conversation

HWiman commented Mar 22, 2018

HWiman commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HWiman commented Mar 23, 2018

HWiman commented Mar 23, 2018

rouault commented Apr 7, 2018

HWiman commented Apr 11, 2018

rouault commented Apr 11, 2018

HWiman commented Apr 11, 2018

detonin commented Apr 11, 2018

HWiman commented Apr 12, 2018

rouault commented Apr 18, 2018

HWiman commented May 28, 2018 via email

rouault commented Jun 16, 2018

rouault commented Jun 16, 2018

rouault commented Aug 3, 2018

emiax commented Oct 4, 2019

HWiman commented Mar 22, 2018 •

edited

Loading