Repository navigation
Phase 27: Extended Codec Support - #51
Merged
Merged
Conversation
Phase 27: Extended Codec Support - Implementation decisions documented - Phase boundary established
Phase 27: Extended Codec Support - Standard stack identified (libjpeg-turbo dual-build, FFmpeg+x264+x265, stb_image) - Architecture patterns documented (symbol-prefixed fat library, handle-based encoder API) - Pitfalls catalogued (12-bit disables SIMD, symbol collision, GPU detection) - DICOM video transfer syntax reference compiled (9 UIDs + SOP classes)
Phase 27: Extended Codec Support - 10 plans in 6 waves - 3 parallel (wave 2), 7 sequential - 12-bit JPEG (managed + native) and video encoding (MPEG2/H264/HEVC) - Ready for execution
- Plan 27-04: resolve NativeJpeg8Codec ambiguity - NativeJpeg8Codec replaces NativeJpegCodec registration, NativeJpeg12Codec added alongside - Plan 27-06: strengthen Task 1 verify to include zig build in stub mode - Plan 27-08: add stb_image P/Invoke declarations, NativeImageLoader class for loading image files into VideoFrames (closes managed path gap for image file sequence input) - Roadmap: update 27-08 description to include NativeImageLoader
- Add MPEG2, H264, HEVC values to CompressionType enum - Add JPEGExtended transfer syntax (Process 2 and 4, 12-bit) - Add 2 MPEG2 transfer syntaxes (Main Profile ML/HL) - Add 5 H.264 transfer syntaxes (High/BD/Stereo profiles) - Add 2 HEVC transfer syntaxes (Main/Main10 profiles) - Update FromUID() to recognize all 10 new transfer syntaxes
- Add VideoEndoscopicImageStorage UID - Add VideoMicroscopicImageStorage UID - Add VideoPhotographicImageStorage UID - Add EnhancedXAImageStorage UID - Add EnhancedXRFImageStorage UID - Add USMultiFrameImageStorage UID - Add SCMultiFrameTrueColorImageStorage UID
Tasks completed: 2/2 - Add CompressionType enum values and TransferSyntax definitions - Add video SOP class UIDs SUMMARY: .planning/phases/27-extended-codec-support/27-01-SUMMARY.md
- Create jpeg12_wrapper.h with decode/encode/free/has_support API - Create jpeg12_wrapper.c with full raw libjpeg implementation (symbol-prefixed) and stub fallback when not compiled - Add SHARPDICOM_HAS_JPEG12 feature flag (bit 9) to sharpdicom_codecs.h - Update sharpdicom_features() to report JPEG12 when compiled with SHARPDICOM_WITH_JPEG12
- JpegExtendedDecoder handles SOF1 with 8-bit and 12-bit precision - JpegExtendedEncoder produces SOF1 JPEG with configurable precision - 12-bit output uses ushort (little-endian) in destination buffer - Reuses existing DCT, Huffman, and quantization infrastructure - Supports grayscale and RGB for both bit depths
- Add have_libjpeg12 flag for 12-bit libjpeg-turbo support - Define symbol prefix flags for all public libjpeg API functions (jpeg_* -> jpeg12_jpeg_*) to avoid symbol collisions - Add jpeg12_wrapper.c compilation in cross-build loop, test exe, and native build targets - Core flags propagate SHARPDICOM_WITH_JPEG12 when 12-bit available - Stub mode compiles jpeg12_wrapper.c without vendor libs
- Create video_encoder.h with handle-based encoding API - Implement video_encoder.c with MPEG2/H264/HEVC encoding - GPU-accelerated encoder detection (NVENC/VideoToolbox/VAAPI) with CPU fallback - Quality presets (Diagnostic/Review/Archive) with CRF and bitrate modes - Audio encoding support (AAC/PCM) with interleaving - In-memory muxing via avio dynamic buffer - Stub implementations when SHARPDICOM_WITH_FFMPEG_ENC not defined - Update build.zig for video_encoder.c in all build targets
Tasks completed: 2/2 - Task 1: Create jpeg12_wrapper.c/h and update feature flags - Task 2: Update build.zig for dual libjpeg-turbo compilation SUMMARY: .planning/phases/27-extended-codec-support/27-03-SUMMARY.md
- JpegExtendedCodec implements IPixelDataCodec for TransferSyntax.JPEGExtended - Supports 8-bit and 12-bit lossy JPEG via SOF1 (Extended Sequential DCT) - Registered in CodecInitializer after JpegBaselineCodec - Updated codec count test from 10 to 11 codecs - Added JPEGExtended to registry inclusion assertions - JpegLosslessCodec already handles up to 16-bit precision (verified)
- Vendor stb_image.h v2.30 (public domain, ~8000 lines) - Create stb_image_wrapper.h/c for memory-based image loading - Supports PNG, BMP, JPEG, TGA formats from memory buffers - Uses STBI_NO_STDIO and STBI_NO_HDR for minimal footprint - Stub implementations when SHARPDICOM_WITH_STB_IMAGE not defined - Update build.zig with have_stb_image flag and wrapper compilation
Tasks completed: 2/2 - Task 1: Create video_encoder.h/c - Task 2: Vendor stb_image and create wrapper SUMMARY: .planning/phases/27-extended-codec-support/27-06-SUMMARY.md
Tasks completed: 2/2 - Create JpegExtendedDecoder and JpegExtendedEncoder - Create JpegExtendedCodec, register in CodecInitializer SUMMARY: .planning/phases/27-extended-codec-support/27-02-SUMMARY.md
… detection - Add jpeg12_decode, jpeg12_encode, jpeg12_free, jpeg12_has_support P/Invoke declarations - Add both LibraryImport (NET7+) and DllImport (netstandard2.0) versions - Add Jpeg12Bit flag to NativeFeatures enum (1 << 9) - Add Jpeg12Bit entry to NativeCodecFeature enum - Add HasFeature case for Jpeg12Bit feature check - Add EnableJpeg12Bit property with NativeCodecOptions support
- Add SHARPDICOM_HAS_VIDEO_ENC (1 << 10) and SHARPDICOM_HAS_STB_IMAGE (1 << 11) to header - Report SHARPDICOM_WITH_FFMPEG_ENC and SHARPDICOM_WITH_STB_IMAGE in sharpdicom_features() - Add have_ffmpeg_enc flag in build.zig (separate from have_ffmpeg for decode) - Pass feature defines to sharpdicom_codecs.c core compilation - Separate video_encoder.c from video_wrapper.c build paths
…tration - Add NativeJpeg8Codec: explicit 8-bit native JPEG codec via jpeg_decode/jpeg_encode - Add NativeJpeg12Codec: 12-bit JPEG codec via jpeg12_decode/jpeg12_encode P/Invoke - NativeJpeg12Codec targets TransferSyntax.JPEGExtended with 8/12-bit support - 12-bit decode outputs uint16_t samples (2 bytes per sample) - Register NativeJpeg12Codec at PriorityNative when Jpeg12Bit feature detected - All 2263 tests pass with zero regressions
- Add addX264Sources() for compiling x264 H.264 encoder from source - Add addX265Sources() for compiling x265 HEVC encoder from source via C++ - Add addFfmpegEncSources() for compiling FFmpeg encoding libraries from source - FFmpeg sources organized by library: libavutil, libavcodec, libswscale, libswresample, libavformat - Source file lists cover minimal encoding subset (MPEG-2, H.264, HEVC, AAC, PCM) - Vendor source download handled by CI; build.zig ready for integration
Tasks completed: 2/2 - Task 1: Add jpeg12_* P/Invoke declarations and Jpeg12Bit feature - Task 2: Create NativeJpeg8Codec and NativeJpeg12Codec SUMMARY: .planning/phases/27-extended-codec-support/27-04-SUMMARY.md
Tasks completed: 2/2 - Task 1: Add video encoding feature flag and stb_image build support - Task 2: Add FFmpeg and x264/x265 build configuration SUMMARY: .planning/phases/27-extended-codec-support/27-07-SUMMARY.md
…s, VideoEncodeProgress) - VideoFrame: pixel data container with dimensions, format, optional audio - VideoEncoderOptions: codec selection, quality presets (Diagnostic/Review/Archive), frame rate - VideoEncodeProgress: encoding progress reporting with ETA estimation - Supporting enums: VideoPixelFormat, AudioSampleFormat, VideoCodecType, etc.
- Properties tests: TransferSyntax, Name, Capabilities, bit depths - Registry registration via CodecInitializer.RegisterAll() - Validation: SOF1 marker presence, missing SOI detection - 8-bit grayscale roundtrip with PSNR verification - 8-bit RGB roundtrip with per-channel PSNR verification - Encoded format: SOF1 marker verification, even length - Decode edge cases: invalid/negative frame index
…pper - VideoEncoder: high-level static API with EncodeFromFrames, EncodeFromFramesAsync, EncodeFromDicom - VideoEncoderBackend delegate pattern avoids hard dependency on SharpDicom.Codecs - DetectFrameRate reads CineRate, RecommendedDisplayFrameRate, FrameTime, FrameTimeVector - NativeVideoEncoder: managed wrapper with handle lifecycle for native FFmpeg encoder - NativeImageLoader: stb_image P/Invoke wrapper loading PNG/JPEG/BMP/TGA to VideoFrame - P/Invoke: video_encoder_create/encode_frame/flush/get_output/destroy, stbi_load/free - NativeFeatures: VideoEnc (1<<10), StbImage (1<<11) flags - NativeCodecFeature: VideoEncoder, StbImage enum entries - VideoEncoderHandle: SafeHandle for encoder lifecycle - VideoEncoderConfig: StructLayout Sequential matching native struct
- 10 tests for 12-bit JPEG Extended (Process 2,4): gradient roundtrip, uniform image, high-value gradient, gradient pattern, alternating pattern, mid-range values, lenient decode, SOF1 precision=12 verification, even-length check - 7 tests for 16-bit JPEG Lossless: full-range roundtrip, 12-bit in 16-bit container, random data, all-zeros, all-max, alternating, medical imaging range, compression ratio - All tests use synthetic data (no external files) - 12-bit tests use mid-range values near level shift (2048) to stay within standard Huffman table capabilities - Lossless tests verify bit-exact reconstruction
Tasks completed: 2/2 - Create VideoFrame, VideoEncoderOptions, VideoEncodeProgress types - Create VideoEncoder and NativeVideoEncoder with P/Invoke layer SUMMARY: .planning/phases/27-extended-codec-support/27-08-SUMMARY.md
Tasks completed: 2/2 - Task 1: JpegExtendedCodec core tests (21 tests) - Task 2: 12-bit specific tests and 16-bit lossless tests (17 tests) SUMMARY: .planning/phases/27-extended-codec-support/27-05-SUMMARY.md
- VideoSopClass enum for all 7 video DICOM SOP classes
- VideoDicomBuilder fluent builder for creating video DICOM files
- Auto-generates 2.25.{uuid} UIDs when not provided
- Maps SOP class to correct UID, modality, and photometric interpretation
- Creates encapsulated pixel data with single fragment for video bitstream
- Template dataset support copies patient/study-level attributes
- Proper Image Pixel Module and Cine Module attributes
…-end pipeline - MapCodecToTransferSyntax maps MPEG2/H264/HEVC to DICOM transfer syntaxes - CreateVideoDicom combines frame encoding with DICOM file creation - CreateVideoDicomAsync provides async IAsyncEnumerable-to-DICOM pipeline - Transfer syntax auto-selected from VideoEncoderOptions.Codec
Tasks completed: 2/2 - Create VideoSopClass enum and VideoDicomBuilder - Integration between VideoEncoder and VideoDicomBuilder SUMMARY: .planning/phases/27-extended-codec-support/27-09-SUMMARY.md
- 19 builder tests: SOP class mapping, UID generation, template copying, validation, pixel data packaging, transfer syntax defaults - 14 options tests: default values, quality presets, raw parameters, codec/audio/hwaccel init properties
- 9 frame rate detection tests: CineRate, RecommendedDisplayFrameRate, FrameTime, FrameTimeVector, priority order, empty/null dataset - 10 VideoFrame tests: constructor, formats, validation, dispose - 7 VideoEncodeProgress tests: percentage, estimation, equality, toString - 4 MapCodecToTransferSyntax tests: all codecs plus invalid - 3 encoder availability tests: backend registration, null guard
Tasks completed: 2/2 - Task 1: VideoDicomBuilder and VideoEncoderOptions tests (33 tests) - Task 2: VideoEncoder API and frame rate detection tests (33 tests) Phase 27 (Extended Codec Support) now complete: 10/10 plans. SUMMARY: .planning/phases/27-extended-codec-support/27-10-SUMMARY.md
…t build docs Phase 27: Extended Codec Support - 2 gap closure plan(s) in 1 wave(s) - 27-11: Wire VideoEncoder.RegisterBackend() in NativeCodecs (gaps 1+2) - 27-12: Document native 12-bit JPEG build requirements (gap 3) - Both plans run in parallel (wave 1, no dependencies)
…ibraries - Document all optional vendor library flags and paths in build.zig - Detail 12-bit JPEG enabling steps with symbol prefix approach - Explain stub fallback behavior and managed codec failover - Add CI notes for testing native 12-bit JPEG builds
- Add using SharpDicom.Codecs.Video for VideoEncoder access - Replace commented-out video codec stub with working VideoEncoder.RegisterBackend() call - Backend delegates to NativeVideoEncoder for frame-by-frame encoding
Tasks completed: 1/1 - Document native build requirements for optional vendor libraries SUMMARY: .planning/phases/27-extended-codec-support/27-12-SUMMARY.md
Tasks completed: 1/1 - Wire VideoEncoder.RegisterBackend() in NativeCodecs.RegisterCodecs() SUMMARY: .planning/phases/27-extended-codec-support/27-11-SUMMARY.md
All 3 verification gaps closed: - Gap 1: VideoEncoder backend registered in NativeCodecs - Gap 2: VideoDicomBuilder chain wired (depends on gap 1) - Gap 3: Native build requirements documented
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
========================================
- Coverage 8.32% 8.29% -0.03%
========================================
Files 225 233 +8
Lines 16690 17955 +1265
Branches 3195 3425 +230
========================================
+ Hits 1390 1490 +100
- Misses 15118 16283 +1165
Partials 182 182 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on b9c17f0..49b1017
| Severity | Location | Issue | Delete |
|---|---|---|---|
 |
src/SharpDicom.Codecs/NativeCodecs.cs:562 | Progress parameter silently ignored |  |
✅ Files analyzed, no issues (49)
• .planning/ROADMAP.md
• .planning/STATE.md
• .planning/phases/27-extended-codec-support/27-01-PLAN.md
• .planning/phases/27-extended-codec-support/27-01-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-02-PLAN.md
• .planning/phases/27-extended-codec-support/27-02-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-03-PLAN.md
• .planning/phases/27-extended-codec-support/27-03-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-04-PLAN.md
• .planning/phases/27-extended-codec-support/27-04-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-05-PLAN.md
• .planning/phases/27-extended-codec-support/27-05-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-06-PLAN.md
• .planning/phases/27-extended-codec-support/27-06-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-07-PLAN.md
• .planning/phases/27-extended-codec-support/27-07-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-08-PLAN.md
• .planning/phases/27-extended-codec-support/27-08-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-09-PLAN.md
• .planning/phases/27-extended-codec-support/27-09-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-10-PLAN.md
• .planning/phases/27-extended-codec-support/27-10-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-11-PLAN.md
• .planning/phases/27-extended-codec-support/27-11-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-12-PLAN.md
• .planning/phases/27-extended-codec-support/27-12-SUMMARY.md
• .planning/phases/27-extended-codec-support/27-CONTEXT.md
• .planning/phases/27-extended-codec-support/27-RESEARCH.md
• .planning/phases/27-extended-codec-support/27-VERIFICATION.md
• native/BUILD-REQUIREMENTS.md
• native/build.zig
• native/src/jpeg12_wrapper.c
• native/src/jpeg12_wrapper.h
• native/src/sharpdicom_codecs.c
• native/src/sharpdicom_codecs.h
• native/src/stb_image_wrapper.c
• native/src/stb_image_wrapper.h
• native/src/video_encoder.c
• native/src/video_encoder.h
• src/SharpDicom.Codecs/Codecs/NativeImageLoader.cs
• src/SharpDicom.Codecs/Codecs/NativeJpeg12Codec.cs
• src/SharpDicom.Codecs/Codecs/NativeJpeg8Codec.cs
• src/SharpDicom.Codecs/Codecs/NativeVideoEncoder.cs
• src/SharpDicom.Codecs/Interop/NativeMethods.cs
• src/SharpDicom.Codecs/Interop/SafeHandles.cs
• src/SharpDicom/Codecs/CodecInitializer.cs
• src/SharpDicom/Codecs/Jpeg/JpegExtendedCodec.cs
• src/SharpDicom/Codecs/Jpeg/JpegExtendedDecoder.cs
• src/SharpDicom/Codecs/Jpeg/JpegExtendedEncoder.cs
⏭️ Files skipped (17)
| Locations |
|---|
native/vendor/stb/stb_image.h |
src/SharpDicom/Codecs/Video/VideoDicomBuilder.cs |
src/SharpDicom/Codecs/Video/VideoEncodeProgress.cs |
src/SharpDicom/Codecs/Video/VideoEncoder.cs |
src/SharpDicom/Codecs/Video/VideoEncoderOptions.cs |
src/SharpDicom/Codecs/Video/VideoFrame.cs |
src/SharpDicom/Codecs/Video/VideoSopClass.cs |
src/SharpDicom/Data/CompressionType.cs |
src/SharpDicom/Data/DicomUID.WellKnown.cs |
src/SharpDicom/Data/TransferSyntax.cs |
tests/SharpDicom.Tests/Codecs/CodecRegistryIntegrationTests.cs |
tests/SharpDicom.Tests/Codecs/Jpeg/JpegExtended12BitTests.cs |
tests/SharpDicom.Tests/Codecs/Jpeg/JpegExtendedCodecTests.cs |
tests/SharpDicom.Tests/Codecs/JpegLossless/JpegLossless16BitTests.cs |
tests/SharpDicom.Tests/Codecs/Video/VideoDicomBuilderTests.cs |
tests/SharpDicom.Tests/Codecs/Video/VideoEncoderOptionsTests.cs |
tests/SharpDicom.Tests/Codecs/Video/VideoEncoderTests.cs |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 67 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/video_encoder.h">
<violation number="1" location="native/src/video_encoder.h:84">
P1: Zero-initialized `crf` (0) does not match the documented sentinel value (-1) for "use preset default". In FFmpeg, CRF=0 means lossless for x264/x265, so users following the "zero-init is safe" contract will unknowingly get lossless encoding instead of the preset default. Consider changing the sentinel to 0 (e.g., `0 = use preset default`) and shifting valid CRF values, or updating the struct-level documentation to note that `crf` must be explicitly set to -1.</violation>
</file>
<file name="src/SharpDicom/Codecs/Video/VideoEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Video/VideoEncoder.cs:221">
P2: Unsupported RGB bit depths fall through to Rgb24, so 16-bit RGB frames will be encoded as 8-bit RGB. This silently corrupts pixel data instead of rejecting unsupported formats.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/JpegExtendedEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/JpegExtendedEncoder.cs:636">
P1: Bug: Premature ZRL emission when accumulating zero-valued AC coefficients. Per the JPEG spec (ITU-T T.81 §F.1.2.2.1), ZRL codes should only be emitted preceding a non-zero coefficient. When all remaining coefficients are zero, only EOB should be emitted. The current code emits spurious ZRL codes during zero-only runs, producing spec-violating output (e.g., `ZRL, EOB` instead of `EOB`) that may be rejected by strict decoders.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/JpegExtendedDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/JpegExtendedDecoder.cs:369">
P2: `ArrayPool<int>.Shared.Rent` does not guarantee zeroed memory. For subsampled images, not all positions in chroma component planes are written, leaving stale data that gets read during output conversion. Clear the rented buffer after renting.</violation>
<violation number="2" location="src/SharpDicom/Codecs/Jpeg/JpegExtendedDecoder.cs:611">
P1: Missing chroma upsampling for 12-bit color images. The 8-bit path (`WriteOutput8Bit`) checks for `maxH > 1 || maxV > 1` and calls `UpsampleComponent` before color conversion, but the 12-bit path (`WriteOutput12Bit`) directly uses the subsampled chroma planes without upsampling. This produces incorrect color output for 12-bit images with chroma subsampling.</violation>
<violation number="3" location="src/SharpDicom/Codecs/Jpeg/JpegExtendedDecoder.cs:734">
P0: Stride mismatch between chroma storage and `UpsampleComponent` reading. The block decoder writes subsampled chroma using full image `width` as stride, but `UpsampleComponent` reads using `inputWidth` (the subsampled width) as stride. For 4:2:0 images, this produces garbled color output.
Either change the block coordinate calculation so chroma is stored at subsampled resolution with subsampled stride, or change `UpsampleComponent` to read with stride `width`.</violation>
</file>
<file name="native/src/jpeg12_wrapper.c">
<violation number="1" location="native/src/jpeg12_wrapper.c:419">
P0: Passing `sizeof(cinfo_buf)` (4096) as the `structsize` parameter to `jpeg12_jpeg_CreateDecompress` will trigger a `JERR_BAD_STRUCT_SIZE` error at runtime, because libjpeg performs a strict equality check against `sizeof(struct jpeg_decompress_struct)` (typically ~500-700 bytes). The same issue exists on the compression side at `jpeg12_jpeg_CreateCompress`. Since the actual `jpeglib.h` header isn't included, the correct struct size isn't available. Consider defining a build-time constant derived from the 12-bit libjpeg-turbo build, or including the actual prefixed header.</violation>
</file>
<file name="src/SharpDicom/Codecs/Video/VideoDicomBuilder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Video/VideoDicomBuilder.cs:251">
P2: Copying template attributes overwrites the previously set StudyInstanceUID, so `WithStudyInstanceUid()` (or the generated UID) is ignored whenever the template has its own StudyInstanceUID. Reapply the explicit StudyInstanceUID after copying the template (only when the caller provided one).</violation>
</file>
<file name=".planning/STATE.md">
<violation number="1" location=".planning/STATE.md:545">
P3: The test count in the session context conflicts with the updated test status earlier in the file, so the documentation now reports two different totals. Align the session context test coverage line with the actual test status to avoid confusion.</violation>
</file>
<file name="native/src/video_encoder.c">
<violation number="1" location="native/src/video_encoder.c:626">
P2: Performance: `sws_ctx` is unconditionally destroyed and recreated on every frame, despite the comment saying "if input format changed". This is expensive — `sws_getContext` allocates internal conversion tables each time. Cache the current source format and only recreate when it actually changes.</violation>
<violation number="2" location="native/src/video_encoder.c:830">
P0: Heap buffer overflow: `audio_frame->nb_samples` is overwritten with the input sample count, but the frame's data buffer was only allocated for `frame_size` samples. If the caller provides more samples than `frame_size`, `swr_convert` writes past the allocated buffer.
You need to either cap conversion to the allocated frame size (processing in chunks), or reallocate the frame buffer when the input exceeds the original allocation.</violation>
</file>
<file name="src/SharpDicom.Codecs/Codecs/NativeImageLoader.cs">
<violation number="1" location="src/SharpDicom.Codecs/Codecs/NativeImageLoader.cs:65">
P1: The code incorrectly handles 4-channel (RGBA) and 2-channel (Gray+Alpha) images by treating them as `VideoPixelFormat.Rgb24` or `Gray8` without converting the data.
1. `VideoFrame.Rgb24` expects 3 bytes per pixel. If `actualChannels` is 4, the data contains 4 bytes per pixel, resulting in a stride mismatch and image corruption.
2. If `actualChannels` is 2, the code falls through to `Rgb24`, but the data size (2 bytes/pixel) is insufficient for `Rgb24`, causing `VideoFrame` constructor to throw an `ArgumentException`.
The fix should strip the alpha channel when `actualChannels` doesn't match the target format.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…codecs - Fix chroma stride mismatch in JpegExtendedDecoder UpsampleComponent (P0) - Fix audio buffer overflow in video_encoder.c (P0) - Fix jpeg12_wrapper.c struct size passed to jpeg_Create*() (P0, latent) - Add 12-bit chroma upsampling in WriteOutput12Bit (P1) - Remove premature ZRL emission in JpegExtendedEncoder (P1) - Fix NativeImageLoader RGBA/GrayAlpha channel mapping (P1) - Fix CRF zero-init sentinel in video_encoder (P1) - Zero ArrayPool buffer in JpegExtendedDecoder (P2) - Forward progress parameter in NativeCodecs backend lambda (P2) - Add explicit check for unsupported 16-bit RGB in VideoEncoder (P2) - Fix template overwriting StudyInstanceUID in VideoDicomBuilder (P2) - Cache sws_ctx in video_encoder.c to avoid per-frame recreation (P2) - Update stale test count in STATE.md (P3)
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
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="src/SharpDicom.Codecs/Codecs/NativeImageLoader.cs">
<violation number="1" location="src/SharpDicom.Codecs/Codecs/NativeImageLoader.cs:79">
P2: The new 4-channel rejection contradicts the method's XML documentation (which still advertises desiredChannels=4 for RGBA). Either update the public docs or support RGBA conversion; otherwise callers following the documented contract will now fail at runtime.</violation>
</file>
<file name="src/SharpDicom.Codecs/NativeCodecs.cs">
<violation number="1" location="src/SharpDicom.Codecs/NativeCodecs.cs:565">
P2: Progress reports always send 0 total frames/0% and a zero elapsed time, so every callback looks like no progress even after frames are encoded. Track elapsed time and (when possible) total frame count to populate `VideoEncodeProgress` correctly (e.g., via `VideoEncodeProgress.Create`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Update XML docs to remove RGBA (4-channel) from desiredChannels - Track elapsed time with Stopwatch in video encoder progress reports
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.
Summary
Test plan
dotnet buildpasses with zero warningsdotnet testpasses (4844 tests, 0 failures)High-level PR Summary
This PR adds comprehensive 12-bit/16-bit JPEG encoding/decoding capabilities to SharpDicom through both managed C# implementations (
JpegExtendedCodec) and native libjpeg-turbo wrappers (NativeJpeg12Codec), along with complete video DICOM encoding infrastructure supporting MPEG-2, H.264, and HEVC codecs with audio support (AAC/PCM). The implementation includes 9 video transfer syntaxes, 7 video SOP class UIDs, a native FFmpeg-based video encoder C layer with GPU acceleration support (NVENC, VideoToolbox, VAAPI), a managedVideoEncoderAPI using a delegate backend pattern, and a fluentVideoDicomBuilderAPI for creating valid video DICOM files. The native build system uses Zig for cross-compilation with symbol-prefixed dual libjpeg-turbo builds (8-bit retains SIMD, 12-bit uses raw libjpeg API), FFmpeg/x264/x265 compilation from source, and vendored stb_image for image sequence loading. All functionality is comprehensively tested with 104 new tests (38 for 12-bit JPEG, 66 for video encoding) using synthetic test data, achieving zero failures across 4844 total tests.⏱️ Estimated Review Time: 3+ hours
💡 Review Order Suggestion
.planning/ROADMAP.md.planning/STATE.md.planning/phases/27-extended-codec-support/27-01-PLAN.md.planning/phases/27-extended-codec-support/27-01-SUMMARY.mdsrc/SharpDicom/Data/CompressionType.cssrc/SharpDicom/Data/TransferSyntax.cssrc/SharpDicom/Data/DicomUID.WellKnown.cs.planning/phases/27-extended-codec-support/27-02-PLAN.md.planning/phases/27-extended-codec-support/27-02-SUMMARY.mdsrc/SharpDicom/Codecs/Jpeg/JpegExtendedCodec.cssrc/SharpDicom/Codecs/Jpeg/JpegExtendedDecoder.cssrc/SharpDicom/Codecs/Jpeg/JpegExtendedEncoder.cssrc/SharpDicom/Codecs/CodecInitializer.cs.planning/phases/27-extended-codec-support/27-03-PLAN.md.planning/phases/27-extended-codec-support/27-03-SUMMARY.mdnative/src/jpeg12_wrapper.hnative/src/jpeg12_wrapper.cnative/src/sharpdicom_codecs.hnative/src/sharpdicom_codecs.c.planning/phases/27-extended-codec-support/27-04-PLAN.md.planning/phases/27-extended-codec-support/27-04-SUMMARY.mdsrc/SharpDicom.Codecs/Codecs/NativeJpeg8Codec.cssrc/SharpDicom.Codecs/Codecs/NativeJpeg12Codec.cssrc/SharpDicom.Codecs/Interop/NativeMethods.cs.planning/phases/27-extended-codec-support/27-05-PLAN.md.planning/phases/27-extended-codec-support/27-05-SUMMARY.mdtests/SharpDicom.Tests/Codecs/Jpeg/JpegExtendedCodecTests.cstests/SharpDicom.Tests/Codecs/Jpeg/JpegExtended12BitTests.cstests/SharpDicom.Tests/Codecs/JpegLossless/JpegLossless16BitTests.cs.planning/phases/27-extended-codec-support/27-06-PLAN.md.planning/phases/27-extended-codec-support/27-06-SUMMARY.mdnative/src/video_encoder.hnative/src/video_encoder.cnative/src/stb_image_wrapper.hnative/src/stb_image_wrapper.cnative/vendor/stb/stb_image.h.planning/phases/27-extended-codec-support/27-07-PLAN.md.planning/phases/27-extended-codec-support/27-07-SUMMARY.mdnative/build.zig.planning/phases/27-extended-codec-support/27-08-PLAN.md.planning/phases/27-extended-codec-support/27-08-SUMMARY.mdsrc/SharpDicom/Codecs/Video/VideoFrame.cssrc/SharpDicom/Codecs/Video/VideoEncoderOptions.cssrc/SharpDicom/Codecs/Video/VideoEncodeProgress.cssrc/SharpDicom/Codecs/Video/VideoEncoder.cssrc/SharpDicom.Codecs/Codecs/NativeVideoEncoder.cssrc/SharpDicom.Codecs/Codecs/NativeImageLoader.cssrc/SharpDicom.Codecs/Interop/SafeHandles.cs.planning/phases/27-extended-codec-support/27-09-PLAN.md.planning/phases/27-extended-codec-support/27-09-SUMMARY.mdsrc/SharpDicom/Codecs/Video/VideoSopClass.cssrc/SharpDicom/Codecs/Video/VideoDicomBuilder.cs.planning/phases/27-extended-codec-support/27-10-PLAN.md.planning/phases/27-extended-codec-support/27-10-SUMMARY.mdtests/SharpDicom.Tests/Codecs/Video/VideoDicomBuilderTests.cstests/SharpDicom.Tests/Codecs/Video/VideoEncoderOptionsTests.cstests/SharpDicom.Tests/Codecs/Video/VideoEncoderTests.cs.planning/phases/27-extended-codec-support/27-11-PLAN.md.planning/phases/27-extended-codec-support/27-11-SUMMARY.mdsrc/SharpDicom.Codecs/NativeCodecs.cs.planning/phases/27-extended-codec-support/27-12-PLAN.md.planning/phases/27-extended-codec-support/27-12-SUMMARY.mdnative/BUILD-REQUIREMENTS.md.planning/phases/27-extended-codec-support/27-CONTEXT.md.planning/phases/27-extended-codec-support/27-RESEARCH.md.planning/phases/27-extended-codec-support/27-VERIFICATION.mdtests/SharpDicom.Tests/Codecs/CodecRegistryIntegrationTests.cs