Opened 4 months ago

Closed 4 months ago

Last modified 4 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 4 months ago.
Archive of repro GeoTIFF data

Download all attachments as: .zip

Change History (6)

comment:1 Changed 4 months ago by Even Rouault

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

Changed 4 months ago by molnar

Attachment: repro_geotiffs.tar.gz added

Archive of repro GeoTIFF data

comment:2 Changed 4 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 4 months ago by Even Rouault

Resolution: fixed
Status: newclosed

In 41265:

GTiff: fix reading PCSCitationGeoKey (fixes #7199)

comment:4 Changed 4 months ago by Even Rouault

Milestone: 2.3.0

Indeed there was a logic error

comment:5 in reply to:  4 Changed 4 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.