zero min/max, extent error

[hmzhu]

Zero min and max values Min and max values of source IDRISI dataset are modified to 0 after conversion is completed. For example, an IDRISI dataset cannot be displayed again due to zero min and max values after it ever was opened in ArcMap. It happens when the source dataset is opened with UPDATE access.

Extent error Wrong min.X/Y and max. X/Y values are produced upon completion of format conversion, which results in failure of correctly displaying the outputted RST file in IDRISI. Numerically there is an abstract error of 0.5 on values of min. X/Y and max. X/Y, and min.Y and max.Y are reverse. It occurs when the input dataset is in Tiff format without spatial reference system.

[hmzhu]

revised codes for IDRISI dataset driver

[warmerdam]

Hongmei,

As discussed by email, please provide either focused patches, or a complete driver modified from trunk which is substantially changed from 1.8.1.

Thanks

[Even Rouault]

A few additional remarks :

• the indentation should be done with 4 spaces and not tabulation character
• the "static int nIdrisiFlag" isn't thread safe. A potential solution would be to set an instance member on the IdrisiDataset* instance returned by GDALOpen() at the end of IdrisiDataset::Create().

[hmzhu]

Thank you for your suggestion! The indentation was created automatically within MSVC2010.But I still will pay attention to it later on. I use static variable because create() is a static function. I want to record if create() is called or not.

Replying to rouault:

A few additional remarks :

• the indentation should be done with 4 spaces and not tabulation character
• the "static int nIdrisiFlag" isn't thread safe. A potential solution would be to set an instance member on the IdrisiDataset* instance returned by GDALOpen() at the end of IdrisiDataset::Create().

[hmzhu]

I tested the trunk version. It has same bugs I reported.

Min and max values of SOURCE dataset still are modified to 0. They are held in the metadata file RDC file. The difference is the dataset can be correctly opened in ArcMap. This is because min and max are calculated on the fly due to the lack of XML file.But the metadata remains wrong.

[hmzhu]

I am incorporating my codes into the trunk version. Actually no need to have the static variable anymore.

[warmerdam]

I have applied the changes in trunk (r26033); however, I am somewhat hesitant to back port to 1.10 branch without more testing. +cc Ivan in case he has any thoughts on the changes.

I am investigating whether the ComputeStatistics is getting called sometimes in the IdsrisiDataset destructor when it should not.

[warmerdam]

OK, I'm quite confused by why the destructor always recomputes the statistics for the image. Is this intended to update them for the case that the application has changed something during the lifetime of the dataset?

If so, we should be setting a dirty flag on any raster write and using that as a clue to drive refreshes.

I'm leaving this ticket open till this is resolved.

[warmerdam]

Reviewing the code, I don't see why the old mechanism for keeping track of min/max during disk writes is not still being used in the close to write the RDC file. Could you explain? Otherwise, perhaps I should try to restore that.

[ilucena]

Frank,if I remember it correctly, these unconventional min/max computation was added into the drivers because of ArcMap visualization issues. An unconventional support for RAT was also added just to support Category Names in that same environment. I have no idea if all those special features are still needed and what else has evolved in between that format and software in relation to GDAL.

[hmzhu]

Hi Frank,

Let me answer the question why re-compute min/max in destructor?

Min/max values need to be set in RDC file when two situations happen:

1. After a new RST dataset is created by Create() or CreateCopy() because min/max values in newly created RDC file are default 0 then.
2. After Open() with UPDATE access mode, need to modify min/max values to avoid error min/max caused by potential changes of pixel values.

Setting min/max values was implemented in SetStatistics(), and was triggered in destructor when Ivan wrote the driver. The original purpose of this kind of design was good because client doesn't need to take care of writing min/max explicitly when calling GDAL. But in the old code the band's dfMinimum and dfMaximum don't hold the values they are supposed to hold. Instead, they just pass 0 to SetStatistics() in the destructor. This causes the 0 min/max error.

Solution: Compute min/max values when the two situations happen. Considering only in another situation, Open() with READONLY mode, min/max will remain untouched, I removed "if condition" in destructor to allow min/max always computed. This works well, and insures the minimum changes on the original code although it is not necessary for READONLY.

These modifications I made work well. We tested GDAL with revised Idrisi driver, all known bugs have been fixed. It will be included in Idrisi version 18. If you restore that, Idrisi raster datasets cannot be display in ArcGIS once closed. The bug will remain.

Best regards,

Hongmei

[Even Rouault]

Hongmei,

Implementing dirty flag as suggested by Frank shouldn't be very hard. Actually instead of lengthy discussion, I've taken a crack at it. See the attached patch (only compile tested). Please test. I also believe that all the parts in IWriteBlock() that compute min and max is completely useless now, since you call ComputeStatistics(). the fMinimum and fMaximum member variables are also likely obsolete.

[Even Rouault]

Implement dirty flag to only trigger ComputeStatistics() when needed

[hmzhu]

Hi rouault,

I tested the patch. The dirty flag works. But do you think it's really needed? In Create() and CreateCopy(), open mode is set to UPDATE. In Open(), access mode could be UPDATE or READONLY. So, eAccess can be used as flag. Modify something like this in destructor: if eAccess = GA_Update, then compute statistics and set min/max. This also achieves the goal that computing statistics when needed, although CreateCopy() actually doesn't need this step. Anyway, I also tested this idea, it works good. This is not a big change. I am not going to submit updates. After you make any changes, I can test it if you want. Or I may submit a patch after I know how to do it.

As for other parts like IWriteBlock and the two member variables, leave them there untouched. Maybe someone would like to call IWriteBlock() instead of RasterIO(). For the two variables, still keep them unless you want to delete them from very place they present.

Hongmei

[warmerdam]

Hongmei,

Many applications open files in update mode if they can even if no updates are applied. So I would not suggest you do a bunch of work for a file just because it was opened in update mode.

[hmzhu]

Frank,

I agree with you. However, my main concern is the integrity and correction of the metadata held in the RDC file. Correct min and max values are required in RDC file so that the raster can work correctly in Idrisi even if Idrisi provides function to recompute them when necessary, but not always.

Let me summarize the issues related to min/max values in the previous code(by the trunk I used):

1.When RST is source data

Open with update(sorry. yes, I have to say "update" again), min/max go to 0.

2.When RST is destination data

Create() doesn't refresh default zero min/max to correct values, neither is there a function exposed to client to do so in the driver, which results in 0 min/max.

To solve these, setting min/max in destructor without client's involvement is a reasonable practice because very likely a programmer even doesn't realize that he/she needs to write min/max when he/she converts raster with Create() at all. This is different from other raster formats.

I will refine the code recently according to your suggestion and our discussions, have it tested and submit updates. This version will be expected cleaner.

Thank you and rouault very much!

Hongmei

[Jukka Rahkonen]

If I understand right the issue was fixed like the end users at Clark Labs wanted it to be but GDAL developers would have liked to introduce a dirty flag for avoiding computation of statistics in certain cases.

Ticket contains a patch that was tested and accepted by hmzhu but it has not been applied to GDAL code in /trunk/gdal/frmts/idrisi/IdrisiDataset.cpp. Reopen the ticket, or perhaps create a new for clarity, if it feels like a good idea to utilize the patch.

[hmzhu]

They were fixed. I can see the updates in GDAL 1.10. No need to create a new one.