Repository navigation
geotiff: reject mixed SampleFormat per band - #1869
Merged
Merged
Conversation
IFD.sample_format previously returned v[0] for tuple SampleFormat tags, silently collapsing per-band sample formats to whatever the first band declared. A file with BitsPerSample=(32,32,32) and SampleFormat=(3,3,1) decoded every band as float32 instead of failing, reinterpreting the uint band as a float. Add resolve_sample_format alongside resolve_bits_per_sample. It keeps the empty-tuple fallback exercised by the fuzz regression at tests/test_fuzz_hypothesis_1661.py:311 and raises ValueError on mixed per-band values. IFD.sample_format delegates to it so all readers inherit the validation.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes silent pixel corruption when a TIFF's SampleFormat tag carries different values per band. Introduces resolve_sample_format mirroring resolve_bits_per_sample, and routes IFD.sample_format through it so mixed per-band values raise a typed ValueError instead of being collapsed to the first band's format.
Changes:
- Add
resolve_sample_formathelper in_dtypes.py(scalar passthrough, uniform tuple, mixed-raise, empty-tuple fallback to 1). - Replace inline logic in
IFD.sample_formatwith a delegation to the new helper. - Add unit and end-to-end tests, including a minimal TIFF builder for multi-band
SampleFormat.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| xrspatial/geotiff/_dtypes.py | New resolve_sample_format validator with typed error mirroring resolve_bits_per_sample. |
| xrspatial/geotiff/_header.py | IFD.sample_format delegates to resolve_sample_format. |
| xrspatial/geotiff/tests/test_mixed_sample_format.py | New unit + end-to-end tests including a synthetic TIFF builder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1868.
Summary
IFD.sample_formatcollapsed tuple SampleFormat tags tov[0], so a TIFF withSampleFormat=(3,3,1)(two float bands plus one uint band at the same bit depth) decoded every band as float32. The uint band came back reinterpreted as a float with no error or warning.Adds
resolve_sample_formatin_dtypes.pymirroringresolve_bits_per_sample:ValueErrorwith the offending tuple and a pointer to gdal_translate / rasterio.1(uint), so the fuzz regression attests/test_fuzz_hypothesis_1661.py:311still passes.IFD.sample_formatdelegates to the new helper, so both reader call sites (_reader.py:2507and_reader.py:2934) get the validation for free.Test plan
resolve_sample_format(scalar, uniform tuple, mixed, empty fallback).open_geotifffor float/uint mix and signed/unsigned mix.test_mixed_bps.py,test_fuzz_hypothesis_1661.py,test_header.py,test_reader.py,test_edge_cases.pyall pass (153 tests).