jiayuasu commented on code in PR #3312:
URL: https://github.com/apache/sedona/pull/3312#discussion_r3926861747
##########
common/src/main/java/org/apache/sedona/common/raster/RasterOutputs.java:
##########
@@ -62,6 +63,19 @@ public static byte[] asGeoTiff(
throw new RuntimeException(e);
}
ParameterValueGroup defaultParams =
writer.getFormat().getWriteParameters();
+ // Without this, GeoTiffWriter writes a default GDAL_NODATA of 0 for
coverages that
+ // have no no-data value, so a cleared (or never set) no-data value comes
back as 0
+ // when the written bytes are read again.
+ boolean hasNoDataValue = false;
+ for (int band = 1; band <= raster.getNumSampleDimensions(); band++) {
Review Comment:
I may be missing a GeoTIFF constraint here, but this still looks lossy for
mixed-band metadata. I reproduced it with a two-band GeoTIFF where both bands
start with `-999`:
- clear band 2: `(-999, null)` in memory, `(-999, -999)` after
`RS_FromGeoTiff(RS_AsGeoTiff(...))`
- clear band 1: `(null, -999)` in memory, `(null, null)` after the same
round trip
GeoTools writes one dataset-wide NoData value derived from band 1, so
`hasNoDataValue` cannot encode either heterogeneous state. Would it make sense
to reject this state or use a lossless representation, and add both cases to
the round-trip test?
##########
common/src/main/java/org/apache/sedona/common/raster/RasterBandEditors.java:
##########
@@ -53,14 +55,22 @@ public static GridCoverage2D setBandNoDataValue(
// Remove no-Data if it is null
if (noDataValue == null) {
- if (RasterBandAccessors.getBandNoDataValue(raster) == null) {
+ if (rasterNoData == null) {
return raster;
}
GridSampleDimension[] sampleDimensions = raster.getSampleDimensions();
sampleDimensions[bandIndex - 1] =
RasterUtils.removeNoDataValue(sampleDimensions[bandIndex - 1]);
- return RasterUtils.clone(
- raster.getRenderedImage(), null, sampleDimensions, raster, null,
true);
+ // The GC_NODATA sentinel also rides on the rendered image itself as an
image
+ // property and survives serialization there, so cloning the image would
let
+ // readers and writers resurrect the cleared value. Copy the pixels into
a
+ // property-free image and drop the sentinel from the coverage
properties too.
+ Map propertyMap = raster.getProperties();
+ if (propertyMap != null) {
+ propertyMap.remove(NoDataContainer.GC_NODATA);
+ }
+ WritableRaster pixels = raster.getRenderedImage().copyData(null);
Review Comment:
Could we keep this on a `RenderedImage` path? I reproduced a clear on a
valid coverage with image origin `(5, 7)` and NoData `0`. `copyData(null)`
keeps that origin, then the `BufferedImage` constructed in `RasterUtils.clone`
throws `IllegalArgumentException: Raster ... has minX or minY not equal to
zero: 5 7`. The previous clone path succeeds and preserves the origin. A
translated-raster regression test would cover it.
##########
common/src/main/java/org/apache/sedona/common/raster/RasterBandEditors.java:
##########
@@ -53,14 +55,22 @@ public static GridCoverage2D setBandNoDataValue(
// Remove no-Data if it is null
if (noDataValue == null) {
- if (RasterBandAccessors.getBandNoDataValue(raster) == null) {
+ if (rasterNoData == null) {
return raster;
}
GridSampleDimension[] sampleDimensions = raster.getSampleDimensions();
sampleDimensions[bandIndex - 1] =
RasterUtils.removeNoDataValue(sampleDimensions[bandIndex - 1]);
- return RasterUtils.clone(
- raster.getRenderedImage(), null, sampleDimensions, raster, null,
true);
+ // The GC_NODATA sentinel also rides on the rendered image itself as an
image
+ // property and survives serialization there, so cloning the image would
let
+ // readers and writers resurrect the cleared value. Copy the pixels into
a
Review Comment:
This may also be expensive for the streaming GeoTIFF path.
`RasterConstructors.fromGeoTiff(ImageInputStream)` returns a lazily decoded
image, but `copyData(null)` allocates a raster for the full bounds and walks
every tile. A 10,000 × 10,000 four-band float raster is about 1.6 GB of copied
samples. Could the property be filtered with a delegating `RenderedImage`
instead, so clearing metadata does not force pixel materialization?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
