diff --git a/autotest/gdrivers/data/netcdf/short_as_unsigned.nc b/autotest/gdrivers/data/netcdf/short_as_unsigned.nc new file mode 100644 index 000000000000..73d56c1e3836 Binary files /dev/null and b/autotest/gdrivers/data/netcdf/short_as_unsigned.nc differ diff --git a/autotest/gdrivers/netcdf.py b/autotest/gdrivers/netcdf.py index 2a478200dbc1..c7cf28508663 100755 --- a/autotest/gdrivers/netcdf.py +++ b/autotest/gdrivers/netcdf.py @@ -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. diff --git a/autotest/gdrivers/netcdf_multidim.py b/autotest/gdrivers/netcdf_multidim.py index c4ebf06551fc..fae648764f73 100755 --- a/autotest/gdrivers/netcdf_multidim.py +++ b/autotest/gdrivers/netcdf_multidim.py @@ -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) diff --git a/frmts/netcdf/netcdfdataset.cpp b/frmts/netcdf/netcdfdataset.cpp index 1ff88cab11dd..2c8417d54587 100644 --- a/frmts/netcdf/netcdfdataset.cpp +++ b/frmts/netcdf/netcdfdataset.cpp @@ -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. @@ -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(adfValidRange[0]), + static_cast(adfValidRange[1]))); + } + } + if (bValidRangeValid && adfValidRange[0] > adfValidRange[1]) { CPLError( @@ -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; } @@ -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 ) @@ -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(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(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", @@ -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 ) { @@ -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(pImageNC)); if( status == NC_NOERR ) - CheckData(pImage, pImageNC, edge[nBandXPos], nYChunkSize, - false); + { + if( eDataType == GDT_Int16 ) + { + CheckData(pImage, pImageNC, edge[nBandXPos], nYChunkSize, + false); + } + else + { + CheckData(pImage, pImageNC, edge[nBandXPos], nYChunkSize, + false); + } + } } else if( eDataType == GDT_Int32 ) { @@ -2702,7 +2766,7 @@ CPLErr netCDFRasterBand::IWriteBlock( CPL_UNUSED int nBlockXOff, status = nc_put_vara_uchar(cdfid, nZId, start, edge, static_cast(pImage)); } - else if( eDataType == GDT_Int16 ) + else if( nc_datatype == NC_SHORT ) { status = nc_put_vara_short(cdfid, nZId, start, edge, static_cast(pImage)); diff --git a/frmts/netcdf/netcdfmultidim.cpp b/frmts/netcdf/netcdfmultidim.cpp index 4d2339b90b88..62248ca1de1b 100644 --- a/frmts/netcdf/netcdfmultidim.cpp +++ b/frmts/netcdf/netcdfmultidim.cpp @@ -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 ) { @@ -3292,7 +3309,8 @@ std::vector> 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));