Potential fix for #2037 - #51
Closed
John-Skinner wants to merge 1 commit into
Closed
Conversation
There was a problem hiding this comment.
Pull Request Overview
The PR aims to fix the Wasm generation issue by replacing function pointers with explicit stream-type-based calls and converting BufferedStream from C++ to C.
- Update include directory paths in CMakeLists.txt.
- Change header file extension from .hpp to .h for BufferedStream in J2KEncoder.hpp and J2KDecoder.hpp.
- Modify the build-native.sh script to clean, build, run tests, and install, and update the submodule URL in .gitmodules.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/openjpeg/test/cpp/CMakeLists.txt | Updated include directory paths to match the new project structure. |
| packages/openjpeg/src/J2KEncoder.hpp | Changed header inclusion to use the C-style extension (.h) for BufferedStream. |
| packages/openjpeg/src/J2KDecoder.hpp | Changed header inclusion to use the C-style extension (.h) for BufferedStream. |
| packages/openjpeg/extern/openjpeg | Updated submodule commit reflecting the new repository. |
| packages/openjpeg/build-native.sh | Enhanced build script by cleaning previous builds, setting install prefix, and performing installation. |
| .gitmodules | Updated the submodule URL to point to the new repository. |
Comments suppressed due to low confidence (2)
packages/openjpeg/src/J2KEncoder.hpp:20
- [nitpick] Ensure that updating the header file from 'BufferStream.hpp' to 'BufferStream.h' is reflected consistently in the project and build configuration for C compatibility.
#include "BufferStream.h"
packages/openjpeg/src/J2KDecoder.hpp:25
- [nitpick] Ensure that the change of header file inclusion from 'BufferStream.hpp' to 'BufferStream.h' is consistent across all related modules for proper C linkage.
#include "BufferStream.h"
| @@ -1,7 +1,9 @@ | |||
| #!/bin/sh | |||
| rm -rf build-native | |||
There was a problem hiding this comment.
[nitpick] Consider adding a safeguard to ensure that 'rm -rf build-native' only targets the intended build directory, avoiding accidental deletion if the script is run from an unexpected location.
wayfarer3130
added a commit
to ahmedezzat85/codecs
that referenced
this pull request
Sep 2, 2026
This is the defect cornerstonejs#62 and cornerstonejs#51 were both working around. opj_skip_from_buffer was declared OPJ_SIZE_T(OPJ_SIZE_T, opj_buffer_info_t*) and then cast to opj_stream_skip_fn, which openjpeg.h:649 defines as OPJ_OFF_T(OPJ_OFF_T, void*). OPJ_OFF_T is int64_t and OPJ_SIZE_T is size_t, so under wasm32 the cast produced an (i64,i32)->i64 indirect call onto an (i32,i32)->i32 table entry: RuntimeError "function signature mismatch". Only skip was wrong. read/write are OPJ_SIZE_T on both sides and seek already took OPJ_OFF_T, which is why decoding worked at all. Widening the parameter and return type to OPJ_OFF_T fixes it at the source, so EMULATE_FUNCTION_POINTER_CASTS is no longer needed to mask it -- no program-wide function-pointer wrapping, no size regression, and no submodule fork as cornerstonejs#51 proposed. The exhaustion return is now an explicit (OPJ_OFF_T)-1, which is what cio.c compares against; it used to be (OPJ_SIZE_T)-1 laundered through the bad cast. Negative lengths are rejected rather than cast to a huge unsigned, though openjpeg asserts p_size >= 0 before calling. Refs cornerstonejs#62, cornerstoneWADOImageLoader#400. Supersedes the approach in cornerstonejs#51. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wayfarer3130
pushed a commit
to ahmedezzat85/codecs
that referenced
this pull request
Sep 2, 2026
…keLists Cherry-picked from cornerstonejs#51 (John-Skinner), which is otherwise superseded by the skip-callback fix in this branch. test/cpp/CMakeLists.txt resolves relative include_directories against test/cpp/, so "../extern/openjpeg/..." pointed at test/extern/, which does not exist. It needs one more level up to reach packages/openjpeg/extern/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Thanks for this @John-Skinner — your diagnosis was right, and it turned out to be the same root cause as #62/#63. Closing in favour of #63, which fixes it at the source rather than in the submodule. The skip callback in Your Please do reopen or shout if you think the source-level fix misses a case yours covered. |
wayfarer3130
added a commit
that referenced
this pull request
Sep 2, 2026
…mage dimensions (#63) * fix: use x1-x0 / y1-y0 for correct image dimensions + add opj_end_decompress * ci: add GitHub Actions workflow to build openjpeg WASM and commit dist artifacts * ci: fix artifact copy paths to build/src/ + force-track dist/ + trigger first build * ci: fix copy paths to build/extern/openjpeg/bin/ (matching build.sh) + init submodules * ci: fix exit code 128 - use GITHUB_TOKEN for push + add token to checkout * ci: update openjpeg WASM dist artifacts [skip ci] * fix: add -s ALLOW_TABLE_GROWTH=1 to fix indirect call signature mismatch in opj_decode() * ci: update openjpeg WASM dist artifacts [skip ci] * fix: add ALLOW_TABLE_GROWTH=1 to openjpegjs and openjpegjs_decode targets + ASSERTIONS=1 for debug logging * ci: update openjpeg WASM dist artifacts [skip ci] * fix: add build version string to force recompile with ALLOW_TABLE_GROWTH=1 * debug: add verbose decode logs + ASSERTIONS=2 + SAFE_HEAP to trace error origin * ci: update openjpeg WASM dist artifacts [skip ci] * fix: add EMULATE_FUNCTION_POINTER_CASTS=1 to all 4 targets to fix indirect call signature mismatch in openjpeg j2k_exec pipeline * ci: update openjpeg WASM dist artifacts [skip ci] * cleanup: remove debug instrumentation, restore clean J2KDecoder bindings * ci: update openjpeg WASM dist artifacts [skip ci] * fix: add EMULATE_FUNCTION_POINTER_CASTS=1 to resolve indirect call signature mismatch in openjpeg j2k_exec pipeline WebAssembly's typed function table traps on any indirect call where the caller and callee signatures don't match exactly. openjpeg's j2k_exec() builds a procedure list of function pointers at runtime and invokes them with casts that are valid in C but illegal in WASM. EMULATE_FUNCTION_POINTER_CASTS=1 instructs Emscripten to emit a trampoline shim for each mismatched indirect call, padding/truncating arguments to match the actual call site signature, preventing the hard WASM trap. * fix: correct image dimensions and add opj_end_decompress in J2KDecoder Two independent correctness fixes: 1. Use x1-x0 / y1-y0 for image dimensions instead of x1/y1 directly. image->x1 and image->y1 are absolute grid coordinates, not pixel counts. For any DICOM image where the image origin (x0, y0) is non-zero (tiled datasets, multi-frame, images with a non-zero offset), using x1/y1 directly produces an incorrect buffer size and pixel mapping. 2. Call opj_end_decompress() after opj_decode(). This is required by the openjpeg API to properly finalize decompression and release internal codec state before destroying the codec/stream. Omitting it can leave codec resources in an inconsistent state. 3. Remove unused variable: int comp_num. * chore: remove openjpeg info_callback to silence verbose tile decode logs The info handler printed [INFO] for every tile header read, tile decode, and image data update. This floods the browser console with ~4 lines per tile. Warning and error handlers are retained for diagnostics. * ci: update openjpeg WASM dist artifacts [skip ci] * restore: J2KDecoder.hpp and jslib-decode.cpp to intended versions Restore info_callback and opj_set_info_handler in J2KDecoder.hpp. Restore full EMSCRIPTEN_BINDINGS in jslib-decode.cpp. * ci: update openjpeg WASM dist artifacts [skip ci] * revert: drop the CI workflow, committed wasm and .gitignore changes These were scaffolding for debugging #62 in CI, not part of the fix: - .github/workflows/build-openjpeg.yml built wasm on an emsdk 3.1.44 that no longer matches the 3.1.74 container pr-checks and release use, committed the result straight to main with no fetch/rebase, and dispatched secrets.CROSS_REPO_PAT to a personal cornerstoneWADOImageLoader fork. The repo already rebuilds dist at publish time in release.yml. - packages/openjpeg/dist/* — 8 built artifacts. No package in this repo tracks dist. - packages/openjpeg/.gitignore claimed to un-ignore dist, which is a no-op against the root .gitignore's bare `dist`, and in the process dropped the build-native and test-fixture ignores. Restored. - src/jslib-decode.cpp had been rewritten to CRLF with no content change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * revert: restore the release build flags in openjpeg CMakeLists All four shipped targets had been switched to debug settings while chasing the signature mismatch: - EMULATE_FUNCTION_POINTER_CASTS=1 wraps every function pointer in the program to paper over one bad cast. With embind on all four targets that is a broad size and speed cost, and it hides the defect rather than fixing it — the next commit fixes the cast itself. - ALLOW_TABLE_GROWTH=1 was an earlier guess at the same symptom; the table was never the problem. - ASSERTIONS=1 and DISABLE_EXCEPTION_CATCHING=0 are debug-only and would ship in the published wasm, failing the dist-size gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(openjpeg): derive decoded size from x1-x0 / y1-y0, and end decompression Reapplied onto main's formatting — the branch had rewritten the whole file to CRLF and stripped its indentation, which buried these three changes in a 1853-line diff. - frameInfo_ width/height came from image->x1/y1, which are absolute reference-grid coordinates. Any image with a nonzero offset (x0/y0) overstated the size, and since the copy loop indexes comps[].data with sizeAtDecompositionLevel, that was an out-of-bounds read past the component buffer, not just a wrong reported size. - opj_end_decompress is now called after a successful opj_decode, before HandleGuard tears down the codec and stream. - Dropped the unused `int comp_num` declaration. Known remaining divergence, left alone here: calculateSizeAtDecompositionLevel still recomputes ceil(w/2) per level from frameInfo_, which is not openjpeg's ceildivpow2 of the component extent. Reduced-resolution decodes of an image with a nonzero offset can still disagree with comps[0].w/h. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(openjpeg): match opj_stream_skip_fn's signature in the buffer stream This is the defect #62 and #51 were both working around. opj_skip_from_buffer was declared OPJ_SIZE_T(OPJ_SIZE_T, opj_buffer_info_t*) and then cast to opj_stream_skip_fn, which openjpeg.h:649 defines as OPJ_OFF_T(OPJ_OFF_T, void*). OPJ_OFF_T is int64_t and OPJ_SIZE_T is size_t, so under wasm32 the cast produced an (i64,i32)->i64 indirect call onto an (i32,i32)->i32 table entry: RuntimeError "function signature mismatch". Only skip was wrong. read/write are OPJ_SIZE_T on both sides and seek already took OPJ_OFF_T, which is why decoding worked at all. Widening the parameter and return type to OPJ_OFF_T fixes it at the source, so EMULATE_FUNCTION_POINTER_CASTS is no longer needed to mask it -- no program-wide function-pointer wrapping, no size regression, and no submodule fork as #51 proposed. The exhaustion return is now an explicit (OPJ_OFF_T)-1, which is what cio.c compares against; it used to be (OPJ_SIZE_T)-1 laundered through the bad cast. Negative lengths are rejected rather than cast to a huge unsigned, though openjpeg asserts p_size >= 0 before calling. Refs #62, cornerstoneWADOImageLoader#400. Supersedes the approach in #51. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(openjpeg): correct the openjpeg include paths in the C++ test CMakeLists Cherry-picked from #51 (John-Skinner), which is otherwise superseded by the skip-callback fix in this branch. test/cpp/CMakeLists.txt resolves relative include_directories against test/cpp/, so "../extern/openjpeg/..." pointed at test/extern/, which does not exist. It needs one more level up to reach packages/openjpeg/extern/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(openjpeg): cover the skip callback with a generated JP2 fixture None of the 26 .j2k fixtures make openjpeg delegate a skip to opj_skip_from_buffer, so nothing in the suite exercised the signature mismatch this branch fixes -- a green CI run said nothing about it. test/helpers/jp2.mjs wraps a bare codestream in the minimum JP2 boxes, synthesising ihdr from the codestream's own SIZ marker, and inserts a 'free' box (which openjpeg has no handler for) ahead of jp2c so opj_jp2_read_header has to skip it. The filler box must exceed 1MB: opj_stream_read_data always refills a full OPJ_J2K_STREAM_CHUNK_SIZE chunk and opj_stream_read_skip serves any skip within m_bytes_in_buffer without calling the callback at all. That is why the fixture is generated rather than committed, and why this bug only ever showed up on large images. Confirmed discriminating by rebuilding with the unfixed BufferStream.hpp (tools/docker/build.sh openjpeg, emsdk 3.1.74): openjpegwasm RuntimeError: null function or function signature mismatch openjpegwasm_decode RuntimeError: null function or function signature mismatch openjpegjs (asm.js) no trap at all -- width 0, pixels do not match The asm.js result is why the test asserts on decoded output rather than just that decode() did not throw. With the fix, all three pass and the full openjpeg suite is 57 passed / 8 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: never run the release workflow in a fork release.yml triggers on push to `main`. A contributor whose PR branch IS their fork's `main` therefore gets the entire workflow run inside their own repository on every push to that branch. That is not hypothetical: pushing this branch bumped all nine packages in ahmedezzat85/codecs and pushed a `chore(release): publish` commit onto this pull request, authenticated with that fork's own GITHUB_TOKEN. The publish job failed for want of credentials, so nothing reached npm, but the version commit landed on the PR and had to be stripped. Guarding `build` alone would gate the chain -- release needs build, publish needs release, github-releases needs both -- but the release job repeats the condition since it is the one that commits and pushes. The existing head-commit guard on `build` is preserved verbatim, now ANDed under the repository check. Verified against the org cases: a normal push still runs, the workflow's own release commit is still skipped, a human typing that subject still runs, and workflow_dispatch still runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Bill Wallace <wayfarer3130@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: John Skinner <j.v.skinner.jr@gmail.com>
Contributor
Author
|
Thanks much wayfarer3130, yes, I believe your change will fix my issue. I have since made a private version of but i just don't know wasm that well, so i ran through with if/then/else's to avoid the casts. Let me know if you need the test dicom file that was not working for testing. |
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
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.
wasm generation had difficulty with the use of function pointer for the stream skipping function with BufferedStream.hpp.
This change resolves the wasm issue with the function pointer by 1) replacing all of the function pointers with explicit calls that depend on the type of stream (file or buffer), and 2) using C rather than C++ to match the language of cio. The change also moves BufferStream into openjp2 core functions, which enables the
direct call from cio.c. But in doing so, the change does compromise the abstraction of the stream types away from the core functions. One can view the BufferedStream as a core stream type as rationale for moving it into the core.
cio.c functions are 'C', and so the change makes BufferedStream C rather than C++ by using '.c' rather than '.hpp
The issue only appears with wasm and not C/C++, so the changes does retain the function pointers and their setters for
possible future stream types besides file streams although no known stream types are known a this time.
To minimize changes to the core of cio, there are near-equivalent functions to the function pointers for skip, read, write, and seek. You see this simple replacement for example where p_stream->m_write_fn is replaced with write_stream.
The codecs repo uses submodules which slightly complicates the PR.
This PR will likely capture the repo out of John-Skinner.
The only submodule that needed any change is the openjpeg repo.
So that pull request is integral to the overall change.
For codec, this change updates the build-native.sh to produce an install; but that is optional for this fix.
The packages/openjpeg/test/cpp/CMakeLists.txt is updated for the include directories to go one level up.
I only ran testing on decode from browser javascript.
I can expand my testing with a little guidance where additional tests are.