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