Skip to content

Commit

Permalink
Merge pull request #6369 from rouault/fix_6352
Browse files Browse the repository at this point in the history 
netCDF: handle variables of type NC_SHORT with _Unsigned=true as GDT_UInt16 (fixes #6352)
  • Loading branch information
@rouault
rouault authored Sep 16, 2022
2 parents 8881801 + b7cf47c commit d9abe2e
Show file tree
Hide file tree
Showing 5 changed files with 196 additions and 39 deletions.
Binary file added autotest/gdrivers/data/netcdf/short_as_unsigned.nc
View file
Binary file not shown.
63 changes: 63 additions & 0 deletions autotest/gdrivers/netcdf.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6075,6 +6075,69 @@ def test_netcdf_stats():
gdal.GetDriverByName("netCDF").Delete(filename)


###############################################################################


def test_netcdf_short_as_unsigned():
"""Test https://github.com/OSGeo/gdal/issues/6352"""

ds = gdal.Open("data/netcdf/short_as_unsigned.nc")
assert ds.GetRasterBand(1).DataType == gdal.GDT_UInt16
assert ds.GetRasterBand(1).GetMetadata() == {
"NETCDF_VARNAME": "Band1",
"_FillValue": "65535",
"_Unsigned": "true",
"valid_range": "{1,65533}",
}
# Values not in valid_range=[1,65533] are set to _FillValue=65535
assert struct.unpack("H" * 7, ds.GetRasterBand(1).ReadRaster()) == (
65532,
65533,
65535,
65535,
65535,
1,
2,
)
ds = None

ds = gdal.OpenEx(
"data/netcdf/short_as_unsigned.nc", open_options=["HONOUR_VALID_RANGE=NO"]
)
assert struct.unpack("H" * 7, ds.GetRasterBand(1).ReadRaster()) == (
65532,
65533,
65534,
65535,
0,
1,
2,
)
ds = None

shutil.copy("data/netcdf/short_as_unsigned.nc", "tmp/short_as_unsigned.nc")

ds = gdal.Open("tmp/short_as_unsigned.nc", gdal.GA_Update)
ds.GetRasterBand(1).WriteRaster(
0, 0, 7, 1, struct.pack("H" * 7, 2, 1, 0, 65535, 65534, 65533, 65532)
)
ds = None

ds = gdal.OpenEx("tmp/short_as_unsigned.nc", open_options=["HONOUR_VALID_RANGE=NO"])
assert struct.unpack("H" * 7, ds.GetRasterBand(1).ReadRaster()) == (
2,
1,
0,
65535,
65534,
65533,
65532,
)
ds = None

gdal.GetDriverByName("netCDF").Delete("tmp/short_as_unsigned.nc")


def test_clean_tmp():
# [KEEP THIS AS THE LAST TEST]
# i.e. please do not add any tests after this one. Put new ones above.
Expand Down
12 changes: 12 additions & 0 deletions autotest/gdrivers/netcdf_multidim.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2629,3 +2629,15 @@ def test_netcdf_multidim_var_alldatatypes_opened_twice():
rg2 = ds2.GetRootGroup()
assert rg2
assert rg2.OpenMDArray("string_var") is not None


def test_netcdf_multidim_short_as_unsigned():
"""Test https://github.com/OSGeo/gdal/issues/6352"""

ds = gdal.OpenEx("data/netcdf/short_as_unsigned.nc", gdal.OF_MULTIDIM_RASTER)
rg = ds.GetRootGroup()
var = rg.OpenMDArray("Band1")
assert var.GetDataType().GetNumericDataType() == gdal.GDT_UInt16
assert var.GetAttribute("valid_range").Read() == (1, 65533)
assert var.GetAttribute("_FillValue").Read() == 65535
assert struct.unpack("H" * 7, var.Read()) == (65532, 65533, 65534, 65535, 0, 1, 2)
138 changes: 101 additions & 37 deletions frmts/netcdf/netcdfdataset.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,26 @@ netCDFRasterBand::netCDFRasterBand( const netCDFRasterBand::CONSTRUCTOR_OPEN&,
}
}

bool bHasUnderscoreUnsignedAttr = false;
bool bUnderscoreUnsignedAttrVal = false;
{
char *pszTemp = nullptr;
if( NCDFGetAttr(cdfid, nZId, "_Unsigned", &pszTemp) == CE_None )
{
if( EQUAL(pszTemp, "true") )
{
bHasUnderscoreUnsignedAttr = true;
bUnderscoreUnsignedAttrVal = true;
}
else if( EQUAL(pszTemp, "false") )
{
bHasUnderscoreUnsignedAttr = true;
bUnderscoreUnsignedAttrVal = false;
}
CPLFree(pszTemp);
}
}

// Look for valid_range or valid_min/valid_max.

// First look for valid_range.
Expand Down Expand Up @@ -611,6 +631,26 @@ netCDFRasterBand::netCDFRasterBand( const netCDFRasterBand::CONSTRUCTOR_OPEN&,
}
}

if( bValidRangeValid &&
(adfValidRange[0] < 0 || adfValidRange[1] < 0) &&
nc_datatype == NC_SHORT &&
bHasUnderscoreUnsignedAttr &&
bUnderscoreUnsignedAttrVal )
{
if( adfValidRange[0] < 0 )
adfValidRange[0] += 65536;
if( adfValidRange[1] < 0 )
adfValidRange[1] += 65536;
if( adfValidRange[0] <= adfValidRange[1] )
{
// Updating metadata item
GDALPamRasterBand::SetMetadataItem("valid_range",
CPLSPrintf("{%d,%d}",
static_cast<int>(adfValidRange[0]),
static_cast<int>(adfValidRange[1])));
}
}

if (bValidRangeValid && adfValidRange[0] > adfValidRange[1])
{
CPLError(
Expand Down Expand Up @@ -641,26 +681,14 @@ netCDFRasterBand::netCDFRasterBand( const netCDFRasterBand::CONSTRUCTOR_OPEN&,
bSignedData = true;
}

// Fix nodata value as it was stored signed.
if( !bSignedData && dfNoData < 0 )
{
dfNoData += 256;
}

// If we got valid_range, test for signed/unsigned range.
// http://www.unidata.ucar.edu/software/netcdf/docs/netcdf/Attribute-Conventions.html
// https://docs.unidata.ucar.edu/netcdf-c/current/attribute_conventions.html
if( bValidRangeValid )
{
// If we got valid_range={0,255}, treat as unsigned.
if( adfValidRange[0] == 0 && adfValidRange[1] == 255 )
{
bSignedData = false;
// Fix nodata value as it was stored signed.
if( dfNoData < 0 )
{
dfNoData += 256;
}

// Reset valid_range.
bValidRangeValid = false;
}
Expand All @@ -673,24 +701,11 @@ netCDFRasterBand::netCDFRasterBand( const netCDFRasterBand::CONSTRUCTOR_OPEN&,
}
}
// Else test for _Unsigned.
// http://www.unidata.ucar.edu/software/netcdf/docs/BestPractices.html
// https://docs.unidata.ucar.edu/nug/current/best_practices.html
else
{
char *pszTemp = nullptr;
if( NCDFGetAttr(cdfid, nZId, "_Unsigned", &pszTemp) == CE_None )
{
if( EQUAL(pszTemp, "true") )
bSignedData = false;
else if( EQUAL(pszTemp, "false") )
bSignedData = true;
CPLFree(pszTemp);
}

// Fix nodata value as it was stored signed.
if( !bSignedData && dfNoData < 0 )
{
dfNoData += 256;
}
if( bHasUnderscoreUnsignedAttr )
bSignedData = !bUnderscoreUnsignedAttrVal;
}

if( bSignedData )
Expand All @@ -699,14 +714,48 @@ netCDFRasterBand::netCDFRasterBand( const netCDFRasterBand::CONSTRUCTOR_OPEN&,
// See http://trac.osgeo.org/gdal/wiki/rfc14_imagestructure
GDALPamRasterBand::SetMetadataItem("PIXELTYPE", "SIGNEDBYTE", "IMAGE_STRUCTURE");
}
else if( dfNoData < 0 )
{
// Fix nodata value as it was stored signed.
dfNoData += 256;
if( pszNoValueName )
{
// Updating metadata item
GDALPamRasterBand::SetMetadataItem(pszNoValueName,
CPLSPrintf("%d", static_cast<int>(dfNoData)));
}
}
}
else if( nc_datatype == NC_SHORT )
{
if( bHasUnderscoreUnsignedAttr )
{
bSignedData = !bUnderscoreUnsignedAttrVal;
if( !bSignedData )
eDataType = GDT_UInt16;
}

// Fix nodata value as it was stored signed.
if( !bSignedData && dfNoData < 0 )
{
dfNoData += 65536;
if( pszNoValueName )
{
// Updating metadata item
GDALPamRasterBand::SetMetadataItem(pszNoValueName,
CPLSPrintf("%d", static_cast<int>(dfNoData)));
}
}
}

#ifdef NETCDF_HAS_NC4
if( nc_datatype == NC_UBYTE ||
nc_datatype == NC_USHORT ||
nc_datatype == NC_UINT ||
nc_datatype == NC_UINT64 )
else if( nc_datatype == NC_UBYTE ||
nc_datatype == NC_USHORT ||
nc_datatype == NC_UINT ||
nc_datatype == NC_UINT64 )
{
bSignedData = false;
}
#endif

CPLDebug("GDAL_netCDF", "netcdf type=%d gdal type=%d signedByte=%d",
Expand Down Expand Up @@ -2086,6 +2135,11 @@ CPLErr netCDFRasterBand::CreateBandMetadata( const int *paDimIds,
if( status != NC_NOERR )
continue;

if( GetMetadataItem(szMetaName) != nullptr )
{
continue;
}

char *pszMetaValue = nullptr;
if( NCDFGetAttr(cdfid, nZId, szMetaName, &pszMetaValue) == CE_None )
{
Expand Down Expand Up @@ -2360,13 +2414,23 @@ bool netCDFRasterBand::FetchNetcdfChunk( size_t xstart,
nYChunkSize, false);
}
}
else if( eDataType == GDT_Int16 )
else if( nc_datatype == NC_SHORT )
{
status = nc_get_vara_short(cdfid, nZId, start, edge,
static_cast<short *>(pImageNC));
if( status == NC_NOERR )
CheckData<short>(pImage, pImageNC, edge[nBandXPos], nYChunkSize,
false);
{
if( eDataType == GDT_Int16 )
{
CheckData<GInt16>(pImage, pImageNC, edge[nBandXPos], nYChunkSize,
false);
}
else
{
CheckData<GUInt16>(pImage, pImageNC, edge[nBandXPos], nYChunkSize,
false);
}
}
}
else if( eDataType == GDT_Int32 )
{
Expand Down Expand Up @@ -2702,7 +2766,7 @@ CPLErr netCDFRasterBand::IWriteBlock( CPL_UNUSED int nBlockXOff,
status = nc_put_vara_uchar(cdfid, nZId, start, edge,
static_cast<unsigned char *>(pImage));
}
else if( eDataType == GDT_Int16 )
else if( nc_datatype == NC_SHORT )
{
status = nc_put_vara_short(cdfid, nZId, start, edge,
static_cast<short *>(pImage));
Expand Down
22 changes: 20 additions & 2 deletions frmts/netcdf/netcdfmultidim.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1884,7 +1884,24 @@ static bool BuildDataType(int gid, int varid, int nVarType,
else if( nVarType == NC_SHORT )
{
bPerfectDataTypeMatch = true;
eDataType = GDT_Int16;
char *pszTemp = nullptr;
bool bSignedData = true;
if( varid >= 0 && NCDFGetAttr(gid, varid, "_Unsigned", &pszTemp) == CE_None )
{
if( EQUAL(pszTemp, "true") )
bSignedData = false;
else if( EQUAL(pszTemp, "false") )
bSignedData = true;
CPLFree(pszTemp);
}
if( !bSignedData )
{
eDataType = GDT_UInt16;
}
else
{
eDataType = GDT_Int16;
}
}
else if( nVarType == NC_INT )
{
Expand Down Expand Up @@ -3292,7 +3309,8 @@ std::vector<std::shared_ptr<GDALAttribute>> netCDFVariable::GetAttributes(CSLCon
!EQUAL(szAttrName, CF_SCALE_FACTOR) &&
!EQUAL(szAttrName, CF_ADD_OFFSET) &&
!EQUAL(szAttrName, CF_GRD_MAPPING) &&
!(EQUAL(szAttrName, "_Unsigned") && m_nVarType == NC_BYTE)) )
!(EQUAL(szAttrName, "_Unsigned") &&
(m_nVarType == NC_BYTE || m_nVarType == NC_SHORT))) )
{
res.emplace_back(netCDFAttribute::Create(
m_poShared, m_gid, m_varid, szAttrName));
Expand Down

0 comments on commit d9abe2e

Please sign in to comment.