Conversation
…d overflow-sized frames
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
sedghi
changed the title
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR enables the libjpeg-turbo 12-bit JPEG codec in the dispatcher, switches its decoder to grayscale 12-bit output, and ensures codecFactory always cleans up encoder/decoder instances via try/finally. It also adds overflow-checked buffer size computations across libjpeg-turbo 8-bit, openjpeg, and openjphjs decoders, plus new tests and Vitest configs. Changes12-bit JPEG codec enablement
Decoder buffer-size overflow hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant codecFactory
participant JPEGDecoder
Dispatcher->>codecFactory: decode(imageFrame, imageInfo)
codecFactory->>JPEGDecoder: decode() (via runProcess)
JPEGDecoder->>JPEGDecoder: checkedDecodedSize / grayscale 12-bit decode
alt decode succeeds
JPEGDecoder-->>codecFactory: decoded buffer
codecFactory->>JPEGDecoder: delete() (finally)
codecFactory-->>Dispatcher: imageFrame, imageInfo, duration
else decode throws
JPEGDecoder-->>codecFactory: error
codecFactory->>JPEGDecoder: delete() (finally)
codecFactory-->>Dispatcher: rethrow error
end
🚥 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
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | decode jpeg400jfif.jpg (600x800x8bit) — warm |
10 ms | 10.9 ms | -8.44% |
| ⚡ | encode CT1.RAW (HTJ2K lossless) — cold |
36.7 ms | 34.5 ms | +6.21% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fixes (3179998) with main (04c3e87)
Footnotes
-
19 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. ↩
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/libjpeg-turbo-8bit/src/JPEGDecoder.hpp (1)
118-141: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
tjInstanceleaks ifcheckedDecodedSizethrows.
checkedDecodedSize(...)at line 131 can throwstd::runtime_errorfor an oversized/zero-sized frame, buttjInstance(created at line 120) is only destroyed on thetjDecompress2failure path and at normal completion — not on this new throw path. Since this validation is exercised precisely on attacker-controlled/malformed image dimensions, this introduces a leak of the native decompressor handle on every such input.🐛 Proposed fix
int pixelFormat = (frameInfo_.componentCount == 1) ? TJPF_GRAY : TJPF_RGB; - const size_t destinationSize = checkedDecodedSize(frameInfo_.width, frameInfo_.height, 1, tjPixelSize[pixelFormat]); + size_t destinationSize; + try { + destinationSize = checkedDecodedSize(frameInfo_.width, frameInfo_.height, 1, tjPixelSize[pixelFormat]); + } catch (...) { + tjDestroy(tjInstance); + throw; + } decoded_.resize(destinationSize);🤖 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/libjpeg-turbo-8bit/src/JPEGDecoder.hpp` around lines 118 - 141, `decode()` leaks the native `tjInstance` if `checkedDecodedSize(...)` throws before the explicit `tjDestroy` calls. Update `JPEGDecoder::decode` to ensure the decompressor handle is always released on every exit path, including exceptions from `checkedDecodedSize`, by using an RAII guard or equivalent cleanup tied to `tjInitDecompress()`/`tjDestroy()`. Keep the existing error handling in `readHeader_i` and `tjDecompress2`, but make the cleanup automatic so malformed image dimensions cannot leak the handle.packages/openjpeg/src/J2KDecoder.hpp (1)
862-867: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
checkedDecodedSizethrow leaksl_stream,l_codec, andimage.If the computed destination size is zero or exceeds the 512 MiB cap (attacker-influenced via decomposition-level/dimension fields), the exception unwinds past
l_stream/l_codec/imagewithout any of the three being released, unlike the other error paths in this function.🐛 Proposed fix
const size_t bytesPerPixel = (frameInfo_.bitsPerSample + 8 - 1) / 8; - const size_t destinationSize = checkedDecodedSize(sizeAtDecompositionLevel.width, sizeAtDecompositionLevel.height, frameInfo_.componentCount, bytesPerPixel); + size_t destinationSize; + try { + destinationSize = checkedDecodedSize(sizeAtDecompositionLevel.width, sizeAtDecompositionLevel.height, frameInfo_.componentCount, bytesPerPixel); + } catch (...) { + opj_stream_destroy(l_stream); + opj_destroy_codec(l_codec); + opj_image_destroy(image); + throw; + } decoded_.resize(destinationSize);🤖 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/openjpeg/src/J2KDecoder.hpp` around lines 862 - 867, The destination-size computation in J2KDecoder’s decode path can throw from checkedDecodedSize before l_stream, l_codec, and image are released, causing a leak on invalid or oversized dimensions. Update the cleanup flow around the sizeAtDecompositionLevel/decoded_.resize block so any exception from checkedDecodedSize or later allocation still frees those resources, matching the other error paths in this function; use the existing cleanup logic for l_stream, l_codec, and image rather than letting the exception escape first.packages/libjpeg-turbo-12bit/src/JPEGDecoder.hpp (1)
115-172: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftInstall a non-terminating libjpeg error handler
jpeg_std_errorkeeps libjpeg’s defaulterror_exit, which aborts the runtime on fatal JPEG errors. Add a custom error manager withsetjmp/longjmpso bad input can unwind as an exception instead of terminating the whole process/WASM instance.🤖 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/libjpeg-turbo-12bit/src/JPEGDecoder.hpp` around lines 115 - 172, The JPEGDecoder::decode path still uses jpeg_std_error with libjpeg’s default fatal error_exit, so malformed input can terminate the process instead of unwinding safely. Add a custom error manager for JPEGDecoder that installs a non-terminating handler and uses setjmp/longjmp around jpeg_create_decompress, jpeg_read_header, and jpeg_start_decompress so libjpeg errors are converted into a C++ exception. Keep the existing cleanup via jpeg_destroy_decompress in the error path and preserve the current buffer-size checks and grayscale decoding setup.
🧹 Nitpick comments (3)
packages/dicom-codec/test/dispatch.test.js (1)
40-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a symmetric encoder cleanup test.
This test covers
decode()cleanup on throw, butencode()got the sametry/finallytreatment in codecFactory.js and has no equivalent coverage.🤖 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/dispatch.test.js` around lines 40 - 79, Add a matching cleanup test for the encode path in codecFactory by extending the existing codecFactory instance cleanup coverage: create a FakeEncoder with encode() throwing and delete() setting a flag, then call codecFactory.encode with a similar context/codecConfig setup and assert the thrown error is preserved and delete() still runs. Place the new test alongside the current decode() cleanup test so both try/finally paths in codecFactory are covered symmetrically.packages/libjpeg-turbo-12bit/test/decode.test.js (2)
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture read isn't gated, so a missing fixture crashes the whole file instead of skipping.
ct12bitis read at module scope for both build variants, unlike theisBuiltdist-artifact gating. If the fixture file is ever absent, every test (including build-variant-skipped ones) fails hard rather than skipping gracefully, which runs counter to this PR's stated test-infra goal.🤖 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/libjpeg-turbo-12bit/test/decode.test.js` around lines 9 - 11, The fixture load for ct12bit is happening at module scope, so a missing JPEG file crashes the entire test file before any build-variant gating can skip tests. Move the readFileSync/resolve lookup into the test setup or guard it with the same conditional used by isBuilt so decode.test.js can skip gracefully when the fixture is absent. Keep the change localized around ct12bit and the existing built-vs-skipped test paths.
34-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap decoder in try/finally to avoid leaking the WASM instance on assertion failure.
decoder.delete()only runs if every precedingexpect()passes. A failing assertion leaves the WASM decoder instance undeleted for the rest of the test run.♻️ Proposed fix
it.skipIf(!isBuilt)( "decodes the CT-512x512 12-bit fixture and reports correct dimensions/format", () => { const decoder = new codec.JPEGDecoder() - const encodedBuffer = decoder.getEncodedBuffer(ct12bit.length) - encodedBuffer.set(ct12bit) - - decoder.decode() - - const frameInfo = decoder.getFrameInfo() - expect(frameInfo.width).toBe(512) - expect(frameInfo.height).toBe(512) - expect(frameInfo.bitsPerSample).toBe(12) - expect(frameInfo.componentCount).toBe(1) - - const decoded = decoder.getDecodedBuffer() - // One 16-bit-wide sample per pixel (grayscale, 1 component/pixel). - expect(decoded.length).toBe(512 * 512) - - decoder.delete() + try { + const encodedBuffer = decoder.getEncodedBuffer(ct12bit.length) + encodedBuffer.set(ct12bit) + + decoder.decode() + + const frameInfo = decoder.getFrameInfo() + expect(frameInfo.width).toBe(512) + expect(frameInfo.height).toBe(512) + expect(frameInfo.bitsPerSample).toBe(12) + expect(frameInfo.componentCount).toBe(1) + + const decoded = decoder.getDecodedBuffer() + expect(decoded.length).toBe(512 * 512) + } finally { + decoder.delete() + } } )Same pattern applies to the truncated-input test at Lines 68-77.
Also applies to: 68-77
🤖 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/libjpeg-turbo-12bit/test/decode.test.js` around lines 34 - 55, The JPEGDecoder test cases in decode.test.js are leaking the WASM instance because decoder.delete() is only reached after all expectations pass. Update the test bodies that use codec.JPEGDecoder to wrap decode/getFrameInfo/getDecodedBuffer assertions in a try/finally block so decoder.delete() always runs, including the CT-512x512 case and the truncated-input test mentioned in the comment.
🤖 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 `@packages/libjpeg-turbo-12bit/src/JPEGDecoder.hpp`:
- Around line 168-172: The decoded buffer in getDecodedBuffer() is being exposed
through Uint8ClampedArray.new_(), which clamps away 12-bit sample values before
codecFactory.js can reconstruct them. Update the getDecodedBuffer() path in
JPEGDecoder.hpp to return the raw typed_memory_view(...) or another 16-bit typed
array so the original sample data is preserved; use the existing decoded_ buffer
and stride/output_size logic as the place to adjust the return type.
In `@packages/openjpeg/src/J2KDecoder.hpp`:
- Around line 835-841: The component-count validation in decode_i leaks native
OpenJPEG resources because the new unsupported count throw path only destroys
image and skips releasing l_codec and l_stream. Update this branch to mirror the
other failure paths in J2KDecoder::decode_i by cleaning up all allocated handles
before throwing, using the same teardown pattern already present for earlier
error cases.
---
Outside diff comments:
In `@packages/libjpeg-turbo-12bit/src/JPEGDecoder.hpp`:
- Around line 115-172: The JPEGDecoder::decode path still uses jpeg_std_error
with libjpeg’s default fatal error_exit, so malformed input can terminate the
process instead of unwinding safely. Add a custom error manager for JPEGDecoder
that installs a non-terminating handler and uses setjmp/longjmp around
jpeg_create_decompress, jpeg_read_header, and jpeg_start_decompress so libjpeg
errors are converted into a C++ exception. Keep the existing cleanup via
jpeg_destroy_decompress in the error path and preserve the current buffer-size
checks and grayscale decoding setup.
In `@packages/libjpeg-turbo-8bit/src/JPEGDecoder.hpp`:
- Around line 118-141: `decode()` leaks the native `tjInstance` if
`checkedDecodedSize(...)` throws before the explicit `tjDestroy` calls. Update
`JPEGDecoder::decode` to ensure the decompressor handle is always released on
every exit path, including exceptions from `checkedDecodedSize`, by using an
RAII guard or equivalent cleanup tied to `tjInitDecompress()`/`tjDestroy()`.
Keep the existing error handling in `readHeader_i` and `tjDecompress2`, but make
the cleanup automatic so malformed image dimensions cannot leak the handle.
In `@packages/openjpeg/src/J2KDecoder.hpp`:
- Around line 862-867: The destination-size computation in J2KDecoder’s decode
path can throw from checkedDecodedSize before l_stream, l_codec, and image are
released, causing a leak on invalid or oversized dimensions. Update the cleanup
flow around the sizeAtDecompositionLevel/decoded_.resize block so any exception
from checkedDecodedSize or later allocation still frees those resources,
matching the other error paths in this function; use the existing cleanup logic
for l_stream, l_codec, and image rather than letting the exception escape first.
---
Nitpick comments:
In `@packages/dicom-codec/test/dispatch.test.js`:
- Around line 40-79: Add a matching cleanup test for the encode path in
codecFactory by extending the existing codecFactory instance cleanup coverage:
create a FakeEncoder with encode() throwing and delete() setting a flag, then
call codecFactory.encode with a similar context/codecConfig setup and assert the
thrown error is preserved and delete() still runs. Place the new test alongside
the current decode() cleanup test so both try/finally paths in codecFactory are
covered symmetrically.
In `@packages/libjpeg-turbo-12bit/test/decode.test.js`:
- Around line 9-11: The fixture load for ct12bit is happening at module scope,
so a missing JPEG file crashes the entire test file before any build-variant
gating can skip tests. Move the readFileSync/resolve lookup into the test setup
or guard it with the same conditional used by isBuilt so decode.test.js can skip
gracefully when the fixture is absent. Keep the change localized around ct12bit
and the existing built-vs-skipped test paths.
- Around line 34-55: The JPEGDecoder test cases in decode.test.js are leaking
the WASM instance because decoder.delete() is only reached after all
expectations pass. Update the test bodies that use codec.JPEGDecoder to wrap
decode/getFrameInfo/getDecodedBuffer assertions in a try/finally block so
decoder.delete() always runs, including the CT-512x512 case and the
truncated-input test mentioned in the comment.
🪄 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: a85cc5ec-e4ee-4c59-9e98-dbf54b33e9af
📒 Files selected for processing (16)
package.jsonpackages/dicom-codec/src/codecs/codecFactory.jspackages/dicom-codec/src/codecs/libjpegTurbo12bit.jspackages/dicom-codec/test/dispatch.test.jspackages/dicom-codec/test/integration.test.jspackages/libjpeg-turbo-12bit/package.jsonpackages/libjpeg-turbo-12bit/src/JPEGDecoder.hpppackages/libjpeg-turbo-12bit/test/decode.test.jspackages/libjpeg-turbo-12bit/vitest.config.mjspackages/libjpeg-turbo-8bit/src/JPEGDecoder.hpppackages/libjpeg-turbo-8bit/test/decode.test.jspackages/openjpeg/src/BufferStream.hpppackages/openjpeg/src/J2KDecoder.hpppackages/openjpeg/src/J2KEncoder.hpppackages/openjpeg/test/decode.test.jspackages/openjphjs/src/HTJ2KDecoder.hpp
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
…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++.
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.
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).
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.
|
Closing in favor of #73: every fix on this branch (12-bit decode path and package entry points, dispatcher wiring, codecFactory instance cleanup, openjpeg decoder guards and BufferStream bounds, wasm32 size-overflow checks) now lives there, consolidated with the second-round fixes found by the pixel-correctness suite, each paired with the tests that fail without it. Merge order: #72 (tests only, targets main) first, then #73. |
1 participant
Summary
A batch of correctness, robustness, and test-infrastructure fixes across the codec packages. Each change is an independent commit.
Changes
libjpeg-turbo-12bit — implement the codec end to end. The 12-bit decoder produced a single-component 16-bit frame but was decoding into a mismatched color space and buffer size; this corrects the output format and buffer sizing so the decoded dimensions and sample count are right. The
dicom-codecwrapper for transfer syntax1.2.840.10008.1.2.4.51(previously an unimplemented stub that always threw) is now wired to the real decoder, and the packagemain/exportsare corrected to the filenames the build actually emits. Adds a decode test against a real 12-bit fixture.Overflow-safe buffer sizing (openjpeg, openjph, libjpeg-8bit/12bit). The decoded-buffer size is
width * height * components * bytesPerPixel, all taken from the encoded header. On the 32-bit wasm target that product can wrap around; the size math is now computed with a checked 64-bit multiply and a sane upper bound, so a malformed header can't produce an undersized allocation.openjpeg decoder — reject inputs it can't handle. Bail out cleanly on unsupported component counts and on buffers too small to contain a header, instead of reading/writing past the intended bounds.
openjpeg BufferStream — bounds-check the callbacks. The skip/write/seek stream callbacks now clamp to the buffer extents rather than advancing past them.
dicom-codec — free wasm instances on the error path.
decode/encodenow release the decoder/encoder instance in afinally, so a failed decode no longer leaks the instance and its heap buffers.Test infrastructure. The libjpeg-turbo-8bit suite is gated on a built
dist/so a build-lessyarn testskips cleanly instead of failing with a module-not-found error; a roottestscript is added.Verification
yarn testpasses locally (13 passed | 75 skipped); the wasm-backed suites skip cleanly because the WebAssembly artifacts aren't built in a plain checkout. The wasm packages are compiled and their decode tests run in CI (the emsdk build matrix), which is the authoritative gate for the C++ changes here.Summary by CodeRabbit
New Features
Bug Fixes
Tests