Update fork to upstream OpenJPH 0.30.1 (re-apply resilient-decode patches)#5
Open
sedghi wants to merge 441 commits into
Open
Update fork to upstream OpenJPH 0.30.1 (re-apply resilient-decode patches)#5sedghi wants to merge 441 commits into
sedghi wants to merge 441 commits into
Conversation
Adds documentation for ASAN build type
Two bug fixes; 1. It defines the function gen_irv_tx_from_cb64, which should be rarely called -- it makes little sense that someone creates a lossy bitstream, and then uses a needlessly fine quantization. For this reason, I did not bother defining corresponding sse2, avx2, or wasm variants; some of these accelerated functions require work. 2. There was a bug in one assert function assert(top_atk == NULL);.
* arch: AVX-512 detection, Release CI, and portable inline macros Tighten runtime AVX-512 feature detection, build the CI matrix in Release, and add OJPH_FORCE_INLINE / OJPH_NO_INLINE macros in ojph_arch.h that the AVX2 block coder uses in place of repeated per-function #ifdef blocks. * AVX2 block decoder optimizations Group the AVX2 HTJ2K cleanup-pass decoder optimizations: - Wide UVLC table for non-initial quad rows, merged dual MagSgn fetch, widened frwd_advance, and reduced scratch-buffer memset. - Force-inline decode_two_quad32_avx2 (MSVC-guarded __forceinline). - Decompose the ~770-line ojph_decode_codeblock_avx2 into a thin dispatcher over four noinline helpers (decode_cb_step1_vlc, decode_cb_step2_32bit, decode_cb_step2_16bit, decode_cb_spp_mrp) so each pass gets its own register allocation, cutting spills in the hot step-1 VLC loop. Decoder output is byte-identical to the previous implementation and the full ctest suite passes. On Zen 5 / GCC the decomposition reduces decode instruction count ~5.5% (8-bit) / ~1.5% (16-bit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * AVX2 block encoder optimizations Reduce branch overhead in the AVX2 HTJ2K block encoder's MEL, MagSgn, and VLC paths, and add an Apple-Clang/Intel fallback (separate header) to work around a codegen issue observed on that toolchain. Also adds a 64-bit count_trailing_zeros to ojph_arch.h, used by the encoder's SWAR MagSgn accumulator drain. * AVX2 decoder: destuff MagSgn/SPP once, fetch at absolute bit positions Replace the stateful frwd_struct fetch/advance machinery in the AVX2 block decoder with a one-pass de-stuffer that flattens the MagSgn (and SPP) segment into a stack buffer, from which bits are fetched at absolute positions. Since the bit counts (total_mn) depend only on VLC outputs, the loop-carried dependency in step-2 collapses from a fetch->advance memory round-trip through frwd_struct::tmp to a single scalar position add; the refill branches and the merged-fetch condition disappear from the hot loops entirely. Positions past the end of the data are clamped into the buffer padding, preserving the fill semantics (1s for MagSgn, 0s for SPP) without initializing the whole buffer. Also replace the 14-instruction uniform-shift emulation of the missing _mm256_sllv_epi16 in decode_four_quad16 with a pshufb power-of-two lookup and a 16-bit multiply (6 instructions), and drop the now-unused frwd_struct machinery from this file. Measured on Zen 5 (9950X), single thread, in-memory full decode, median of 30, vs previous commit (AVX2 path, AVX-512 disabled): - u10_8bit lossless 64x64: 53.68 -> 51.60 ms (-3.9%) - u10_8bit lossless 512x8: 54.41 -> 52.04 ms (-4.3%) - u10_8bit lossy qstep 0.001 64x64: 69.00 -> 64.70 ms (-6.2%) - ElephantDream_4K 16bit 64x64: 67.93 -> 63.91 ms (-5.9%) Decoded output verified byte-identical on all of the above plus an 8-wide/512-tall block configuration, 24 truncated-stream cases and 28 corrupted-byte cases (identical output or identical error); ASAN-clean; all 82 ctest tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * AVX2 decoder: destuff the backward VLC/MRP streams as well Extend the destuffed-bitstream approach of the previous commit to the backward-growing streams. destuff_rev flattens the VLC (and MRP) segment once per codeblock -- a SIMD fast path handles 16 bytes at a time when no byte needs unstuffing, exploiting that the unstuff state always equals (next byte in memory > 0x8F) after the first byte -- and the step-1 VLC loop reads it through a branchless register-resident window: drefill inserts whole bytes above the remaining bits (8-byte load + or, no branches), so the refill load stays off the per-quad-pair critical chain, and dconsume is a plain shift. With at least 56 bits after refill and at most 44 bits consumed per quad pair, one refill per pair always suffices. This removes the rev_read unstuffing work and its refill branches from the hot loop; the MRP loop uses a positional dfetch64 (it consumes at most 16 bits per stripe). The rev_struct machinery is no longer used in this file and is deleted. Note that a first variant that re-fetched the window positionally per quad pair was a wash: branch misses fell 38% but the fetch load lengthened the loop-carried chain. Keeping the window across pairs captures the miss reduction without the latency: on u10_8bit lossless, full-decode branch misses drop 38% and IPC rises from 3.09 to 3.41. Measured on Zen 5 (9950X), single thread, in-memory full decode, median of 30, vs previous commit (AVX2 path, AVX-512 disabled): - u10_8bit lossless 64x64: 49.92 -> 48.13 ms (-3.6%) - u10_8bit lossless 512x8: 51.24 -> 48.14 ms (-6.0%) - u10_8bit lossy qstep 0.001 64x64: 62.93 -> 59.18 ms (-6.0%) - ElephantDream_4K 16bit 64x64: 63.40 -> 59.80 ms (-5.7%) - kdu HT -rate 2 (CUP+SPP+MRP): 34.42 -> 33.54 ms (-2.6%) Verified byte-identical against the pre-optimization decoder on 12 configurations, including Kakadu-encoded multipass HT codestreams (decode_cb_spp_mrp confirmed reached with num_passes=3; OpenJPH-encoded streams never exercise it), plus 24 truncation and 30 corrupted-byte cases (identical output or identical error); ASAN-clean on all of the above; all 82 ctest tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: let CMake auto-detect Visual Studio on windows-latest The windows-latest runner image moved to windows-2025-vs2026 on 2026-06-08, which no longer ships Visual Studio 17 2022, so the hardcoded generator fails at configure time. Dropping -G lets CMake pick the newest installed Visual Studio on both old and new images; -A x64 still selects the platform. The WindowsOnARM jobs keep the explicit generator, as the windows-11-arm image still provides it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * AVX2 decoder: fix strict-aliasing UB and clamp the VLC window refill The multipass irv97 conformance streams decoded incorrectly (~4% MSE deviation) when built with MinGW GCC 15.2/16.1 at -O2/-O3, which broke the Windows-MSYS2 CI job after the runner image update; -O1 or -fno-strict-aliasing or -fno-tree-dse made the problem disappear. The root cause is pre-existing type-punned accesses in the SPP/MRP pass: prev_sig/cur_sig entries are written as ui16 but read through *(ui32*) casts, and the sigma rearrangement stored through a *(ui32*) cast into the ui16 array. Under strict aliasing these accesses do not alias, and GCC 15/16's TBAA-driven dead-store elimination began exploiting that once the surrounding fetch code changed shape. All punned accesses (including the MEL reader's) are replaced with memcpy, which compiles to the same single loads and stores but carries the universal alias set. Verified on MinGW GCC 16.1 (-O3): all 82 tests pass; bisected with a destuff-vs-frwd fuzz harness (primitives were bit-exact; the miscompilation was localized to decode_cb_spp_mrp via #pragma GCC optimize placement). Also clamp the read offset in drefill the same way dfetch/dfetch64 already do: when VLC consumption overruns a truncated stream, reads now come from the zero padding (the exact fill the stream would produce) instead of uninitialized buffer memory beyond the pad. Byte-identical output re-verified on Linux against the pre-optimization decoder for 12 configurations plus truncation cases; 82/82 ctest on both Linux GCC 14 and MinGW GCC 16.1; ASAN clean; decode performance unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The destuffing fast path added in aous72#276 uses _mm_cvtsi128_si64 and _mm_extract_epi64, which are only available when targeting x86-64, breaking compilation on 32-bit x86 (e.g. MinGW32 -march=pentium4). Assemble the two 64-bit halves from 32-bit extracts on i386 instead, mirroring the OJPH_ARCH_X86_64 / i386 split already used by proc_cq2 in ojph_block_encoder_avx2.cpp. The x86-64 path is unchanged (byte-identical disassembly apart from __LINE__ shifts in asserts). Fixes aous72#280. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Bug fixes. * Fixes issues with CI build on Windows * Fix OJPH_ERROR argument order for NLT/SIZ mismatch message (aous72#283) The format string expects SIZ bit depth, signedness, NLT bit depth, signedness, then component index. Passing the component index first caused vfprintf to treat an integer bit depth as a string pointer and crash when reporting the error. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Cary Phillips <cary@ilm.com> --------- Signed-off-by: Cary Phillips <cary@ilm.com> Co-authored-by: Aous Naman <aous@unsw.edu.au> Co-authored-by: Cursor <cursoragent@cursor.com> * Version bump --------- Signed-off-by: Cary Phillips <cary@ilm.com> Co-authored-by: Cary Phillips <cary@ilm.com> Co-authored-by: Cursor <cursoragent@cursor.com>
) The shared precinct_scratch buffer was sized from the main COD codeblock geometry only, but consumed using each component's effective (COC) geometry. A COC declaring a smaller codeblock than the COD causes tag_tree::init to write past the end of the buffer (heap OOB write in ojph_precinct.cpp:73). Iterate all components via get_coc(c) when computing the scratch reservation so the buffer covers the worst-case ratio across every component.
* A bug fix.
I make a mistake in my PR.
* Issues/0266 add support for big-endian architecture (aous72#287) * add CI for big-endian s390x architecture * update Dockerfile * Update Dockerfile to Ubuntu 26.04 LTS * add Alpine Linux Dockerfile including instructions for running it using big-endian emulation with QEMU * skip byteswap in encoding functions on big-endian architecture * skip byteswap in decoding functions on big-endian architecture * skip byteswap for ppm 16bit samples with big-endian architecture * add big-endian architecture support for file-formats that assume little-endian on disk * write little-endian pfm files on big-endian architecture * add #include ojph_arch.h, clean up formatting and typos * move #include "ojph_arch.h" below c-library includes * shorten swap_bytes_if... function names * Touch-ups * Touchup2. * Modifying Dockerfile * Modify CI for s390x to make it manual, bc it is too slow. * Update the workflow (forgotten) * Update the workflow (Forgotten 2) * Reduces number of CI builds * Version bump * Extinguishing warnings on MacOS --------- Co-authored-by: michaeldsmith <37905569+michaeldsmith@users.noreply.github.com>
…lder Dockerfile_alpine (aous72#292)
* VSX decoder: Initial support (POWER9/POWER10, ppc64le) (aous72#282) Native 128-bit VSX implementations of the wavelet, colour, and codestream kernels and the HTJ2K block decoder, with runtime dispatch via hwcap. Supported targets are POWER9 (ISA 3.0) and newer, little-endian only; other PPC targets use the generic code paths. Beyond a straight port, the kernels use POWER-specific forms where measurement showed a win: xvrspi for round-to-nearest-away in the float-to-int conversions, vec_sel for masked selects, and a block decoder that destuffs the MagSgn bitstream upfront so per-quad bit consumption is a GPR add instead of a vector-window shift. The SIMD block decoder is dispatched everywhere on POWER10, and for irreversible tile components on POWER9, where it beats the scalar decoder; reversible content on POWER9 stays scalar, which is slightly faster there. Assisted-by: Lance Albertson <lance@osuosl.org> Assisted-by: Thushan Fernando <thushan@thushanfernando.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * Update CI to include emulated ppc64le * Modify code to work on 3 Ubuntu platforms, and test them in CI * Fixes Some Strict Aliasing Rule violations. * Touchups, and moving ojph_simd_vsx.h file to a new shared folder --------- Co-authored-by: Trung Lê <135605+runlevel5@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Lock down and clean up the workflows. * Update ccp-workflow.yml --------- Co-authored-by: Aous Naman <aous@unsw.edu.au>
Bumps [msys2/setup-msys2](https://github.com/msys2/setup-msys2) from 2.31.1 to 2.32.0. - [Release notes](https://github.com/msys2/setup-msys2/releases) - [Changelog](https://github.com/msys2/setup-msys2/blob/main/CHANGELOG.md) - [Commits](msys2/setup-msys2@e989830...66cd2cc) --- updated-dependencies: - dependency-name: msys2/setup-msys2 dependency-version: 2.32.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 7.0.0 to 7.0.1. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@bbbca2d...043fb46) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-version: 7.0.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Fixes an undefined behaviour that shows up in clang * More undefined behaviour fixes.
(cherry picked from commit d6297b2)
(cherry picked from commit 4417d4b)
📝 WalkthroughWalkthroughThis PR overhauls OpenJPH's build infrastructure (CI workflows, CMake, Dockerfile, docs), adds fuzzing harnesses and a new RTP-based streaming decoder app with socket/thread-pool utilities, and substantially refactors the core codec to support 32/64-bit codeblock precision, DFS/NLT-driven wavelet transforms, and updated marker parsing/decoding across SIMD backends. ChangesBuild, CI, and Documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Fuzzing Harness
Estimated code review effort: 2 (Simple) | ~15 minutes Apps Utilities and CLI
Estimated code review effort: 4 (Complex) | ~60 minutes New Stream Expand Application
Estimated code review effort: 5 (Critical) | ~100 minutes Core Codec Engine Refactor
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant Client as UDP Sender
participant Socket as socket/socket_manager
participant PacketsHandler as packets_handler
participant FramesHandler as frames_handler
participant ThreadPool as thread_pool
participant Storer as j2k_frame_storer
Client->>Socket: recvfrom(rtp_packet)
Socket->>PacketsHandler: exchange(packet)
PacketsHandler->>PacketsHandler: order by clipped seq_num
PacketsHandler->>FramesHandler: consume_packet -> push(packet)
FramesHandler->>FramesHandler: assemble frame, track timestamps
FramesHandler->>ThreadPool: add_task(storer) on marked packet
ThreadPool->>Storer: execute()
Storer->>Storer: write_to_file(frame)
Storer->>FramesHandler: notify_file_completion()
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apps/ojph_expand/ojph_expand.cpp (1)
342-345: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing space in
.yuverror message.
"...usage of yuv"concatenates with"file."producing "yuvfile.".✏️ Proposed fix
" needed; in any case, this is not the normal usage of yuv" - "file."); + " file.");🤖 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 `@src/apps/ojph_expand/ojph_expand.cpp` around lines 342 - 345, The error string in the yuv save path is missing a space, causing “yuvfile.” to be concatenated in the message. Update the string literal in ojph_expand’s file-saving error message so the phrase “usage of yuv” and “file.” are separated correctly, keeping the rest of the message unchanged.
🧹 Nitpick comments (3)
src/apps/ojph_compress/CMakeLists.txt (1)
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove empty conditional block or add a comment.
The ARM conditional block is empty with no body or comment. If no ARM-specific image I/O SIMD sources are needed, consider adding a brief comment for clarity, or removing the block entirely.
if (("${OJPH_TARGET_ARCH}" MATCHES "OJPH_ARCH_ARM") OR MULTI_GEN_ARM64) - + # No ARM-specific SIMD image I/O sources needed endif()🤖 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 `@src/apps/ojph_compress/CMakeLists.txt` around lines 46 - 48, The ARM-specific conditional in CMakeLists is empty, so either remove the unused if()/endif() block entirely or add a brief comment inside it to document that no ARM image I/O SIMD sources are currently required. Update the OJPH target architecture check around the empty conditional so the intent is clear when reviewing the build logic later.docs/compiling.md (1)
114-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the
OJPH_DISABLE_SIMDdescription.The sentence "which should include SIMD instructions wherever they are supported" is contradictory —
OJPH_DISABLE_SIMDdisables SIMD, not includes it. Reword for clarity.-The code now employs the architecture-agnostic option `OJPH_DISABLE_SIMD`, which should include SIMD instructions wherever they are supported. This can be achieved with `-DOJPH_DISABLE_SIMD=ON` option during CMake configuration. Individual instruction sets can be disabled; see the options in the main CMakeLists.txt file. +The code employs SIMD instructions by default wherever they are supported. SIMD can be disabled entirely using the architecture-agnostic `-DOJPH_DISABLE_SIMD=ON` option during CMake configuration. Individual instruction sets can also be disabled; see the options in the main CMakeLists.txt file.🤖 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 `@docs/compiling.md` around lines 114 - 117, The description of OJPH_DISABLE_SIMD is contradictory in the compiling guide: it says the option “should include SIMD instructions” even though the name indicates disabling SIMD. Update the wording in the SIMD section to clearly state that OJPH_DISABLE_SIMD turns SIMD off, and keep the reference to the CMake configuration option and the per-instruction-set disable options consistent with that behavior.README.md (1)
15-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix inconsistent link path for Status doc.
Line 15 uses
/docs/status.md(absolute path) while all other Table of Contents entries use./docs/...(relative path). This inconsistency can break rendering on platforms that resolve paths differently.-* [Status](/docs/status.md) +* [Status](./docs/status.md)🤖 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 `@README.md` around lines 15 - 21, The Status entry in the README table of contents uses an absolute docs path while the other entries use relative paths, so update that link to match the same relative style as the surrounding items. Make the change in the README Table of Contents entry for Status so it is consistent with the other links and renders reliably across platforms.
🤖 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 @.github/workflows/ccp-workflow.yml:
- Around line 241-267: The test_ppc64le job is misconfigured in two places: it
still uses the s390x architecture and includes an invalid distro entry. Update
the run-on-arch-action configuration in the test_ppc64le job to use the
PowerPC64LE architecture symbol and replace the matrix value using ubuntu_latest
with a valid supported Ubuntu distro string, keeping the job name aligned with
the actual target platform.
In `@docs/doxygen_style.md`:
- Line 5: The updated documentation sentence still contains two typos in the
prose around the description of source-code documentation. In the text in
docs/doxygen_style.md, fix the wording in that introductory sentence so that the
subject-verb agreement is corrected from “represent” to “represents” and the
phrase “so sort” is corrected to “some sort”; keep the rest of the paragraph
unchanged and make sure the surrounding explanation still reads naturally.
In `@docs/status.md`:
- Line 9: The supported file types summary in the documentation is missing .pfm
even though ojph_compress and ojph_expand now handle it. Update the format list
in the status documentation to include .pfm alongside the existing .pgm, .ppm,
.yuv, .raw, and .dpx entries, keeping the wording consistent with the current
command-line tools description.
In `@docs/web_demos.md`:
- Line 3: Fix the grammar in the web demo description by changing the sentence
in docs/web_demos.md from “It currently host” to “It currently hosts”; update
the prose around the openjph.org and javascript demo reference so the sentence
reads naturally and consistently with the rest of the paragraph.
In `@src/apps/ojph_compress/ojph_compress.cpp`:
- Around line 397-401: The error message in the component-limit check has a
missing space after “JPEG2000.” and uses the wrong format specifier for
num_comps. Update the printf in ojph_compress.cpp’s component-count guard so the
message reads cleanly and prints the ojph::ui32 value with an appropriate
unsigned 32-bit format, keeping the existing check and return behavior
unchanged.
In `@src/apps/ojph_expand/ojph_expand.cpp`:
- Around line 290-293: The `.pfm` validation branch in `ojph_expand.cpp` has
copy-pasted error text that incorrectly says `ppm` instead of `pfm`. Update the
`OJPH_ERROR` message in the `all_same` check inside the `.pfm` save path so it
references the correct format, keeping the rest of the condition and error
handling unchanged.
In `@src/apps/ojph_stream_expand/ojph_stream_expand.cpp`:
- Around line 202-205: The OJPH_ERROR messages in ojph_stream_expand.cpp are
using the wrong format specifier for recv_port and src_port, which are char*
values rather than integers. Update the error handling in the affected
validation blocks near the port parsing logic and the corresponding source-port
check to print the provided string with a string format specifier, using the
existing OJPH_ERROR calls so the messages remain accurate and safe.
- Around line 248-254: `inet_ntop` failure handling in `ojph_stream_expand.cpp`
still falls through to `printf` with a null `%s` argument; update the
`inet_ntop` result checks in the listening-address print path and the other two
matching blocks so they do not call `printf` when `t == NULL`. In the relevant
code near the `inet_ntop` calls, either return/skip the print after logging the
error with `OJPH_INFO` or provide a safe fallback string before formatting, and
apply the same fix consistently to all three occurrences.
- Around line 274-280: The get_arguments path in ojph_stream_expand currently
assumes src_port is set whenever src_addr is provided, and atoi(src_port) can
dereference a null pointer if -src_port is omitted. Add validation in the
src_addr handling block to check src_port before converting it, and raise the
same OJPH_ERROR path with a clear message when -src_addr is used without a port.
Keep the fix localized to get_arguments and the src_addr/src_port parsing logic.
In `@src/apps/ojph_stream_expand/stream_expand_support.cpp`:
- Around line 408-441: The counter update in check_files_in_processing is
happening too early and can make the loop stop before all completed files are
reclaimed. Move the num_complete_files decrement so it only happens after a
stex_file is confirmed done and actually transferred from processing to avail,
and keep the traversal logic in sync for both the head case and the pf->next
case. Verify the loop in check_files_in_processing still walks the full
processing list without reducing num_complete_files for incomplete entries.
In `@src/apps/others/ojph_img_io.cpp`:
- Around line 1664-1670: The sample clamping in the `upper_val` setup inside
`ojph_img_io.cpp` is off by one for both signed and unsigned paths. Update the
bounds in this block so `upper_val` reflects the largest representable sample
value used by the write path: `2^bit_depth - 1` for unsigned and `2^(bit_depth -
1) - 1` for signed. Keep the `is_signed` branch and the surrounding clamping
logic intact, just correct the computed maximum before values are narrowed to
the output type.
In `@src/core/codestream/ojph_codestream_local.cpp`:
- Around line 1059-1062: The NLT handling branch in skip_marker currently logs
the wrong warning text, which still says PPT instead of NLT. Update the
skip_marker call in the marker_idx == 9 branch so the message matches the NLT
marker being processed, keeping the same OJPH_MSG_WARN and resilient behavior.
- Around line 804-831: The COC/QCC diagnostics in read_headers() are using
num_comps, which can still be 0 at this point and produce misleading “0
components” messages. Update the error/info messages in the marker parsing logic
around get_coc(), get_qcc(), and read_qcc() to use siz.get_num_components()
directly for the component count, while keeping the existing component index
checks unchanged.
In `@src/core/codestream/ojph_params.cpp`:
- Around line 1086-1091: The COC missing-ATK error in the loop over COC objects
uses the wrong wavelet/kernel value, so the message reports the COD value
instead of the current COC’s setting. Update the OJPH_ERROR call in the code
path that checks p->atk after get_atk so it references the COC object’s
SPcod.wavelet_trans from p, keeping the message aligned with the failing
segment.
- Around line 459-465: The COM marker length validation in the parameter setter
is checking the wrong value: it should validate the newly computed string length
`t` rather than the member `len`, which may be stale or uninitialized. Update
the length guard in the same code path that assigns `this->data`, `this->len`,
and `this->Rcom` so it rejects any string longer than the allowed maximum before
truncation occurs, matching the validation approach used by `set_data()`.
In `@src/core/coding/ojph_block_decoder64.cpp`:
- Line 1549: The high-precision shift in the block decoder still uses 32-bit
arithmetic, so update the expression in ojph_block_decoder64.cpp where the value
is computed in the decoder logic to use a 64-bit literal instead of the current
unsigned 32-bit one. Locate the shift in the code path that builds the `ui64
val` value and change the literal so the operation stays 64-bit throughout,
preserving the intended range for larger shift counts.
---
Outside diff comments:
In `@src/apps/ojph_expand/ojph_expand.cpp`:
- Around line 342-345: The error string in the yuv save path is missing a space,
causing “yuvfile.” to be concatenated in the message. Update the string literal
in ojph_expand’s file-saving error message so the phrase “usage of yuv” and
“file.” are separated correctly, keeping the rest of the message unchanged.
---
Nitpick comments:
In `@docs/compiling.md`:
- Around line 114-117: The description of OJPH_DISABLE_SIMD is contradictory in
the compiling guide: it says the option “should include SIMD instructions” even
though the name indicates disabling SIMD. Update the wording in the SIMD section
to clearly state that OJPH_DISABLE_SIMD turns SIMD off, and keep the reference
to the CMake configuration option and the per-instruction-set disable options
consistent with that behavior.
In `@README.md`:
- Around line 15-21: The Status entry in the README table of contents uses an
absolute docs path while the other entries use relative paths, so update that
link to match the same relative style as the surrounding items. Make the change
in the README Table of Contents entry for Status so it is consistent with the
other links and renders reliably across platforms.
In `@src/apps/ojph_compress/CMakeLists.txt`:
- Around line 46-48: The ARM-specific conditional in CMakeLists is empty, so
either remove the unused if()/endif() block entirely or add a brief comment
inside it to document that no ARM image I/O SIMD sources are currently required.
Update the OJPH target architecture check around the empty conditional so the
intent is clear when reviewing the build logic later.
🪄 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: 7637c01d-b3d0-4421-8e48-f56fd39b8f95
⛔ Files ignored due to path filters (1)
fuzzing/seed_corpus/ojph_compress_fuzz_target/w128_h128_b2_79_b3_09.binis excluded by!**/*.bin
📒 Files selected for processing (136)
.github/dependabot.yml.github/workflows/ccp-workflow.yml.github/workflows/cifuzz.yml.github/workflows/codeql.yml.github/workflows/emcc.yml.gitignoreCMakeLists.txtDockerfileREADME.mdbin/.gitignoredocs/compiling.mddocs/docker.mddocs/doxygen_style.mddocs/fuzzing.mddocs/status.mddocs/usage_examples.mddocs/web_demos.mdfuzzing/CMakeLists.txtfuzzing/fuzz_targets/ojph_compress_fuzz_target.cppfuzzing/fuzz_targets/ojph_expand_fuzz_target.cppojph_version.cmakesrc/apps/CMakeLists.txtsrc/apps/common/ojph_img_io.hsrc/apps/common/ojph_sockets.hsrc/apps/common/ojph_threads.hsrc/apps/ojph_compress/CMakeLists.txtsrc/apps/ojph_compress/ojph_compress.cppsrc/apps/ojph_expand/CMakeLists.txtsrc/apps/ojph_expand/ojph_expand.cppsrc/apps/ojph_stream_expand/CMakeLists.txtsrc/apps/ojph_stream_expand/ojph_stream_expand.cppsrc/apps/ojph_stream_expand/stream_expand_support.cppsrc/apps/ojph_stream_expand/stream_expand_support.hsrc/apps/ojph_stream_expand/threaded_frame_processors.cppsrc/apps/ojph_stream_expand/threaded_frame_processors.hsrc/apps/others/ojph_img_io.cppsrc/apps/others/ojph_img_io_avx2.cppsrc/apps/others/ojph_img_io_sse41.cppsrc/apps/others/ojph_sockets.cppsrc/apps/others/ojph_threads.cppsrc/core/CMakeLists.txtsrc/core/codestream/ojph_bitbuffer_read.hsrc/core/codestream/ojph_bitbuffer_write.hsrc/core/codestream/ojph_codeblock.cppsrc/core/codestream/ojph_codeblock.hsrc/core/codestream/ojph_codeblock_fun.cppsrc/core/codestream/ojph_codeblock_fun.hsrc/core/codestream/ojph_codestream.cppsrc/core/codestream/ojph_codestream_avx.cppsrc/core/codestream/ojph_codestream_avx2.cppsrc/core/codestream/ojph_codestream_gen.cppsrc/core/codestream/ojph_codestream_local.cppsrc/core/codestream/ojph_codestream_local.hsrc/core/codestream/ojph_codestream_sse.cppsrc/core/codestream/ojph_codestream_sse2.cppsrc/core/codestream/ojph_codestream_vsx.cppsrc/core/codestream/ojph_codestream_wasm.cppsrc/core/codestream/ojph_params.cppsrc/core/codestream/ojph_params_local.hsrc/core/codestream/ojph_precinct.cppsrc/core/codestream/ojph_precinct.hsrc/core/codestream/ojph_resolution.cppsrc/core/codestream/ojph_resolution.hsrc/core/codestream/ojph_subband.cppsrc/core/codestream/ojph_subband.hsrc/core/codestream/ojph_tile.cppsrc/core/codestream/ojph_tile.hsrc/core/codestream/ojph_tile_comp.cppsrc/core/codestream/ojph_tile_comp.hsrc/core/coding/ojph_block_common.cppsrc/core/coding/ojph_block_common.hsrc/core/coding/ojph_block_decoder.hsrc/core/coding/ojph_block_decoder32.cppsrc/core/coding/ojph_block_decoder64.cppsrc/core/coding/ojph_block_decoder_avx2.cppsrc/core/coding/ojph_block_decoder_ssse3.cppsrc/core/coding/ojph_block_decoder_vsx.cppsrc/core/coding/ojph_block_decoder_wasm.cppsrc/core/coding/ojph_block_encoder.cppsrc/core/coding/ojph_block_encoder.hsrc/core/coding/ojph_block_encoder_avx2.cppsrc/core/coding/ojph_block_encoder_avx2_apple.hsrc/core/coding/ojph_block_encoder_avx512.cppsrc/core/common/ojph_arch.hsrc/core/common/ojph_codestream.hsrc/core/common/ojph_message.hsrc/core/common/ojph_params.hsrc/core/openjph/ojph_arch.hsrc/core/openjph/ojph_arg.hsrc/core/openjph/ojph_base.hsrc/core/openjph/ojph_codestream.hsrc/core/openjph/ojph_defs.hsrc/core/openjph/ojph_file.hsrc/core/openjph/ojph_mem.hsrc/core/openjph/ojph_message.hsrc/core/openjph/ojph_params.hsrc/core/openjph/ojph_simd_vsx.hsrc/core/openjph/ojph_version.hsrc/core/others/ojph_arch.cppsrc/core/others/ojph_file.cppsrc/core/others/ojph_mem.cppsrc/core/others/ojph_mem_c.csrc/core/others/ojph_message.cppsrc/core/shared/ojph_simd_vsx.hsrc/core/transform/ojph_colour.cppsrc/core/transform/ojph_colour.hsrc/core/transform/ojph_colour_avx.cppsrc/core/transform/ojph_colour_avx2.cppsrc/core/transform/ojph_colour_local.hsrc/core/transform/ojph_colour_sse.cppsrc/core/transform/ojph_colour_sse2.cppsrc/core/transform/ojph_colour_vsx.cppsrc/core/transform/ojph_colour_wasm.cppsrc/core/transform/ojph_transform.cppsrc/core/transform/ojph_transform.hsrc/core/transform/ojph_transform_avx.cppsrc/core/transform/ojph_transform_avx2.cppsrc/core/transform/ojph_transform_avx512.cppsrc/core/transform/ojph_transform_local.hsrc/core/transform/ojph_transform_sse.cppsrc/core/transform/ojph_transform_sse2.cppsrc/core/transform/ojph_transform_vsx.cppsrc/core/transform/ojph_transform_wasm.cppsrc/openjph-config.cmake.insrc/openjph.pc.insrc/pkg-config.pc.cmakesubprojects/js/CMakeLists.txtsubprojects/js/build.shtarget_arch.cmaketests/CMakeLists.txttests/mse_pae.cmaketests/mse_pae.cpptests/test.pytests/test_executables.cpptests/test_helpers/convert_mse_pae_to_tests.cpptests/test_helpers/ht_cmdlines.txt
💤 Files with no reviewable changes (1)
- bin/.gitignore
Comment on lines
+241
to
+267
| test_ppc64le: | ||
| if: ${{ github.event_name != 'workflow_dispatch' || github.event.inputs.job_to_run == '' || github.event.inputs.job_to_run == 'test_ppc64le' }} | ||
|
|
||
| name: Linux-PowerPC64LE (little-endian) Build and Test | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| include: [ { dist: ubuntu22.04 }, { dist: ubuntu_latest } ] | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 #v6.0.3 | ||
| with: | ||
| persist-credentials: false | ||
|
|
||
| - uses: uraimo/run-on-arch-action@f9b26e3a1a408d5fd530d20c17b9f3f4428ff8d9 #3.1.0 | ||
| with: | ||
| arch: s390x | ||
| distro: ${{ matrix.dist }} | ||
| githubToken: ${{ github.token }} | ||
| install: | | ||
| apt-get update -q -y | ||
| apt-get install -q -y cmake make g++ libtiff-dev python3 | ||
| run: | | ||
| cd build | ||
| cmake -DCMAKE_BUILD_TYPE=Release -DOJPH_BUILD_STREAM_EXPAND=ON -DOJPH_ENABLE_TIFF_SUPPORT=ON -DOJPH_BUILD_TESTS=ON .. | ||
| make | ||
| ctest --output-on-failure |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
test_ppc64le job uses wrong architecture (s390x instead of ppc64le)
The job is named "Linux-PowerPC64LE (little-endian) Build and Test" but sets arch: s390x on line 257. This is a copy-paste error from the test_s390x job — the job will run s390x instead of PowerPC64LE, making it a duplicate of test_s390x rather than testing the intended architecture.
Additionally, dist: ubuntu_latest (line 248) is not a valid distro identifier for uraimo/run-on-arch-action, which expects specific version strings like ubuntu22.04. This will likely cause the second matrix entry to fail.
🔧 Proposed fix for `test_ppc64le` job
- uses: uraimo/run-on-arch-action@f9b26e3a1a408d5fd530d20c17b9f3f4428ff8d9 `#3.1.0`
with:
- arch: s390x
+ arch: ppc64le
distro: ${{ matrix.dist }}And fix the invalid distro value:
strategy:
matrix:
- include: [ { dist: ubuntu22.04 }, { dist: ubuntu_latest } ]
+ include: [ { dist: ubuntu22.04 }, { dist: ubuntu24.04 } ]📝 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.
Suggested change
| test_ppc64le: | |
| if: ${{ github.event_name != 'workflow_dispatch' || github.event.inputs.job_to_run == '' || github.event.inputs.job_to_run == 'test_ppc64le' }} | |
| name: Linux-PowerPC64LE (little-endian) Build and Test | |
| runs-on: ubuntu-latest | |
| strategy: | |
| matrix: | |
| include: [ { dist: ubuntu22.04 }, { dist: ubuntu_latest } ] | |
| steps: | |
| - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 #v6.0.3 | |
| with: | |
| persist-credentials: false | |
| - uses: uraimo/run-on-arch-action@f9b26e3a1a408d5fd530d20c17b9f3f4428ff8d9 #3.1.0 | |
| with: | |
| arch: s390x | |
| distro: ${{ matrix.dist }} | |
| githubToken: ${{ github.token }} | |
| install: | | |
| apt-get update -q -y | |
| apt-get install -q -y cmake make g++ libtiff-dev python3 | |
| run: | | |
| cd build | |
| cmake -DCMAKE_BUILD_TYPE=Release -DOJPH_BUILD_STREAM_EXPAND=ON -DOJPH_ENABLE_TIFF_SUPPORT=ON -DOJPH_BUILD_TESTS=ON .. | |
| make | |
| ctest --output-on-failure | |
| test_ppc64le: | |
| if: ${{ github.event_name != 'workflow_dispatch' || github.event.inputs.job_to_run == '' || github.event.inputs.job_to_run == 'test_ppc64le' }} | |
| name: Linux-PowerPC64LE (little-endian) Build and Test | |
| runs-on: ubuntu-latest | |
| strategy: | |
| matrix: | |
| include: [ { dist: ubuntu22.04 }, { dist: ubuntu24.04 } ] | |
| steps: | |
| - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 `#v6.0.3` | |
| with: | |
| persist-credentials: false | |
| - uses: uraimo/run-on-arch-action@f9b26e3a1a408d5fd530d20c17b9f3f4428ff8d9 `#3.1.0` | |
| with: | |
| arch: ppc64le | |
| distro: ${{ matrix.dist }} | |
| githubToken: ${{ github.token }} | |
| install: | | |
| apt-get update -q -y | |
| apt-get install -q -y cmake make g++ libtiff-dev python3 | |
| run: | | |
| cd build | |
| cmake -DCMAKE_BUILD_TYPE=Release -DOJPH_BUILD_STREAM_EXPAND=ON -DOJPH_ENABLE_TIFF_SUPPORT=ON -DOJPH_BUILD_TESTS=ON .. | |
| make | |
| ctest --output-on-failure |
🤖 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/ccp-workflow.yml around lines 241 - 267, The test_ppc64le
job is misconfigured in two places: it still uses the s390x architecture and
includes an invalid distro entry. Update the run-on-arch-action configuration in
the test_ppc64le job to use the PowerPC64LE architecture symbol and replace the
matrix value using ubuntu_latest with a valid supported Ubuntu distro string,
keeping the job name aligned with the actual target platform.
| Documentation is still experimental for me, and I might change things down the line. | ||
|
|
||
| Here, we describe how to document the source code. This represent so sort of minial set of markers that need to be used. Other markers can be used to enhance the documentation of the code. This serves as a live document that can be updated when needed. | ||
| Here, we describe how to document the source code. This represent so sort of minimal set of markers that need to be used. Other markers can be used to enhance the documentation of the code. This serves as a live document that can be updated when needed. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typos remain after the wording update.
The line range change details indicate this line was updated to correct typos, but two issues persist: "represent" should be "represents" (subject-verb agreement) and "so sort" should be "some sort".
📝 Proposed fix
-Here, we describe how to document the source code. This represent so sort of minimal set of markers that need to be used. Other markers can be used to enhance the documentation of the code. This serves as a live document that can be updated when needed.
+Here, we describe how to document the source code. This represents some sort of minimal set of markers that need to be used. Other markers can be used to enhance the documentation of the code. This serves as a live document that can be updated when needed.📝 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.
Suggested change
| Here, we describe how to document the source code. This represent so sort of minimal set of markers that need to be used. Other markers can be used to enhance the documentation of the code. This serves as a live document that can be updated when needed. | |
| Here, we describe how to document the source code. This represents some sort of minimal set of markers that need to be used. Other markers can be used to enhance the documentation of the code. This serves as a live document that can be updated when needed. |
🤖 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 `@docs/doxygen_style.md` at line 5, The updated documentation sentence still
contains two typos in the prose around the description of source-code
documentation. In the text in docs/doxygen_style.md, fix the wording in that
introductory sentence so that the subject-verb agreement is corrected from
“represent” to “represents” and the phrase “so sort” is corrected to “some
sort”; keep the rest of the paragraph unchanged and make sure the surrounding
explanation still reads naturally.
|
|
||
| As it stands, the OpenJPH library needs documentation. The provided encoder ojph\_compress only generates HTJ2K codestreams, with the extension j2c; the generated files lack the .jph header. Adding the .jph header is of little urgency, as the codestream contains all needed information to properly decode an image. The .jph header will be added at a future point in time. The provided decoder ojph\_expand decodes .jph files, by ignoring the .jph header if it is present. | ||
|
|
||
| The provided command line tools ojph\_compress and ojph\_expand accepts and generates .pgm, .ppm, .yuv, .raw, and .dpx. See the usage examples below. No newline at end of file |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Missing .pfm format in supported file types list.
This PR adds PFM input/output support (documented in docs/usage_examples.md line 25 and confirmed by the review stack context for ojph_compress/ojph_expand), but line 9 does not list .pfm alongside the other supported formats.
📝 Proposed fix
-The provided command line tools ojph\_compress and ojph\_expand accepts and generates .pgm, .ppm, .yuv, .raw, and .dpx. See the usage examples below.
+The provided command line tools ojph\_compress and ojph\_expand accepts and generates .pgm, .ppm, .pfm, .yuv, .raw, and .dpx. See the usage examples below.📝 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.
Suggested change
| The provided command line tools ojph\_compress and ojph\_expand accepts and generates .pgm, .ppm, .yuv, .raw, and .dpx. See the usage examples below. | |
| The provided command line tools ojph\_compress and ojph\_expand accepts and generates .pgm, .ppm, .pfm, .yuv, .raw, and .dpx. See the usage examples below. |
🤖 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 `@docs/status.md` at line 9, The supported file types summary in the
documentation is missing .pfm even though ojph_compress and ojph_expand now
handle it. Update the format list in the status documentation to include .pfm
alongside the existing .pgm, .ppm, .yuv, .raw, and .dpx entries, keeping the
wording consistent with the current command-line tools description.
| @@ -0,0 +1,5 @@ | |||
| # Web-based Demos # | |||
|
|
|||
| The associate site [openjph.org](https://openjph.org) serves as a blog. It currently host the [javascript](https://openjph.org/javascript/demo.html) demo of the decoder; the webpage demonstrates that the library can be compiled to javascript, and can run inside a web-browser. Any browser supporting webassembly can be used to view this webpage; examples include Firefox, Chrome, Safari, and Edge, on a desktop, mobile, or tablet. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Grammar: "host" should be "hosts"
"It currently host the" should read "It currently hosts the".
✏️ Proposed fix
-The associate site [openjph.org](https://openjph.org) serves as a blog. It currently host the [javascript](https://openjph.org/javascript/demo.html) demo of the decoder;
+The associate site [openjph.org](https://openjph.org) serves as a blog. It currently hosts the [javascript](https://openjph.org/javascript/demo.html) demo of the decoder;📝 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.
Suggested change
| The associate site [openjph.org](https://openjph.org) serves as a blog. It currently host the [javascript](https://openjph.org/javascript/demo.html) demo of the decoder; the webpage demonstrates that the library can be compiled to javascript, and can run inside a web-browser. Any browser supporting webassembly can be used to view this webpage; examples include Firefox, Chrome, Safari, and Edge, on a desktop, mobile, or tablet. | |
| The associate site [openjph.org](https://openjph.org) serves as a blog. It currently hosts the [javascript](https://openjph.org/javascript/demo.html) demo of the decoder; the webpage demonstrates that the library can be compiled to javascript, and can run inside a web-browser. Any browser supporting webassembly can be used to view this webpage; examples include Firefox, Chrome, Safari, and Edge, on a desktop, mobile, or tablet. |
🤖 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 `@docs/web_demos.md` at line 3, Fix the grammar in the web demo description by
changing the sentence in docs/web_demos.md from “It currently host” to “It
currently hosts”; update the prose around the openjph.org and javascript demo
reference so the sentence reads naturally and consistently with the rest of the
paragraph.
Comment on lines
+397
to
+401
| if (num_comps > 16384) { | ||
| printf("More than 16384 components is not supported in JPEG2000." | ||
| "You specified %d\n", num_comps); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Minor: message spacing and format specifier.
The two string literals concatenate to ...JPEG2000.You specified (missing space), and %d is used for an ojph::ui32 (value can exceed INT_MAX, which is UB for %d).
✏️ Proposed fix
if (num_comps > 16384) {
- printf("More than 16384 components is not supported in JPEG2000."
- "You specified %d\n", num_comps);
+ printf("More than 16384 components is not supported in JPEG2000. "
+ "You specified %u\n", num_comps);
return false;
}📝 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.
Suggested change
| if (num_comps > 16384) { | |
| printf("More than 16384 components is not supported in JPEG2000." | |
| "You specified %d\n", num_comps); | |
| return false; | |
| } | |
| if (num_comps > 16384) { | |
| printf("More than 16384 components is not supported in JPEG2000. " | |
| "You specified %u\n", num_comps); | |
| return false; | |
| } |
🤖 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 `@src/apps/ojph_compress/ojph_compress.cpp` around lines 397 - 401, The error
message in the component-limit check has a missing space after “JPEG2000.” and
uses the wrong format specifier for num_comps. Update the printf in
ojph_compress.cpp’s component-count guard so the message reads cleanly and
prints the ojph::ui32 value with an appropriate unsigned 32-bit format, keeping
the existing check and return behavior unchanged.
Comment on lines
+804
to
+831
| if (p->get_comp_idx() >= siz.get_num_components()) | ||
| OJPH_INFO(0x00030056, "The codestream carries a COC marker " | ||
| "segment for a component indexed by %d, which is more than the " | ||
| "allowed index number, since the codestream has %d components", | ||
| p->get_comp_idx(), num_comps); | ||
| param_cod *q = cod.get_coc(p->get_comp_idx()); | ||
| if (p != q && p->get_comp_idx() == q->get_comp_idx()) | ||
| OJPH_ERROR(0x00030057, "The codestream has two COC marker " | ||
| "segments for one component of index %d", p->get_comp_idx()); | ||
| } | ||
| else if (marker_idx == 5) | ||
| { qcd.read(file); received_markers |= 2; } | ||
| { | ||
| qcd.read(file); | ||
| received_markers |= 2; | ||
| } | ||
| else if (marker_idx == 6) | ||
| { | ||
| ui32 num_comps = siz.get_num_components(); | ||
| if (qcc == qcc_store && | ||
| num_comps * sizeof(param_qcc) > sizeof(qcc_store)) | ||
| { | ||
| qcc = new param_qcc[num_comps]; | ||
| } | ||
| qcc[used_qcc_fields++].read(file, num_comps); | ||
| } | ||
| { | ||
| param_qcd* p = qcd.add_qcc_object(param_qcd::OJPH_QCD_UNKNOWN); | ||
| p->read_qcc(file, siz.get_num_components()); | ||
| if (p->get_comp_idx() >= siz.get_num_components()) | ||
| OJPH_ERROR(0x00030054, "The codestream carries a QCC marker " | ||
| "segment for a component indexed by %d, which is more than the " | ||
| "allowed index number, since the codestream has %d components", | ||
| p->get_comp_idx(), num_comps); | ||
| param_qcd *q = qcd.get_qcc(p->get_comp_idx()); | ||
| if (p != q && p->get_comp_idx() == q->get_comp_idx()) | ||
| OJPH_ERROR(0x00030055, "The codestream has two QCC marker " | ||
| "segments for one component of index %d", p->get_comp_idx()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use siz.get_num_components() in these diagnostics
num_comps is still 0 while read_headers() parses COC/QCC, so these messages can print “0 components”. Use the already-checked siz.get_num_components() instead.
🤖 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 `@src/core/codestream/ojph_codestream_local.cpp` around lines 804 - 831, The
COC/QCC diagnostics in read_headers() are using num_comps, which can still be 0
at this point and produce misleading “0 components” messages. Update the
error/info messages in the marker parsing logic around get_coc(), get_qcc(), and
read_qcc() to use siz.get_num_components() directly for the component count,
while keeping the existing component index checks unchanged.
Comment on lines
+1059
to
+1062
| else if (marker_idx == 9) | ||
| result = skip_marker(infile, "NLT", | ||
| "PPT marker segment in a tile is not supported yet", | ||
| OJPH_MSG_WARN, resilient); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrong marker name in the NLT warning text.
This branch handles the NLT marker but the message says "PPT marker segment in a tile is not supported yet". Copy/paste from the PPT branch.
✏️ Proposed fix
else if (marker_idx == 9)
result = skip_marker(infile, "NLT",
- "PPT marker segment in a tile is not supported yet",
+ "NLT marker segment in a tile is not supported yet",
OJPH_MSG_WARN, resilient);📝 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.
Suggested change
| else if (marker_idx == 9) | |
| result = skip_marker(infile, "NLT", | |
| "PPT marker segment in a tile is not supported yet", | |
| OJPH_MSG_WARN, resilient); | |
| else if (marker_idx == 9) | |
| result = skip_marker(infile, "NLT", | |
| "NLT marker segment in a tile is not supported yet", | |
| OJPH_MSG_WARN, resilient); |
🤖 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 `@src/core/codestream/ojph_codestream_local.cpp` around lines 1059 - 1062, The
NLT handling branch in skip_marker currently logs the wrong warning text, which
still says PPT instead of NLT. Update the skip_marker call in the marker_idx ==
9 branch so the message matches the NLT marker being processed, keeping the same
OJPH_MSG_WARN and resilient behavior.
Comment on lines
459
to
465
| size_t t = strlen(str); | ||
| if (len > 65531) | ||
| OJPH_ERROR(0x000500C1, | ||
| OJPH_ERROR(0x000500C1, | ||
| "COM marker string length cannot be larger than 65531"); | ||
| this->data = str; | ||
| this->data = str; | ||
| this->len = (ui16)t; | ||
| this->Rcom = 1; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Length guard checks the wrong variable.
t holds the new string length, but the guard tests the (pre-existing/uninitialized) member len. An over-long string bypasses the check and then gets truncated by this->len = (ui16)t, producing a malformed COM segment length. Compare with set_data(), which correctly validates its len parameter.
🐛 Proposed fix
size_t t = strlen(str);
- if (len > 65531)
+ if (t > 65531)
OJPH_ERROR(0x000500C1,
"COM marker string length cannot be larger than 65531");📝 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.
Suggested change
| size_t t = strlen(str); | |
| if (len > 65531) | |
| OJPH_ERROR(0x000500C1, | |
| OJPH_ERROR(0x000500C1, | |
| "COM marker string length cannot be larger than 65531"); | |
| this->data = str; | |
| this->data = str; | |
| this->len = (ui16)t; | |
| this->Rcom = 1; | |
| size_t t = strlen(str); | |
| if (t > 65531) | |
| OJPH_ERROR(0x000500C1, | |
| "COM marker string length cannot be larger than 65531"); | |
| this->data = str; | |
| this->len = (ui16)t; | |
| this->Rcom = 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 `@src/core/codestream/ojph_params.cpp` around lines 459 - 465, The COM marker
length validation in the parameter setter is checking the wrong value: it should
validate the newly computed string length `t` rather than the member `len`,
which may be stale or uninitialized. Update the length guard in the same code
path that assigns `this->data`, `this->len`, and `this->Rcom` so it rejects any
string longer than the allowed maximum before truncation occurs, matching the
validation approach used by `set_data()`.
Comment on lines
+1086
to
+1091
| p->atk = atk->get_atk(p->SPcod.wavelet_trans); | ||
| if (p->atk == NULL) | ||
| OJPH_ERROR(0x00050132, "A COC segment employs the DWT kernel " | ||
| "atk = %d, but a corresponding ATK segment cannot be found", | ||
| SPcod.wavelet_trans); | ||
| p = p->next; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
COC error reports the COD's kernel, not the COC's.
Inside the loop over COC objects (p), the "corresponding ATK segment cannot be found" message interpolates SPcod.wavelet_trans (this == COD_MAIN) instead of p->SPcod.wavelet_trans, so it will report the wrong ATK index for a failing COC.
🤖 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 `@src/core/codestream/ojph_params.cpp` around lines 1086 - 1091, The COC
missing-ATK error in the loop over COC objects uses the wrong wavelet/kernel
value, so the message reports the COD value instead of the current COC’s
setting. Update the OJPH_ERROR call in the code path that checks p->atk after
get_atk so it references the COC object’s SPcod.wavelet_trans from p, keeping
the message aligned with the failing segment.
| // new_sig has newly-discovered sig. samples during SPP | ||
| // find the signs and update decoded_data | ||
| ui64 *dp = dpp + x; | ||
| ui64 val = 3u << (p - 2); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the local context around the suspected line.
sed -n '1528,1562p' src/core/coding/ojph_block_decoder64.cpp
# Find other similar magnitude-shift literals in this file for context.
rg -n "3u <<|3ull <<|1ULL <<|v_n \+ 2" src/core/coding/ojph_block_decoder64.cppRepository: cornerstonejs/OpenJPH
Length of output: 1965
🌐 Web query:
OpenJPH 0.30.1 ojph_block_decoder64.cpp 3u << (p - 2) 3ull
💡 Result:
In the OpenJPH library, specifically within the block decoder implementations (such as ojph_block_decoder64.cpp), the code patterns 3u << (p - 2) and 3ull are typically used for bitwise operations related to HTJ2K coefficient decoding [1][2]. 1. 3u << (p - 2): This expression is used to generate a bitmask or shift a value by a dynamic amount, where p relates to the bit-plane index or precision within the HTJ2K decoding process [1][2]. The u suffix specifies that the literal 3 is treated as an unsigned int. 2. 3ull: This is an unsigned long long literal, used to ensure 64-bit precision during bitwise arithmetic to prevent overflow or truncation when working with 64-bit codeblock data [1][2]. Technical Context: These operations appear within the core decoding logic, which handles High-Throughput JPEG2000 (HTJ2K) coefficient reconstruction [1][3]. OpenJPH employs various SIMD and scalar optimizations to accelerate this process [2][4]. Recently, developers have addressed strict-aliasing and undefined behavior issues in these decoder files (e.g., ojph_block_decoder32.cpp and ojph_block_decoder64.cpp) by replacing type-punned pointer accesses (like *(ui32*)) with safer memcpy operations to prevent miscompilation by modern compilers such as GCC 15 [5][4].
Citations:
- 1: https://deepwiki.com/aous72/OpenJPH/5.2-block-decoding
- 2: https://deepwiki.com/aous72/OpenJPH/8-simd-optimizations
- 3: https://github.com/aous72/OpenJPH
- 4: AVX2 block coder optimizations (encoder + decoder) aous72/OpenJPH#276
- 5: Test fails to pass when building with GCC 15 aous72/OpenJPH#186
🌐 Web query:
OpenJPH 0.30.1 raw ojph_block_decoder64.cpp "3u << (p - 2)" OR "3ull << (p - 2)"
💡 Result:
In OpenJPH version 0.30.1, the code in ojph_block_decoder64.cpp utilizes bitwise shift expressions such as 3u << (p - 2) and 3ull << (p - 2). This distinction relates to C++ type handling and potential integer overflow issues, particularly when compiling with newer compilers like GCC 15 [1]. The suffix u denotes an unsigned int (typically 32-bit), whereas ull denotes an unsigned long long (typically 64-bit). In the context of the ojph_block_decoder64.cpp file, these expressions are used for bit-shifting operations to process JPEG2000 Part-15 (HTJ2K) code streams. Using 3u can lead to undefined behavior or truncation if the shift operation results in a value that exceeds the width of a 32-bit integer [1]. The transition to 3ull ensures that the calculation is performed using 64-bit precision, which is necessary to accommodate larger shift amounts without overflowing the underlying type [1]. This specific issue has been tracked in relation to build failures with GCC 15, where stricter type checking and warning levels may flag these expressions as potential overflows [1]. Using the ull suffix correctly promotes the constant to a 64-bit type, ensuring safety and portability across different architectures and compiler versions.
Citations:
Use a 64-bit literal here
3u << (p - 2) is still 32-bit arithmetic, so shifts of 32+ are undefined and the high-precision path loses the intended range. Switch this to 3ull << (p - 2).
🤖 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 `@src/core/coding/ojph_block_decoder64.cpp` at line 1549, The high-precision
shift in the block decoder still uses 32-bit arithmetic, so update the
expression in ojph_block_decoder64.cpp where the value is computed in the
decoder logic to use a 64-bit literal instead of the current unsigned 32-bit
one. Locate the shift in the code path that builds the `ui64 val` value and
change the literal so the operation stays 64-bit throughout, preserving the
intended range for larger shift counts.
master holds the old (~0.21-era) fork state whose custom patches have been re-applied on top of upstream 0.30.1 in this branch, so its content is fully superseded. Recorded as a merge parent (strategy=ours) purely to make the PR fast-forward-mergeable; the tree stays 0.30.1 + the carried resilient-decode patches.
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
20 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.
What this is
Re-bases the cornerstonejs OpenJPH fork onto upstream
0.30.1(latest stable, 2026-06-21) and re-applies our custom patches. The pinned commit the codecs repo currently uses (e01c7b7, branchmerge/aous-20240423) was ~22 months behind upstream.The large file diff is just adopting upstream 0.30.1. The cornerstonejs-specific delta from stock 0.30.1 is 3 lines in one file:
Custom patches — carry / drop decisions
4417d4b—resilient = false→true4417d4b— descriptive SIZ marker error-message textswap_byte()→swap_bytes_if_le()rename on the same lines; kept upstream's code (which also has 2 years of its own message work). No behavior change.d6297b2— suppress the "File terminated early" INFO loga15d3b3— "Change to debug build for a while" (subprojects/js/build.sh→CMAKE_BUILD_TYPE=Debug)Net effect vs stock 0.30.1
Validation status — NOT yet built
This was prepared with git only; the wasm was not compiled or run here. Needs a real emscripten build + the codecs pixel-correctness/browser-smoke goldens to confirm 0.30.1 decodes the corpus byte-identically and that
resilient=truedoesn't change output on valid streams. That runs in CI on the codecs-repo submodule-bump PR (separate). Treat this as unvalidated until that goes green.Summary by CodeRabbit
New Features
Bug Fixes
Documentation