Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#7199 closed defect (fixed)

Overwriting PROJCS with "unnamed" in GTIFGetOGISDefn()

Reported by: molnar Owned by: warmerdam
Priority: normal Milestone: 2.3.0
Component: default Version: unspecified
Severity: normal Keywords:
Cc:

Description

I'm not 100% certain that this change https://github.com/OSGeo/gdal/commit/7e5107f98c719c3e27873440c36f2e71b7b75039 accomplishes its intent.

In the case where bTryGTCitationGeoKey is false, ie. this block has executed just above:

if( !STARTS_WITH_CI(szCTString, "LUnits = ") )
{
    oSRS.SetNode( "PROJCS",szCTString );
}

then do we really want to overwrite the "PROJCS" with "unnamed"?

It seems like it might be more appropriate to guard this block:

if( bTryGTCitationGeoKey &&
    GDALGTIFKeyGetASCII( hGTIF, GTCitationGeoKey, szCTString,
                         0, sizeof(szCTString) ) )
{
    if( !SetCitationToSRS( hGTIF, szCTString, sizeof(szCTString),
                           GTCitationGeoKey, &oSRS,
                           &linearUnitIsSet ) )
        oSRS.SetNode( "PROJCS", szCTString );
}
else
{
    oSRS.SetNode( "PROJCS", "unnamed" );
}

like so:

if( bTryGTCitationGeoKey )
{
    if( GDALGTIFKeyGetASCII( hGTIF, GTCitationGeoKey, szCTString,
                             0, sizeof(szCTString) ) )
    {
        if( !SetCitationToSRS( hGTIF, szCTString, sizeof(szCTString),
                               GTCitationGeoKey, &oSRS,
                               &linearUnitIsSet ) )
            oSRS.SetNode( "PROJCS", szCTString );
    }
    else
    {
        oSRS.SetNode( "PROJCS", "unnamed" );
    }
}

I don't have all the context here though, so I may be wrong. I'm just investigating some coordinate system changes after migrating to GDAL v2.2.3 from v2.1.2, and it's hard for me to say whether this is more correct or not.

Attachments (1)

repro_geotiffs.tar.gz (7.6 KB) - added by molnar 11 months ago.
Archive of repro GeoTIFF data

Download all attachments as: .zip

Change History (6)

comment:1 Changed 11 months ago by Even Rouault

Could you attach a dataset that would show an issue with that change ?

Changed 11 months ago by molnar

Attachment: repro_geotiffs.tar.gz added

Archive of repro GeoTIFF data

comment:2 Changed 11 months ago by molnar

Again, not to say whether it's right or wrong, but "PROJCS" is set to "Projection: Mercator; Datum: WGS84; Ellipsoid: WGS84" before the changes and "unnamed" after the changes.

I guess my question is this then:

if( !STARTS_WITH_CI(szCTString, "LUnits = ") )
{
    oSRS.SetNode( "PROJCS",szCTString );
}

can never have an effect, no? Since bTryGTCitationGeoKey will be false in that case, it will always be overwritten. Thus, there was no point in setting the "PROJCS" node in the first place if the following block:

if( bTryGTCitationGeoKey &&
    GDALGTIFKeyGetASCII( hGTIF, GTCitationGeoKey, szCTString,
                         0, sizeof(szCTString) ) )
{
    if( !SetCitationToSRS( hGTIF, szCTString, sizeof(szCTString),
                           GTCitationGeoKey, &oSRS,
                           &linearUnitIsSet ) )
        oSRS.SetNode( "PROJCS", szCTString );
}
else
{
    oSRS.SetNode( "PROJCS", "unnamed" );
}

is correct.

comment:3 Changed 11 months ago by Even Rouault

Resolution: fixed
Status: newclosed

In 41265:

GTiff: fix reading PCSCitationGeoKey (fixes #7199)

comment:4 Changed 11 months ago by Even Rouault

Milestone: 2.3.0

Indeed there was a logic error

comment:5 in reply to:  4 Changed 11 months ago by molnar

Replying to Even Rouault:

Indeed there was a logic error

Glad this was valuable then! Thanks for the prompt attention and fix

Note: See TracTickets for help on using tickets.