Skip to content

fix: consolidated codec correctness fixes (supersedes #71)#73

Open
sedghi wants to merge 6 commits into
test-pixel-correctnessfrom
codec-correctness-fixes
Open

fix: consolidated codec correctness fixes (supersedes #71)#73
sedghi wants to merge 6 commits into
test-pixel-correctnessfrom
codec-correctness-fixes

Conversation

@sedghi

@sedghi sedghi commented Jul 8, 2026

Copy link
Copy Markdown
Member

All codec source fixes in one PR, stacked on the pixel-correctness test PR #72 (merge #72 first; GitHub retargets this to main automatically when its base branch is deleted). Consolidates the fixes formerly on the fixes branch (#71, closed in favor of this) with the second round found by #72's test suite. Each fix travels with the tests that fail without it — classification is empirical: the wasm packages were rebuilt from unfixed C++ (same emsdk 3.1.74 image CI uses) and the full workspace run against them; whatever failed belongs here, whatever passed stayed in #72.

First round (formerly #71)

libjpeg-turbo-12bit — decoder was unusable: it forced JCS_EXT_RGBA while sizing the output for 1 sample/pixel, so libjpeg wrote ~2x past the buffer (heap overflow), and the result went through Uint8ClampedArray, flattening 12-bit samples to 255. Also the package main field pointed to a dist file that does not exist, so require() failed outright. Now decodes single-component grayscale into 16-bit output, has working entry points, and is wired into dicom-codec's .51 dispatch (previously Decoder not found). Pinned by the 12-bit suite (asm.js + wasm variants), the dispatcher integration test, and the browser-smoke variants.

openjpeg decoder — rejects <4-byte input (OOB magic-number read) and unsupported component counts (freeing handles on the rejection path); BufferStream write/skip/seek callbacks are bounds-checked instead of writing/seeking past the buffer. Pinned by the small-buffer throw tests across all build variants.

dicom-codec codecFactory — reads encode/decode results before delete() and frees wasm instances in finally, so a throwing decode no longer leaks the instance. Pinned by the cleanup-on-throw test.

Overflow-checked decoded-buffer sizing on wasm32 in openjpeg, openjphjs, and libjpeg-turbo-8bit decoders (width*height*components*bytes capped and checked).

Second round (found by #72's suite)

openjpeg encoderJ2KEncoder::encode() returned silently on failure, leaving the full pre-sized allocation as the "encoded" result, and leaked codec/stream/image handles on every path including success (repeated encodes grow the wasm heap monotonically). Now throws with the encoded buffer zeroed, frees handles on every exit path, and sizes the output with headroom so clamped writes surface as errors. Pinned by the encoder-failure-throw tests, the encode/delete heap-stability test, and both dicom-codec J2K round-trips (encode .90, transcode .80.90), which fail byte-exactness without this.

openjphjs encoderbytesPerPixel = bitsPerSample / 8 truncated to 1 for 9..15-bit samples, halving the row stride so every row after the first was read from the wrong offset (12-bit encodes corrupted). Pinned by the 12-bit encoder round-trip.

libjpeg-turbo-12bit — fail closed on multi-component input: forcing JCS_GRAYSCALE on a color 12-bit JPEG silently discards chroma and reports componentCount=1. Pinned by the multi-component rejection test. Also adds the CodSpeed bench.

dicom-codecadaptImageInfo preserves planarConfiguration (decode8Planar was unreachable; PlanarConfiguration=1 RLE silently decoded interleaved). Pinned by the planar RLE test.

little-endian / big-endian — 32-bit PixelData decoded as Float32Array unconditionally; per review (@wayfarer3130) it is integer data, signed per pixelRepresentation, float only for float pixel data elements. Now Uint32Array/Int32Array with Float32Array as the no-pixelRepresentation fallback, matching cornerstone3D's decodeLittleEndian; same typing applied to dicom-codec's littleEndian.getPixelData. 32-bit views realign to 4-byte boundaries (the old offset % 2 check threw a RangeError at offset % 4 == 2); big-endian gains 1-bit passthrough and byte-swapped 32-bit support. Pinned by the typed-array and realignment tests in both packages.

Note the 32-bit typing change is behavior-visible: consumers relying on always-Float32Array for bitsAllocated: 32 now get integer arrays when pixelRepresentation is set.

Verification

  • Full workspace green with CI=1 (194/194) with wasm rebuilt from this branch's C++.
  • dist-size gate passes: the committed baseline was generated from builds that include these C++ changes.

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 31e862a0-239a-4ef4-9ce3-7a047083ad8d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codec-correctness-fixes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codspeed-hq

codspeed-hq Bot commented Jul 8, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 4 improved benchmarks
❌ 3 regressed benchmarks
✅ 99 untouched benchmarks
🆕 4 new benchmarks
⏩ 13 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
WallTime JPEG-LS Near-Lossless (.81) 20.6 ms 24.1 ms -14.47%
WallTime 32-bit float, 512x512 x100 13.2 µs 14.2 µs -6.76%
Simulation 32-bit float, 512x512 x100 135.5 µs 144.8 µs -6.43%
WallTime JPEG Lossless P14 SV1 (.70) 43.6 ms 39.8 ms +9.61%
Simulation decode jpeg400jfif.jpg (600x800x8bit) — warm 10.9 ms 10 ms +9.34%
WallTime instantiate+destroy J2KEncoder x50 162.9 µs 150.2 µs +8.43%
WallTime instantiate+destroy JPEGDecoder x50 148.4 µs 140.3 µs +5.8%
🆕 Simulation decode CT-512x512-12bit.jpg (512x512x12bit) — cold N/A 63.4 ms N/A
🆕 Simulation decode CT-512x512-12bit.jpg (512x512x12bit) — warm N/A 13.5 ms N/A
🆕 WallTime decode CT-512x512-12bit.jpg (512x512x12bit) — cold N/A 59.8 ms N/A
🆕 WallTime decode CT-512x512-12bit.jpg (512x512x12bit) — warm N/A 12.8 ms N/A

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing codec-correctness-fixes (0e28946) with test-pixel-correctness (aaa5154)

Open in CodSpeed

Footnotes

  1. 13 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

All source fixes for the codec packages in one PR, stacked on the
pixel-correctness test PR (#72): the first round formerly on the fixes
branch (#71, closed in favor of this) plus the second round found by
the new test suite. Each fix travels with the tests that fail without
it — classification was empirical, running the full workspace against
wasm rebuilt from unfixed C++ with CI's emsdk 3.1.74 image.

First round (formerly #71):
- libjpeg-turbo-12bit: decode as single-component grayscale into 16-bit
  output. Forcing JCS_EXT_RGBA sized the buffer for 1 sample/pixel
  while libjpeg wrote 4 (heap overflow), and Uint8ClampedArray
  flattened 12-bit samples to 255. Fix the package entry points
  (dist/libjpegturbo12js.js) so the package is requireable at all, and
  wire the decoder into dicom-codec's dispatcher (.51), which
  previously threw 'Decoder not found'.
- openjpeg: decoder rejects <4-byte input and unsupported component
  counts, frees handles on the rejection path; BufferStream
  write/skip/seek callbacks are bounds-checked.
- dicom-codec: codecFactory reads results before delete() and frees
  decoder/encoder instances in finally, so failures no longer leak
  wasm instances.
- overflow-checked decoded-buffer sizing on wasm32 (openjpeg,
  openjphjs, libjpeg-turbo-8bit).

Second round (found by the pixel-correctness suite):
- openjpeg: J2KEncoder throws on setup/compress failure instead of
  silently returning a garbage pre-sized buffer, frees
  codec/stream/image on every exit path (repeated encodes grew the
  wasm heap monotonically), and sizes the output buffer with headroom.
- openjphjs: HTJ2KEncoder rounds bytesPerPixel UP; bitsPerSample/8
  truncated to 1 for 9..15-bit samples, halving the row stride and
  corrupting every row after the first in 12-bit encodes.
- libjpeg-turbo-12bit: fail closed on multi-component input instead of
  silently discarding chroma; add the CodSpeed bench.
- dicom-codec: adaptImageInfo preserves planarConfiguration
  (decode8Planar was unreachable; PlanarConfiguration=1 RLE silently
  produced interleaved output).
- little-endian/big-endian: 32-bit pixel data decodes to
  Uint32Array/Int32Array per pixelRepresentation with Float32Array
  only as the no-pixelRepresentation fallback (review feedback from
  wayfarer3130, matching cornerstone3D's decodeLittleEndian); 32-bit
  views realign to 4-byte boundaries; big-endian gains 1-bit
  passthrough and byte-swapped 32-bit support. Same typing fix applied
  to dicom-codec's littleEndian getPixelData.
@sedghi sedghi force-pushed the codec-correctness-fixes branch from 569b224 to a325750 Compare July 8, 2026 16:58
@sedghi sedghi changed the title fix: codec correctness fixes (split from #72) fix: consolidated codec correctness fixes (supersedes #71) Jul 8, 2026
sedghi and others added 5 commits July 9, 2026 09:32
# Conflicts:
#	.github/workflows/pr-checks.yml
#	packages/dicom-codec/test/color-and-depth.test.js
#	packages/dicom-codec/test/dispatch.test.js
#	packages/dicom-codec/test/transcode-and-pixeldata.test.js
#	packages/openjpeg/test/decode.test.js
#	packages/openjpeg/test/heap-stability.test.js
#	packages/openjphjs/test/matrix.test.js
#	tools/browser-smoke/run.js
The emsdk 3.1.74 image ships node 20.18.0, but vite 7 (transitive via
vitest 3) requires ^20.19.0 || >=22.12.0, so 'yarn install' in the
NPM_PUBLISH job fails its engine check and the publish never runs. Install
node 22 and put it first on PATH before yarn install, mirroring the
setup-node@v4 step the GitHub Actions build job already uses. emcc keeps
its own configured node, so the wasm build is unaffected.
 - @cornerstonejs/codec-big-endian@0.1.1
 - @cornerstonejs/codec-charls@1.2.4
 - @cornerstonejs/dicom-codec@1.0.10
 - @cornerstonejs/codec-libjpeg-turbo-12bit@0.4.2
 - @cornerstonejs/codec-libjpeg-turbo-8bit@1.2.3
 - @cornerstonejs/codec-little-endian@0.0.7
 - @cornerstonejs/codec-openjpeg@1.3.1
 - @cornerstonejs/codec-openjph@2.4.8
@sedghi

sedghi commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

CodSpeed note (for reviewers)

The aggregate shows −2.8% with 3 "regressions", but only one is real — and it's an intended trade. Comparison is 0e28946 (head) vs a88a461 (main); no "Environment Differences" section, so base and head ran on matching runners — these numbers are trustworthy, not the CPU-lottery noise we've seen on other runs.

1 real, expected regression

  • little-endian decode :: 32-bit float, 512x512 x100−7.85% WallTime and −6.28% Simulation.
  • Genuine: it moved in both modes, and the deterministic Simulation number only shifts on a real instruction-count change. It maps directly to this PR's 32-bit fix — the path now branches on pixelRepresentation (Uint32/Int32 vs Float32) and realigns 32-bit views to a 4-byte boundary. That's strictly more work than the old path.
  • This is correctness over speed and worth keeping: the old (faster) path typed 32-bit integer pixel data as float and threw a RangeError at offset % 4 == 2. ~7% on an uncommon path (32-bit pixel data) buys correct output.

2 that are noise, not regressions

  • JPEG-LS Near-Lossless (.81) −14.56% (WallTime) — this drags the aggregate down but is wall-clock variance: the PR doesn't touch the charls decode path (charls C++ is unchanged here), and the deterministic Simulation twin is unchanged. A 20 ms WallTime dispatch bench swinging 14% while its Simulation counterpart is flat is jitter on the shared runner.
  • The two +8% "improvements" (instantiate+destroy … x50, WallTime) are the same jitter in the other direction.

New benchmarks (expected): the 4 libjpeg-turbo-12bit decode entries are new because this PR wires up and benches the 12-bit codec for the first time.

TL;DR: nothing to fix. The only real delta is the deliberate cost of the little-endian 32-bit correctness fix; the number inflating the aggregate is WallTime noise on an unchanged path.

sedghi added a commit that referenced this pull request Jul 9, 2026
…recision API)

3.x forbids add_subdirectory() and removed WITH_12BIT (one build is now
multi-precision). Build libjpeg-turbo standalone and link libjpeg.a as an
IMPORTED target (two-phase build.sh), and rewrite the decoder for 3.x:
- decode grayscale 12-bit via jpeg12_read_scanlines + J12SAMPARRAY (the 3.x
  per-precision API) instead of jpeg_read_scanlines (the old WITH_12BIT model)
- guard on num_components==1 and data_precision==12; overflow-checked sizing
- correct single-component int16 output (no JCS_EXT_RGBA overflow)

3.x headers moved under src/. No dependency on #73 (left untouched); the
decode-correctness fix here mirrors #73's grayscale logic but on the 3.x API.
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.

2 participants