Opened 11 years ago

Closed 11 years ago

#4854 closed enhancement (fixed)

New IRIS Format driver

Reported by: rveciana Owned by: warmerdam
Priority: normal Milestone: 1.10.0
Component: GDAL_Raster Version: unspecified
Severity: normal Keywords: IRIS
Cc:

Description

As I told at the mailing list: http://lists.osgeo.org/pipermail/gdal-dev/2012-August/033737.html I've developed a driver for the IRIS format. I send the code attached with an example file.

Attachments (4)

iris_driver.zip (42.9 KB ) - added by rveciana 11 years ago.
The code with an example CAPPI file
iris_driver.2.zip (24.7 KB ) - added by rveciana 11 years ago.
iris_data.zip (10.5 KB ) - added by rveciana 11 years ago.
test.zip (18.2 KB ) - added by rveciana 11 years ago.

Download all attachments as: .zip

Change History (14)

by rveciana, 11 years ago

Attachment: iris_driver.zip added

The code with an example CAPPI file

comment:1 by Even Rouault, 11 years ago

Thanks for your contribution. I've reviewed the code, which looks good overall. Here are my comments :

1) There are a lot of *(int *)(stuff), or *(short*)(stuff) occurences in the code when reading the header. This will only work on Intel-based (little-endian) machines. We have portability macros in GDAL to address this. You can use instead CPL_LSBINT32PTR(stuff) or CPL_LSBINT16PTR(stuff) that will work on little-endian and big-endian machines.

2) The logic around lines 145-155 to avoid retrying a previously failed memory allocation is good, but doing that by altering nBlockXSize is a bit weird and might cause issues to client code (nBlockXSize isn't supposed to change during the dataset object life). Setting a boolean flag would be a more standard approach to address this.

3) The poTransform allocated at line 372 should be freed with OGRCoordinateTransformation::DestroyCT()

4) There's a memory leak in the use of oSRSOut.exportToWkt(). GetProjectionRef() returns a string that must not be freed by the caller, hence the leak. One way to fix that is to have pszSRS_WKT as a member of the dataset object, and free it in the dataset destructor. And check it at the beginning of GetProjectionRef() to return it directly if it is not NULL.

5) A help topic is declared to "frmt_various.html#IRIS", but you didn't include additional content in frmts/frmt_various.html to describe a bit what the driver does.

6) Can the BCN120419144822.CAPNRN7 file be freely redistribuable to be included in the GDAL autotest suite ?

7) If you have some Python knowledge, providing a test in the GDAL autotest suite would be appropriate. That would be a autotest/gdrivers/iris.py file to create. Even without deep Python knowledge, you could easily take inspiration from existing tests for other drivers.

comment:2 by rveciana, 11 years ago

Thank you Roualt, I'll try to change all the code as soon as I can.

by rveciana, 11 years ago

Attachment: iris_driver.2.zip added

comment:3 by rveciana, 11 years ago

I will answrer the points you posted in the same order:

1.-OK, I've changed all the *(int *)(stuff) and *(short*)(stuff), but I still have many *(float) *, *(unsigned int) and *(unsigned short). I can't find the correspondent macros at port/cpl_port.h. Do they exist, or it's better to manage these situations in my own code? 2.-I copied the idea from the JDEM example driver, where they do the same using the nRecordSize variable as follows:

if (pszRecord == NULL) {

CPLError(CE_Failure, CPLE_OutOfMemory,

"Cannot allocate scanline buffer");

nRecordSize = -1; return CE_Failure;

}

I though that setting the value to -1 was useful someplace, but I don't check it actually. It's better simply to remove it?

3.-Done, I forgotted it...

4.-I have followed the approach that you suggested. I declare the variable as a member and check if is null before calculating it. I haven't undestood the destructor part. Do I have to do it or GDAL manages that?

5.- I have attached the part that could be included in the frmt_various.html file

6.- I have asked for a written official permission so the file can be used without any problem. I won't put any file without this permission. 7.- I include the test and example file. I've checked it with the example file I'm waiting to confirm. Many of the examples just run a checksum. Is this ok?

I attach the source code with the changes and without the example file.

Roger

comment:4 by Even Rouault, 11 years ago

1) Hum, actually the macros will return an unsigned type. It is up to you to cast it to int or short for having signed values. See the below test program and the output I got

#include <stdio.h>

typedef unsigned char GByte;

#define CPL_LSBINT16PTR(x)    ((*(GByte*)(x)) | ((*(GByte*)((x)+1)) << 8))

#define CPL_LSBINT32PTR(x)    ((*(GByte*)(x)) | ((*(GByte*)((x)+1)) << 8) | \
                              ((*(GByte*)((x)+2)) << 16) | ((*(GByte*)((x)+3)) << 24))

int main(int argc, char* argv[])
{
    GByte buf[4] = { 0xff, 0xff, 0xff, 0xff };
    printf("%u\n", (unsigned int)CPL_LSBINT32PTR(buf));
    printf("%d\n", (int)CPL_LSBINT32PTR(buf));
    printf("%d\n", (unsigned short)CPL_LSBINT16PTR(buf));
    printf("%d\n", CPL_LSBINT16PTR(buf));
    printf("%d\n", (short)CPL_LSBINT16PTR(buf));
    return 0;
}

4294967295
-1
65535
65535
-1

For the sake of clarity, I'd suggest to introduce the following macros in cpl_port.h to remove any ambiguity about the signedness and use them :

#define CPL_LSBSINT16PTR(x) ((GInt16) CPL_LSBINT16PTR(x))
#define CPL_LSBUINT16PTR(x) ((GUInt16)CPL_LSBINT16PTR(x))
#define CPL_LSBSINT32PTR(x) ((GInt32) CPL_LSBINT32PTR(x))
#define CPL_LSBUINT32PTR(x) ((GUInt32)CPL_LSBINT32PTR(x))

As far as (float *) is concerned, the only occurences I see in your code are in IReadBlock() when setting the values of the block buffer. It is a totally different case, since you are not reading a float from a file, but assigning a memory buffer of type float* with a float value, so there's nothing particular to do. The code is just fine like that.

2) nRecordSize is actually tested at line 146 "if (nBlockXSize < 0)". I actually see that it is done like that in the JDEM driver, which I find not very nice. The irony being that I'm the author of the changeset that introduced that !!! ( http://trac.osgeo.org/gdal/changeset/16256 ). Shame on me. A dedicated boolean that is set when a failed allocation has occured, and tested instead of if (nBlockXSize < 0 ) would be much nicer.

3) OK

4) You have declared the pszSRS_WKT as a member variable of IRISDataset, so it is up to you to initialize it to NULL in the constructor of IRISDataset, and to CPLFree() it in the destructor. GDAL is powerful, but does no perform (yet) any magic ;-)

5) I haven't seen the documentation in the attached zip file

6 and 7) OK

comment:5 by Even Rouault, 11 years ago

About 2), I've just changed the JDEM driver in r25143 to implement my suggestion. You can do the same.

by rveciana, 11 years ago

Attachment: iris_data.zip added

comment:6 by rveciana, 11 years ago

Well, I have added a new file (irid_data.zip, I put a wrong name) with the changes:

1.- I have changed the file with your suggestion, so the file cpl_port.h must be changed to support the new macros. I agree that is much clear for everyone.

If changing cpl_port.h is a problem, I can generate the file, but without the need for the new macros.

2.- Yes, I "tributed" the JDAL driver to code my own. So I have used the same solution than you now.

4.- Ok, this time I think it's ok. I set to NULL at the constructor and call CPLFree at the destructor. I am a bit slow understanding this kind of things.

5.- It's true, now the file should be there.

I'll post the example file as soon as I can, or at least, find a solution.

comment:7 by Even Rouault, 11 years ago

Milestone: 2.0.0

I've commited your latest code in trunk in r25150. I've made a few reformatting to avoid huge lines, and also added some tests to validate some values read to avoid reading outside arrays in case the data is corrupted/hostile. Please test that it works for you.

I've left aside the testing part until you have cleared that up.

comment:8 by rveciana, 11 years ago

Everything is working fine in my computer after your changes.

Thank you very much for you patience.

by rveciana, 11 years ago

Attachment: test.zip added

comment:9 by rveciana, 11 years ago

Finally, I got the written permission to use and re-distribute an example IRIS file. I have attached it with the test that works for this file.

comment:10 by Even Rouault, 11 years ago

Resolution: fixed
Status: newclosed

Test added in r25175.

Note: See TracTickets for help on using tickets.