Repository navigation
Phase 13a: Native Codec Infrastructure - #22
Conversation
There was a problem hiding this comment.
26 issues found across 43 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="native/src/sharpdicom_codecs.c">
<violation number="1" location="native/src/sharpdicom_codecs.c:125">
P2: AVX/AVX2/AVX512 detection needs OSXSAVE/XGETBV checks; CPUID alone can report AVX on CPUs where the OS doesn’t enable AVX state, leading to illegal-instruction crashes if used.</violation>
</file>
<file name="native/src/gpu_wrapper.c">
<violation number="1" location="native/src/gpu_wrapper.c:148">
P1: Race condition in `init_lock()`: two threads could simultaneously initialize the CRITICAL_SECTION, causing undefined behavior. Consider using `InitOnceExecuteOnce()` for thread-safe one-time initialization, or replace with a statically-initializable `SRWLOCK` using `SRWLOCK_INIT`.</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-09-PLAN.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-09-PLAN.md:152">
P2: This test expects `Initialize()` to be idempotent, but the phase context says it should throw on second call. Update the assertion to expect a failure so the test matches the intended behavior.</violation>
<violation number="2" location=".planning/phases/13-native-codecs-package/13-09-PLAN.md:161">
P2: This test passes even when no exception is thrown, so it doesn’t actually validate the missing-library behavior. Add an assertion or ignore/skip the test when initialization succeeds.</violation>
<violation number="3" location=".planning/phases/13-native-codecs-package/13-09-PLAN.md:556">
P3: This test unconditionally passes, so it won’t catch regressions. Either implement an actual assertion or mark it ignored until the functionality can be validated.</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-01-PLAN.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-01-PLAN.md:6">
P2: The plan metadata omits the Phase 12 dependency even though the roadmap states Phase 13 must follow Phase 12. This can break automation that relies on depends_on for ordering.</violation>
</file>
<file name="native/src/jpeg_wrapper.c">
<violation number="1" location="native/src/jpeg_wrapper.c:283">
P0: Integer overflow in buffer size calculation can lead to heap buffer overflow. The multiplication `w * h * comps` can overflow for large image dimensions, causing the size check to pass incorrectly. Use safe integer multiplication with overflow checking (e.g., check if `w > INT_MAX / h` before multiplying, or use `size_t` with overflow guards).</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-03-PLAN.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-03-PLAN.md:45">
P3: Execution context uses a user-specific absolute path, so the plan cannot be run on other machines. Use a repo-relative reference or remove the local filesystem path.</violation>
<violation number="2" location=".planning/phases/13-native-codecs-package/13-03-PLAN.md:83">
P3: Verification commands use absolute local paths, which breaks portability. Use repo-relative paths so the commands work for all contributors and CI.</violation>
</file>
<file name="native/src/j2k_wrapper.c">
<violation number="1" location="native/src/j2k_wrapper.c:678">
P1: Missing input buffer size validation. The function doesn't verify that `input_len` is sufficient for the specified image dimensions before reading pixel data, which could cause a buffer over-read.</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-02-SUMMARY.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-02-SUMMARY.md:49">
P3: Completion date is inconsistent with the started/completed timestamps later in the summary; align the frontmatter/footer completion date with the actual performance timestamps to avoid conflicting records.</violation>
</file>
<file name=".planning/STATE.md">
<violation number="1" location=".planning/STATE.md:375">
P3: The last-updated note contradicts the updated Phase 13 completion status. Update it to reflect Phase 13 completion and 13-05 being done.</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-RESEARCH.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-RESEARCH.md:164">
P3: The ModuleInitializer example swallows initialization failures, which conflicts with the phase decision to fail fast on load errors. This guidance should rethrow so the documented behavior is consistent.</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-02-PLAN.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-02-PLAN.md:81">
P2: The plan’s CI download step pipes a remote tarball directly into `tar` without integrity verification. Add checksum/signature validation (and ideally avoid `curl | tar`) to prevent compromised or corrupted vendor sources from being used in builds.</violation>
<violation number="2" location=".planning/phases/13-native-codecs-package/13-02-PLAN.md:89">
P3: Verification steps should be repo-relative rather than hardcoded to a specific developer’s home directory. This makes the plan portable across machines and CI.</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-04-PLAN.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-04-PLAN.md:81">
P2: This step contradicts the documented requirement to vendor CharLS source in `native/vendor/` and build from repo sources. Downloading in CI bypasses the vendored-source design and breaks reproducibility/offline builds.</violation>
<violation number="2" location=".planning/phases/13-native-codecs-package/13-04-PLAN.md:157">
P2: The plan calls for prebuilt FFmpeg installs, which conflicts with the vendored-source + static-linking build decisions. This would diverge from the documented packaging strategy and can break reproducibility and licensing control.</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-07-PLAN.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-07-PLAN.md:222">
P2: Native return codes are defined as success = 0, but this check only treats negative values as failures. Positive error codes would be treated as success and could return invalid pixel data. Use `!= 0` to detect all error codes.</violation>
<violation number="2" location=".planning/phases/13-native-codecs-package/13-07-PLAN.md:295">
P2: Validation claims to check SOI/EOI markers but only checks SOI. This can pass truncated JPEG data and is misleading. Include EOI validation (and update the length guard) if SOI/EOI checking is intended.</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-05-PLAN.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-05-PLAN.md:231">
P2: The planned `dlopen("nvjpeg2k_wrapper.so")` hardcodes a Linux-only name and doesn’t match the default `libnvjpeg2k_wrapper.so` output from `add_library(...)`. This will fail to load on Linux and is non-portable to Windows/macOS. Use a platform-specific loader/name or align the output name in CMake.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:105">
P2: Vendor archives are downloaded and extracted without checksum/signature verification, which weakens supply-chain integrity for the native build. Consider pinning expected hashes (or signatures) and validating them before extraction.</violation>
</file>
<file name="native/cuda/CMakeLists.txt">
<violation number="1" location="native/cuda/CMakeLists.txt:57">
P2: The architecture list includes sm_90 while the minimum toolkit is 11.0. CUDA 11.0–11.7 cannot compile sm_90, so builds will fail unless you gate that architecture or raise the minimum CUDA version.</violation>
</file>
<file name=".github/workflows/native-build.yml">
<violation number="1" location=".github/workflows/native-build.yml:49">
P1: Add checksum or signature verification for the downloaded vendor archives to prevent tampering.</violation>
</file>
<file name=".planning/phases/13-native-codecs-package/13-06-PLAN.md">
<violation number="1" location=".planning/phases/13-native-codecs-package/13-06-PLAN.md:419">
P2: Auto-initialization suppresses native load failures, but the phase requirements call for fail-fast behavior with exceptions on load failure. This would silently fall back to managed codecs instead of surfacing the error.</violation>
<violation number="2" location=".planning/phases/13-native-codecs-package/13-06-PLAN.md:431">
P2: Initialize returns silently on repeat calls and doesn’t block concurrent callers until initialization completes. The phase requirements call for thread-safe single initialization and throwing if Initialize is called more than once.</violation>
</file>
<file name="native/build.zig">
<violation number="1" location="native/build.zig:104">
P2: `SHARPDICOM_WITH_JPEG` is defined for the core library even when libjpeg-turbo isn’t linked, so `sharpdicom_features()` reports JPEG support despite using the stub wrapper. Make the core flags conditional on libjpeg availability.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| </action> | ||
| <verify> | ||
| ```bash | ||
| ls /Users/jas88/Developer/Github/SharpDicom/native/vendor/openjpeg/ |
There was a problem hiding this comment.
P3: Verification commands use absolute local paths, which breaks portability. Use repo-relative paths so the commands work for all contributors and CI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .planning/phases/13-native-codecs-package/13-03-PLAN.md, line 83:
<comment>Verification commands use absolute local paths, which breaks portability. Use repo-relative paths so the commands work for all contributors and CI.</comment>
<file context>
@@ -0,0 +1,214 @@
+ </action>
+ <verify>
+```bash
+ls /Users/jas88/Developer/Github/SharpDicom/native/vendor/openjpeg/
+grep -i openjpeg /Users/jas88/Developer/Github/SharpDicom/native/vendor/README.md
+```
</file context>
|
|
||
| # Metrics | ||
| duration: 5min | ||
| completed: 2026-01-29 |
There was a problem hiding this comment.
P3: Completion date is inconsistent with the started/completed timestamps later in the summary; align the frontmatter/footer completion date with the actual performance timestamps to avoid conflicting records.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .planning/phases/13-native-codecs-package/13-02-SUMMARY.md, line 49:
<comment>Completion date is inconsistent with the started/completed timestamps later in the summary; align the frontmatter/footer completion date with the actual performance timestamps to avoid conflicting records.</comment>
<file context>
@@ -0,0 +1,174 @@
+
+# Metrics
+duration: 5min
+completed: 2026-01-29
+---
+
</file context>
|
|
||
| --- | ||
| *Last updated: 2026-01-29 (Phase 12 complete - 12-07 done)* | ||
| *Last updated: 2026-01-30 (Phase 13 in progress - 13-04 done)* |
There was a problem hiding this comment.
P3: The last-updated note contradicts the updated Phase 13 completion status. Update it to reflect Phase 13 completion and 13-05 being done.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .planning/STATE.md, line 375:
<comment>The last-updated note contradicts the updated Phase 13 completion status. Update it to reflect Phase 13 completion and 13-05 being done.</comment>
<file context>
@@ -343,4 +372,4 @@ If resuming after a break:
---
-*Last updated: 2026-01-29 (Phase 12 complete - 12-07 done)*
+*Last updated: 2026-01-30 (Phase 13 in progress - 13-04 done)*
</file context>
| *Last updated: 2026-01-30 (Phase 13 in progress - 13-04 done)* | |
| *Last updated: 2026-01-30 (Phase 13 complete - 13-05 done)* |
| { | ||
| Initialize(); | ||
| } | ||
| catch |
There was a problem hiding this comment.
P3: The ModuleInitializer example swallows initialization failures, which conflicts with the phase decision to fail fast on load errors. This guidance should rethrow so the documented behavior is consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .planning/phases/13-native-codecs-package/13-RESEARCH.md, line 164:
<comment>The ModuleInitializer example swallows initialization failures, which conflicts with the phase decision to fail fast on load errors. This guidance should rethrow so the documented behavior is consistent.</comment>
<file context>
@@ -0,0 +1,594 @@
+ {
+ Initialize();
+ }
+ catch
+ {
+ // Swallow on auto-init, user can call Initialize() explicitly for exception
</file context>
| </action> | ||
| <verify> | ||
| ```bash | ||
| cat /Users/jas88/Developer/Github/SharpDicom/native/vendor/README.md |
There was a problem hiding this comment.
P3: Verification steps should be repo-relative rather than hardcoded to a specific developer’s home directory. This makes the plan portable across machines and CI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .planning/phases/13-native-codecs-package/13-02-PLAN.md, line 89:
<comment>Verification steps should be repo-relative rather than hardcoded to a specific developer’s home directory. This makes the plan portable across machines and CI.</comment>
<file context>
@@ -0,0 +1,204 @@
+ </action>
+ <verify>
+```bash
+cat /Users/jas88/Developer/Github/SharpDicom/native/vendor/README.md
+ls /Users/jas88/Developer/Github/SharpDicom/native/vendor/libjpeg-turbo/
+```
</file context>
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 725db0e..2838782
| Severity | Location | Issue | Delete |
|---|---|---|---|
| native/src/j2k_wrapper.c:438 | Integer overflow in buffer validation |
✅ Files analyzed, no issues (41)
• .github/workflows/ci.yml
• .github/workflows/native-build.yml
• .planning/ROADMAP.md
• .planning/STATE.md
• .planning/phases/13-native-codecs-package/13-01-PLAN.md
• .planning/phases/13-native-codecs-package/13-01-SUMMARY.md
• .planning/phases/13-native-codecs-package/13-02-PLAN.md
• .planning/phases/13-native-codecs-package/13-02-SUMMARY.md
• .planning/phases/13-native-codecs-package/13-03-PLAN.md
• .planning/phases/13-native-codecs-package/13-03-SUMMARY.md
• .planning/phases/13-native-codecs-package/13-04-PLAN.md
• .planning/phases/13-native-codecs-package/13-04-SUMMARY.md
• .planning/phases/13-native-codecs-package/13-05-PLAN.md
• .planning/phases/13-native-codecs-package/13-05-SUMMARY.md
• .planning/phases/13-native-codecs-package/13-06-PLAN.md
• .planning/phases/13-native-codecs-package/13-07-PLAN.md
• .planning/phases/13-native-codecs-package/13-08-PLAN.md
• .planning/phases/13-native-codecs-package/13-09-PLAN.md
• .planning/phases/13-native-codecs-package/13-CONTEXT.md
• .planning/phases/13-native-codecs-package/13-RESEARCH.md
• native/build.zig
• native/cuda/CMakeLists.txt
• native/cuda/nvjpeg2k_wrapper.c
• native/cuda/nvjpeg2k_wrapper.h
• native/cuda/test_nvjpeg2k_wrapper.c
• native/src/gpu_wrapper.c
• native/src/gpu_wrapper.h
• native/src/j2k_wrapper.h
• native/src/jls_wrapper.c
• native/src/jls_wrapper.h
• native/src/jpeg_wrapper.c
• native/src/jpeg_wrapper.h
• native/src/sharpdicom_codecs.c
• native/src/sharpdicom_codecs.h
• native/src/video_wrapper.c
• native/src/video_wrapper.h
• native/test/test_version.c
• native/vendor/README.md
• native/vendor/charls/.gitkeep
• native/vendor/ffmpeg/.gitkeep
• native/vendor/libjpeg-turbo/.gitkeep
⏭️ Files skipped (1)
| Locations |
|---|
native/vendor/openjpeg/.gitkeep |
725db0e to
450a7e0
Compare
Add managed image codec implementations: - JPEG Baseline encoder/decoder with DCT - JPEG Lossless encoder/decoder with DPCM predictors - JPEG 2000 with DWT (5/3, 9/7), EBCOT, MQ coder - AOT-compatible codec registration system - BitReader/BitWriter for Huffman coding - Color space conversion (YCbCr, RGB) Plans 12-01 through 12-07.
Add native codec build infrastructure: - Zig build system with native C API - libjpeg-turbo vendoring and wrapper - OpenJPEG vendoring and JPEG 2000 wrapper - CharLS JPEG-LS wrapper - FFmpeg video wrapper - nvJPEG2000 GPU acceleration wrapper - CI workflow for native builds Plans 13-01 through 13-05.
Add safe_mul_size() family of helpers to sharpdicom_codecs.h that use compiler intrinsics (__builtin_mul_overflow on GCC/Clang, _umul128 on MSVC 64-bit) or fallback division checks to prevent integer overflow. Fixed overflow vulnerabilities in: - jpeg_wrapper.c: requiredSize calculation now uses safe_mul3_size() - j2k_wrapper.c: both j2k_decode() and j2k_decode_region() fixed - gpu_wrapper.c: output_size calculation in fallback decode path - nvjpeg2k_wrapper.c: expected_size and comp_size calculations Also fixed thread-safety race condition in gpu_wrapper.c init_lock() on Windows by using InterlockedCompareExchange state machine pattern. These changes prevent potential buffer overflow attacks via maliciously crafted images with large dimensions that overflow size calculations.
AVX detection (sharpdicom_codecs.c): - Add OSXSAVE and XGETBV checks before reporting AVX/AVX2/AVX512 - Without OS-level state enabling, AVX instructions crash with SIGILL - Uses _xgetbv() on MSVC, inline asm on GCC/Clang Input buffer validation (j2k_wrapper.c): - Add input_len validation in j2k_encode() before reading pixel data - Uses safe_mul4_size() to prevent overflow in size calculation - Prevents buffer over-read with malicious dimension parameters Build system fixes: - build.zig: Only define SHARPDICOM_WITH_JPEG when libjpeg available Previously reported JPEG support even when using stub wrapper - CMakeLists.txt: Gate CUDA architectures by toolkit version sm_89 requires CUDA 11.8+, sm_90 requires CUDA 12.0+
20b30b4 to
b958249
Compare
Zig build.zig:
- Fix InstallArtifact API for Zig 0.13.0: use .override = .{ .custom = ... }
- Fix OpenJPEG flags: define as comptime constant to avoid runtime slice
concatenation error (Zig requires comptime-known values for ++)
CI workflow:
- Update Orthanc Docker image from 24.12.1 (doesn't exist) to 25.12.3
Only OpenJPEG is currently compiled from source. libjpeg-turbo, CharLS, and FFmpeg require source compilation support (TODO for Phase 13b). - Disable have_libjpeg, have_charls, have_ffmpeg flags in build.zig - Remove downloading of unused vendor libraries in CI - Fix native_jpeg_flags to respect have_libjpeg setting
- Disable all vendor libraries (OpenJPEG also needs CMake config) - Add linkLibC() for cross-compilation standard headers - Add unused attribute to set_error_fmt in stub wrappers - Remove vendor library downloads from CI (not used in Phase 13a) Phase 13a builds stub-only native library. Proper vendor library cross-compilation will be addressed in Phase 13b.
- Change Windows targets from MSVC to GNU ABI for better Zig support - Add unused attribute to get_pixel_format in jpeg_wrapper.c - Fix set_error linkage in j2k_wrapper.c (was static, should use extern)
- Move set_error extern declaration outside #ifdef SHARPDICOM_HAS_OPENJPEG - Fix THREAD_LOCAL macro to check _MSC_VER instead of _WIN32 (fixes cross-compilation with Zig GNU ABI on Windows)
Wrap libjpeg-turbo code in #ifdef SHARPDICOM_WITH_JPEG with stub implementations in the #else section. This allows building without libjpeg-turbo when have_libjpeg=false.
- Use mlugg/setup-zig@v1 instead of goto-bus-stop/setup-zig@v2 - Remove -Doptimize flag (build.zig has default ReleaseFast) - Remove unused vendor library downloads (Phase 13a builds stubs only) - Revert Orthanc to 24.12.1 with longer health check timeouts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
==========================================
+ Coverage 74.18% 74.55% +0.37%
==========================================
Files 156 156
Lines 11699 11599 -100
Branches 2242 2219 -23
==========================================
- Hits 8679 8648 -31
+ Misses 2409 2335 -74
- Partials 611 616 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
Add native codec build infrastructure:
Dependencies
Merge order:
Test plan
Summary by cubic
Introduces cross‑platform native codec infrastructure using Zig with a simple C API, feature‑gated stub wrappers for JPEG/JPEG‑LS/JPEG 2000/video in 13a, and optional NVIDIA GPU acceleration hooks. Adds native build GitHub Actions, Orthanc integration tests in CI, and hardens size calculations, AVX feature detection, and GPU init for safety.
New Features
Bug Fixes
Written for commit 8280663. Summary will update on new commits.
High-level PR Summary
This PR introduces native codec infrastructure for SharpDicom, establishing a Zig-based cross-compilation build system that produces shared libraries for six platforms (Windows/Linux/macOS on x64/arm64). The implementation wraps libjpeg-turbo for JPEG encoding/decoding, OpenJPEG for JPEG 2000 support, CharLS for JPEG-LS, and FFmpeg for video codecs. The PR includes an optional GPU acceleration layer using NVIDIA's nvJPEG2000 with automatic CPU fallback. The native C API provides version detection, feature detection, SIMD capability detection, and thread-local error handling. GitHub Actions workflows are configured to build native binaries across all target platforms with artifact uploads, and the CI workflow is enhanced with Orthanc integration testing for validating DICOM operations.
⏱️ Estimated Review Time: 3+ hours
💡 Review Order Suggestion
.planning/ROADMAP.md.planning/STATE.md.planning/phases/13-native-codecs-package/13-CONTEXT.md.planning/phases/13-native-codecs-package/13-RESEARCH.md.planning/phases/13-native-codecs-package/13-01-PLAN.mdnative/build.zignative/src/sharpdicom_codecs.hnative/src/sharpdicom_codecs.cnative/test/test_version.c.planning/phases/13-native-codecs-package/13-01-SUMMARY.md.planning/phases/13-native-codecs-package/13-02-PLAN.mdnative/vendor/README.mdnative/vendor/libjpeg-turbo/.gitkeepnative/src/jpeg_wrapper.hnative/src/jpeg_wrapper.c.planning/phases/13-native-codecs-package/13-02-SUMMARY.md.planning/phases/13-native-codecs-package/13-03-PLAN.mdnative/vendor/openjpeg/.gitkeepnative/src/j2k_wrapper.hnative/src/j2k_wrapper.c.planning/phases/13-native-codecs-package/13-03-SUMMARY.md.planning/phases/13-native-codecs-package/13-04-PLAN.mdnative/vendor/charls/.gitkeepnative/vendor/ffmpeg/.gitkeepnative/src/jls_wrapper.hnative/src/jls_wrapper.cnative/src/video_wrapper.hnative/src/video_wrapper.c.planning/phases/13-native-codecs-package/13-04-SUMMARY.md.planning/phases/13-native-codecs-package/13-05-PLAN.mdnative/cuda/nvjpeg2k_wrapper.hnative/cuda/nvjpeg2k_wrapper.cnative/cuda/CMakeLists.txtnative/cuda/test_nvjpeg2k_wrapper.cnative/src/gpu_wrapper.hnative/src/gpu_wrapper.c.planning/phases/13-native-codecs-package/13-05-SUMMARY.md.github/workflows/native-build.yml.github/workflows/ci.yml.planning/phases/13-native-codecs-package/13-06-PLAN.md.planning/phases/13-native-codecs-package/13-07-PLAN.md.planning/phases/13-native-codecs-package/13-08-PLAN.md.planning/phases/13-native-codecs-package/13-09-PLAN.md