Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#3376 closed defect (fixed)

Code submission for NITF driver. Added CGM wirting option, text header copying and other minor fixes

Reported by: thchiou Owned by: warmerdam
Priority: normal Milestone: 1.8.0
Component: GDAL_Raster Version: svn-trunk
Severity: major Keywords: NITF, CGM
Cc: warmerdam

Description

At first, my apology for stuffing multiple fixes in one ticket. I have being writing a NITF viewer and editor using GDAL and OpenEV and I made some changes to fix some of the bugs and also added CGM writing feature to the GDAL NITF driver. With the tight schedule and waiting for GDAL ticket:3285 to be accepted, I didn’t get around to submit these changes in a timely faction. I have synchronize the changes with with GDAL trunk as of 1/26/2010 (after jpeg compression fix) and the test the resulted image with CIVA, NITF official validation tool.

Below is a summery of the changes:

  1. [Major] Text segment header copying.
    1. Description: Copy over text header when CreateCopy is called. Same as ticket:3285, please refer to that ticket for more detail. Ticket:3285 has not being integrated with GDAL trunk as of 02/02/2010
    2. Fix Location: nitfdataset.cpp 2676-2699, 4268-4356
  2. [Major] CGM writing support.
    1. Description: Added a “CGM” option and user can pass in back slash escaped CGM string separated by semicolon.
    2. Solution: Added a NITFWriteCGMSegment methods. Change create option, added a “CGM” tag.
    3. Fix Location: nitfdataset.cpp 4043-4151, nitffile.c, 486-495, 677-705
  3. [Minor] COMRAT warning fix.
    1. Description: Fix the error where a error indicating can “Unable to locate COMRAT to update in NITF header” if try to compress files with multiple image or graphic segments.
    2. Solution: original implementation hard coded the COMRAT location. Switched to Dynamically calculate the offset.
    3. Fix Location: nitfdataset.cpp 3974-4004
  4. [Minor] Dynamically calculate text segment offset.
    1. Text header location hardcoded, which result in error if an image has multiple images, graphics segment alone with text.
    2. Solution: Dynamically calculate the offset.
    3. Fix Location: 4162-4208 and 4380

Let me know if you have any question or anything I can help with: kevin.chiou@…

Attachments (9)

nitfdataset_fix.cpp (181.3 KB ) - added by thchiou 14 years ago.
New nitfdataset.cpp
nitffile_fix.c (65.0 KB ) - added by thchiou 14 years ago.
New nitffile.cpp
nitfdataset.patch (23.1 KB ) - added by thchiou 14 years ago.
patch file for nitfdataset.cpp
nitffile.patch (2.9 KB ) - added by thchiou 14 years ago.
patch file for nitffile.cpp
nitfdataset.cpp (192.5 KB ) - added by thchiou 14 years ago.
Add updates to supports create copy and CGM creation
nitffile.c (64.6 KB ) - added by thchiou 14 years ago.
Add updates to supports create copy and CGM creation
frmt_nitf_advanced.html (4.7 KB ) - added by thchiou 14 years ago.
Updates to the advance usage page.
nitffile.2.patch (2.4 KB ) - added by thchiou 14 years ago.
patch file for nitffile
nitfdataset.2.patch (32.5 KB ) - added by thchiou 14 years ago.
patch file for nitfdataset

Download all attachments as: .zip

Change History (20)

by thchiou, 14 years ago

Attachment: nitfdataset_fix.cpp added

New nitfdataset.cpp

by thchiou, 14 years ago

Attachment: nitffile_fix.c added

New nitffile.cpp

by thchiou, 14 years ago

Attachment: nitfdataset.patch added

patch file for nitfdataset.cpp

by thchiou, 14 years ago

Attachment: nitffile.patch added

patch file for nitffile.cpp

comment:1 by thchiou, 14 years ago

Summary: code submit for NITF driver. Added CGM wirting option, text header copying and other minor fixesCode submission for NITF driver. Added CGM wirting option, text header copying and other minor fixes

comment:2 by Even Rouault, 14 years ago

Cc: warmerdam added

Kevin,

I think your proposal for writing CGM segments is not very consistant with how we currently return them as metadata when reading them, such as documented in http://gdal.org/frmt_nitf_advanced.html ?

The NITF driver being a complex beast for both users and developers, I think we need as much consistency as possible, so I'd suggest you update your patch with the following principles :

  • maintain the NUMS creation option that can be used to reserve the necessary space in the header. This option should be just user provided, like the NUMI and NUMT values
  • change NITFWriteCGMSegments() to take a char papszList with the metadata of the CGM domain as done in the NITFWriteTextSegments(). The papszList would contain strings like SEGMENT_0_SLOC_ROW=25, SEGMENT_0_SLOC_COL=25, SEGMENT_0_SDLVL=2, SEGMENT_0_SALVL=1, SEGMENT_0_CCS_ROW=00025, SEGMENT_0_CCS_COL=00025, SEGMENT_0_DATA=\0!\0... . Of course, no need of SEGMENT_COUNT which would be deduced from the other metadata, and a test would be done to check that the number of segments passed through the metadata is consistant with the NUMS value (similarly to NITFWriteTextSegments())

CC'ing Frank so he can share his opinion.

comment:3 by warmerdam, 14 years ago

I agree with Even's suggestions.

comment:4 by Even Rouault, 14 years ago

Actually, similarly to what is done in the TEXT case (line 3566-3571 of nitfdataset_fix.cpp), if there is metadata in the CGM metadata domain, the NUMS value can be computed. That way, a CreateCopy() from a source NITF to a destination NITF will not require any extra creation option to be passed.

comment:5 by thchiou, 14 years ago

Thanks for the feedback. I will look into resolving those.

One reason I defined CGM data as a character array was because I need a CGM creation option and I couldn’t define a creation option that supports indexed metadata. By indexed metadata I mean a metadata that has a indexed like "Data_3"

For example,
If you can get text segment data back as following:
“DATA_0=message1”
“DATA_1=message2”

You cannot pass “DATA_0=some other message” as a create option, because GDAL option processor would not recognize that as a valid create option

Let me know if there is a way to accept indexed metadata as create option.

comment:6 by Even Rouault, 14 years ago

Yes, that's true you cannot properly adds the definition of index metadata as creation option. But, actually, we have already done that in the past for BLOCKA_*. When it finds a creation option with a star character, the creation option parser/checker will then accept any creation option starting with the string up to the star character.

But you could also pass them as metadata in the source dataset. Actually, that's how it works already with your proposed addition of the HEADER_xxx metadata for the TEXT segments. The advantage of this is consistency between the read & write part of the driver.

I can understand that setting metadata on the source dataset is not very convenient. A way of doing this without altering the source dataset is to use a temporary VRT file. That's actually what is done internally by gdal_translate when the user specifies metadata. gdal_translate wraps the source dataset into a VRT dataset, adds metadata to the VRT, and passes the VRT dataset as the source dataset of the CreateCopy() method. The only part missing currently in gdal_translate is a way to specify the metada *domain*. If you need adding CGM content with gdal_translate, we could extent the -mo option to accept the metadata domain, such as -mo CGM:SEGMENT_0_SLOC_ROW=25 (but I drop this idea for the following proposal)

Or maybe, we can make the 2 ways both accepted. The metadata approach is the most appropriate for NITF->NITF conversion; the creation option easier to use when creating from other format).

For example, any creation option starting with CGM=, like CGM=SEGMENT_0_SLOC_ROW=25 would be recognized by the NITF driver as being SEGMENT_0_SLOC_ROW=25 for the CGM metadata domain. The same for TEXT=. That wouldn't be very hard to implement in the NITF driver. And that would be consistant with how with specify the TRE content as creation options : "TRE=tre-name=tre-contents"

Frank, any ideas ?

comment:7 by thchiou, 14 years ago

Finally find myself sometime to work on the NITF drive.

Anyway, I am leaning toward implementing options with “CGM=SEGMENT_0_SLOC_ROW=25” style, since it’s more consistent with the TRE options.

However, I do need some inputs on one issue. How to handle CreateCopy with a NITF file that has existing CGM? Looks like text segment bypass the issue by not providing a creation option.

Here are some possible approaches:
1) Append new segments to the existing segments.
This is not good because user cannot remove or update existing CGM segment. Also, there is the question of how to deal with the case when user passed in the data for an existing segment.

2) Overwrite exisiting CGM segments
If users want to keep original CGM segments, they would have to combine the original CGM segment with the new CGM themselves.

I am leaning toward option 2 to just overwrite original CGM segment. Let me know what you think, so I can start implementing the changes.

comment:8 by Even Rouault, 14 years ago

If I've understood well your propositions, I think your option 2 would be the preferred one.

by thchiou, 14 years ago

Attachment: nitfdataset.cpp added

Add updates to supports create copy and CGM creation

by thchiou, 14 years ago

Attachment: nitffile.c added

Add updates to supports create copy and CGM creation

by thchiou, 14 years ago

Attachment: frmt_nitf_advanced.html added

Updates to the advance usage page.

by thchiou, 14 years ago

Attachment: nitffile.2.patch added

patch file for nitffile

by thchiou, 14 years ago

Attachment: nitfdataset.2.patch added

patch file for nitfdataset

comment:9 by thchiou, 14 years ago

Modified made as suggested.

  1. A NITF CreateCopy call will now copy over CGM segments.
  2. User can now passing CGM create option in following format:
    CGM=SEGMENT_COUNT=1
    CGM=SEGMENT_0_SLOC_ROW=25
    CGM=SEGMENT_0_SLOC_COL=25
    CGM=SEGMENT_0_SDLVL=2
    CGM=SEGMENT_0_SALVL=1
    CGM=SEGMENT_0_DATA=\0!\0...
    

Also, any CGM creation option will overwrite existing CGM data.

Let me know if anything else needs to be changed for this code submission to be accepted.

comment:10 by Even Rouault, 14 years ago

Milestone: 1.8.0
Resolution: fixed
Status: newclosed

Kevin,

I've commited your patch with a few fixes : formatting, a memory leak fix, a use of uninitialized memory. For consistency, I've also added the capability of writing TEXT segment as creation option.

You're welcome to test that it is still giving the results you expect.

Thanks!

r19250 /trunk/gdal/frmts/nitf/ (frmt_nitf_advanced.html nitfdataset.cpp nitffile.c): NITF: add capability of writing CGM segment as creation option (or from the source CGM metatada domain if no CGM= creation option); for consistency, also add the capability of writing TEXT segment as creation option, in addition to the existing capability of writing it from the source TEXT metadata domain (patch from Kevin Thchiou, #3376)

r19251 /trunk/autotest/gdrivers/nitf.py: NITF: test TEXT and CGM creation options (#3376)

comment:11 by Even Rouault, 14 years ago

r19264 /trunk/gdal/frmts/nitf/nitfdataset.cpp: NITF: allow use of TEXT= and CGM= creation options with Create() interface too

r19265 /trunk/autotest/gdrivers/nitf.py: NITF: test use of TEXT= and CGM= creation options with Create() interface too (#3376)

Note: See TracTickets for help on using tickets.