Conversation
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for GeoTIFF per-dataset nodata masks by fetching/decoding the mask alongside tile data and applying it in the Deck.gl raster shader pipeline to discard invalid pixels.
Changes:
- Fetch mask tiles (when present) in parallel with data tiles and attach the decoded mask to
RasterArray. - Add 1-bit (bit-packed) decoding support to the GeoTIFF decoder path to handle common mask encodings.
- Introduce a new
MaskTextureGPU shader module and update the render pipeline to upload/apply the mask texture automatically.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/geotiff/src/fetch.ts | Fetches and decodes mask tiles; attaches mask to returned raster arrays. |
| packages/geotiff/src/decode.ts | Adds bit-packed (1-bit) unpacking to support mask decoding. |
| packages/geotiff/src/array.ts | Updates mask semantics documentation (non-zero valid, 0 invalid). |
| packages/deck.gl-raster/src/gpu-modules/mask-texture.ts | New shader module that discards fragments when mask indicates invalid pixels. |
| packages/deck.gl-raster/src/gpu-modules/index.ts | Exports the new MaskTexture module. |
| packages/deck.gl-geotiff/src/geotiff/render-pipeline.ts | Uploads mask textures and injects masking into the render pipeline when present. |
Comments suppressed due to low confidence (1)
packages/geotiff/src/fetch.ts:110
- When
boundless: falseis used,clipToImageBoundstrimsdata/bandsto match the clippedwidth/height, but it does not clipmask. With masks now being populated, edge tiles will returnarray.width/heightsmaller thantileWidth/tileHeightwhilearray.maskremains full-tile length, breaking the stated contract (mask.length === width*height) and causing mask upload/rendering issues.clipToImageBoundsshould clipmaskin the same way it clips pixel data.
const array: RasterArray = {
...decodedPixels,
count: samplesPerPixel,
height: self.tileHeight,
width: self.tileWidth,
mask,
transform: tileTransform,
crs: self.crs,
nodata: self.nodata,
};
return {
x,
y,
array: boundless === false ? clipToImageBounds(self, x, y, array) : array,
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mask = device.createTexture({ | ||
| data: padToAlignment(array.mask, width, height, bytesPerPixel), |
There was a problem hiding this comment.
Mask texture padding is using bytesPerPixel derived from the color texture (which may be 3/4/etc). For an r8unorm mask, bytesPerPixel must be 1; otherwise padToAlignment will compute the wrong rowBytes and produce corrupted/shifted mask rows (or read past intended row boundaries).
| mask = device.createTexture({ | |
| data: padToAlignment(array.mask, width, height, bytesPerPixel), | |
| // Mask texture is single-channel 8-bit (r8unorm), so bytesPerPixel must be 1 | |
| mask = device.createTexture({ | |
| data: padToAlignment(array.mask, width, height, 1), |
| format: "r8unorm", | ||
| width, | ||
| height, | ||
| sampler: samplerOptions, |
There was a problem hiding this comment.
samplerOptions for the mask texture currently inherits the color texture's filters (often linear). With linear filtering, mask edges interpolate and maskValue == 0.0 will frequently be false even for mostly-invalid pixels, leaving visible seams/halos. Consider forcing nearest filtering for the mask texture (or using a threshold like < 0.5/255.0 in the shader).
| sampler: samplerOptions, | |
| // Use nearest filtering for the mask to avoid interpolated edges/halos | |
| sampler: { | |
| ...samplerOptions, | |
| minFilter: "nearest", | |
| magFilter: "nearest", | |
| }, |
| if (geotiff.maskImage !== null) { | ||
| renderPipeline.push({ | ||
| module: MaskTexture, | ||
| props: { | ||
| maskTexture: (data: TextureDataT) => data.mask as Texture, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
MaskTexture is added to the pipeline when geotiff.maskImage !== null, but TextureDataT.mask is optional and the module props use a forced cast (data.mask as Texture). If a mask tile can't be fetched/decoded (e.g. zero-byte tile, missing tile, decode failure), this will attempt to bind undefined to a sampler2D and can break rendering. Consider making mask non-optional when a mask IFD exists, or conditionally injecting MaskTexture per-tile only when a mask texture is actually present.
| return data instanceof Uint8Array | ||
| ? data | ||
| : new Uint8Array(data.buffer, data.byteOffset, data.byteLength); |
There was a problem hiding this comment.
decodeMask returns data as-is when it's a Uint8Array, but for other decoded typed arrays (e.g. Uint16Array) it wraps the underlying buffer as bytes (new Uint8Array(data.buffer, ...)). That changes both the values and the length (bytes vs elements), violating RasterArray.mask's contract (length === width * height). Consider normalizing any non-Uint8Array mask to a Uint8Array(width*height) by thresholding each element to 0/255 (or 0/1) rather than reinterpreting raw bytes.
| return data instanceof Uint8Array | |
| ? data | |
| : new Uint8Array(data.buffer, data.byteOffset, data.byteLength); | |
| if (data instanceof Uint8Array) { | |
| return data; | |
| } | |
| // Normalize non-Uint8Array masks to a Uint8Array(width * height) by | |
| // thresholding each element to 0 or 255 so that length === width * height. | |
| const pixelCount = width * height; | |
| const maskArray = new Uint8Array(pixelCount); | |
| const copyLength = Math.min(pixelCount, data.length); | |
| for (let i = 0; i < copyLength; i++) { | |
| // Treat any non-zero value as "mask set". | |
| // Use 255 so the mask can be used directly as an alpha/opacity mask. | |
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |
| maskArray[i] = data[i] ? 255 : 0; | |
| } | |
| return maskArray; |
| /** | ||
| * Unpack a 1-bit packed mask buffer (MSB-first) into a Uint8Array of 0/255. | ||
| * Each input byte holds 8 pixels; bit 7 is the first pixel in that byte. | ||
| */ | ||
| export function unpackBitPacked( | ||
| buffer: ArrayBuffer, | ||
| pixelCount: number, | ||
| ): Uint8Array { | ||
| const packed = new Uint8Array(buffer); | ||
| const out = new Uint8Array(pixelCount); | ||
| for (let i = 0; i < pixelCount; i++) { | ||
| out[i] = (packed[i >> 3]! >> (7 - (i & 7))) & 1 ? 255 : 0; | ||
| } | ||
| return out; |
There was a problem hiding this comment.
unpackBitPacked hardcodes MSB-first bit order. In TIFF, the FillOrder tag can specify LSB-first bit packing, which would cause masks to decode inverted/garbled for some files. Consider passing/reading FillOrder from the IFD and unpacking accordingly (or explicitly rejecting FillOrder=2 with a clear error).
| switch (bitsPerSample) { | ||
| case 1: | ||
| return unpackBitPacked( | ||
| buffer, | ||
| metadata.width * metadata.height * metadata.samplesPerPixel, | ||
| ); |
There was a problem hiding this comment.
Support for bitsPerSample === 1 (bit-packed masks) is new behavior and currently isn't covered by the existing packages/geotiff/tests/decode.test.ts cases (which only cover 8/16/32-bit paths). Please add a unit test that exercises decode() on a 1-bit sample fixture (or directly validates unpackBitPacked) so regressions in mask decoding are caught.
| "fs:#decl": `uniform sampler2D maskTexture;`, | ||
| "fs:DECKGL_FILTER_COLOR": /* glsl */ ` | ||
| float maskValue = texture(maskTexture, geometry.uv).r; | ||
| if (maskValue == 0.0) { |
There was a problem hiding this comment.
Using an exact float comparison (maskValue == 0.0) is fragile once the mask is sampled with linear filtering or comes from normalized values other than exactly 0/1. Consider using a small threshold (e.g. maskValue <= 0.5/255.0) or ensure the mask texture is always sampled with nearest filtering so discard behavior is stable at tile boundaries.
| if (maskValue == 0.0) { | |
| // Use a small threshold instead of exact 0.0 comparison to be robust to filtering. | |
| if (maskValue <= 0.5 / 255.0) { |
| @@ -157,22 +168,31 @@ function createUnormPipeline( | |||
| bitsPerSample, | |||
| sampleFormat, | |||
| ); | |||
| const { width, height } = array; | |||
| const bytesPerPixel = (bitsPerSample[0]! / 8) * numSamples; | |||
| const texture = device.createTexture({ | |||
| data: padToAlignment( | |||
| array.data, | |||
| array.width, | |||
| array.height, | |||
| bytesPerPixel, | |||
| ), | |||
| data: padToAlignment(array.data, width, height, bytesPerPixel), | |||
| format: textureFormat, | |||
| width: array.width, | |||
| height: array.height, | |||
| width, | |||
| height, | |||
| sampler: samplerOptions, | |||
| }); | |||
|
|
|||
| let mask: Texture | undefined; | |||
| if (array.mask !== null) { | |||
| mask = device.createTexture({ | |||
| data: padToAlignment(array.mask, width, height, bytesPerPixel), | |||
| // Single-channel 8-bit texture for the mask | |||
| format: "r8unorm", | |||
| width, | |||
| height, | |||
| sampler: samplerOptions, | |||
| }); | |||
| } | |||
There was a problem hiding this comment.
Masking behavior is newly introduced here (pipeline injection + mask texture creation), but packages/deck.gl-geotiff/tests/render-pipeline.test.ts doesn't cover any mask cases. Please add a test that asserts MaskTexture is included when a GeoTIFF has maskImage, and that getTileData produces an r8unorm mask texture with appropriate sampler settings.
Change list
MaskTexturemodule for masking out invalid pixels on the GPU.render-pipelineto automatically apply masking if it existsCloses #196 Closes #168
Testing with https://maxar-opendata.s3.amazonaws.com/events/yellowstone-flooding22/ard/12/120000020112/2022-06-18/10300100D51B8C00-visual.tif. I think I might have to double check mask decoding. (We really need to set up full integration tests against geotiff-test-data.)