nvImageCodec JPEGLossless 9-15 bit Guard#582
nvImageCodec JPEGLossless 9-15 bit Guard#582bluna301 wants to merge 1 commit intoProject-MONAI:mainfrom
Conversation
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
WalkthroughThis PR adds handling for a limitation in the nvimgcodec decoder: JPEG Lossless (SOF3) streams with BitsStored between 9–15 are unsupported and require fallback to other decoders. The operator now detects this condition, issues a single warning, suppresses pydicom's repeated error logs, and raises ChangesJPEG Lossless BitsStored Fallback Handling
Type Annotation Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/deploy/operators/monet_bundle_inference_operator.py (1)
1-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCritical inconsistency: File contents don't match PR objectives.
The PR is titled "nvImageCodecJPEGLossless 9-15 bit Guard" and describes adding JPEG Lossless SOF3 decoder fallback logic, warning messages, pydicom error suppression, and
NotImplementedErrorhandling. However, this file (monet_bundle_inference_operator.py) contains only:
- Import statement modifications
- A type-checking suppression comment on a
ConcatItemsdcallThere is no JPEG, DICOM, nvimgcodec, image codec, or decoder-related logic in this file. This suggests either:
- The wrong file was included in this review
- Critical files implementing the PR objectives are missing from the review
- The PR metadata/description is incorrect
Please verify that all files related to the JPEG Lossless guard are included in this pull request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/deploy/operators/monet_bundle_inference_operator.py` around lines 1 - 99, The PR description and title reference nvimgcodec/jpeg lossless fallback logic but this file (MONetBundleInferenceOperator, methods _init_config, _set_model_network, predict, and attribute _nnunet_predictor) contains unrelated MONet/nnUNet inference code; ensure the intended JPEG/DICOM guard code files are actually added to the branch or update the PR to target the correct files: either add the missing decoder/fallback implementation files (the nvimgcodec/jpeglossless and pydicom suppression modules) and include tests, or correct the PR metadata to reference the MONet operator changes only; verify branch commits include the new decoder logic and that no wrong files (like monet_bundle_inference_operator.py) were accidentally staged for this PR.
🧹 Nitpick comments (2)
tests/unit/test_decoder_nvimgcodec.py (1)
166-177: 💤 Low valueConsider logging exceptions instead of silently passing.
The bare
except Exception: passsuppresses all errors during the pre-check. While the intent is to fall through to the main test logic when metadata can't be read, silently swallowing exceptions makes debugging harder if files unexpectedly fail. At minimum, log at DEBUG level.🔧 Proposed fix
try: _ds_meta = dcmread(path, stop_before_pixels=True) _ts = _ds_meta.file_meta.TransferSyntaxUID _bits_stored = getattr(_ds_meta, "BitsStored", 16) if _ts in _JPEG_LOSSLESS_SYNTAXES and 9 <= _bits_stored <= 15: pytest.skip( f"Skipping {Path(path).name}: JPEG Lossless with BitsStored={_bits_stored} (9-15) " "is not supported by nvimgcodec (intentional fallback)" ) - except Exception: - pass + except Exception as exc: + _logger.debug(f"Could not pre-check {Path(path).name} for skip condition: {exc}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_decoder_nvimgcodec.py` around lines 166 - 177, Replace the bare except in the pre-check block with a logged debug-level exception so failures to read metadata aren't silently swallowed; specifically, catch Exception around the dcmread(...) call and use a logger (e.g. module logger) to log the exception details (including path and the exception) at DEBUG before falling through to the main test, keeping the existing logic that checks _ds_meta, _ts, _bits_stored and calls pytest.skip when appropriate.monai/deploy/operators/monet_bundle_inference_operator.py (1)
92-92: ⚡ Quick winTighten the
dataparameter type to improve type safety for this operation.The
# type: ignore[arg-type]suppression exists because thedataparameter is typed asAny(line 81), makingmultimodal_dataan implicitly typedDict[str, Any].ConcatItemsdexpects a dict with concrete tensor types, which the type checker cannot verify.Consider stricter typing for the
dataparameter (e.g.,Tensor | MetaTensor) or an explicit type annotation onmultimodal_datato narrow types properly. If the suppression remains necessary due to MONAI's type stubs, the current approach is reasonable since the code functions correctly at runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/deploy/operators/monet_bundle_inference_operator.py` at line 92, Tighten the typing around the ConcatItemsd call by changing the loose data parameter (currently annotated as Any) to a narrower type—e.g., annotate the function parameter named data as Tensor | MetaTensor or annotate multimodal_data as Dict[str, Tensor | MetaTensor]—so ConcatItemsd(keys=list(multimodal_data.keys()), name="image")(multimodal_data)["image"] no longer needs the # type: ignore[arg-type] suppression; if MONAI stubs prevent that, explicitly annotate multimodal_data with the concrete dict type (Dict[str, Tensor | MetaTensor]) to satisfy the type checker while leaving runtime logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@monai/deploy/operators/monet_bundle_inference_operator.py`:
- Around line 1-99: The PR description and title reference nvimgcodec/jpeg
lossless fallback logic but this file (MONetBundleInferenceOperator, methods
_init_config, _set_model_network, predict, and attribute _nnunet_predictor)
contains unrelated MONet/nnUNet inference code; ensure the intended JPEG/DICOM
guard code files are actually added to the branch or update the PR to target the
correct files: either add the missing decoder/fallback implementation files (the
nvimgcodec/jpeglossless and pydicom suppression modules) and include tests, or
correct the PR metadata to reference the MONet operator changes only; verify
branch commits include the new decoder logic and that no wrong files (like
monet_bundle_inference_operator.py) were accidentally staged for this PR.
---
Nitpick comments:
In `@monai/deploy/operators/monet_bundle_inference_operator.py`:
- Line 92: Tighten the typing around the ConcatItemsd call by changing the loose
data parameter (currently annotated as Any) to a narrower type—e.g., annotate
the function parameter named data as Tensor | MetaTensor or annotate
multimodal_data as Dict[str, Tensor | MetaTensor]—so
ConcatItemsd(keys=list(multimodal_data.keys()),
name="image")(multimodal_data)["image"] no longer needs the # type:
ignore[arg-type] suppression; if MONAI stubs prevent that, explicitly annotate
multimodal_data with the concrete dict type (Dict[str, Tensor | MetaTensor]) to
satisfy the type checker while leaving runtime logic unchanged.
In `@tests/unit/test_decoder_nvimgcodec.py`:
- Around line 166-177: Replace the bare except in the pre-check block with a
logged debug-level exception so failures to read metadata aren't silently
swallowed; specifically, catch Exception around the dcmread(...) call and use a
logger (e.g. module logger) to log the exception details (including path and the
exception) at DEBUG before falling through to the main test, keeping the
existing logic that checks _ds_meta, _ts, _bits_stored and calls pytest.skip
when appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9be16e1-6a4a-4af8-8eca-ac30035dc812
📒 Files selected for processing (3)
monai/deploy/operators/decoder_nvimgcodec.pymonai/deploy/operators/monet_bundle_inference_operator.pytests/unit/test_decoder_nvimgcodec.py
bluna301
changed the title
1 participant
Tighten
JPEGLosslessfallback guard to9 <= BitsStored <= 15Background
When testing MAPs built with
monai-deploy-app-sdk==3.5.0, two cases (one CT, one MR) were identified that produced zero-segmentations after model processing. Both cases are from Philips scanners withBitsStored = 12and Transfer SyntaxJPEGLosslessSV1(1.2.840.10008.1.2.4.70). Further investigation of these cases showed thatdecoder_nvimgcodec.pyproduced a zero-filled output image buffer - essentially a silent decoding error which propagates and manifests as an empty segmentation.When testing older MAPs built with
monai-deploy-app-sdk==3.1.0on these cases, non-zero segmentations are produced. Non-nvImageCodecdecoders (e.g.GDCM) produce a non-zero-filled output image buffer.See below existing
decoder_nvimgcodec.pyvs.GDCMbehavior for one of the files in the CT case - the decoder pipeline log shows thatnvimgcodecprocesses the file and produces a zero output buffer, instead of falling back to subsequent decoders likeGDCM:There is a file that meets this same criteria (
BitsStored = 12, Transfer SyntaxJPEGLosslessSV1[1.2.840.10008.1.2.4.70]) in thedecoder_nvimgcodecunit test suite -bad_sequence.dcm.decoder_nvimgcodec.pyproduces a non-zero image buffer after decoding.Per-File Analysis
All three files share
BitsStored=12in their DICOM tags, yet produce different outcomes. The difference is entirely in the JPEG byte stream — specifically the SOF3Pbyte and segment ordering — neither of which is visible to the Python decoder viarunner.bits_stored.bad_sequence.dcmMR.dcmCT.dcmbad_sequence.dcm(Siemens — works): AlthoughBitsStored=12in the DICOM tag, Siemens writesP=16in the SOF3 header and uses standard segment ordering. Fromnvjpeg's perspective this is a valid 16-bit lossless stream; it decodes correctly and produces a non-zero image. Note that writingP=16whenBitsStored=12is itself non-conformant per DICOM (SOF3Pshould equalBitsStored, but it happens to match whatnvjpegexpects.)MR.dcm(Philips — Bug 1): Philips writesP=12in the SOF3 header, correctly matchingBitsStored=12per the DICOM standard. ThenvImageCodecJPEG parser mapsP=12toUINT16(any precision 9–16 maps to UINT16), socanDecodeaccepts the stream. However,nvjpegDecodeBatchedinternally assumes all UINT16 lossless streams areP=16. WithP=12the Huffman-coded bitstream uses 12-bit samples; the GPU kernel misreads the entropy-coded data and silently writes zeros.CT.dcm(Philips — Bug 2): AlthoughBitsStored=12in the DICOM tag, Philips writesP=16in the SOF3 header but inserts a COM (comment) marker and a DHT (Huffman table) segment before SOF3 — non-standard ordering that is technically valid per ITU-T T.81 but not handled bynvjpeg. ThenvImageCodecparser scans past COM and DHT to find SOF3, correctly readsP=16, andcanDecodeaccepts the stream. However,nvjpegDecodeBatchedhas its own internal parser that expects SOF3 before DHT. Encountering DHT first, it fails to build the decode tables correctly and silently writes zeros.Critically,
nvjpegDecodeBatchedSupportedreturns "supported" for both Bug 1 and Bug 2 streams — this is the core defect that makes both failures silent.Root Cause
The
nvimgcodeclossless JPEG decoder (NVJPEG_BACKEND_LOSSLESS_JPEG) silently produces zero-filled output buffers for certain JPEG Lossless (SOF3) DICOM frames. Investigation of the nvImageCodec source identified two independent root causes, both in the closed-sourcenvjpegDecodeBatchedbinary:Sub-16-bit SOF3 precision (Bug 1): When the SOF3
Pbyte encodes a precision of 9–15 (e.g. Philips scanners writingP=12for a 12-bit series),nvjpegDecodeBatchedsilently zero-fills the output buffer.DHT-before-SOF3 segment ordering (Bug 2): When a Huffman table (DHT) segment appears before the SOF3 marker — non-standard but valid per ITU-T T.81, and emitted by some Philips scanners —
nvjpegDecodeBatchedagain silently zero-fills.nvjpegDecodeBatchedSupportedincorrectly returns "supported" in both cases, which is why the failures are silent.Both failing files have
BitsStored=12in the DICOM tag.Why BitsStored=8 does not need a guard
Source code analysis of
extensions/nvjpeg/lossless_decoder.cppandsrc/parsers/jpeg.cppshows that forP=8, the JPEG parser maps precision toNVIMGCODEC_SAMPLE_DATA_TYPE_UINT8. The lossless decoder'scanDecodefunction then explicitly rejects UINT8 output (it only supports UINT16), returningSAMPLE_TYPE_UNSUPPORTEDbeforenvjpegDecodeBatchedis ever called. The framework falls through cleanly tolibjpeg-turbo, which decodes correctly. No silent zeros — no guard needed.Changes
runner.bits_stored < 16→9 <= runner.bits_stored <= 15. This reflects the actual vulnerable range: only precision values that map to UINT16 (socanDecodeaccepts them) but are not 16-bit (sonvjpegDecodeBatchedzero-fills)._JPEG_LOSSLESS_SYNTAXES: Promoted from a per-call tuple to a module-level constant.9–15range and explain why BitsStored=8 is handled correctly without a guard.bad_sequence.dcmuntilnvImageCodecfix implementedUpstream fix
The correct long-term fix is in
extensions/nvjpeg/lossless_decoder.cppin the nvImageCodec repo:canDecodeshould reject streams whereplane_info[0].precisionis not 0 or 16, and pre-scan for DHT-before-SOF3 ordering before callingnvjpegJpegStreamParse. This Python-layer guard is a workaround until that upstream fix is available.