Repository navigation
Phase 12: Pure C# Codecs - #19
Merged
Merged
Conversation
This was referenced Jan 30, 2026
![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 ef3c1fe..725db0e
| Severity | Location | Issue | Delete |
|---|---|---|---|
 |
src/SharpDicom/Codecs/Jpeg/JpegMarkers.cs:205 | <- Click for details |  |
✅ Files analyzed, no issues (48)
• .planning/STATE.md
• .planning/phases/12-pure-csharp-codecs/12-01-SUMMARY.md
• .planning/phases/12-pure-csharp-codecs/12-02-SUMMARY.md
• .planning/phases/12-pure-csharp-codecs/12-03-SUMMARY.md
• .planning/phases/12-pure-csharp-codecs/12-04-SUMMARY.md
• .planning/phases/12-pure-csharp-codecs/12-05-SUMMARY.md
• .planning/phases/12-pure-csharp-codecs/12-06-SUMMARY.md
• .planning/phases/12-pure-csharp-codecs/12-07-SUMMARY.md
• .planning/phases/12-pure-csharp-codecs/12-CONTEXT.md
• .planning/phases/12-pure-csharp-codecs/12-RESEARCH.md
• src/SharpDicom/Codecs/CodecInitializer.cs
• src/SharpDicom/Codecs/ColorConversion.cs
• src/SharpDicom/Codecs/Jpeg/BitReader.cs
• src/SharpDicom/Codecs/Jpeg/BitWriter.cs
• src/SharpDicom/Codecs/Jpeg/DctTransform.cs
• src/SharpDicom/Codecs/Jpeg/HuffmanTable.cs
• src/SharpDicom/Codecs/Jpeg/JpegBaselineCodec.cs
• src/SharpDicom/Codecs/Jpeg/JpegBaselineDecoder.cs
• src/SharpDicom/Codecs/Jpeg/JpegBaselineEncoder.cs
• src/SharpDicom/Codecs/Jpeg/JpegCodecOptions.cs
• src/SharpDicom/Codecs/Jpeg/JpegFrameInfo.cs
• src/SharpDicom/Codecs/Jpeg/QuantizationTable.cs
• src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs
• src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs
• src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs
• src/SharpDicom/Codecs/Jpeg2000/Jpeg2000CodecOptions.cs
• src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LosslessCodec.cs
• src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cs
• src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotDecoder.cs
• src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotEncoder.cs
• src/SharpDicom/Codecs/Jpeg2000/Tier1/MqCoder.cs
• src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketDecoder.cs
• src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs
• src/SharpDicom/Codecs/Jpeg2000/Wavelet/Dwt53.cs
• src/SharpDicom/Codecs/Jpeg2000/Wavelet/Dwt97.cs
• src/SharpDicom/Codecs/Jpeg2000/Wavelet/DwtTransform.cs
• src/SharpDicom/Codecs/JpegLossless/JpegLosslessCodec.cs
• src/SharpDicom/Codecs/JpegLossless/JpegLosslessDecoder.cs
• src/SharpDicom/Codecs/JpegLossless/JpegLosslessEncoder.cs
• src/SharpDicom/Codecs/JpegLossless/LosslessHuffman.cs
• src/SharpDicom/Codecs/JpegLossless/Predictor.cs
• src/SharpDicom/Data/TransferSyntax.cs
• src/SharpDicom/Internal/ArrayBufferWriterPolyfill.cs
• tests/SharpDicom.Tests/Codecs/CodecRegistryIntegrationTests.cs
• tests/SharpDicom.Tests/Codecs/Jpeg2000/Jpeg2000CodecTests.cs
• tests/SharpDicom.Tests/Codecs/Jpeg2000/Jpeg2000Tests.cs
• tests/SharpDicom.Tests/Codecs/JpegBaselineCodecTests.cs
• tests/SharpDicom.Tests/Codecs/JpegLosslessCodecTests.cs
There was a problem hiding this comment.
Pull request overview
This PR introduces a full pure-C# codec stack for JPEG Baseline, JPEG Lossless, and JPEG 2000 (lossless and lossy), together with an explicit AOT-safe codec registration mechanism and supporting infrastructure (bit I/O, color conversion, DWT, EBCOT, etc.). It also wires these codecs into DICOM TransferSyntax definitions and adds focused unit/integration tests for codec behavior and registry registration.
Changes:
- Add JPEG Lossless encoder/decoder (
JpegLosslessEncoder/JpegLosslessDecoder) with DPCM predictors, extended Huffman coding, and anIPixelDataCodecwrapper (JpegLosslessCodec). - Add JPEG 2000 infrastructure (DWT 5/3 and 9/7, MQ coder, Tier-2 packet parsing) and codec wrappers (
Jpeg2000LosslessCodec,Jpeg2000LossyCodec) plus configurableJpeg2000CodecOptions. - Add JPEG Baseline codec wrapper (
JpegBaselineCodec) and supporting JPEG primitives (markers, quant tables, bit reader/writer, frame info, codec options) and a netstandard2.0ArrayBufferWriterpolyfill. - Introduce
CodecInitializerand registry integration tests to ensure all built-in codecs are registered in an AOT/trimming-friendly way.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 40 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SharpDicom.Tests/Codecs/Jpeg2000/Jpeg2000CodecTests.cs | Verifies JPEG 2000 lossless/lossy codec capabilities, basic encode/decode paths, options defaults, validation, and multi-frame handling. |
| tests/SharpDicom.Tests/Codecs/CodecRegistryIntegrationTests.cs | Ensures CodecInitializer.RegisterAll registers all codecs, registry type resolution works, and registration/reset semantics are correct. |
| src/SharpDicom/Internal/ArrayBufferWriterPolyfill.cs | Provides a minimal IBufferWriter<T> implementation for netstandard2.0, used for PDU/codec buffer building without adding framework dependencies. |
| src/SharpDicom/Data/TransferSyntax.cs | Adds JPEGLossless and JPEG2000Lossy transfer syntaxes and updates FromUID to map their UIDs to the new static instances. |
| src/SharpDicom/Codecs/JpegLossless/Predictor.cs | Implements JPEG Lossless DPCM predictors and neighbor fetching logic per ITU-T.81, including default value calculation for the first sample. |
| src/SharpDicom/Codecs/JpegLossless/LosslessHuffman.cs | Implements Huffman encode/decode for lossless JPEG residuals, plus DHT parsing and serialization; note: default BITS/VALUES tables are currently inconsistent and can lead to out-of-range indices when decoding. |
| src/SharpDicom/Codecs/JpegLossless/JpegLosslessEncoder.cs | Encodes a frame to JPEG Lossless (SOI/DHT/SOF3/SOS/entropy/EOI) using predictors and LosslessHuffman; currently assumes planar sample layout for multi-component data and ignores PlanarConfiguration. |
| src/SharpDicom/Codecs/JpegLossless/JpegLosslessDecoder.cs | Parses JPEG Lossless streams and reconstructs samples; writes them sequentially without honoring PixelDataInfo.PlanarConfiguration, always producing a planar layout for multi-component images. |
| src/SharpDicom/Codecs/JpegLossless/JpegLosslessCodec.cs | Wraps the lossless JPEG encoder/decoder in an IPixelDataCodec with capability metadata, sync/async encode/decode, validation, and optional basic offset table generation. |
| src/SharpDicom/Codecs/Jpeg2000/Wavelet/DwtTransform.cs | Coordinates multi-level 2D DWT forward/inverse calls using either 5/3 or 9/7 transforms and exposes subband/LL dimension helpers. |
| src/SharpDicom/Codecs/Jpeg2000/Wavelet/Dwt53.cs | Implements the reversible 5/3 lifting transform (forward/inverse, horizontal/vertical, interleaving/deinterleaving) for lossless JPEG 2000. |
| src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketDecoder.cs | Decodes JPEG 2000 packet headers and code-block segments, and provides helpers to aggregate segments across packets into full code-block streams. |
| src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cs | IPixelDataCodec wrapper for JPEG 2000 lossy coding; delegates frame encode/decode to J2kEncoder/J2kDecoder and validates codestream structure. |
| src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LosslessCodec.cs | IPixelDataCodec wrapper for JPEG 2000 lossless coding; uses J2K infrastructure and enforces reversible transform for lossless codestreams. |
| src/SharpDicom/Codecs/Jpeg2000/Jpeg2000CodecOptions.cs | Defines configurable encoding options (compression ratio, decomposition levels, layers, code-block size, offset table flag) with Default and MedicalImaging presets. |
| src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs | Implements a JPEG 2000 decoder pipeline (header parsing, packet decoding, EBCOT, inverse DWT, inverse color transform, and output layout honoring planar vs interleaved). |
| src/SharpDicom/Codecs/Jpeg/QuantizationTable.cs | Adds quantization-table representation, default luminance/chrominance tables, zigzag mapping, DQT parsing, and quality-based scaling. |
| src/SharpDicom/Codecs/Jpeg/JpegMarkers.cs | Centralizes JPEG marker constants and helper classification methods (SOF, RST, APP, lossless/progressive/arithmetic). |
| src/SharpDicom/Codecs/Jpeg/JpegFrameInfo.cs | Parses SOF frame headers into a structured representation used by the JPEG baseline decoder. |
| src/SharpDicom/Codecs/Jpeg/JpegCodecOptions.cs | Provides configurable options for JPEG baseline encoding (quality, subsampling, JFIF marker, Huffman optimization, restart, progressive), including presets like MedicalImaging. |
| src/SharpDicom/Codecs/Jpeg/JpegBaselineCodec.cs | IPixelDataCodec wrapper for JPEG Baseline, delegating to JpegBaselineEncoder/JpegBaselineDecoder and performing minimal validation. |
| src/SharpDicom/Codecs/Jpeg/BitWriter.cs | Bit-level writer used by JPEG Huffman and lossless coding, handling byte buffering and 0xFF byte stuffing. |
| src/SharpDicom/Codecs/Jpeg/BitReader.cs | Bit-level reader for JPEG entropy-coded data, handling byte stuffing and marker detection. |
| src/SharpDicom/Codecs/ColorConversion.cs | Implements RGB↔YCbCr, JPEG 2000 RCT/ICT transforms, and 8/16-bit YCbCr→RGB conversions using BT.601 coefficients. |
| src/SharpDicom/Codecs/CodecInitializer.cs | Provides an explicit, idempotent, thread-safe registration entry point (RegisterAll) for all built-in codecs and an IsInitialized/Reset API. |
| .planning/phases/12-pure-csharp-codecs/12-CONTEXT.md | High-level context and constraints for Phase 12, including codec priorities, error-handling strategy, and photometric conversion expectations. |
| .planning/phases/12-pure-csharp-codecs/12-07-SUMMARY.md | Summary of JPEG 2000 codec wrapper and registry integration work (Plan 07). |
| .planning/phases/12-pure-csharp-codecs/12-06-SUMMARY.md | Summary of JPEG 2000 Tier-1/Tier-2/EBCOT infrastructure implementation (Plan 06). |
| .planning/phases/12-pure-csharp-codecs/12-05-SUMMARY.md | Summary of JPEG 2000 codestream, DWT, and MQ coder infrastructure (Plan 05). |
| .planning/phases/12-pure-csharp-codecs/12-04-SUMMARY.md | Summary of JPEG Lossless codec implementation (Plan 04). |
| .planning/phases/12-pure-csharp-codecs/12-03-SUMMARY.md | Summary of JPEG Baseline codec implementation (Plan 03). |
| .planning/phases/12-pure-csharp-codecs/12-02-SUMMARY.md | Summary of DCT and bit I/O infrastructure for JPEG (Plan 02). |
| .planning/phases/12-pure-csharp-codecs/12-01-SUMMARY.md | Summary of initial JPEG infrastructure (markers, tables, color conversion, transfer syntaxes) (Plan 01). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+55
to
+63
| int width = info.Columns; | ||
| int height = info.Rows; | ||
| int precision = info.BitsStored; | ||
| int components = info.SamplesPerPixel; | ||
| int bytesPerSample = info.BytesPerSample; | ||
|
|
||
| // Read samples from input | ||
| int[] samples = ReadSamplesFromInput(pixelData, width * height * components, bytesPerSample); | ||
|
|
There was a problem hiding this comment.
ReadSamplesFromInput combined with the loop in EncodeEntropyData assumes that multi-component images are stored as planar data (all samples for component 0, then all for component 1, etc.), because componentOffset is computed as component * width * height. For DICOM pixel data with PlanarConfiguration == 0 (the common RGB case, PixelDataInfo.IsPlanar == false), samples are actually interleaved per pixel, so this logic will misassign samples to components when SamplesPerPixel > 1. Either restrict SupportedSamplesPerPixel to 1 for this codec or add explicit handling to reorder interleaved input into per-component planes based on PixelDataInfo.PlanarConfiguration before encoding.
Comment on lines
+296
to
+316
| private static int WriteSamplesToOutput(int[] samples, Span<byte> output, int bytesPerSample) | ||
| { | ||
| int outputIndex = 0; | ||
|
|
||
| for (int i = 0; i < samples.Length; i++) | ||
| { | ||
| int sample = samples[i]; | ||
|
|
||
| if (bytesPerSample == 1) | ||
| { | ||
| output[outputIndex++] = (byte)sample; | ||
| } | ||
| else | ||
| { | ||
| // Little-endian 16-bit output (DICOM native format) | ||
| output[outputIndex++] = (byte)(sample & 0xFF); | ||
| output[outputIndex++] = (byte)((sample >> 8) & 0xFF); | ||
| } | ||
| } | ||
|
|
||
| return outputIndex; |
There was a problem hiding this comment.
WriteSamplesToOutput always emits samples in a planar layout (all samples for component 0, followed by component 1, etc.) and does not consult PixelDataInfo.PlanarConfiguration/IsPlanar. This is inconsistent with the JPEG 2000 decoder’s WriteOutput (src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs:300–341), which respects planar vs interleaved output, and can produce incorrect decompressed layout for multi-component images when the dataset expects pixel‑interleaved data (PlanarConfiguration == 0). Consider branching here on the planar configuration and writing either interleaved or planar output to match the DICOM metadata, or clearly restricting this codec to single‑component images.
Comment on lines
+575
to
+581
| foreach (var comp in components) | ||
| { | ||
| if (comp.ComponentId == componentId) | ||
| { | ||
| return comp; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Comment on lines
+259
to
+266
| foreach (var contrib in contributions) | ||
| { | ||
| if (contrib.NumNewPasses > 0) | ||
| { | ||
| hasContributions = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Comment on lines
+139
to
+146
| if (coefficient >= 0) | ||
| { | ||
| return (coefficient + (divisor >> 1)) / divisor; | ||
| } | ||
| else | ||
| { | ||
| return (coefficient - (divisor >> 1)) / divisor; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
Suggested change
| if (coefficient >= 0) | |
| { | |
| return (coefficient + (divisor >> 1)) / divisor; | |
| } | |
| else | |
| { | |
| return (coefficient - (divisor >> 1)) / divisor; | |
| } | |
| return (coefficient >= 0 | |
| ? coefficient + (divisor >> 1) | |
| : coefficient - (divisor >> 1)) / divisor; |
Comment on lines
+243
to
+251
| int scale; | ||
| if (quality < 50) | ||
| { | ||
| scale = 5000 / quality; | ||
| } | ||
| else | ||
| { | ||
| scale = 200 - (quality * 2); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Suggested change
| int scale; | |
| if (quality < 50) | |
| { | |
| scale = 5000 / quality; | |
| } | |
| else | |
| { | |
| scale = 200 - (quality * 2); | |
| } | |
| int scale = quality < 50 | |
| ? 5000 / quality | |
| : 200 - (quality * 2); |
Comment on lines
+128
to
+135
| if (_signState![i] != 0) | ||
| { | ||
| coefficients[i] = -magnitude; | ||
| } | ||
| else | ||
| { | ||
| coefficients[i] = magnitude; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Suggested change
| if (_signState![i] != 0) | |
| { | |
| coefficients[i] = -magnitude; | |
| } | |
| else | |
| { | |
| coefficients[i] = magnitude; | |
| } | |
| coefficients[i] = _signState![i] != 0 ? -magnitude : magnitude; |
Comment on lines
+526
to
+533
| if (_a < qe) | ||
| { | ||
| d = 1; | ||
| } | ||
| else | ||
| { | ||
| d = 0; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Comment on lines
+545
to
+552
| if (_a < qe) | ||
| { | ||
| d = 0; | ||
| } | ||
| else | ||
| { | ||
| d = 1; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
13 issues found across 49 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="src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs:630">
P2: Validate code-block exponent values before shifting; otherwise large exponent values wrap and produce invalid code-block sizes that pass the later bounds check.</violation>
</file>
<file name="src/SharpDicom/Codecs/JpegLossless/LosslessHuffman.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/LosslessHuffman.cs:37">
P1: DefaultBits defines 19 Huffman codes but DefaultValues only defines 17 symbols, which can drive DecodeCategory to index past _values and break encoding/decoding. Ensure the BITS counts sum to the number of values.</violation>
</file>
<file name="src/SharpDicom/Codecs/JpegLossless/JpegLosslessDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/JpegLosslessDecoder.cs:59">
P2: Missing validation for SOS marker presence. If SOS marker is missing but SOF3 is present, `scanDataStart` remains 0 and decoding proceeds with invalid data starting from the beginning of the frame.</violation>
<violation number="2" location="src/SharpDicom/Codecs/JpegLossless/JpegLosslessDecoder.cs:89">
P1: Missing bounds validation before slicing segment. A malformed JPEG with a large segment length could cause `ArgumentOutOfRangeException` instead of returning a proper error result.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Wavelet/DwtTransform.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Wavelet/DwtTransform.cs:38">
P2: Buffer size validation can overflow when computing width * height. If dimensions are large, the int multiplication can wrap and allow an undersized buffer through, leading to out-of-range access later. Cast to long (or use checked) before multiplying.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs:234">
P1: The 32-bit case ignores the `isSigned` parameter - both branches return `(int)value`. For unsigned values > `int.MaxValue`, this will overflow. If 32-bit unsigned samples are supported, the return type needs to be `long` or the method needs overflow handling.</violation>
</file>
<file name="tests/SharpDicom.Tests/Codecs/Jpeg2000/Jpeg2000CodecTests.cs">
<violation number="1" location="tests/SharpDicom.Tests/Codecs/Jpeg2000/Jpeg2000CodecTests.cs:260">
P2: This test assumes lossy JPEG 2000 output is always smaller than the input, which is not guaranteed and can make the test flaky for small images. Consider validating codestream markers or just that the output is non-empty instead.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/BitWriter.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/BitWriter.cs:74">
P2: Validate the lower bound for bit count. The current guard only rejects values > 25, so count == 0 is accepted even though the method contract requires 1-25.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/JpegBaselineDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/JpegBaselineDecoder.cs:161">
P1: Missing validation that SOF0 was parsed before processing SOS marker. If a malformed JPEG lacks SOF0 or has markers in wrong order, `FrameInfo` will be uninitialized, potentially causing null reference or logic errors in `DecodeScanData`.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/JpegCodecOptions.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/JpegCodecOptions.cs:156">
P2: Preset options are exposed as mutable singletons. Any caller can modify `MedicalImaging`, `Default`, or `HighCompression` and affect all subsequent usages, which is a surprising global side effect. Consider returning a new instance (or a defensive clone) each time instead of a shared mutable object.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/HuffmanTable.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/HuffmanTable.cs:395">
P1: End-of-data detection is incorrect. When `FetchByte()` fails due to end-of-data, `_bitPosition` remains 0, but the condition `_bytePosition > _data.Length` doesn't catch this case (it should be `>=`). This causes a right-shift by -1 (undefined behavior).</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/QuantizationTable.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/QuantizationTable.cs:260">
P2: CreateScaled clamps to 255 regardless of the base table precision, which truncates 16-bit tables even though the comment says 16-bit is supported. Use a max value based on baseTable.Precision.</violation>
<violation number="2" location="src/SharpDicom/Codecs/Jpeg/QuantizationTable.cs:263">
P2: CreateScaled hardcodes precision to 8, so scaling a 16-bit base table returns a table with the wrong precision. Preserve the base table precision.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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.
P1 fixes: - LosslessHuffman: correct BITS/VALUES mismatch (sum 17 not 19) - JpegLosslessDecoder: add segment length bounds validation - JpegLosslessDecoder: validate SOS marker presence before decode - J2kEncoder: handle signed vs unsigned 32-bit samples correctly - JpegBaselineDecoder: validate SOF0 before processing SOS - HuffmanTable: fix end-of-data detection in bit reader P2 fixes: - J2kCodestream: validate code-block exponents (0-8 range) - DwtTransform: use long multiplication to avoid overflow - BitWriter: validate bit count lower bound (must be >= 1) - JpegCodecOptions: presets return new instances to prevent mutation - QuantizationTable: use long arithmetic in CreateScaled - Jpeg2000CodecTests: use larger image for reliable compression test
jas88
force-pushed
the
pr/phase-12-codecs
branch
from
January 30, 2026 17:36
725db0e to
450a7e0
Compare
- JpegMarkers: add parentheses for operator precedence clarity - JpegBaselineCodec: remove redundant marker != SOF0 check - JpegBaselineDecoder: cast to float before multiplication, remove unused var - JpegBaselineEncoder: remove unused width/height variables - DctTransform: remove obsolete r1/r3/r5/r7 pre-assignments - J2kCodestream: remove unreachable SOD handling, fix offset increment - J2kDecoder: remove unused cbBuffer allocation - J2kEncoder: remove unused variables, fix offset increments - EbcotDecoder: remove unused bitMask in cleanup pass - EbcotEncoder: remove unused bitMask/numBitplanes, simplify GetSignContext
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+406
to
+412
| if (marker == J2kMarkers.SOT) | ||
| { | ||
| // SOT segment contains tile index at bytes 0-1 (after length) | ||
| if (position + 4 <= data.Length) | ||
| { | ||
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Suggested change
| if (marker == J2kMarkers.SOT) | |
| { | |
| // SOT segment contains tile index at bytes 0-1 (after length) | |
| if (position + 4 <= data.Length) | |
| { | |
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | |
| } | |
| // SOT segment contains tile index at bytes 0-1 (after length) | |
| if (marker == J2kMarkers.SOT && position + 4 <= data.Length) | |
| { | |
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); |
Comment on lines
+422
to
+427
| if (nextMarker == J2kMarkers.SOD) | ||
| { | ||
| if (currentTile == tileIndex) | ||
| { | ||
| return position + 2; // Return position after SOD marker | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Suggested change
| if (nextMarker == J2kMarkers.SOD) | |
| { | |
| if (currentTile == tileIndex) | |
| { | |
| return position + 2; // Return position after SOD marker | |
| } | |
| if (nextMarker == J2kMarkers.SOD && currentTile == tileIndex) | |
| { | |
| return position + 2; // Return position after SOD marker |
Comment on lines
+244
to
+251
| if (quality < 50) | ||
| { | ||
| scale = 5000 / quality; | ||
| } | ||
| else | ||
| { | ||
| scale = 200 - (quality * 2); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Comment on lines
+128
to
+135
| if (_signState![i] != 0) | ||
| { | ||
| coefficients[i] = -magnitude; | ||
| } | ||
| else | ||
| { | ||
| coefficients[i] = magnitude; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
CreateScaled was hardcoding precision to 8 and clamping to 255, which would truncate values and return wrong precision for 16-bit tables. Now preserves baseTable.Precision and clamps to appropriate max value.
There was a problem hiding this comment.
13 issues found across 48 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="src/SharpDicom/Codecs/CodecInitializer.cs">
<violation number="1" location="src/SharpDicom/Codecs/CodecInitializer.cs:29">
P2: The double-checked locking uses a non-volatile flag, so another thread can see _initialized == true without guaranteed visibility of the codec registrations. Mark the flag volatile (or use Volatile.Read/Write) to make the thread-safety claim accurate.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs:212">
P2: Layer pass allocation forces at least one new pass even when the target for this layer has already been met, which over-includes coding passes and breaks rate control. Allow zero passes when `targetPassesThisLayer - alreadyIncluded` is 0 or negative.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotEncoder.cs:464">
P1: Cleanup pass skips samples that gain a significant neighbor during the same cleanup stripe, so later coefficients may never be encoded for this bitplane. Cleanup should only skip samples already encoded in the significance propagation pass, not those that just gained a neighbor during cleanup.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketDecoder.cs:305">
P1: ReadLength doesn’t match the encoder’s zero-length encoding (encoder writes a single 0 bit), so zero-length contributions will shift the header and corrupt decoding. Align the decoder/encoder length scheme to handle the zero-length case consistently.</violation>
</file>
<file name="src/SharpDicom/Codecs/ColorConversion.cs">
<violation number="1" location="src/SharpDicom/Codecs/ColorConversion.cs:302">
P2: YCbCr chroma level shift should use (maxValue + 1) / 2 to match the midpoint of an unsigned range (e.g., 4095 -> 2048). Using maxValue / 2 causes an off-by-one bias for 12/16-bit samples.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs:392">
P1: The SIZ segment length calculation is incorrect - it adds an extra `+2` that shouldn't be there. This causes 2 bytes of uninitialized memory in the output and an incorrect length field in the codestream, which will break decoding.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cs:99">
P2: Lossy encoding ignores `Jpeg2000CodecOptions`, so caller-specified compression/decomposition settings never apply. Map the codec options into `J2kEncoderOptions` and pass the overload that accepts options.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs:267">
P2: FindTileDataOffset only detects SOD if it immediately follows SOT and otherwise treats SOD as a segment-less marker, so tile-part data is parsed as markers. This will miss tile data offsets for codestreams with tile-part header markers and for tiles after the first. Parse Psot in SOT and use it to skip tile-part data, or explicitly handle SOD in the loop and advance using Psot.</violation>
</file>
<file name="src/SharpDicom/Codecs/JpegLossless/Predictor.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/Predictor.cs:96">
P3: The boundary condition comment for the first row contradicts the actual logic (Ra comes from the left neighbor, Rb/Rc use defaultValue). Update the comment so it reflects the implemented behavior.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Wavelet/Dwt97.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Wavelet/Dwt97.cs:72">
P2: Forward2D rounds the horizontal 9/7 results back to int before the vertical transform, which adds an extra quantization step and increases loss beyond the intended JPEG 2000 workflow. The 9/7 DWT should keep full floating-point precision until after the 2D transform is complete.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/JpegBaselineDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/JpegBaselineDecoder.cs:616">
P1: Stride mismatch in chroma upsampling: input data is stored with `width` stride but read with `inputWidth` stride, causing corrupted colors in subsampled JPEGs (4:2:0, 4:2:2).</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotDecoder.cs:521">
P1: `GetSignContext` always returns `CtxSign`, ignoring neighbor state. Sign coding in JPEG 2000 is context-adaptive; using a constant context will mis-decode sign bits for standard codestreams. Implement the neighbor-based sign context (T.800 Table D.3) instead of the placeholder.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs:122">
P1: The `dataOffset` variable is never updated after decoding each component's packets. For multi-component images, each component's packet data should be at different offsets in the tile data, but currently all components decode from offset 0. Consider having `DecodeComponentPackets` return the number of bytes consumed so `dataOffset` can be advanced.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| private int ReadNumPasses() | ||
| { | ||
| // ITU-T T.800 Table B.4 | ||
| if (ReadBit() == 0) |
There was a problem hiding this comment.
P1: ReadLength doesn’t match the encoder’s zero-length encoding (encoder writes a single 0 bit), so zero-length contributions will shift the header and corrupt decoding. Align the decoder/encoder length scheme to handle the zero-length case consistently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketDecoder.cs, line 305:
<comment>ReadLength doesn’t match the encoder’s zero-length encoding (encoder writes a single 0 bit), so zero-length contributions will shift the header and corrupt decoding. Align the decoder/encoder length scheme to handle the zero-length case consistently.</comment>
<file context>
@@ -0,0 +1,373 @@
+ private int ReadNumPasses()
+ {
+ // ITU-T T.800 Table B.4
+ if (ReadBit() == 0)
+ {
+ return 1;
</file context>
| _ = y; | ||
| _ = width; | ||
| _ = height; | ||
| return CtxSign; |
There was a problem hiding this comment.
P1: GetSignContext always returns CtxSign, ignoring neighbor state. Sign coding in JPEG 2000 is context-adaptive; using a constant context will mis-decode sign bits for standard codestreams. Implement the neighbor-based sign context (T.800 Table D.3) instead of the placeholder.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotDecoder.cs, line 521:
<comment>`GetSignContext` always returns `CtxSign`, ignoring neighbor state. Sign coding in JPEG 2000 is context-adaptive; using a constant context will mis-decode sign bits for standard codestreams. Implement the neighbor-based sign context (T.800 Table D.3) instead of the placeholder.</comment>
<file context>
@@ -0,0 +1,531 @@
+ _ = y;
+ _ = width;
+ _ = height;
+ return CtxSign;
+ }
+
</file context>
| // Copy back to int | ||
| for (int x = 0; x < width; x++) | ||
| { | ||
| row[x] = (int)Math.Round(tempFloat[x]); |
There was a problem hiding this comment.
P2: Forward2D rounds the horizontal 9/7 results back to int before the vertical transform, which adds an extra quantization step and increases loss beyond the intended JPEG 2000 workflow. The 9/7 DWT should keep full floating-point precision until after the 2D transform is complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/Wavelet/Dwt97.cs, line 72:
<comment>Forward2D rounds the horizontal 9/7 results back to int before the vertical transform, which adds an extra quantization step and increases loss beyond the intended JPEG 2000 workflow. The 9/7 DWT should keep full floating-point precision until after the 2D transform is complete.</comment>
<file context>
@@ -0,0 +1,447 @@
+ // Copy back to int
+ for (int x = 0; x < width; x++)
+ {
+ row[x] = (int)Math.Round(tempFloat[x]);
+ }
+ }
</file context>
P1 fixes: - EbcotEncoder: fix cleanup pass skipping samples that gain neighbors during same stripe by tracking visited samples in sigprop pass - PacketEncoder: fix WriteLength zero-length encoding to match decoder - JpegBaselineDecoder: fix chroma upsampling stride mismatch - J2kCodestream: fix FindTileDataOffset to scan for SOD after SOT P2 fixes: - CodecInitializer: make _initialized volatile for thread safety - ColorConversion: fix YCbCr level shift to (maxValue + 1) / 2 - J2kEncoder: fix SIZ segment length calculation (remove extra +2) - Jpeg2000LossyCodec: map Jpeg2000CodecOptions to J2kEncoderOptions - Dwt97: document precision loss between transforms - EbcotDecoder/Encoder: document simplified sign context selection - J2kDecoder: document multi-component packet parsing limitation P3 fixes: - Predictor: fix boundary condition documentation to match code
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+61
to
+99
| // Read samples from input | ||
| int[] samples = ReadSamplesFromInput(pixelData, width * height * components, bytesPerSample); | ||
|
|
||
| // Estimate output size: in worst case (random data), output may exceed input | ||
| // For each sample: Huffman code (up to 16 bits) + category bits (up to 16 bits) | ||
| // So worst case is ~4 bytes per sample. Add header overhead. | ||
| int totalSamples = width * height * components; | ||
| int estimatedSize = (totalSamples * 4) + 1024; | ||
| byte[] outputBuffer = ArrayPool<byte>.Shared.Rent(estimatedSize); | ||
|
|
||
| try | ||
| { | ||
| int outputPosition = 0; | ||
|
|
||
| // 1. Write SOI marker | ||
| outputBuffer[outputPosition++] = JpegMarkers.Prefix; | ||
| outputBuffer[outputPosition++] = JpegMarkers.SOI; | ||
|
|
||
| // 2. Write DHT marker with Huffman table | ||
| WriteHuffmanTable(outputBuffer, ref outputPosition, LosslessHuffman.Default); | ||
|
|
||
| // 3. Write SOF3 marker (lossless frame header) | ||
| WriteSOF3(outputBuffer, ref outputPosition, precision, height, width, components); | ||
|
|
||
| // 4. Write SOS marker (start of scan) | ||
| WriteSOS(outputBuffer, ref outputPosition, components, selectionValue); | ||
|
|
||
| // 5. Encode entropy data | ||
| int defaultValue = Predictor.GetDefaultValue(precision, 0); | ||
| int entropyBytesWritten = EncodeEntropyData( | ||
| samples, | ||
| outputBuffer.AsSpan(outputPosition), | ||
| width, height, components, | ||
| precision, selectionValue, defaultValue, | ||
| LosslessHuffman.Default); | ||
| outputPosition += entropyBytesWritten; | ||
|
|
||
| // 6. Write EOI marker | ||
| outputBuffer[outputPosition++] = JpegMarkers.Prefix; |
There was a problem hiding this comment.
For multi-sample images (SamplesPerPixel > 1), this encoder ignores the PixelDataInfo.PlanarConfiguration/IsPlanar flag and assumes that samples for each component are stored in contiguous planes (componentOffset = component * width * height in EncodeEntropyData). However ReadSamplesFromInput reads the raw frame buffer sequentially, which for the common DICOM case PlanarConfiguration=0 (pixel-interleaved RGB) produces a samples array laid out as [R0,G0,B0,R1,G1,B1,...], not planar. As a result, color images with PlanarConfiguration=0 will be encoded incorrectly and cannot roundtrip losslessly. The encoder needs to branch on info.IsPlanar and map pixelData into per-component sample planes (similar to J2kEncoder.ExtractComponents) before prediction/entropy coding, and likewise compute componentOffset appropriately for planar vs interleaved layouts.
Comment on lines
+417
to
+423
| if (dcCategory > 0) | ||
| { | ||
| if (!bitReader.TryReadCoefficient(dcCategory, out dcDiff)) | ||
| { | ||
| return "Failed to read DC difference"; | ||
| } | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Comment on lines
+418
to
+425
| if (marker == J2kMarkers.SOT) | ||
| { | ||
| // SOT segment contains tile index at bytes 0-1 (after length) | ||
| if (position + 4 <= data.Length) | ||
| { | ||
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | ||
| inTargetTile = (currentTile == tileIndex); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Suggested change
| if (marker == J2kMarkers.SOT) | |
| { | |
| // SOT segment contains tile index at bytes 0-1 (after length) | |
| if (position + 4 <= data.Length) | |
| { | |
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | |
| inTargetTile = (currentTile == tileIndex); | |
| } | |
| // SOT segment contains tile index at bytes 0-1 (after length) | |
| if (marker == J2kMarkers.SOT && position + 4 <= data.Length) | |
| { | |
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | |
| inTargetTile = (currentTile == tileIndex); |
Comment on lines
+138
to
+146
| // Round to nearest (per JPEG spec: add divisor/2 before integer division) | ||
| if (coefficient >= 0) | ||
| { | ||
| return (coefficient + (divisor >> 1)) / divisor; | ||
| } | ||
| else | ||
| { | ||
| return (coefficient - (divisor >> 1)) / divisor; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
Suggested change
| // Round to nearest (per JPEG spec: add divisor/2 before integer division) | |
| if (coefficient >= 0) | |
| { | |
| return (coefficient + (divisor >> 1)) / divisor; | |
| } | |
| else | |
| { | |
| return (coefficient - (divisor >> 1)) / divisor; | |
| } | |
| int halfDivisor = divisor >> 1; | |
| // Round to nearest (per JPEG spec: add divisor/2 before integer division) | |
| return (coefficient >= 0 | |
| ? coefficient + halfDivisor | |
| : coefficient - halfDivisor) / divisor; |
There was a problem hiding this comment.
13 issues found across 48 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="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs:151">
P2: EncodePackets ignores progression/numResolutions; for any non-default values it still encodes only resolution 0. Add validation (e.g., reject numResolutions != 1 or unsupported progression) or implement the proper progression handling to avoid silently incorrect packets.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/JpegFrameInfo.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/JpegFrameInfo.cs:141">
P2: Validate sampling factors and quantization table IDs when parsing the SOF component data. As written, a malformed SOF can supply a quantTableId >= 4, which later causes an IndexOutOfRangeException when indexing `QuantTables` (size 4).</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs:592">
P2: Dead code: `tileData.CopyTo(dataSpan.ToArray())` does nothing useful. `ToArray()` creates a new array that's immediately discarded after the copy. The subsequent for-loop already handles the data copy correctly. Remove this line to avoid unnecessary memory allocation.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/BitReader.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/BitReader.cs:137">
P2: PeekBits sign-extends when buffering fewer bits because the cast to int happens before the right shift. This can return incorrect negative values when the high bit is set. Perform the right shift on the unsigned value and cast after to keep zero-padding.</violation>
</file>
<file name="src/SharpDicom/Codecs/CodecInitializer.cs">
<violation number="1" location="src/SharpDicom/Codecs/CodecInitializer.cs:45">
P2: Reset() isn’t atomic with CodecRegistry.Reset(), so another thread can register codecs between the unlock and registry reset, leaving _initialized=true while the registry is empty. Consider keeping the registry reset inside the same lock block to avoid inconsistent state.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/JpegBaselineEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/JpegBaselineEncoder.cs:51">
P1: Buffer size estimation doesn't account for byte stuffing, which can cause `InvalidOperationException` for images with many 0xFF values. The method `GetMaxEncodedSize` exists and correctly includes byte stuffing overhead (`pixelSize / 255`), but it's not used here.</violation>
</file>
<file name="tests/SharpDicom.Tests/Codecs/Jpeg2000/Jpeg2000CodecTests.cs">
<violation number="1" location="tests/SharpDicom.Tests/Codecs/Jpeg2000/Jpeg2000CodecTests.cs:265">
P2: This test assumes lossy encoding always produces smaller output than the raw buffer, which isn’t guaranteed and can make the test flaky. Consider removing the strict size assertion or tying it to an explicit compression target.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs:269">
P2: After encountering SOD for a non-target tile, the code never skips the tile data to the next marker. The loop then interprets tile data as markers, which can produce incorrect offsets or premature termination. Implement the marker scan described in the comment before continuing.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotDecoder.cs:358">
P1: Cleanup pass skips samples based on current neighbor significance, which can change during cleanup and causes missing decodes. Track which samples were processed in significance propagation (as the encoder does) and use that flag instead.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs:125">
P1: `dataOffset` is never updated in the component loop, causing all components to decode from the same position. For multi-component images (RGB), this will produce incorrect decoding results since each component should read from where the previous component's data ended.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cs:96">
P2: The lossy encoder ignores Jpeg2000CodecOptions.CompressionRatio, so callers cannot control the requested compression ratio/quality. This makes the lossy output use fixed defaults even when options specify a ratio.</violation>
</file>
<file name="src/SharpDicom/Codecs/JpegLossless/JpegLosslessDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/JpegLosslessDecoder.cs:246">
P1: Integer overflow in `totalSamples` calculation can cause buffer allocation issues for large images. Use `checked` arithmetic or cast to `long` before multiplication, similar to the pattern used in `DwtTransform.cs`.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LosslessCodec.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LosslessCodec.cs:98">
P2: Lossless encoding ignores the caller’s Jpeg2000CodecOptions (decomposition levels, quality layers, code-block size). This makes configuration ineffective and can lead to unexpected compression behavior. Map the codec options to J2kEncoderOptions when calling the encoder.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // TODO: For multi-component images with interleaved packets (LRCP progression), | ||
| // proper packet boundary parsing is required. This simplified approach works | ||
| // for single-component images or when components are stored in separate tiles. | ||
| int dataOffset = 0; |
There was a problem hiding this comment.
P1: dataOffset is never updated in the component loop, causing all components to decode from the same position. For multi-component images (RGB), this will produce incorrect decoding results since each component should read from where the previous component's data ended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs, line 125:
<comment>`dataOffset` is never updated in the component loop, causing all components to decode from the same position. For multi-component images (RGB), this will produce incorrect decoding results since each component should read from where the previous component's data ended.</comment>
<file context>
@@ -0,0 +1,377 @@
+ // TODO: For multi-component images with interleaved packets (LRCP progression),
+ // proper packet boundary parsing is required. This simplified approach works
+ // for single-component images or when components are stored in separate tiles.
+ int dataOffset = 0;
+
+ for (int c = 0; c < components; c++)
</file context>
- JpegBaselineEncoder: Use GetMaxEncodedSize() for buffer allocation to account for worst-case byte stuffing - PacketEncoder: Add validation for numLayers, numResolutions, and progression order parameters - Jpeg2000CodecTests: Remove flaky lossy size assertion - J2kCodestream: Fix FindTileDataOffset to skip non-target tile data by scanning for next marker after SOD - EbcotDecoder: Add visited flag tracking for cleanup pass to match encoder behavior and avoid double processing - Jpeg2000LossyCodec: Map CompressionRatio from codec options to encoder options - JpegLosslessDecoder: Fix integer overflow in totalSamples calculation by using long arithmetic with overflow check - Jpeg2000LosslessCodec: Map codec options to J2kEncoderOptions - J2kEncoder: Add CompressionRatio property to J2kEncoderOptions
There was a problem hiding this comment.
12 issues found across 48 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="src/SharpDicom/Codecs/JpegLossless/JpegLosslessEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/JpegLosslessEncoder.cs:68">
P2: The output size estimate ignores JPEG byte-stuffing overhead, so valid data can overflow the fixed buffer and throw `InvalidOperationException`. Consider allocating for worst-case byte-stuffing (or use a growable buffer) to avoid encoder failures on valid inputs.</violation>
</file>
<file name="src/SharpDicom/Codecs/JpegLossless/LosslessHuffman.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/LosslessHuffman.cs:316">
P2: Validate DHT symbol values are within the expected lossless category range (0–16). Without this, malformed DHT segments can produce categories that drive `ReadBits` with an unsupported bit count, causing runtime exceptions during decode instead of cleanly failing the parse.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs:125">
P1: `dataOffset` is never updated after processing each component, causing all components to read from the same position. For multi-component images, this will result in corrupted output where all color channels decode identical data. The `DecodeComponentPackets` method should return how many bytes were consumed so `dataOffset` can be advanced.</violation>
</file>
<file name="src/SharpDicom/Codecs/JpegLossless/JpegLosslessCodec.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/JpegLosslessCodec.cs:88">
P2: Validate SelectionValue (1-7) before encoding to avoid generating invalid lossless JPEG streams.</violation>
<violation number="2" location="src/SharpDicom/Codecs/JpegLossless/JpegLosslessCodec.cs:91">
P2: Validate that pixelData.Length is a multiple of frameSize before computing frameCount; otherwise extra bytes are silently dropped.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotEncoder.cs:353">
P1: Cleanup-pass run-length mode should be gated by the current significant-neighbor state, not just whether the sample was visited in the significance-propagation pass. Otherwise the encoder can emit run-length mode when neighbors become significant earlier in the cleanup pass, but the decoder will refuse run-length mode and desync.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs:522">
P2: Lossy quantization style is incorrect. The ternary always returns `0x00` for both branches (dead code), and the comment indicates lossy should use style 1 or 2. This will cause lossy encoding to lack proper quantization in the QCD marker.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs:491">
P2: ImageWidth/ImageHeight should be computed from the reference grid size minus the image offsets (Xsiz - XOsiz / Ysiz - YOsiz). Using the raw Xsiz/Ysiz will reject valid codestreams with non-zero offsets and can lead to incorrect dimension checks or buffer sizing.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs:512">
P2: WriteLength silently truncates code-block data lengths larger than 65535 bytes to 16 bits, which will corrupt packet parsing for large code-blocks. Add a guard (or extend the length coding) to handle larger lengths explicitly.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cs:102">
P2: CompressionRatio from Jpeg2000CodecOptions is ignored by the encoder, so lossy output quality/size cannot be controlled as documented.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LosslessCodec.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LosslessCodec.cs:92">
P2: Encode silently drops trailing bytes when pixelData.Length is not a multiple of frameSize. Add a validation check so invalid input fails fast instead of producing truncated frames.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/JpegBaselineEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/JpegBaselineEncoder.cs:659">
P1: Buffer size calculation underestimates worst-case byte stuffing. JPEG byte stuffing adds 0x00 after each 0xFF byte, so worst case is `pixelSize` additional bytes (every byte is 0xFF), not `pixelSize / 255`. This could cause buffer overflow.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // TODO: For multi-component images with interleaved packets (LRCP progression), | ||
| // proper packet boundary parsing is required. This simplified approach works | ||
| // for single-component images or when components are stored in separate tiles. | ||
| int dataOffset = 0; |
There was a problem hiding this comment.
P1: dataOffset is never updated after processing each component, causing all components to read from the same position. For multi-component images, this will result in corrupted output where all color channels decode identical data. The DecodeComponentPackets method should return how many bytes were consumed so dataOffset can be advanced.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cs, line 125:
<comment>`dataOffset` is never updated after processing each component, causing all components to read from the same position. For multi-component images, this will result in corrupted output where all color channels decode identical data. The `DecodeComponentPackets` method should return how many bytes were consumed so `dataOffset` can be advanced.</comment>
<file context>
@@ -0,0 +1,377 @@
+ // TODO: For multi-component images with interleaved packets (LRCP progression),
+ // proper packet boundary parsing is required. This simplified approach works
+ // for single-component images or when components are stored in separate tiles.
+ int dataOffset = 0;
+
+ for (int c = 0; c < components; c++)
</file context>
| { | ||
| // Check if we can use run-length coding | ||
| bool allInsignificant = true; | ||
| bool noneVisitedInSigProp = true; |
There was a problem hiding this comment.
P1: Cleanup-pass run-length mode should be gated by the current significant-neighbor state, not just whether the sample was visited in the significance-propagation pass. Otherwise the encoder can emit run-length mode when neighbors become significant earlier in the cleanup pass, but the decoder will refuse run-length mode and desync.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotEncoder.cs, line 353:
<comment>Cleanup-pass run-length mode should be gated by the current significant-neighbor state, not just whether the sample was visited in the significance-propagation pass. Otherwise the encoder can emit run-length mode when neighbors become significant earlier in the cleanup pass, but the decoder will refuse run-length mode and desync.</comment>
<file context>
@@ -0,0 +1,693 @@
+ {
+ // Check if we can use run-length coding
+ bool allInsignificant = true;
+ bool noneVisitedInSigProp = true;
+
+ for (int dy = 0; dy < stripeHeight && allInsignificant; dy++)
</file context>
| // Worst case: no compression + headers + byte stuffing | ||
| int pixelSize = info.FrameSize; | ||
| int headerOverhead = 1024; // Conservative estimate for all markers | ||
| int byteStuffing = pixelSize / 255; // Worst case stuffing |
There was a problem hiding this comment.
P1: Buffer size calculation underestimates worst-case byte stuffing. JPEG byte stuffing adds 0x00 after each 0xFF byte, so worst case is pixelSize additional bytes (every byte is 0xFF), not pixelSize / 255. This could cause buffer overflow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg/JpegBaselineEncoder.cs, line 659:
<comment>Buffer size calculation underestimates worst-case byte stuffing. JPEG byte stuffing adds 0x00 after each 0xFF byte, so worst case is `pixelSize` additional bytes (every byte is 0xFF), not `pixelSize / 255`. This could cause buffer overflow.</comment>
<file context>
@@ -0,0 +1,663 @@
+ // Worst case: no compression + headers + byte stuffing
+ int pixelSize = info.FrameSize;
+ int headerOverhead = 1024; // Conservative estimate for all markers
+ int byteStuffing = pixelSize / 255; // Worst case stuffing
+ return pixelSize + headerOverhead + byteStuffing;
+ }
</file context>
| // Skip Rsiz (capabilities) | ||
| offset += 2; | ||
|
|
||
| imageWidth = (int)BinaryPrimitives.ReadUInt32BigEndian(data.Slice(offset)); |
There was a problem hiding this comment.
P2: ImageWidth/ImageHeight should be computed from the reference grid size minus the image offsets (Xsiz - XOsiz / Ysiz - YOsiz). Using the raw Xsiz/Ysiz will reject valid codestreams with non-zero offsets and can lead to incorrect dimension checks or buffer sizing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cs, line 491:
<comment>ImageWidth/ImageHeight should be computed from the reference grid size minus the image offsets (Xsiz - XOsiz / Ysiz - YOsiz). Using the raw Xsiz/Ysiz will reject valid codestreams with non-zero offsets and can lead to incorrect dimension checks or buffer sizing.</comment>
<file context>
@@ -0,0 +1,665 @@
+ // Skip Rsiz (capabilities)
+ offset += 2;
+
+ imageWidth = (int)BinaryPrimitives.ReadUInt32BigEndian(data.Slice(offset));
+ offset += 4;
+
</file context>
| // 10 + 8 bits = medium (0-255) | ||
| // 11 + 16 bits = long (0-65535) | ||
|
|
||
| if (length <= 15) |
There was a problem hiding this comment.
P2: WriteLength silently truncates code-block data lengths larger than 65535 bytes to 16 bits, which will corrupt packet parsing for large code-blocks. Add a guard (or extend the length coding) to handle larger lengths explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs, line 512:
<comment>WriteLength silently truncates code-block data lengths larger than 65535 bytes to 16 bits, which will corrupt packet parsing for large code-blocks. Add a guard (or extend the length coding) to handle larger lengths explicitly.</comment>
<file context>
@@ -0,0 +1,543 @@
+ // 10 + 8 bits = medium (0-255)
+ // 11 + 16 bits = long (0-65535)
+
+ if (length <= 15)
+ {
+ // Short length: 0 + 4 bits (handles 0-15)
</file context>
| CodeBlockWidth = j2kOptions.CodeBlockSize, | ||
| CodeBlockHeight = j2kOptions.CodeBlockSize, | ||
| NumberOfLayers = j2kOptions.QualityLayers, | ||
| CompressionRatio = j2kOptions.CompressionRatio |
There was a problem hiding this comment.
P2: CompressionRatio from Jpeg2000CodecOptions is ignored by the encoder, so lossy output quality/size cannot be controlled as documented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cs, line 102:
<comment>CompressionRatio from Jpeg2000CodecOptions is ignored by the encoder, so lossy output quality/size cannot be controlled as documented.</comment>
<file context>
@@ -0,0 +1,185 @@
+ CodeBlockWidth = j2kOptions.CodeBlockSize,
+ CodeBlockHeight = j2kOptions.CodeBlockSize,
+ NumberOfLayers = j2kOptions.QualityLayers,
+ CompressionRatio = j2kOptions.CompressionRatio
+ };
+
</file context>
| var j2kOptions = options is Jpeg2000CodecOptions opt ? opt : Jpeg2000CodecOptions.Default; | ||
|
|
||
| int frameSize = info.FrameSize; | ||
| int frameCount = pixelData.Length / frameSize; |
There was a problem hiding this comment.
P2: Encode silently drops trailing bytes when pixelData.Length is not a multiple of frameSize. Add a validation check so invalid input fails fast instead of producing truncated frames.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/SharpDicom/Codecs/Jpeg2000/Jpeg2000LosslessCodec.cs, line 92:
<comment>Encode silently drops trailing bytes when pixelData.Length is not a multiple of frameSize. Add a validation check so invalid input fails fast instead of producing truncated frames.</comment>
<file context>
@@ -0,0 +1,188 @@
+ var j2kOptions = options is Jpeg2000CodecOptions opt ? opt : Jpeg2000CodecOptions.Default;
+
+ int frameSize = info.FrameSize;
+ int frameCount = pixelData.Length / frameSize;
+ var fragments = new List<ReadOnlyMemory<byte>>(frameCount);
+
</file context>
- JpegLosslessEncoder: Convert interleaved input (RGB RGB RGB...) to planar layout (RRR... GGG... BBB...) when IsPlanar is false - JpegLosslessDecoder: Convert planar output to interleaved when IsPlanar is false to match DICOM PlanarConfiguration metadata Addresses RecurseML review comments about incorrect multi-component image handling when PlanarConfiguration == 0 (interleaved).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+273
to
+280
| foreach (var contrib in contributions) | ||
| { | ||
| if (contrib.NumNewPasses > 0) | ||
| { | ||
| hasContributions = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Comment on lines
+336
to
+342
| foreach (var contrib in contributions) | ||
| { | ||
| if (contrib.NumNewPasses > 0 && !contrib.Data.IsEmpty) | ||
| { | ||
| packetData.AddRange(contrib.Data.ToArray()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Comment on lines
+427
to
+434
| if (marker == J2kMarkers.SOT) | ||
| { | ||
| // SOT segment contains tile index at bytes 0-1 (after length) | ||
| if (position + 4 <= data.Length) | ||
| { | ||
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | ||
| inTargetTile = (currentTile == tileIndex); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Suggested change
| if (marker == J2kMarkers.SOT) | |
| { | |
| // SOT segment contains tile index at bytes 0-1 (after length) | |
| if (position + 4 <= data.Length) | |
| { | |
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | |
| inTargetTile = (currentTile == tileIndex); | |
| } | |
| if (marker == J2kMarkers.SOT && position + 4 <= data.Length) | |
| { | |
| // SOT segment contains tile index at bytes 0-1 (after length) | |
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | |
| inTargetTile = (currentTile == tileIndex); |
Comment on lines
+110
to
+121
| int encodedValue; | ||
| if (value < 0) | ||
| { | ||
| // Negative value: add (2^magnitude - 1) to get the encoded value | ||
| encodedValue = value + (1 << magnitude) - 1; | ||
| } | ||
| else | ||
| { | ||
| encodedValue = value; | ||
| } | ||
|
|
||
| WriteBits(encodedValue, magnitude); |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Suggested change
| int encodedValue; | |
| if (value < 0) | |
| { | |
| // Negative value: add (2^magnitude - 1) to get the encoded value | |
| encodedValue = value + (1 << magnitude) - 1; | |
| } | |
| else | |
| { | |
| encodedValue = value; | |
| } | |
| WriteBits(encodedValue, magnitude); | |
| // Negative value: add (2^magnitude - 1) to get the encoded value | |
| WriteBits(value < 0 ? value + (1 << magnitude) - 1 : value, magnitude); |
Comment on lines
+130
to
+137
| if (_signState![i] != 0) | ||
| { | ||
| coefficients[i] = -magnitude; | ||
| } | ||
| else | ||
| { | ||
| coefficients[i] = magnitude; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Suggested change
| if (_signState![i] != 0) | |
| { | |
| coefficients[i] = -magnitude; | |
| } | |
| else | |
| { | |
| coefficients[i] = magnitude; | |
| } | |
| coefficients[i] = _signState![i] != 0 ? -magnitude : magnitude; |
There was a problem hiding this comment.
9 issues found across 48 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="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cs:522">
P1: Bug: Ternary expression returns the same value (0x00) for both `lossless` and lossy cases. According to ITU-T T.800 and the comments above, lossy encoding should use quantization style 0x01 (scalar derived) or 0x02 (scalar expounded), not 0x00.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/JpegBaselineEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/JpegBaselineEncoder.cs:611">
P2: Premature ZRL emission when counting trailing zeros. ZRL should only be emitted when there are 16+ zeros followed by a non-zero coefficient, not when all remaining AC coefficients are zero. The `while (zeroRun >= 16)` block in the else branch already handles ZRL correctly - remove this immediate emission.</violation>
</file>
<file name="src/SharpDicom/Codecs/JpegLossless/JpegLosslessEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/JpegLosslessEncoder.cs:68">
P2: The output buffer size estimate is too small for worst‑case lossless data. With byte‑stuffing, encoded data can exceed 4 bytes/sample and BitWriter will throw when the buffer is full. Consider increasing the estimate or switching to a growable buffer to avoid runtime failures on high‑entropy images.</violation>
</file>
<file name="src/SharpDicom/Codecs/JpegLossless/LosslessHuffman.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/LosslessHuffman.cs:316">
P2: TryParseDHT accepts symbol values outside the valid lossless JPEG category range (0–16), so malformed DHT segments can produce categories >16 and cause invalid bit reads during decoding. Validate the values while parsing and reject out-of-range categories.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketDecoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketDecoder.cs:71">
P3: The `firstInclusion` parameter description is inverted relative to how the method uses it (true means not yet included). This can cause callers to pass the wrong values. Update the comment to match the actual expectation.</violation>
<violation number="2" location="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketDecoder.cs:236">
P2: Avoid converting `accumulatedData[i]` to an array on every segment; this creates repeated full copies as packets accumulate. Defer the `ToArray()` conversion until after all packets are processed to reduce O(n^2) copying overhead.</violation>
</file>
<file name="src/SharpDicom/Codecs/JpegLossless/JpegLosslessCodec.cs">
<violation number="1" location="src/SharpDicom/Codecs/JpegLossless/JpegLosslessCodec.cs:88">
P2: Validate that SelectionValue is 1 before encoding; other selection values are not allowed for the JPEG Lossless SV1 transfer syntax.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cs:470">
P1: WriteNumPasses encodes pass counts with a simplified 1111+5-bit/11111111+7-bit scheme that does not match PacketDecoder.ReadNumPasses. For higher pass counts (e.g., suffix starting with 11), the decoder will treat the suffix as extended prefixes and consume extra bits, corrupting packet headers. Encode the pass count using the same variable-length prefix scheme as ReadNumPasses (ITU-T T.800 Table B.4) so encoder/decoder stay in sync.</violation>
</file>
<file name="src/SharpDicom/Codecs/Jpeg/BitReader.cs">
<violation number="1" location="src/SharpDicom/Codecs/Jpeg/BitReader.cs:137">
P2: PeekBits pads incorrectly when fewer bits remain: the right shift by (32 - count) extracts top bits instead of the remaining low bits, so peeking near end-of-stream can return 0/incorrect prefixes. Mask the left-shifted value instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
P1 bugs fixed: - J2kEncoder: Fix QCD segment - lossy encoding now uses quantization style 0x01 (scalar derived) instead of 0x00 - PacketEncoder: Rewrite WriteNumPasses to match ITU-T T.800 Table B.4 encoding scheme used by PacketDecoder.ReadNumPasses P2 issues fixed: - JpegBaselineEncoder: Remove premature ZRL emission - only emit ZRL when followed by a non-zero coefficient, not when all remaining are zero - JpegLosslessEncoder: Increase buffer estimate to 8 bytes/sample to account for worst-case byte stuffing - LosslessHuffman: Validate DHT symbol values are in range 0-16 for lossless JPEG categories - PacketDecoder: Defer ToArray() conversion until after all packets processed to avoid O(n^2) copying overhead - JpegLosslessCodec: Validate SelectionValue == 1 for SV1 transfer syntax - BitReader: Fix PeekBits padding - mask after left shift to get correct count bits when fewer bits available than requested P3 documentation: - PacketDecoder: Clarify firstInclusion parameter semantics - Add performance comments explaining LINQ avoidance in hot paths
Add comprehensive edge case tests for all three codec implementations: JPEG Lossless: - Extreme pixel values (0/255 for 8-bit, 0/65535 for 16-bit) - Odd dimensions (7x13), single pixel, single row/column - Larger image (256x256), uniform/all-zeros/all-max data - SelectionValue validation test JPEG Baseline: - Odd dimensions, non-multiple-of-8 width/height - Extreme values, uniform data - Single MCU (8x8), larger image (128x128) JPEG 2000: - Odd dimensions for both lossless and lossy - Extreme values, uniform data - Single code-block, larger image (128x128) These tests improve coverage of boundary conditions and edge cases that could expose buffer handling or padding issues.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+427
to
+436
| if (marker == J2kMarkers.SOT) | ||
| { | ||
| // SOT segment contains tile index at bytes 0-1 (after length) | ||
| if (position + 4 <= data.Length) | ||
| { | ||
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | ||
| inTargetTile = (currentTile == tileIndex); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
These 'if' statements can be combined.
Suggested change
| if (marker == J2kMarkers.SOT) | |
| { | |
| // SOT segment contains tile index at bytes 0-1 (after length) | |
| if (position + 4 <= data.Length) | |
| { | |
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | |
| inTargetTile = (currentTile == tileIndex); | |
| } | |
| } | |
| if (marker == J2kMarkers.SOT && position + 4 <= data.Length) | |
| { | |
| // SOT segment contains tile index at bytes 0-1 (after length) | |
| currentTile = BinaryPrimitives.ReadUInt16BigEndian(data.Slice(position)); | |
| inTargetTile = (currentTile == tileIndex); | |
| } |
Comment on lines
+164
to
+179
| if (fragment.Length < 4 || | ||
| fragment.Span[fragment.Length - 2] != 0xFF || | ||
| fragment.Span[fragment.Length - 1] != 0xD9) | ||
| { | ||
| // EOI might be at -3 if padded | ||
| if (fragment.Length < 5 || | ||
| fragment.Span[fragment.Length - 3] != 0xFF || | ||
| fragment.Span[fragment.Length - 2] != 0xD9) | ||
| { | ||
| issues.Add(new CodecDiagnostic( | ||
| i, fragment.Length - 2, | ||
| "Missing EOI marker", | ||
| "FFD9", | ||
| "not found")); | ||
| } | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
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
Add managed image codec implementations:
Stacked PRs
This is part of v2.0.0 split into reviewable chunks:
Test plan
Summary by cubic
Adds pure C# JPEG (baseline and lossless) and JPEG 2000 (lossless and lossy) codecs plus an AOT-safe registration path. This enables cross-platform, trim-friendly compression/decoding for key DICOM transfer syntaxes without native dependencies.
New Features
Migration
Written for commit 01c32e9. Summary will update on new commits.
High-level PR Summary
This PR implements pure C# image codec implementations for JPEG Baseline (Process 1), JPEG Lossless (Process 14 SV1), and JPEG 2000 (both lossless and lossy) with no native dependencies. The implementation includes DCT-based compression for JPEG Baseline using the Loeffler algorithm with AVX2 SIMD acceleration, DPCM predictive coding for JPEG Lossless with seven selection values, and wavelet-based compression for JPEG 2000 using reversible 5/3 and irreversible 9/7 lifting schemes. The codecs integrate with the existing
IPixelDataCodecinterface from Phase 9 and register through an AOT-compatibleCodecInitializersystem that avoids reflection. Supporting infrastructure includes bit-level I/O primitives (BitReader/BitWriter) for Huffman coding, ITU-T T.81 standard Huffman and quantization tables, ITU-R BT.601 color space conversion, EBCOT bitplane coding with MQ arithmetic coder, and tier-2 packet organization. The implementation adds 3,404 tests (up from 2,070) across all codecs with comprehensive coverage of encode/decode roundtrips, quality metrics, multi-frame support, and validation.⏱️ Estimated Review Time: 1-3 hours
💡 Review Order Suggestion
.planning/STATE.md.planning/phases/12-pure-csharp-codecs/12-CONTEXT.md.planning/phases/12-pure-csharp-codecs/12-RESEARCH.mdsrc/SharpDicom/Data/TransferSyntax.cssrc/SharpDicom/Codecs/ColorConversion.cssrc/SharpDicom/Codecs/Jpeg/JpegMarkers.cssrc/SharpDicom/Codecs/Jpeg/JpegFrameInfo.cssrc/SharpDicom/Codecs/Jpeg/QuantizationTable.cssrc/SharpDicom/Codecs/Jpeg/HuffmanTable.cssrc/SharpDicom/Codecs/Jpeg/BitReader.cssrc/SharpDicom/Codecs/Jpeg/BitWriter.cssrc/SharpDicom/Codecs/Jpeg/DctTransform.cssrc/SharpDicom/Codecs/Jpeg/JpegCodecOptions.cssrc/SharpDicom/Codecs/Jpeg/JpegBaselineDecoder.cssrc/SharpDicom/Codecs/Jpeg/JpegBaselineEncoder.cssrc/SharpDicom/Codecs/Jpeg/JpegBaselineCodec.cssrc/SharpDicom/Codecs/JpegLossless/Predictor.cssrc/SharpDicom/Codecs/JpegLossless/LosslessHuffman.cssrc/SharpDicom/Codecs/JpegLossless/JpegLosslessDecoder.cssrc/SharpDicom/Codecs/JpegLossless/JpegLosslessEncoder.cssrc/SharpDicom/Codecs/JpegLossless/JpegLosslessCodec.cssrc/SharpDicom/Codecs/Jpeg2000/Wavelet/DwtTransform.cssrc/SharpDicom/Codecs/Jpeg2000/Wavelet/Dwt53.cssrc/SharpDicom/Codecs/Jpeg2000/Wavelet/Dwt97.cssrc/SharpDicom/Codecs/Jpeg2000/Tier1/MqCoder.cssrc/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotEncoder.cssrc/SharpDicom/Codecs/Jpeg2000/Tier1/EbcotDecoder.cssrc/SharpDicom/Codecs/Jpeg2000/Tier2/PacketEncoder.cssrc/SharpDicom/Codecs/Jpeg2000/Tier2/PacketDecoder.cssrc/SharpDicom/Codecs/Jpeg2000/J2kCodestream.cssrc/SharpDicom/Codecs/Jpeg2000/J2kEncoder.cssrc/SharpDicom/Codecs/Jpeg2000/J2kDecoder.cssrc/SharpDicom/Codecs/Jpeg2000/Jpeg2000CodecOptions.cssrc/SharpDicom/Codecs/Jpeg2000/Jpeg2000LosslessCodec.cssrc/SharpDicom/Codecs/Jpeg2000/Jpeg2000LossyCodec.cssrc/SharpDicom/Internal/ArrayBufferWriterPolyfill.cssrc/SharpDicom/Codecs/CodecInitializer.cstests/SharpDicom.Tests/Codecs/JpegBaselineCodecTests.cstests/SharpDicom.Tests/Codecs/JpegLosslessCodecTests.cstests/SharpDicom.Tests/Codecs/Jpeg2000/Jpeg2000Tests.cstests/SharpDicom.Tests/Codecs/Jpeg2000/Jpeg2000CodecTests.cstests/SharpDicom.Tests/Codecs/CodecRegistryIntegrationTests.cs.planning/phases/12-pure-csharp-codecs/12-01-SUMMARY.md.planning/phases/12-pure-csharp-codecs/12-02-SUMMARY.md.planning/phases/12-pure-csharp-codecs/12-03-SUMMARY.md.planning/phases/12-pure-csharp-codecs/12-04-SUMMARY.md.planning/phases/12-pure-csharp-codecs/12-05-SUMMARY.md.planning/phases/12-pure-csharp-codecs/12-06-SUMMARY.md.planning/phases/12-pure-csharp-codecs/12-07-SUMMARY.md