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.
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