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)
Change History (14)
by , 11 years ago
Attachment: | iris_driver.zip added |
---|
comment:1 by , 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.
by , 11 years ago
Attachment: | iris_driver.2.zip added |
---|
comment:3 by , 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 , 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 , 11 years ago
About 2), I've just changed the JDEM driver in r25143 to implement my suggestion. You can do the same.
by , 11 years ago
Attachment: | iris_data.zip added |
---|
comment:6 by , 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 , 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 , 11 years ago
Everything is working fine in my computer after your changes.
Thank you very much for you patience.
by , 11 years ago
comment:9 by , 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.
The code with an example CAPPI file