Skip to content

Read FITS 16-bit integers as signed and expose BZERO/BSCALE in im.info - #10312

Open
karandhaodiyal28-hash wants to merge 5 commits into
python-pillow:mainfrom
karandhaodiyal28-hash:fix-fits-16bit-signedness
Open

karandhaodiyal28-hash wants to merge 5 commits into
python-pillow:mainfrom
karandhaodiyal28-hash:fix-fits-16bit-signedness

Conversation

@karandhaodiyal28-hash

Copy link
Copy Markdown

FITS integers are signed big-endian, but BITPIX=16 data was unpacked with the unsigned I;16B rawmode. Negative values wrapped around, and unsigned camera data (stored as signed with BZERO=32768 per Section 5.2) came out with its top bit flipped:

# BITPIX=16, values [-32768, -32767, -1, 0, 32767]
# before: [128, 384, 65535, 0, 65407]
# after:  [-32768, -32767, -1, 0, 32767]

This implements option 2 from the issue:

  • BITPIX=16 is now read as signed I;16BS into mode I, like the TIFF plugin does for signed 16-bit. The gzip-compressed path (FitsGzipDecoder) is extended the same way — it expands 2-byte samples to the 4-byte I rawmode so both paths agree.
  • BZERO/BSCALE are now exposed in im.info (int when integral, float otherwise, FITS D exponents handled) instead of being silently dropped. Applying the scaling is left to the caller, per FITS Standard 4.0 Section 4.4.2.5.

Adds regression tests building the files in memory, following the reproducer from the issue.

Fixes #10309

karandhaodiyal28-hash and others added 3 commits October 25, 2026 22:21
FITS integers are signed big-endian, but BITPIX=16 data was unpacked
with the unsigned I;16B rawmode, so negative values wrapped around and
unsigned camera data (stored as signed with BZERO=32768) came out with
its top bit flipped. Read 16-bit data as signed I;16BS into mode I,
matching the TIFF plugin, and extend the gzip decoder the same way.

The BZERO/BSCALE scaling keywords are now exposed in im.info instead of
being silently dropped; applying the scaling is left to the caller.

Fixes python-pillow#10309
@laozidabo

Copy link
Copy Markdown

I independently tested 2c2333b2e81f9300e93614e3a682892cd4454c15 and found an uncovered case in the changed 16-bit gzip path. A single-tile GZIP_1 image with samples [-32768, -1, 0, 1, 32767] loads as [-1, 1, 0, 0, 0] on this PR.

Minimal reproduction using Astropy to generate the compressed file (tested with Astropy 8.0.2 and NumPy 2.5.4):

from io import BytesIO

import numpy as np
from astropy.io import fits
from PIL import Image

values = np.array([[-32768, -1, 0, 1, 32767]], dtype=np.int16)
output = BytesIO()
fits.HDUList([
    fits.PrimaryHDU(),
    fits.CompImageHDU(values, compression_type="GZIP_1", tile_shape=(1, 5)),
]).writeto(output)
blob = output.getvalue()

with fits.open(BytesIO(blob)) as hdus:
    hdus.verify("exception")
    print("Astropy:", hdus[1].data.tolist())
with Image.open(BytesIO(blob)) as image:
    print("Pillow:", [image.getpixel((x, 0)) for x in range(image.width)])

Output:

Astropy: [[-32768, -1, 0, 1, 32767]]
Pillow: [-1, 1, 0, 0, 0]

The decompressed payload contains consecutive 2-byte samples, but the new branch still takes value[offset + 2 : offset + 4] and increments offset by 4. It skips samples and eventually calls int.from_bytes() on empty slices, producing the trailing zeros. Could this two-byte payload be handled with a signed 16-bit compressed regression test, while retaining support for the widened four-byte payload used by existing fixtures? Astropy's decoder also distinguishes these widths by decompressed buffer length. I also reproduced this with a standard-library-built FITS fixture, independently of Astropy's writer.

For comparison, current main f58197e0effb8e650b9f50f858ec8976e6f6a83d raises ValueError: not enough image data on the standard-library fixture. So gzip 16-bit support was already incomplete; the PR changes this case to a successful load with incorrect pixels, rather than breaking a previously correct decode.

Validation on Linux x86_64 / Python 3.14.7: selftest.py passes all 59 tests on both revisions; Tests/test_file_fits.py passes 8 tests on main and 11 on this PR. The full suite was not run for this focused investigation. No source patch is being claimed here.

AI disclosure: this investigation, executed reproductions and comment were prepared with OpenAI Codex. The account owner has not manually reviewed the code.

The decompressed GZIP_1 payload holds consecutive samples, while the
existing fixture uses groups widened to 4 bytes. Distinguish the two by
the decompressed buffer length, like astropy does, and read 16-bit
samples as signed in both layouts. This also replaces the reversed
slices, which read past the start of the buffer for 32-bit data.

Fixes the case reported in python-pillow#10312
@karandhaodiyal28-hash

Copy link
Copy Markdown
Author

Good catch, thanks for the thorough report and the clean reproduction. You're right about the cause — the decoder assumed the widened 4-byte layout and skipped every second sample.

I've pushed a fix that distinguishes the payload widths by the decompressed buffer length, like astropy does, and reads 16-bit samples as signed in both layouts. The signed 16-bit compressed regression test is added too (Tests/images/gzip_1_16bit.fits), and m13_gzip still decodes identically.

While testing the other bit depths I also found the old reversed slice read past the start of the buffer for 32-bit data, so that's fixed along the way (8/16/32-bit and float32 GZIP_1 files now match astropy on my end). Could you re-check with your fixture?

if value is None:
continue
try:
number = float(value.replace(b"D", b"E"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we preserve integer precision when parsing BZERO and BSCALE?

Right now, every value goes through float() before being converted back to an integer. For example, BZERO = 9007199254740993 would end up stored as 9007199254740992 because the original integer isn't exactly representable as a float.

Since the PR leaves image scaling to the caller, losing precision in the metadata could lead to incorrect pixel values when that scaling is applied.

Maybe we could try parsing integer literals with int() first, then fall back to float() for decimal or exponent notation. A regression test with an integer larger than 2**53 would help.

@laozidabo

Copy link
Copy Markdown

Rechecked 8ed26bb2829c4417d1e243d0fc484db371db6169: both my original standard-library-built fixture and the Astropy-generated reproduction now return [-32768, -1, 0, 1, 32767]. The consecutive-sample issue I reported is fixed on my Linux x86_64 host. selftest.py passes 59 tests and Tests/test_file_fits.py passes 12. I also compared 64 generated lossless GZIP_1 files covering 8/16/32-bit integers and float32, multiple image/tile shapes, and both payload layouts against Astropy, plus both repository gzip fixtures; all pass locally.

The remaining s390x failure looks actionable: test_gzip1 gets 1862270976 instead of 111 (0x6f000000 instead of 0x0000006f), with 4504 tests passing and one failing. The decoder now emits little-endian bytes for mode I, but set_as_raw() defaults to the image mode; rawmodes I and F use native byte order (definition). This explains the byte-swapped value on big-endian s390x. The 32-bit integer/float paths also emit little-endian bytes, so they should be covered by the same correction.

A minimal candidate is to retain the current buffer and explicitly select its rawmode at the end of decode():

rawmode = {"I": "I;32S", "F": "F;32F"}.get(self.mode, self.mode)
self.set_as_raw(
    bytes([pixel for row in rows[::-1] for pixel in row]), rawmode
)

These rawmodes decode little-endian signed integers and float32 respectively; L stays unchanged. I tested this mapping in memory against the same 64 generated files and two fixtures, all passing on my little-endian host. I have not run it on s390x; that CI job still needs to verify the fix.

The patch coverage check is also below its target (88.88% vs 90.14%). Its uncovered additions include the non-16-bit gzip branches, so committing regression cases for 8-bit, 32-bit and float32 gzip would help cover the decoding changes already exercised above.

AI disclosure: this investigation, executed checks and comment were prepared with OpenAI Codex. The account owner has not manually reviewed the code.

@radarhere

Copy link
Copy Markdown
Member

laozidabo added a commit to laozidabo/Pillow that referenced this pull request Oct 28, 2026
@laozidabo

Copy link
Copy Markdown

I prepared a tested candidate patch for both open items:
laozidabo@67a1dd6

  • FitsGzipDecoder now selects I;32S / F;32F explicitly for its little-endian output buffer instead of relying on native I / F rawmodes. This addresses the byte-swapped s390x result; L remains unchanged.
  • Integral BZERO / BSCALE values are parsed with int() before falling back to float(), preserving integers above 2**53.
  • Adds standard-library-generated GZIP_1 tests for 8/32-bit integers and float32, plus large-integer and invalid metadata cases. The 16-bit consecutive-sample test also uses the in-memory generator, so the new binary fixture is no longer needed.

Validation on Linux x86_64 / Python 3.12.12: Tests/test_file_fits.py passes 19 tests, selftest.py passes 59 tests, and FitsImagePlugin.py line coverage is 99%. Both large-integer tests fail before the source change as expected. I have not run this on s390x; that CI job still needs to verify the big-endian fix.

The default rawmodes for the I and F modes are native-endian, so the
little-endian rows produced for 32-bit data and expanded 16-bit samples
were byte-swapped on big-endian hosts. Pass I;32S and F;32F explicitly
so the decoder output is host-independent.
@karandhaodiyal28-hash

Copy link
Copy Markdown
Author

Thanks for the big-endian report @radarhere, and for testing @laozidabo — the cause was that the default rawmodes for the "I" and "F" modes are native-endian, so the little-endian rows the decoder builds were byte-swapped on big-endian hosts (0x6F000000 = 111 swapped, matching the failure). Pushed a fix that passes I;32S / F;32F explicitly, so the decoder output is now host-independent. m13 still decodes identically and the astropy comparison matrix (8/16/32-bit, float32, consecutive and widened payloads) passes.

@OKAN-SOFTWARE

Copy link
Copy Markdown

Thanks for the update! One additional observation: for 32-bit integer data, number_of_bytes == 4 and self.mode == "I" still fall through to self.set_as_raw(data), which uses native byte order. Since the buffer is little-endian, could this still fail on big-endian systems?

Also, my earlier comment about preserving integer precision for BZERO and BSCALE remains applicable.

@laozidabo

Copy link
Copy Markdown

Confirmed both points on ce9d598 using the standard-library-generated fixtures.

Capturing the rawmode passed to set_as_raw() gives:

BITPIX=16:  I;32S
BITPIX=32:  None
BITPIX=-32: F;32F

So the 32-bit integer path still falls back to native-endian I, while the 16-bit integer and float32 paths are explicit.

The two large-integer regression tests also still fail on this revision: both BZERO and BSCALE read 9007199254740993 as 9007199254740992. The focused run gives 29 passed and these 2 failed; selftest.py passes all 59 tests.

I then applied only the rawmode mapping for all I / F output and int-first parsing locally. With those two changes, the same PR tests plus additional regression cases pass all 31 tests, selftest.py still passes all 59 tests, and git diff --check passes. The candidate patch linked above covers both changes and includes the regression tests.

No GitHub Actions check runs have appeared for ce9d598 yet, so s390x has not validated this revision.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FITS: BZERO/BSCALE are ignored and 16-bit data is read as unsigned

4 participants