Skip to content

Conversation

John-Skinner
Copy link

@John-Skinner John-Skinner commented May 12, 2025

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.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Preview

Copilot AI May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant