Skip to content

Fix the last sample of a scan whose entropy coded data ends on a byte boundary - #26

Open
wayfarer3130 wants to merge 1 commit into
rii-mango:masterfrom
wayfarer3130:fix/byte-aligned-end-of-scan
Open

wayfarer3130 wants to merge 1 commit into
rii-mango:masterfrom
wayfarer3130:fix/byte-aligned-end-of-scan

Conversation

@wayfarer3130

Copy link
Copy Markdown

The bug

One specific JPEG Lossless Process 14 SV1 frame decodes with its final sample wrong — 0 instead of the real value. Every other sample in the 512×512 frame is bit-identical, so it is not an offset, a sign error or a row shift; the scan simply ends one sample early.

Cause

temp holds index unconsumed bits. When getHuffmanValue/getn shifts in a byte that turns out to be the 0xFF introducing a marker, those 8 bits are not entropy-coded data — only the index - 8 bits above them are real, and the scan is over once they run out.

The three end-of-data guards tested index[0] < this.markerIndex, and markerIndex is 9. Consuming the last real bit leaves index === 8, so the guard fired on a decode that had used only valid bits, discarding a sample that had been decoded correctly.

That only bites when the entropy-coded segment ends exactly on a byte boundary. T.81 B.1.1.2 pads only an incomplete final byte, so a stream that lands flush needs no padding bits at all and EOI follows the last Huffman code immediately.

The attached frame does exactly that. It ends in a long run of the image minimum (−2000, air); a run of equal samples is a run of zero differences, and its table codes a zero difference in 2 bits, so the run tiles the final byte perfectly:

before sample 262141:  index = 6, temp = 0        (6 zero bits left, no padding)
  sample 262141 (x=509):  2 bits consumed -> index = 12
  sample 262142 (x=510):  2 bits consumed -> index = 10
  sample 262143 (x=511):  2 bits consumed -> index =  8   <- guard fires, sample dropped

The fix

readPastEntropyData() puts the boundary at index < 8 and names what it is testing. markerIndex keeps its 9 purely as the "marker seen" sentinel that the other call sites (this.markerIndex !== 0) already treat it as.

This also subsumes the !this.isLastPixel() special case in getn — "this was corrupting the last pixel in some cases" was a band-aid for the same off-by-one, applied at one of the three guard sites.

Test

tests/data/jpeg_lossless_sel1-byte-aligned-end.jpg is the single encapsulated fragment of CTImage.dcm_JPEGProcess14SV1TransferSyntax_1.2.840.10008.1.2.4.70.dcm from cornerstone3D's test images: 512×512, 16-bit signed, one component, SV1, point transform 0, written by DCMTK 3.6.1. The test asserts the last sample and the crc32 of the whole frame against the pixel data of the uncompressed CTImage.dcm it was encoded from, so it is checked against ground truth rather than against whatever this decoder happens to produce.

Verification

  • npm test: 54/54 pass (47 pre-existing + 7 new). npm run lint clean.
  • Byte-for-byte against reference dumps, zero differences on all of: grayscale SV1, grayscale Process 14, colour SV1, colour Process 14 (from viewer-testdata's grayEncode/colorEncode corpora, where twelve lossless encodings of one image must all decode to a single reference), plus the DCMTK frame above.
  • In cornerstone3D, this moves 1.2.840.10008.1.2.4.70 from pending to passing in its decoder ground-truth suite, with no regression across the other 16 transfer-syntax cases.

Relationship to #25

Independent, and this does not depend on it. Point transform is 0 on this frame, so #25's Al handling does not apply. I developed it on top of #25 first (since that PR rewrites the same decode loop), confirmed the full suite passes there, then rebased onto master — it applies cleanly to both and the two compose without conflict. Targeting master so it is not gated on #25 landing.

🤖 Generated with Claude Code

An entropy coded segment whose final Huffman code lands exactly on a byte
boundary needs no padding bits (T.81 B.1.1.2 pads only an incomplete final
byte), so EOI follows the last code immediately. `getHuffmanValue` and `getn`
treated that as having read into the marker and abandoned the scan one sample
early, leaving the frame's last sample 0.

`temp` holds `index` unconsumed bits. Once the 0xFF that introduces a marker
has been shifted in, its 8 bits are not data, so `index - 8` real bits are
left and consuming the last of them leaves `index === 8` - still a valid
decode. The guards tested `index < markerIndex` (9), which rejects it.
`readPastEntropyData` puts the boundary at `index < 8` and names what it is
testing; `markerIndex` keeps its 9 purely as the "marker seen" sentinel that
the other call sites already treat it as.

This replaces the `!isLastPixel()` special case in `getn`, which was covering
the same off-by-one for one of the three guard sites.

Reproduced with the single fragment of
CTImage.dcm_JPEGProcess14SV1TransferSyntax_1.2.840.10008.1.2.4.70.dcm, written
by DCMTK 3.6.1: 512x512 16 bit signed, ending in a long run of the image
minimum whose two-bit zero-difference codes tile the final byte exactly. Its
last sample decoded as 0 instead of -2000; every other sample was already
correct. Added as tests/data/jpeg_lossless_sel1-byte-aligned-end.jpg, checked
against the crc32 of the uncompressed CTImage.dcm pixel data.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant