fix: Decode partial htj2k stream - #68
Conversation
Merging this PR will regress 3 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decode CT1.j2c (.201 lossless, 512x512x16bit) — warm |
36.8 ms | 40.3 ms | -8.69% |
| ❌ | Simulation | decode CT2.j2c (.201 lossless, 512x512x16bit) — warm |
36.6 ms | 39.2 ms | -6.76% |
| ❌ | Simulation | decode CT2.j2c (.201 lossless, 512x512x16bit) — cold |
36.6 ms | 39.2 ms | -6.74% |
| ⚡ | WallTime | HTJ2K Lossless (.201) |
130.5 ms | 31.5 ms | ×4.1 |
| ⚡ | Simulation | HTJ2K Lossless (.201) |
141.4 ms | 40.6 ms | ×3.5 |
| ⚡ | WallTime | instantiate+destroy HTJ2KEncoder x50 |
256.8 µs | 232 µs | +10.69% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/htj2k-partial (9866b66) with main (a88a461)2
Footnotes
-
79 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. ↩
-
No successful run was found on
main(9c086c9) during the generation of this report, so a88a461 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
📝 WalkthroughWalkthroughChangesHTJ2K decoder reuse and resilience
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant HTJ2KDecode
participant CodecFactory
participant CodecConfig
participant Decoder
HTJ2KDecode->>CodecFactory: request decode with reuseDecoder true
CodecFactory->>CodecConfig: read or create reusedDecoder
CodecFactory->>Decoder: decode image frame
Decoder-->>CodecFactory: decoded image data
CodecFactory-->>HTJ2KDecode: return decoded image data
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/openjphjs/src/HTJ2KDecoder.hpp`:
- Around line 163-169: Update decode() at
packages/openjphjs/src/HTJ2KDecoder.hpp#L156-L170 and decodeSubResolution() at
packages/openjphjs/src/HTJ2KDecoder.hpp#L178-L192 to clear frameInfo_, metadata,
and pDecoded_ before each attempt, initialize the destination buffer before
pulling lines, and return or expose an explicit failure/completion status when
exceptions occur so callers cannot observe partial pixels or stale results.
In `@packages/openjphjs/test/node/index.js`:
- Around line 90-91: Ensure the encoded lossless fixture exceeds the truncation
limit before creating partial streams, then always slice at the fixed limit. In
packages/openjphjs/test/node/index.js lines 90-91, assert encodedLossless.length
exceeds 10 KiB and replace the Math.min-based slice; apply the same assertion
and fixed TRUNCATED_BYTE_LIMIT slice in
packages/openjphjs/test/truncated.test.js lines 112-113 and 164-165 before the
benchmark.
In `@packages/openjphjs/test/truncated.test.js`:
- Around line 264-284: Update the benchmark around reusedDecoder and fresh to
use medianDecodeMs for both paths: warm up reusedDecoder before collecting
samples, keep it alive through all reused samples, and create and delete a fresh
HTJ2KDecoder within each fresh-path sample. Compare the resulting median timings
and retain cleanup after sampling.
🪄 Autofix
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: 9b2492c5-07ef-48e5-869c-3388dc300beb
📒 Files selected for processing (8)
packages/dicom-codec/src/codecs/codecFactory.jspackages/dicom-codec/src/codecs/htj2k.jspackages/openjphjs/bench/decode.bench.jspackages/openjphjs/extern/openjphpackages/openjphjs/src/CMakeLists.txtpackages/openjphjs/src/HTJ2KDecoder.hpppackages/openjphjs/test/node/index.jspackages/openjphjs/test/truncated.test.js
| readHeader_(codestream, mem_file); | ||
| decode_(codestream, frameInfo_, 0); | ||
| } | ||
| catch (const std::exception &e) | ||
| { | ||
| OJPH_INFO(0x00010021, "decode failed (likely truncated stream): %s", e.what()); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)HTJ2KDecoder\.hpp$|openjphjs'
echo "== HTJ2KDecoder outline =="
ast-grep outline packages/openjphjs/src/HTJ2KDecoder.hpp || true
echo "== relevant HTJ2KDecoder.hpp lines =="
wc -l packages/openjphjs/src/HTJ2KDecoder.hpp
cat -n packages/openjphjs/src/HTJ2KDecoder.hpp | sed -n '1,240p'
echo "== search result state methods/usages =="
rg -n "pDecoded_|decoded|resize\\(|decode\\(|decodeSubResolution|readHeader_|OJPH_INFO\\(0x0001002" packages/openjphjs/src packages/openjphjs -g '!**/node_modules/**' || trueRepository: cornerstonejs/codecs
Length of output: 31773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== readHeader_/decode_ implementation =="
cat -n packages/openjphjs/src/HTJ2KDecoder.hpp | sed -n '270,455p'
echo "== decoder class members before implementation =="
cat -n packages/openjphjs/src/HTJ2KDecoder.hpp | sed -n '236,290p'
echo "== tests using reuse/failure paths =="
cat -n packages/openjphjs/test/truncated.test.js | sed -n '140,285p'
cat -n packages/openjphjs/test/node/index.js | sed -n '84,105p'Repository: cornerstonejs/codecs
Length of output: 16378
🌐 Web query:
C++ std::vector resize preserves elements unchanged when size stays same documentation
💡 Result:
Yes, the C++ standard explicitly states that std::vector::resize does nothing if the requested size equals the current size of the vector [1][2][3]. Because the function performs no operations when the size remains the same, all existing elements are preserved unchanged, and no iterators, references, or pointers to the elements are invalidated [4]. The C++ standard defines the effects of resize as follows: if the new size is less than the current size, it erases the excess elements at the end; if it is greater, it appends additional default-inserted or specified elements [5]. When the new size is identical to the current size, no elements are erased or appended, resulting in no changes to the container's state [5][4].
Citations:
- 1: https://en.cppreference.com/cpp/container/vector/resize
- 2: https://cppreference.dev/w/cpp/container/vector/resize
- 3: https://en.cppreference.com/Template:cpp/container/resize
- 4: https://stackoverflow.com/questions/53447386/what-if-size-argument-for-stdvectorresize-is-equal-to-the-current-size
- 5: https://timsong-cpp.github.io/cppwp/std23/vector
Reset decoder result state after incomplete decode.
decode() and decodeSubResolution() catch failures and return normally, but they leave frameInfo_, metadata, and pDecoded_ from previous successful decodes unchanged or only resized. Reused decoders must clear result state before each decode attempt, initialize the destination buffer before line pulls, and report a failure/completion status so callers do not return partial pixels or stale metadata.
This applies to:
packages/openjphjs/src/HTJ2KDecoder.hpp#L156-L170packages/openjphjs/src/HTJ2KDecoder.hpp#L178-L192
📍 Affects 1 file
packages/openjphjs/src/HTJ2KDecoder.hpp#L163-L169(this comment)packages/openjphjs/src/HTJ2KDecoder.hpp#L185-L191
🤖 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/openjphjs/src/HTJ2KDecoder.hpp` around lines 163 - 169, Update
decode() at packages/openjphjs/src/HTJ2KDecoder.hpp#L156-L170 and
decodeSubResolution() at packages/openjphjs/src/HTJ2KDecoder.hpp#L178-L192 to
clear frameInfo_, metadata, and pDecoded_ before each attempt, initialize the
destination buffer before pulling lines, and return or expose an explicit
failure/completion status when exceptions occur so callers cannot observe
partial pixels or stale results.
| const truncatedSize = Math.min(10 * 1024, encodedLossless.length) | ||
| const truncatedBitstream = encodedLossless.slice(0, truncatedSize) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guarantee partial-stream coverage.
Math.min(limit, encodedLength) permits a full stream when the fixture is small. Require an encoded fixture larger than the limit, then slice at the fixed limit.
packages/openjphjs/test/node/index.js#L90-L91: assert thatencodedLossless.lengthexceeds 10 KiB before slicing at 10 KiB.packages/openjphjs/test/truncated.test.js#L112-L113: assert thatencodedLossless.lengthexceedsTRUNCATED_BYTE_LIMITbefore slicing.packages/openjphjs/test/truncated.test.js#L164-L165: apply the same assertion and fixed slice before the truncated performance benchmark.
📍 Affects 2 files
packages/openjphjs/test/node/index.js#L90-L91(this comment)packages/openjphjs/test/truncated.test.js#L112-L113packages/openjphjs/test/truncated.test.js#L164-L165
🤖 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/openjphjs/test/node/index.js` around lines 90 - 91, Ensure the
encoded lossless fixture exceeds the truncation limit before creating partial
streams, then always slice at the fixed limit. In
packages/openjphjs/test/node/index.js lines 90-91, assert encodedLossless.length
exceeds 10 KiB and replace the Math.min-based slice; apply the same assertion
and fixed TRUNCATED_BYTE_LIMIT slice in
packages/openjphjs/test/truncated.test.js lines 112-113 and 164-165 before the
benchmark.
| const reusedDecoder = new codec.HTJ2KDecoder() | ||
| const t0 = performance.now() | ||
| reusedDecoder.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded) | ||
| reusedDecoder.decode() | ||
| reusedDecoder.getDecodedBuffer() | ||
| const reusedMs = performance.now() - t0 | ||
| reusedDecoder.delete() | ||
|
|
||
| const t1 = performance.now() | ||
| const fresh = new codec.HTJ2KDecoder() | ||
| fresh.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded) | ||
| fresh.decode() | ||
| fresh.getDecodedBuffer() | ||
| fresh.delete() | ||
| const freshMs = performance.now() - t1 | ||
|
|
||
| console.log( | ||
| `Single decode — reused decoder: ${reusedMs.toFixed(2)} ms, fresh decoder: ${freshMs.toFixed(2)} ms` | ||
| ) | ||
|
|
||
| expect(reusedMs).toBeLessThan(freshMs) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Measure steady-state decoder reuse.
The timed reusedDecoder path performs its first decode after construction. It does not reuse the decoder before measurement. A single wall-clock sample can also fail due to scheduler and WebAssembly warm-up noise.
Use medianDecodeMs for both paths. Keep the reused decoder alive across its warm-up and samples. Create and delete a fresh decoder inside each fresh-path sample.
Proposed change
const reusedDecoder = new codec.HTJ2KDecoder()
-const t0 = performance.now()
-reusedDecoder.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded)
-reusedDecoder.decode()
-reusedDecoder.getDecodedBuffer()
-const reusedMs = performance.now() - t0
-reusedDecoder.delete()
+let reusedMs
+try {
+ reusedMs = medianDecodeMs(() => {
+ reusedDecoder.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded)
+ reusedDecoder.decode()
+ reusedDecoder.getDecodedBuffer()
+ })
+} finally {
+ reusedDecoder.delete()
+}
-const t1 = performance.now()
-const fresh = new codec.HTJ2KDecoder()
-fresh.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded)
-fresh.decode()
-fresh.getDecodedBuffer()
-fresh.delete()
-const freshMs = performance.now() - t1
+const freshMs = medianDecodeMs(() => {
+ const fresh = new codec.HTJ2KDecoder()
+ try {
+ fresh.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded)
+ fresh.decode()
+ fresh.getDecodedBuffer()
+ } finally {
+ fresh.delete()
+ }
+})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const reusedDecoder = new codec.HTJ2KDecoder() | |
| const t0 = performance.now() | |
| reusedDecoder.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded) | |
| reusedDecoder.decode() | |
| reusedDecoder.getDecodedBuffer() | |
| const reusedMs = performance.now() - t0 | |
| reusedDecoder.delete() | |
| const t1 = performance.now() | |
| const fresh = new codec.HTJ2KDecoder() | |
| fresh.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded) | |
| fresh.decode() | |
| fresh.getDecodedBuffer() | |
| fresh.delete() | |
| const freshMs = performance.now() - t1 | |
| console.log( | |
| `Single decode — reused decoder: ${reusedMs.toFixed(2)} ms, fresh decoder: ${freshMs.toFixed(2)} ms` | |
| ) | |
| expect(reusedMs).toBeLessThan(freshMs) | |
| const reusedDecoder = new codec.HTJ2KDecoder() | |
| let reusedMs | |
| try { | |
| reusedMs = medianDecodeMs(() => { | |
| reusedDecoder.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded) | |
| reusedDecoder.decode() | |
| reusedDecoder.getDecodedBuffer() | |
| }) | |
| } finally { | |
| reusedDecoder.delete() | |
| } | |
| const freshMs = medianDecodeMs(() => { | |
| const fresh = new codec.HTJ2KDecoder() | |
| try { | |
| fresh.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded) | |
| fresh.decode() | |
| fresh.getDecodedBuffer() | |
| } finally { | |
| fresh.delete() | |
| } | |
| }) | |
| console.log( | |
| `Single decode — reused decoder: ${reusedMs.toFixed(2)} ms, fresh decoder: ${freshMs.toFixed(2)} ms` | |
| ) | |
| expect(reusedMs).toBeLessThan(freshMs) |
🤖 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/openjphjs/test/truncated.test.js` around lines 264 - 284, Update the
benchmark around reusedDecoder and fresh to use medianDecodeMs for both paths:
warm up reusedDecoder before collecting samples, keep it alive through all
reused samples, and create and delete a fresh HTJ2KDecoder within each
fresh-path sample. Compare the resulting median timings and retain cleanup after
sampling.
Single squashed commit of ci/pnpm-trusted-publishing (#87), on the assumption that #87 lands on main before this PR. Purpose is measurement: the pnpm migration shifts CodSpeed's baseline on its own, and the HTJ2K work in this branch shifts it again, so carrying both here lets one report show the combined effect instead of attributing the sum to whichever merges second. Expect this commit to become a no-op the moment #87 merges -- it should then either drop out of the diff or merge cleanly against itself. It is NOT a second copy of that work to review; review it in #87. Merged with no conflicts. Two things worth noting about the overlap: - The submodule gitlink stayed at this branch's 0748112b rather than taking #87's e01c7b7, because #87 only reverted its own accidental bump back to the value main already had. Updated separately in the next commit. - dicom-codec/src/codecs/codecFactory.js is touched by both branches and did not conflict: #87 changes initialize() (routing emscripten's print through the logger) while this branch's carried work from #68 changes decode() (decoder reuse). They are independent edits to the same file. Includes the pnpm.overrides pinning esbuild/rollup/webpack/terser to the versions yarn.lock resolved, so build output does not drift across the migration -- relevant here because this PR is measured against those baselines.
…ength Carried from #68, which this PR supersedes. Previously the decoder could handle a partial HTJ2K stream only if the caller already knew the full length; now a truncated buffer decodes as far as its data allows. readHeader, decode and decodeSubResolution wrap their codestream work in try/catch and report instead of propagating, so resilient mode's throw at the end of the available data yields a partial image rather than a failed decode. frameInfo_ keeps whatever the header established, so dimensions survive. Two deliberate changes from #68's version: - The diagnostics are OJPH_WARN, not OJPH_INFO. jslib.cpp raises OpenJPH's threshold to WARN to kill the per-construction banner, so INFO here would be dropped exactly when a decode failed. - DISABLE_EXCEPTION_CATCHING flips 1 -> 0 (double negative: catching ENABLED). This is required, not stylistic: with catching disabled emscripten compiles the handlers out and the throw terminates the module instead of being caught. It costs wasm size, so dist-size may need re-baselining. test/truncated.test.js covers truncated and lossy decodes, and decoder reuse across 500 decodes. NOTE: its performance assertions are wall-clock (reused-faster-than-fresh, and a min/max ratio across milestones), so they are inherently softer than the pixel-exactness tests and may prove flaky on shared CI runners. Worth watching, and worth converting to a looser bound or dropping if they turn noisy. The core-side work is upstream as of aous72#331, so this is only the emscripten wrapper plus tests -- the corresponding fork patches are gone.
Carried from #68. codecFactory.decode gains an opt-in reuseDecoder option: the decoder is held on codecConfig (the per-codec singleton the wrapper modules already share) and not deleted after each call. htj2k.js opts in; every other codec keeps the construct-and-delete behaviour. This is very likely the bulk of #68's measured 3.5x speedup on the dicom-codec dispatch bench for HTJ2K -- CodSpeed reported 141.4ms -> 40.6ms there, and 40.6ms is about what openjphjs' own direct decode benches cost, i.e. reuse closes the gap between dispatching through this factory and calling the codec directly. Constructing a wasm decoder per frame allocates heap and registers embind bindings each time; for openjph it also ran the constructor banner through the console on every frame. Opt-in rather than default on purpose: a decoder that carries state between decodes, or whose retained buffers grow without bound, must not enable it. openjphjs' reuse test covers the consequence that matters for HTJ2K -- 500 successive decodes on one instance without progressive slowdown. Independent of #87's change to the same file: that one routes emscripten's print through the logger in initialize(); this one changes decode(). They merged with no conflict.
|
Superseded by #76, which now carries all of this work. Closing. Everything here was folded in, with two deliberate changes:
Carried across verbatim: Also worth recording here: the decoder-reuse change is very likely the bulk of the 3.5× dispatch speedup this PR measured (141.4 ms → 40.6 ms on Your |
1 participant
The previous version of the htj2k decoder could decode a partial htj2k stream IF it knew the exact full length beforehand. This change allows just giving the htj2k a partial input and allowing it to decode. Most of hte changes are in cornerstonejs/OpenJPH#3
Also added new tests for this and change the decoder to work repeatedly rather than getting slower and slower.
Summary by CodeRabbit
Performance
Reliability
Compatibility