geotiff: per-tile dim check uses default cap, not caller budget#1824
geotiff: per-tile dim check uses default cap, not caller budget#1824brendancol wants to merge 1 commit into
Conversation
PR #1803 forwarded the caller's max_pixels to read_to_array inside read_vrt's source loop so a tiny VRT output cannot force a huge source decode (#1796). The output-window check at the source read enforces that correctly. A separate per-tile dimension check at the same call sites also consumed the caller's max_pixels, so a caller setting max_pixels as an output budget (e.g. 10_000) failed the per-tile sanity check on any normal source whose default tile size is 256x256 (= 65_536 pixels). Use MAX_PIXELS_DEFAULT for the per-tile dim check at the two call sites in _read_tiles (local) and _read_tiles_cog_http (HTTP). The output-window check at the same functions continues to enforce the user-supplied max_pixels, preserving the #1796 protection.
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in the GeoTIFF/VRT reader where forwarding a small caller max_pixels budget into VRT source reads caused normal tiled sources (e.g., 256×256 tiles written by to_geotiff) to fail an internal per-tile dimension sanity check, even when the requested output/window fit within the caller’s budget. The change aims to keep the existing VRT “source window must honor caller max_pixels” protection (#1796) while preventing the per-tile header sanity check from incorrectly consuming that same budget.
Changes:
- Update per-tile dimension sanity checks in
_read_tilesand_fetch_decode_cog_http_tilesto useMAX_PIXELS_DEFAULTinstead of the caller-providedmax_pixels. - Add a new regression test suite covering the VRT small-
max_pixelsscenario and verifying the per-tile check is wired toMAX_PIXELS_DEFAULT.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
xrspatial/geotiff/_reader.py |
Decouples per-tile dimension sanity checks from the caller’s max_pixels by using MAX_PIXELS_DEFAULT at the per-tile call sites. |
xrspatial/geotiff/tests/test_vrt_source_tile_check_1823.py |
Adds regression tests ensuring normal tiled sources don’t fail when callers use small max_pixels as an output budget, and that the output-window cap remains enforced. |
Comments suppressed due to low confidence (1)
xrspatial/geotiff/_reader.py:2032
- Same concern here: using
MAX_PIXELS_DEFAULTfor the per-tile dimension check decouples it from the caller’smax_pixels, so smallmax_pixelsvalues no longer protect against very large tile allocations during windowed HTTP COG reads. If the intent is “hard header sanity cap” only, consider a dedicated cap/exception message (since_check_dimensionscurrently suggests increasingmax_pixels), or a separatemax_tile_pixelsknob so callers can still enforce stricter per-tile limits when needed.
# reads (window is None). The per-tile dim check below guards the
# TIFF header against absurd ``TileWidth`` / ``TileLength`` values
# (e.g. 2**31) and uses ``MAX_PIXELS_DEFAULT`` so a caller's small
# ``max_pixels`` -- intended as an output-window budget -- does not
# reject normal 256x256 tiles. See #1823.
if window is None:
_check_dimensions(width, height, samples, max_pixels)
_check_dimensions(tw, th, samples, MAX_PIXELS_DEFAULT)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Reject crafted tile dims (e.g. TileWidth = 2**31). This guards the | ||
| # TIFF header against malformed values; it is not the caller's output | ||
| # budget. The output-window check below uses ``max_pixels`` and is | ||
| # what enforces the user's per-call memory cap. The source-read path | ||
| # under ``read_vrt`` (#1796) relies on that output check to honour a | ||
| # small caller ``max_pixels`` against a normal-tile source; see | ||
| # #1823. | ||
| _check_dimensions(tw, th, samples, MAX_PIXELS_DEFAULT) |
Closes #1823.
Summary
_check_dimensions(tw, th, samples, max_pixels)at_reader.py:1565and:2027is a sanity check onTileWidth/TileLengthheader values (rejects crafted2**31-style tile dims). After Honor max_pixels for VRT source reads #1803 it consumed the caller'smax_pixels, so a small output budget likeread_vrt(..., max_pixels=10000)failed on every normal source:to_geotiffwrites a default 256x256 tile (= 65,536 pixels) which overshoots the caller's output budget at the per-tile check.MAX_PIXELS_DEFAULT(1e9). The output-window check on the same call paths still enforces the caller'smax_pixels, so the VRT source reads do not honor max_pixels safety limit #1796 protection is preserved.Why on main, separate PR
Six open PRs (
#1811 #1817 #1818 #1819 #1820 #1822) all failtest_vrt_dstrect_resample_cap_1737::test_dstrect_at_cap_succeedson Python 3.14 because of this regression. Fixing it on main unblocks all of them at once.Test plan
pytest xrspatial/geotiff/tests/test_vrt_source_tile_check_1823.py(new, 4 tests)pytest xrspatial/geotiff/tests/test_vrt_dstrect_resample_cap_1737.py(regression fixed)pytest xrspatial/geotiff/tests/test_vrt_source_max_pixels_1796.py(VRT source reads do not honor max_pixels safety limit #1796 protection still enforced)