Opened 13 years ago

Closed 7 years ago

#2755 closed defect (fixed)

Omnibus Improvements for ESRI PE Compatability

Reported by: warmerdam Owned by: warmerdam
Priority: normal Milestone:
Component: OGR_SRS Version: 1.6.0
Severity: normal Keywords: esri hfa gtiff
Cc: gaopeng xzhou@…

Description

Attached please find the modified files for fix of SR in GDAL.

With this fix, the persist test should have no crash and no failure for all more than 3000 SRs for BMP, IMG and TIF. It also fixes some of bugs reported in 9.2 and 9.3. More test results will be got from other people.

The modified files from me are only ogr_srs_esri.cpp, ogr_srs_esri_names.h (new), hfadataset.cpp, gt_wkt_srs.cpp, geo_normalize.h, geo_normalize.c

Changes in geo_normalize.h and geo_normalize.c are minor, just to add standard geotif supported invFlatening value.

Some functions are added in gt_wkt_srs.cpp and hfadataset.cpp to write pe string, citation sting parse and translation, write citation for special needs. Following the meeting, I removed all private geokeys. Functions for name re-mapping, add parameters and delete parameters are added in ogr_srs_esri.cpp.

All other files keep the same as Frank sent for projectionX fix.

Frank, I managed EQUIRECTANGULAR (Plate_Carree) and EQUIDISTANT_CYLINDRICAL in ogr_srs_esri.cpp. It not only works for HFA, but also for tif and all other formats. Therefore your change for these two in HFA is not needed, I changed back to what it was.

Xiuguang

Attachments (2)

srFix.zip (143.7 KB) - added by warmerdam 13 years ago.
zip of fixed code.
genbindataset.cpp (28.1 KB) - added by esrixz 13 years ago.

Download all attachments as: .zip

Change History (19)

Changed 13 years ago by warmerdam

Attachment: srFix.zip added

zip of fixed code.

comment:1 Changed 13 years ago by warmerdam

Status: newassigned
Version: unspecified1.6.0

The gdal_datum.csv changes are handled separately in #2752.

The changes to geo_normalize.h and geo_normalize.cpp are being avoided by keeping the invFlattening value as a local variable.

comment:2 Changed 13 years ago by warmerdam

Xiuguang,

I'm attempting to integrate ogr_srs_esri.cpp changes. I have encountered a problem with out test suite with regard to Hotine_Oblique_Mercator_Azimuth_Center. In OGR WKT we use Hotine_Oblique_Mercator with a parameter named Azimuth, but the PE uses a projection name of Hotine_Oblique_Mercator_Azimuth_Center with a parameter named rectified_grid_angle. This appears to be related to bug #423.

There used to be code to do some translation between these forms but it does not seem to be operational after your changes. Can you explain why? I expect I'll integrate things as you have proposed for now, but we will need to resolve this situation.

comment:3 Changed 13 years ago by warmerdam

I have merged the updated ogr_srs_esri.cpp, but I have preserved the logic I recently introduced for handling Equidistant_Cylindrical so this area should continue to be reviewed for correctness. So far the change is only in trunk (r15992).

comment:4 Changed 13 years ago by warmerdam

I have integrated the GeoTIFF driver changes with a few minor changes (citation code moved to a new file - gt_citation.cpp). I do not understand the following change from this:

        adfParm[0] *= psDefn->UOMAngleInDegrees;
        adfParm[1] *= psDefn->UOMAngleInDegrees;
        adfParm[2] *= psDefn->UOMAngleInDegrees;
        adfParm[3] *= psDefn->UOMAngleInDegrees;
        
        adfParm[5] /= psDefn->UOMLengthInMeters;
        adfParm[6] /= psDefn->UOMLengthInMeters;

to this:

        if(!aUnitGot)
        {
          adfParm[0] *= psDefn->UOMAngleInDegrees;
          adfParm[1] *= psDefn->UOMAngleInDegrees;
          adfParm[2] *= psDefn->UOMAngleInDegrees;
          adfParm[3] *= psDefn->UOMAngleInDegrees; 
        }
        int unitCode = 0;
        GTIFKeyGet(hGTIF, ProjLinearUnitsGeoKey, &unitCode, 0, 1  );
        if(unitCode != KvUserDefined)
        {
          adfParm[5] /= psDefn->UOMLengthInMeters;
          adfParm[6] /= psDefn->UOMLengthInMeters;
        }

What is the purpose of the aUnitGot test? Is this to avoid a multiplication by 1.0 if the values are already degrees? Surely this does not introduce a rounding/precision problem!

The unitCode test seems wrong to me, and would appear to preclude appropriate adjustment for a user defined unit.

The changes are integrated in trunk (r15993).

comment:5 Changed 13 years ago by warmerdam

I neglected to incorporate ogr_srs_esri_names.h earlier - now in trunk (r15994).

comment:6 Changed 13 years ago by esrirasterXiu

Frank,

I just registered to be member of GDAL so that I can talk to you here.

The change is to support angular units that are not in degree. The current tif only uses degree. See the string "Write angular units, Always Degrees for now" in the file. "aUnitGot" means the non-degree angular unit is got from the citation string. Then we don't need to do the transformation of angular unit.

Xiuguang Zhou

comment:7 Changed 13 years ago by esrirasterXiu

Frank,

In addition, all my changes try not to block the old logic in hfa and tif. If aUnitGot is FALSE, it keeps the old logic.

Xiuguang

comment:8 Changed 13 years ago by warmerdam

I have integrated the HFA driver changes in trunk (r15995) with the following exceptions:

I have preserved my recently updated equidistant cylindrical / plate caree handling.

I removed the following addition to HFAGetBandNoData() which I did not understand (what should it accomplish?):

--- hfaopen.cpp (revision 15991)
+++ hfaopen.cpp (working copy)
@@ -587,6 +587,9 @@
 
     HFABand *poBand = hHFA->papoBand[nBand-1];
 
+    if (poBand->nOverviews > 0)
+      poBand = poBand->papoOverviews[0];
+
     *pdfNoData = poBand->dfNoData;
     return poBand->bNoDataSet;
 }
@@ -898,7 +901,7 @@
 }

comment:9 Changed 13 years ago by warmerdam

Keywords: hfa gtiff added

comment:10 Changed 13 years ago by Even Rouault

In r15997, I've fixed several potential stack overflows in gt_citation.cpp that could be triggered easily while reading corrupted/hostile GeoTIFFs, or when writing very long citations.

I've tested the changes as much as I could by creating some files at hand with my understanding of how the code works, but without reference GeoTIFFs. So, this should be carefully tested again. Ideally, we would also need some tests in the autotest suite for those new capabilities.

Side node, with the recent changes in the HFA driver, gcore/hfa_write.py and gdrivers/hfa.py fail on several tests.

comment:11 Changed 13 years ago by xz

Frank,

I reviewed equidistant cylindrical / plate caree issue. I don't know if the change I made in ogr_srs_esri.cpp is removed, your change still works for hfa and tif.

Thanks

Xiuguang

comment:12 Changed 13 years ago by warmerdam

I discovered that changes in HFADataset::ReadProjection?() had broken the ability to read coordinate systems from files with a mapinformation but no mapinfo node. Fixed in trunk (r16050), at least sufficiently for the test suite stuff in hfa.py to pass. But I am still suspicious that this logic "gives up" too easily. For instance, why is it a problem if the datum name is "Unknown"?

    if( !psDatum || !psPro ||
        (psMapInfo == NULL && poMapInformation == NULL) ||
        (strlen(psDatum->datumname) == 0 || EQUAL(psDatum->datumname, "Unknown")) && 
        (strlen(psPro->proName) == 0 || EQUAL(psPro->proName, "Unknown")) &&
        (psMapInfo && (strlen(psMapInfo->proName) == 0 || EQUAL(psMapInfo->proName, "Unknown"))) && 
        psPro->proZone == 0 )
    {
        pszProjection = CPLStrdup("LOCAL_CS[\"\"]");
        return CE_None;
    }

comment:13 Changed 13 years ago by esrixz

Frank,

I am still in my vacation.

I put the logic is to match the 9.2/9.3 for an IMG dataset. The dataset read from 9.2/9.3 is unknown coordinate system with empty datum/unit. But uing GDAL, ArcGis? gives some unpected text in the property page.

Could you send me/Gao all souce files that you changed? Thanks.

Xiuguang

Changed 13 years ago by esrixz

Attachment: genbindataset.cpp added

comment:14 Changed 13 years ago by esrixz

Frank,

There is a bug in Raw format. The inverse flatterning calculation is wrong in the file genbindataset.cpp. Please the find the fix in the attached file.

Inseat of 1.0 / (1.0 - dfSemiMajor/dfSemiMinor), it should be 1.0 / (1.0 - dfSemiMinor/dfSemiMajor).

Xiuguang

comment:15 Changed 13 years ago by warmerdam

Xiuguang,

I have applied your inverse flattening calculation correction in trunk (r16461) and 1.6 branch (r16462) for genbindataset.cpp. Thanks.

comment:16 Changed 12 years ago by warmerdam

I have restored the special handling of rectified_grid_angle parameter for HOM removed in r15992. I t is restored in trunk (r16696). With this change the osr_esri.py test script passes again.

comment:17 Changed 7 years ago by Even Rouault

Resolution: fixed
Status: assignedclosed

Closing in the hope all issues have been addressed...

Note: See TracTickets for help on using tickets.