Overwriting PROJCS with "unnamed" in GTIFGetOGISDefn()

[molnar]

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.

[Even Rouault]

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

[molnar]

Archive of repro GeoTIFF data

[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.

[Even Rouault]

In 41265:

GTiff: fix reading PCSCitationGeoKey (fixes #7199)

[Even Rouault]

Indeed there was a logic error

[molnar]

Replying to Even Rouault:

Indeed there was a logic error

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