test: verify exact pixel output for every codec + fail CI on silently-skipped suites#72
Merged
Conversation
…d overflow-sized frames
…-skipped suites Pixel correctness: - libjpeg-turbo-8bit: byte-compare decode against the RAW reference (previously only length was checked) and bound encode round-trip error (measured maxAbsDiff 6 / meanAbsDiff 0.26; asserted <= 10 / <= 1) - libjpeg-turbo-12bit: add RAW reference (verified bit-identical against DCMTK dcmdjpeg) and byte-compare decode against it - charls: add CT1.RAW (cross-validated bit-for-bit against openjpeg and openjph decodes of the same slice) and a pinned golden for the near-lossless .81 path; byte-compare both - openjpeg: pin the lossy .91 decode as a golden and byte-compare - openjphjs: corpus test pinning SHA-256 of decoded pixels for all 13 j2c fixtures (MG/MR/NM/RG/SC/XA modalities) - dicom-codec: every transfer syntax now byte-compares decoded pixels against cross-validated references. RLE and JPEG Lossless Process 14 outputs were verified identical to each other and to DCMTK. - documents a real upstream bug found by these tests: jpeg-lossless-decoder-js decodes the final pixel of the SV1 fixture as 0 instead of -2000 (DCMTK confirms the fixture is correct). Pinned via an all-but-last-pixel compare plus an it.fails() sentinel that flips when upstream fixes it. Endian packages: - big-endian: handle 32-bit (swap32 -> Float32Array) and 1-bit frames, mirroring little-endian; previously 32-bit fell through and left pixelData undefined - little-endian: fix Float32Array alignment check (offset % 4, not % 2) - exact-value tests for both, including unaligned byteOffset cases CI: - tests now fail loudly in CI when a dist is missing instead of silently skipping entire suites (it.runIf(process.env.CI) guards) - any package change builds all packages: dicom-codec integration decodes through every sibling dist, so partial builds would trip the guards; docs-only changes still skip everything - single test job for the whole vitest workspace (per-package test jobs were ~90% runner setup) - CodSpeed benches only the packages the PR touched (main still re-benches everything for full baselines) - node_modules cached keyed on yarn.lock; shallow checkout for builds
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR revises CI orchestration, benchmark/docs handling, package-level benchmark/test behavior, and adds standalone fixture-verification decoders plus a runner for byte-exact comparisons. ChangesCI Pipeline and Supporting Tooling
Codec vitest configuration, benchmark batching, and regression tests
Estimated code review effort: 4 (Complex) | ~75 minutes Independent fixture-verification decoders
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Merging this PR will not alter performance
|
Independent implementations of JPEG-LS (T.87 regular+run mode, NEAR>=0), JPEG Lossless (T.81 SOF3, predictors 1-7), DICOM RLE (PS3.5 Annex G) and sequential DCT JPEG (SOF0/SOF1 with libjpeg's exact islow IDCT constants), sharing no code with the codecs under test. run-all.js binary-compares their output against every committed RAW reference: 12/12 byte-exact. Also re-confirms the jpeg-lossless-decoder-js SV1 last-pixel bug from a fourth independent decoder: the from-scratch SOF3 decoder produces -2000 for the final sample, matching DCMTK/RLE/Process-14 and the committed reference.
Member
Author
Independent fixture verificationAdded
The from-scratch SOF3 decoder also independently reproduces the correct last pixel (−2000) for the SV1 fixture, making it the fourth independent confirmation of the Not independently verifiable by construction (documented in the tool's README): the lossy J2K golden (9/7 wavelet output is implementation-defined) and the openjphjs corpus SHA-256 pins beyond CT1/CT2 — those remain regression goldens of the build's own deterministic output. |
Review findings on the 12-bit/openjpeg changes: - libjpeg-turbo-12bit: decode() forced JCS_GRAYSCALE unconditionally, so a color (multi-component) 12-bit JPEG would silently drop its chroma channels and report componentCount=1. Now rejects num_components != 1 after jpeg_read_header with cleanup + throw. Test splices the grayscale fixture into a syntactically valid 3-component JPEG and asserts the decoder throws. - openjpeg: J2KEncoder::encode() swallowed opj_setup_encoder / opj_start_compress / opj_encode / opj_end_compress failures with bare returns. With the bounded BufferStream, an undersized output estimate surfaces through exactly those return values — and the JS caller would read back the full pre-sized allocation as a successful encode. All failure paths now free the codec/stream/image, empty encoded_, and throw. Also frees those handles on the success path, which previously leaked them on every encode. Test forces opj_setup_encoder failure (41 resolutions > OpenJPEG's max 33) and asserts encode() throws and the encoded buffer is empty.
'Different runtime environments detected' on PR comparisons: every controllable axis already matched between BASE and HEAD (runner image 20260628.225.1, CodSpeedHQ/action v4.18.2, node 22.23.1 — verified from the job logs), so the mismatch is GitHub's runner CPU lottery: standard runners land randomly on Intel Xeon 8370C or AMD EPYC 7763, whose cache sizes and ISA extensions make glibc execute different code paths, shifting instruction counts even in simulation mode. See https://codspeed.io/blog/unrelated-benchmark-regression Mitigations: - push trigger now fires on main only. PR branches were running the whole workflow twice per commit (push + pull_request) and uploading two CodSpeed measurements per commit, each on random hardware — doubling both CI usage and the odds of cross-CPU comparisons. main pushes still seed the baseline; PRs keep their pull_request runs. - codspeed-bench logs lscpu before benching so any future environment warning is diagnosable from the job log. The residual case (PR run and baseline run landing on different CPU models) is inherent to shared runners; the durable fix would be CodSpeed macro runners or GitHub larger runners.
…ation Dual-instrument setup: simulation stays the blocking regression gate (deterministic <1% drift, catches small algorithmic slips); a new advisory codspeed-walltime job measures real wall-clock on CodSpeed's ARM64 bare-metal macro runners, covering simulation's blind spots (real cache/branch behavior, and the pure-JS endian packages where the no-JIT simulation model diverges most from production V8). - upgrade vitest 2.1.9 -> 3.2.7, @vitest/coverage-v8 -> 3.2.4 and @codspeed/vitest-plugin 4.0.1 -> 5.7.1 (walltime requires plugin >= 5, which requires vitest >= 3.2); all 123 tests pass on the new stack - walltime benches run sequentially (--concurrency 1): parallel processes contend for cores and add wall-clock noise, unlike instruction counting - walltime job is continue-on-error while the macro-runner setup beds in, with timeout-minutes: 30 in case runner pickup stalls - cache keys now include runner.os/arch: macro runners are ARM64 and sharing keys with x64 jobs would restore broken native binaries - BENCHMARKING.md documents the two-instrument model
vitest 3 pulls vite 7, whose engines check (>=20.19) rejects the node 20.18.0 bundled in emscripten/emsdk:3.1.74, failing yarn install in every build job. setup-node puts node 22 on PATH for yarn/webpack; emcc keeps using the node binary pinned in its own emsdk config.
- vitest 3 enforces a 60s worker RPC timeout; under valgrind with lerna --parallel, 8 bench processes on 4 cores starved the openjpeg and dicom-codec suites past it. Sequential benching changes only wall time — instruction counts are contention-immune. - runs-on: codspeed-macro queues up to 24h when macro runners aren't provisioned for the org (job timeouts don't cover queue time), so the walltime job now requires CODSPEED_MACRO_ENABLED=true. Set it after enabling macro runners for the org on app.codspeed.io.
New dist-size job compares every shipped artifact (js/wasm/mem, both raw and gzip level-9 sizes) against tools/dist-size/baseline.json and fails when anything grows beyond max(1%, 1 KiB), so a PR cannot increase codec binary size unintentionally. New or missing artifacts also fail, keeping additions deliberate. The baseline was generated from CI-built artifacts (gh run download of run 28905144867), not local builds — the Debug-built wasm embeds source paths, so only CI output is a stable ground truth. Intentional size changes rerun 'node tools/dist-size/check.js --update' against CI artifacts and commit the diff, making growth visible in review.
Under valgrind the entire vitest process (main and forked worker) runs ~60x slow while vitest 3's hard-coded 60s birpc timer counts real seconds, so large suites structurally hit 'Timeout calling onTaskUpdate' AFTER their benches complete — serializing lerna didn't help because the timeout is intra-process. The benches finish and upload correctly; only the exit code was polluted. Forward --dangerouslyIgnoreUnhandledErrors to vitest for the simulation job only (walltime runs native speed and stays strict) and restore --parallel, which was never the culprit.
yarn 1 mangles '--'-forwarded flags (lerna received a bare --dangerouslyIgnoreUnhandledErrors and rejected it), so set the option in each package's vitest config instead, gated on CODSPEED_RUNNER_MODE === 'simulation' — the env var the CodSpeed runner itself sets. Walltime and regular test runs stay strict.
…uppression The CodSpeed runner exports CODSPEED_RUNNER_MODE as 'instrumentation' on some versions and 'simulation' on newer ones (@codspeed/core accepts both), so gate on CODSPEED_ENV being set and mode != walltime instead of an exact 'simulation' match. Verified all three behaviors locally with a synthetic unhandled error: suppressed under instrumentation/simulation, strict under walltime, strict without CodSpeed.
…035) Coverage added: - openjpeg: corpus test activates 15 shipped-but-unused fixtures with committed RAW references — 8/10/12/15/16-bit, signed and unsigned, 3-component color (US1, VL1, VL4, VL6) — all byte-exact, plus the 0-decomposition CT1 variant. - charls: 3-component interleaved color (ILV=sample), 8-bit and 16-bit unsigned grayscale, and the shipped 12-bit SC1 fixture (SHA-pinned). - openjphjs: color with and without the reversible color transform (the RCT path was flagged untested in the source), 8-bit and 12-bit gray. - libjpeg-turbo-8bit: color 4:2:0 YCbCr decode (DCMTK-verified golden) and a progressive SOF2 decode (Pillow-verified golden). - dicom-codec: color JPEG .50, color RLE in both planar configurations, 8-bit JPEG-LS .80. Fixtures are generated deterministically from committed sources by tools/fixture-verification/gen/ (US1.RAW RGB frame, CT2.RAW transforms); lossless tests re-derive their references, so no golden files are needed except the two lossy JPEG raws (DCMTK/Pillow verified). Two real bugs found and fixed by these tests: - HTJ2KEncoder.hpp: row stride computed as bitsPerSample/8, truncating to 1 byte for 9..15-bit samples — every row after the first was read from the wrong offset when encoding 12-bit data (openjphjs rebuilt). - dicom-codec adaptImageInfo dropped planarConfiguration, which made the RLE plane-sequential decode path (decode8Planar) unreachable through the public decode() API.
An emsdk bump edits only this workflow; a vitest/plugin bump edits only the root manifest and lockfile. Neither matched any packages/<pkg>/ path, so detect-changes skipped the entire pipeline — the exact PRs that change every compiled byte or every measurement ran zero CI. Toolchain paths now force packages=ALL and bench=ALL (a full before/after sweep is precisely what a toolchain bump needs). Docs-only changes still skip. The workflow header documents the expected dist-size baseline / lossy-golden regeneration procedure for such bumps.
- openjpeg: header/coding-parameter getters pinned after decode() (5 decomps, LRCP, 1 layer, 64x64 blocks, grayscale for CT1); decodeSubResolution levels 1-2 pinned — the level-1 output is cross-validated byte-identical with openjphjs' decodeSubResolution of the same slice (shared 5/3 LL band); setProgressionOrder and lossy setQuality/setCompressionRatio proven observable with a PSNR floor. - openjphjs: readHeader-only introspection (populates frameInfo without decoding) and decodeSubResolution(1) pinned to the cross-validated hash. - dicom-codec: encode() round-trips for .90/.80, transcode() .80->.90 preserves pixels exactly, getPixelData typed-array contract pinned. Three known defects documented as it.fails sentinels / comments that flip when fixed: - openjpeg readHeader() populates nothing — getters return zeros or uninitialized memory until decode() runs - openjpeg getIsReversible() reports true for the irreversible 9-7 stream - openjpeg encoder setters blockDimensions/tileSize/tileOffset/precincts/ downSample are stored but never applied to opj_cparameters
…s 018) - Size bounds on every lossless encode round-trip (floor 0.5x catches silent truncation, ceiling 1.10x catches compression-ratio regressions), with measured constants dated in comments: charls CT2 115504, openjpeg CT1 174404, openjphjs CT1 185183, 8bit jpeg400 63975. - PSNR floor (48 dB, measured 53.6) on the 8-bit JPEG round-trip. - charls near-lossless: exact T.87 spec bound maxAbsDiff <= NEAR asserted for NEAR 1..3, plus proof it is actually lossy. - libjpeg-turbo-12bit gets its first benches (cold/warm decode) and a bench script — previously invisible to CodSpeed, so a toolchain bump's full sweep measured nothing for it. - dicom-codec: dispatcher-level encode (.80/.90) and transcode (.80->.90) benches.
Repeated decode/delete, encode/delete and failing-decode cycles must leave HEAP8 capacity byte-identical after a settling warmup. Emscripten arenas grow but never shrink, so any native leak (the class fixed in plan 002, commit 3179998 and the PR #72 encoder cleanup) becomes monotonic capacity growth. Loops are sized against the 50 MiB INITIAL_MEMORY slack — verified to fail when a delete() call is removed (100 leaked charls decoders grow the heap ~22 MiB). Error-path loops use garbage headers (fast rejection) rather than truncated streams (seconds of recovery each) so they can iterate enough to exceed the slack.
tools/browser-smoke/run.js serves the repo over local HTTP (correct application/wasm MIME so streaming compilation runs), loads each of the 11 build variants in headless Chromium, decodes the reference fixture in the page, and compares the decoded pixels' SHA-256 against the committed RAW. Catches the emscripten glue regressions node tests cannot see (wasm URL resolution, fetch loading, MIME fallbacks) — the first thing emsdk bumps break. New advisory browser-smoke CI job (continue-on-error while it beds in) with a cached Chromium install. All 11 variants pass locally.
Member
Author
Upgrade-readiness test suite (plans 033–039)Six more commits make emsdk/upstream-codec bumps fully regression-checked. Run 28915465838: 13/13 jobs green — including the two new gates (browser-smoke, and a full-sweep run triggered by the new toolchain rule itself). Workspace is now 21 test files / 190 tests (was 10/123), all pixel-verified.
Three real bugs found by the new tests, fixed here:
|
…as identical Previously only files whose bytes drifted from baseline produced output, so a fully-green run printed nothing per package and it looked like only the drifting package had been checked. Every file now gets an ok line, with byte-identical files labeled 'identical' to distinguish them from sub-0.005% drift that rounds to +0.00%. Claude-Session: https://claude.ai/code/session_017xiEYAJH7wzPnwNpnoGqet
A single instantiate+destroy (~60us) or typed-array-view decode (<1us) is smaller than the fixed per-bench harness overhead (wrapper frames, task attribution) plus the simulation cache model's CPU-to-CPU variation, so those benches flagged spurious regressions on every harness upgrade or runner-hardware change while the ms-scale decode benches stayed clean through both. Loop the fragile bodies (x50 for wasm lifecycle benches, x100 for the little-endian view benches and the big-endian 8-bit passthrough) so the measured work dominates the noise floor. The big-endian 16-bit benches do real per-pixel swap work on a shared buffer (batching would just re-swap it) and stay unbatched. Bench names carry the batch factor (x50/x100), so CodSpeed will report these as new benchmarks and retire the old names - a one-time baseline reset for the six fragile benches. Claude-Session: https://claude.ai/code/session_017xiEYAJH7wzPnwNpnoGqet
sedghi
commented
Jul 8, 2026
sedghi
commented
Jul 8, 2026
sedghi
commented
Jul 8, 2026
sedghi
commented
Jul 8, 2026
sedghi
commented
Jul 8, 2026
sedghi
commented
Jul 8, 2026
wayfarer3130
reviewed
Jul 8, 2026
wayfarer3130
reviewed
Jul 8, 2026
| const length = pixelData.length; | ||
| // Float32Array views must be 4-byte aligned; shift unaligned data | ||
| if (offset % 4) { | ||
| arrayBuffer = arrayBuffer.slice(offset); |
Contributor
There was a problem hiding this comment.
This is probably an exception, not a just a shift since it isn't clear where the actual data is any longer.
wayfarer3130
reviewed
Jul 8, 2026
wayfarer3130
reviewed
Jul 8, 2026
| expect(Array.from(imageFrame.pixelData)).toEqual([1.5, -2.25, 3.75]) | ||
| }) | ||
|
|
||
| it("realigns 32-bit pixel data when byteOffset is not 4-byte aligned", () => { |
Contributor
There was a problem hiding this comment.
This is really problematic - it should throw since it is completely unclear what this means.
…nly as fallback Review feedback from wayfarer3130: BitsAllocated=32 PixelData is integer data (signed per PixelRepresentation); float applies only to float pixel data elements. Decode now returns Uint32Array/Int32Array accordingly and falls back to Float32Array when pixelRepresentation is absent, matching cornerstone3D's decodeLittleEndian. Applied to big-endian, little-endian, and dicom-codec's littleEndian getPixelData. Also documents that 1-bit data must be frame-extracted by the caller and why 32-bit views may need realignment to a 4-byte boundary.
The little-endian package picked up V8 JIT dump files (12 MB) and local CodSpeed run outputs that were never meant to be tracked.
Review follow-up: this PR mixed pixel-correctness tests with source fixes the tests uncovered. To keep it reviewable as a pure test/CI change, every source fix (JS decoders, dicom-codec factory/dispatch, C++ decoder/encoder hardening) moves to a follow-up PR together with the tests that require it. Classification is empirical: the four wasm packages were rebuilt from main's C++ (same emsdk 3.1.74 image as CI) and the whole workspace run against unmodified sources; the 27 failing tests moved out with their fixes, the 146 that pass stay (164 including CI-only guards, all green, dist-size gate and browser smoke verified against those builds): - big-endian: 1-bit passthrough and all 32-bit decode tests - little-endian: 32-bit integer typing and 4-byte realignment tests - dicom-codec: planar RLE output, codecFactory cleanup-on-throw, 12-bit dispatch, and both J2K encode round-trips (encode reads the result after Wasm memory is freed without the codecFactory fix) - libjpeg-turbo-12bit: whole suite + bench + vitest config (decoder is unusable on main: RGBA heap overflow and a broken package entry point) - openjpeg: tiny-buffer throw, encoder-failure throw, encoder heap leak - openjphjs: 12-bit encoder round-trip (row-stride fix) - browser-smoke: 12-bit variants (hash-compare needs the fixed decoder) The 12-bit RAW reference stays: tools/fixture-verification/run-all.js validates it independently of the wasm codecs.
…es), not main The previous split commit reverted sources to main, but this PR is stacked on the fixes branch (#71) which already carries the first round of codec fixes — so the diff showed reversions of #71's work (codecFactory, 12-bit decoder, openjpeg guards, dispatcher wiring). Sources are now pinned to the fixes tree (zero src delta in this PR) and the classification was re-run empirically against wasm rebuilt from the fixes branch's C++: 17 tests depend on fixes this branch itself introduced and move to the follow-up PR; everything that #71's fixes already satisfy stays here, including the 12-bit decode suite, the dispatcher 12-bit integration test, the codecFactory cleanup test, the openjpeg small-buffer guard tests and the 12-bit browser-smoke variants. Moved out (fail without this branch's own fixes): - big-endian 1-bit/32-bit tests, little-endian 32-bit typing/realign - dicom-codec planar RLE and both J2K encode round-trips - openjpeg encoder-failure throw and encode/delete heap stability - openjphjs 12-bit encoder round-trip - libjpeg-turbo-12bit multi-component rejection + bench Full workspace green with CI=1 (177/177); dist-size and browser smoke pass against dists built from the fixes branch's C++.
sedghi
added a commit
that referenced
this pull request
Jul 8, 2026
Second-round fixes found by the pixel-correctness suite (#72), split out so that PR stays a pure test/CI change. First-round fixes (12-bit decode path, codecFactory instance cleanup, openjpeg decoder guards and BufferStream bounds) are already in #71 — this PR carries only what the test branch itself introduced, each fix together with the tests that fail without it (verified against wasm rebuilt from the fixes branch's C++ with CI's emsdk 3.1.74 image): - openjpeg: J2KEncoder throws on setup/compress failure instead of silently returning, with the encoded buffer zeroed (callers previously read back a garbage pre-sized allocation as a successful encode); frees codec/stream/image on every exit path (repeated encodes grew the wasm heap monotonically); sizes the output buffer with headroom so clamped writes surface as errors instead of truncation. Pinned by the encoder-failure-throw tests, the encode/delete heap-stability test, and both dicom-codec J2K round-trips (encode .90 and transcode .80->.90), which fail byte-exactness without this rework. - openjphjs: HTJ2KEncoder rounds bytesPerPixel UP; 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 and its package script. - dicom-codec: adaptImageInfo preserves planarConfiguration (decode8Planar was unreachable; PlanarConfiguration=1 RLE silently produced interleaved output). Pinned by the planar RLE test. - 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 (the old offset % 2 check threw a RangeError at offset % 4 == 2); big-endian gains 1-bit passthrough and byte-swapped 32-bit support. Same typing fix applied to dicom-codec's littleEndian getPixelData.
Member
Author
|
Restructured per review: this PR is now tests/CI only — the diff contains zero src/ changes relative to its base (fixes). The source fixes this branch had introduced moved to #73 (stacked on this branch) together with the 17 tests that fail without them. Classification was empirical: wasm rebuilt from the base branch's C++ with the same emsdk 3.1.74 image CI uses, full workspace run against base sources, failing tests moved out. Everything already covered by #71's fixes (12-bit decode suite, dispatcher 12-bit integration, codecFactory cleanup test, openjpeg small-buffer guards, 12-bit browser-smoke variants) stays here. Merge order: #71 -> #72 -> #73. |
Reverts the fixes-branch-relative classification: #71 is being closed in favor of a single consolidated fixes PR stacked on this one, and this PR retargets to main. With no fix PR below it, this branch must hold only tests that pass against plain main sources — which is exactly the state this restores (verified earlier against wasm rebuilt from main's C++ with CI's emsdk 3.1.74 image: 164/164 with CI=1, dist-size gate and browser smoke green).
sedghi
added a commit
that referenced
this pull request
Jul 8, 2026
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tools/dist-size/check.js (1)
44-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
collectDistslogic is solid.The function correctly handles both local (
packages/*/dist) and CI artifact (dist-*) layouts, filters to tracked file types, and measures both raw and gzip sizes. The early-exit on missing directories and empty file lists prevents false failures for packages without builds.One minor edge case: if
--artifactsis passed without a value (line 42),path.resolve(undefined)will throw an unhandledTypeError. This is a documented CLI tool with a known call path in CI, so the risk is low, but a one-line guard would improve robustness for local usage.🛡️ Optional guard for missing --artifacts value
const artifactsDir = artIdx >= 0 ? path.resolve(args[artIdx + 1]) : null; +if (artIdx >= 0 && !args[artIdx + 1]) { + console.error("--artifacts requires a directory argument"); + process.exit(1); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/dist-size/check.js` around lines 44 - 70, Add a small validation guard in collectDists’ call path for the --artifacts option so path.resolve is never called with an undefined value. Check the parsed artifacts argument before resolving it, and fall back to the local packages/*/dist flow or emit a clear usage/error message if the value is missing. Keep the change near the artifactsDir setup in tools/dist-size/check.js so the collectDists logic remains unchanged.packages/dicom-codec/test/color-and-depth.test.js (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
frameBytesis duplicated across three test files.The same
frameByteshelper appears identically incolor-and-depth.test.js,integration.test.js, andtranscode-and-pixeldata.test.js. Consider extracting it to a shared test utility (e.g.,packages/dicom-codec/test/helpers.js) to prevent drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dicom-codec/test/color-and-depth.test.js` around lines 16 - 18, The frameBytes helper is duplicated across multiple DICOM codec test files, so extract the shared implementation from color-and-depth.test.js, integration.test.js, and transcode-and-pixeldata.test.js into a common test utility such as a helpers module. Update the affected tests to import and use the shared frameBytes helper instead of keeping separate copies, so any future changes happen in one place.packages/dicom-codec/test/integration.test.js (1)
27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting
frameBytesto a shared test helper.
frameBytesis duplicated verbatim acrossintegration.test.js,color-and-depth.test.js, andtranscode-and-pixeldata.test.js. A sharedtest/helpers/frame-bytes.jswould centralize the logic if it ever needs updating. This is low priority for test utilities but worth noting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dicom-codec/test/integration.test.js` around lines 27 - 33, The frameBytes helper is duplicated across multiple test files, so extract it into a shared test utility to keep the Buffer conversion logic in one place. Create a common helper such as frameBytes in a shared test/helpers module, then update integration.test.js and the other tests that currently define their own frameBytes to import and use the shared helper instead. Keep the helper signature and behavior unchanged so existing tests continue to work..github/workflows/pr-checks.yml (1)
262-262: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout steps that don't need git operations.The
dist-size,codspeed-bench, andcodspeed-walltimejobs check out the repo but never push, commit, or run git commands afterward. By defaultactions/checkout@v4persists theGITHUB_TOKENin.git/config, which is unnecessary here and widens the attack surface if a third-party action or script reads it. Addpersist-credentials: falseto these three checkout steps. Thedetect-changesandbuildjobs should keep the default since they need git for diffs and submodule init.🔒 Proposed fix
- uses: actions/checkout@v4 + with: + persist-credentials: false - name: Download all built distsApply the same addition to the checkout steps in
codspeed-bench(line 337) andcodspeed-walltime(line 453).Also applies to: 337-337, 453-453
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-checks.yml at line 262, The checkout steps for dist-size, codspeed-bench, and codspeed-walltime persist the GitHub token even though those jobs do not need git operations. Update the actions/checkout@v4 steps in the relevant job blocks to set persist-credentials to false, using the checkout step names in the workflow as the anchor; keep the default behavior unchanged for detect-changes and build since they rely on git access.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/browser-smoke/run.js`:
- Around line 90-130: The browser smoke test script can exit before cleanup if
`chromium.launch()`, `browser.newPage()`, or `fs.readFileSync` throws, leaving
Chromium or the server running. Refactor the async main flow in `run.js` so the
whole startup-and-loop section is wrapped with a top-level try/finally (or
equivalent) that always calls `browser.close()` and `server.close()`, and move
`browser.newPage()` into the per-variant try block alongside the other variant
work. Also add explicit handling around `chromium.launch()` so launch failures
from `browser` surface with a clear failure message instead of an unhandled
rejection.
In `@tools/fixture-verification/jls.js`:
- Around line 136-137: The buffer swap in jls.js leaves prev[0] carrying the
prior line’s first-sample value, which makes the first-column upper-left
neighbor wrong for y >= 2. Update the line-processing logic around the for-loop
and the swap/reset sequence so that after swapping prev and cur, prev[0] is
explicitly reset to 0 before the next iteration. Keep the fix localized to the
buffer-handling code used for the T.87 context calculation so Rc stays zero at
x=0.
---
Nitpick comments:
In @.github/workflows/pr-checks.yml:
- Line 262: The checkout steps for dist-size, codspeed-bench, and
codspeed-walltime persist the GitHub token even though those jobs do not need
git operations. Update the actions/checkout@v4 steps in the relevant job blocks
to set persist-credentials to false, using the checkout step names in the
workflow as the anchor; keep the default behavior unchanged for detect-changes
and build since they rely on git access.
In `@packages/dicom-codec/test/color-and-depth.test.js`:
- Around line 16-18: The frameBytes helper is duplicated across multiple DICOM
codec test files, so extract the shared implementation from
color-and-depth.test.js, integration.test.js, and
transcode-and-pixeldata.test.js into a common test utility such as a helpers
module. Update the affected tests to import and use the shared frameBytes helper
instead of keeping separate copies, so any future changes happen in one place.
In `@packages/dicom-codec/test/integration.test.js`:
- Around line 27-33: The frameBytes helper is duplicated across multiple test
files, so extract it into a shared test utility to keep the Buffer conversion
logic in one place. Create a common helper such as frameBytes in a shared
test/helpers module, then update integration.test.js and the other tests that
currently define their own frameBytes to import and use the shared helper
instead. Keep the helper signature and behavior unchanged so existing tests
continue to work.
In `@tools/dist-size/check.js`:
- Around line 44-70: Add a small validation guard in collectDists’ call path for
the --artifacts option so path.resolve is never called with an undefined value.
Check the parsed artifacts argument before resolving it, and fall back to the
local packages/*/dist flow or emit a clear usage/error message if the value is
missing. Keep the change near the artifactsDir setup in tools/dist-size/check.js
so the collectDists logic remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f82fe27-2209-4220-8e49-81b1bec5b34e
⛔ Files ignored due to path filters (4)
packages/libjpeg-turbo-8bit/test/fixtures/jpeg/US1-color-420.jpgis excluded by!**/*.jpgtools/fixture-verification/gen/derive.mjsis excluded by!**/gen/**tools/fixture-verification/gen/generate-fixtures.mjsis excluded by!**/gen/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (60)
.github/workflows/pr-checks.ymlBENCHMARKING.mdpackage.jsonpackages/big-endian/bench/decode.bench.jspackages/big-endian/vitest.config.mjspackages/charls/bench/decode.bench.jspackages/charls/test/decode.test.jspackages/charls/test/fixtures/CT-512x512-near-lossless.RAWpackages/charls/test/fixtures/CT1.RAWpackages/charls/test/fixtures/CT2-gray16u.jlspackages/charls/test/fixtures/CT2-gray8.jlspackages/charls/test/fixtures/US1-color-ilv-sample.jlspackages/charls/test/heap-stability.test.jspackages/charls/test/matrix.test.jspackages/charls/vitest.config.mjspackages/dicom-codec/bench/dispatch.bench.jspackages/dicom-codec/test/color-and-depth.test.jspackages/dicom-codec/test/dispatch.test.jspackages/dicom-codec/test/fixtures/raw/CT-512x512.rawpackages/dicom-codec/test/fixtures/rle/US1-color.rlepackages/dicom-codec/test/integration.test.jspackages/dicom-codec/test/transcode-and-pixeldata.test.jspackages/dicom-codec/vitest.config.mjspackages/libjpeg-turbo-12bit/test/fixtures/raw/CT-512x512-12bit.rawpackages/libjpeg-turbo-8bit/bench/decode.bench.jspackages/libjpeg-turbo-8bit/test/decode.test.jspackages/libjpeg-turbo-8bit/test/fixtures/raw/US1-color-420.rawpackages/libjpeg-turbo-8bit/test/fixtures/raw/jpeg400jfif-new.rawpackages/libjpeg-turbo-8bit/test/heap-stability.test.jspackages/libjpeg-turbo-8bit/vitest.config.mjspackages/little-endian/bench/decode.bench.jspackages/little-endian/vitest.config.mjspackages/openjpeg/bench/decode.bench.jspackages/openjpeg/test/api.test.jspackages/openjpeg/test/corpus.test.jspackages/openjpeg/test/decode.test.jspackages/openjpeg/test/fixtures/raw/CT-512x512-lossy.rawpackages/openjpeg/test/heap-stability.test.jspackages/openjpeg/vitest.config.mjspackages/openjphjs/bench/decode.bench.jspackages/openjphjs/test/api.test.jspackages/openjphjs/test/corpus.test.jspackages/openjphjs/test/decode.test.jspackages/openjphjs/test/fixtures/j2c/CT2-gray12.j2cpackages/openjphjs/test/fixtures/j2c/CT2-gray8.j2cpackages/openjphjs/test/fixtures/j2c/US1-color-ct.j2cpackages/openjphjs/test/fixtures/j2c/US1-color-nct.j2cpackages/openjphjs/test/heap-stability.test.jspackages/openjphjs/test/matrix.test.jspackages/openjphjs/vitest.config.mjstools/browser-smoke/blank.htmltools/browser-smoke/run.jstools/dist-size/baseline.jsontools/dist-size/check.jstools/fixture-verification/README.mdtools/fixture-verification/jls.jstools/fixture-verification/jpegdct.jstools/fixture-verification/jpll.jstools/fixture-verification/rle.jstools/fixture-verification/run-all.js
wayfarer3130
approved these changes
Jul 9, 2026
wayfarer3130
left a comment
wayfarer3130
left a comment
Contributor
There was a problem hiding this comment.
Manually reviewed and also got 1 finding from clod that I'd appreciate your resolving before commit.
The one change I'd request before merge is adding tools/browser-smoke/ and tools/fixture-verification/ to TOOLCHAIN_PATHS so the new tooling can't rot untested
Review follow-up: tools/browser-smoke/ drives the browser-smoke job and the test suites import reference derivations from tools/fixture-verification/gen/, but neither directory was in TOOLCHAIN_PATHS — a PR touching only them matched no package path and skipped CI entirely, letting the tooling rot untested.
Member
Author
|
Done in aaa5154 — added tools/browser-smoke/ and tools/fixture-verification/ to TOOLCHAIN_PATHS, so a change to either forces the full build/test/bench pipeline. Both are genuinely load-bearing: browser-smoke drives the browser-smoke job, and the test suites import reference derivations from tools/fixture-verification/gen/. |
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
2 participants
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.
Summary
Every codec test suite now verifies exact decoded pixel data against independently verified references, CI fails loudly instead of silently skipping suites when a dist is missing, and the workflow got faster (scoped benches, single test job, dependency caching).
Before this change, a decoder returning garbage of the correct size would pass CI for: libjpeg-turbo-8bit, libjpeg-turbo-12bit (no tests at all), every lossy/near-lossless path, and every codec dispatched through dicom-codec (RLE and JPEG Lossless had no pixel verification anywhere in the repo).
Real bug found by the new tests
jpeg-lossless-decoder-js(the JPEG Lossless SV1 path, transfer syntax.70) decodes the final pixel of the CT fixture as0instead of-2000.Verified three independent ways: the RLE decode, the JPEG Lossless Process 14 decode, and DCMTK's
dcmdjpegall produce bit-identical pixels for the same slice — only the JS SV1 path differs, in exactly one sample. The suite pins this with an all-but-last-pixel exact compare plus anit.fails()sentinel that flips when upstream fixes it (worth filing against rii-mango/JPEGLosslessDecoderJS).Pixel-correctness coverage
dcmdjpeg, re-derivable viatools/fixture-verification); the decode suite rides with the decoder fixes in #73.81checked dims/length onlyCT1.RAW;.81byte-exact vs pinned golden.91checked dims/length onlybyteLengthonly.51, planar RLE, codecFactory cleanup and J2K encode round-trip tests ride with their fixes in #73)How the golden references were verified
dcmdjpeg(0 of 262144 samples differ).dicom-codec/test/fixtures/raw/CT-512x512.raw..91, charls.81): decoding is deterministic, so the decoder's own output is pinned as a regression golden — identical across the asm.js / wasm / decode-only build variants.emscripten/emsdk:3.1.74image CI uses.CI: no more silently green
There were 14
skipIf(!isBuilt)gates; if artifact upload/replay ever broke, vitest skipped everything and the job stayed green. Each suite now has anit.runIf(process.env.CI)guard that fails when a dist is missing in CI (verified: hidingcharls/distfails 3 tests instead of green-with-14-skips). Local dev without builds still skips gracefully.Consequence: any package change now builds all packages, because dicom-codec's integration tests decode through every sibling dist. This also closes the hole where a dicom-codec-only PR previously tested nothing (all suites skipped silently). Docs-only changes still skip everything.
Endian packages
The 32-bit typing/alignment fixes and the big-endian 32-bit/1-bit support (with their tests) are in #73 — this PR leaves both packages' sources untouched.
Workflow speed
Measured baseline: ~5–7 min wall; critical path = slowest build (~2 min) → codspeed-bench (3.4 min).
benchoutput from detect-changes). Benches run under valgrind — the slowest job — and a PR only needs deltas for what it changed; main/workflow_dispatchstill bench everything to keep full baselines.node_modulescached keyed onyarn.lock(install skipped on hit) instead of only the yarn tarball cache; installs ran ~10× per run before.test (<package>)check contexts, they need updating to the singletestcheck.Deliberately left out
build.shstill compiles openjphjs and libjpeg-turbo-12bit with-DCMAKE_BUILD_TYPE=Debug. That ships unoptimized wasm and skews bench numbers, but flipping it silently shifts every CodSpeed baseline and fix: Decode partial htj2k stream #68 touches the openjphjs build — should be its own PR with a deliberate baseline re-seed.Test plan
CI=1 npx vitest run— 20 files, 164 tests, all pass, all guards executed (against wasm rebuilt from main's C++ with CI's emsdk image; dist-size and browser smoke also pass against those builds)lerna run bench --scope ...runs only scoped packages; packages without a bench script are skipped cleanly)Summary by CodeRabbit
New Features
Bug Fixes
Chores