DICOM Enhancement#4992
Closed
tmart234 wants to merge 40 commits into
Closed
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Spec compliance (PS3.5 Annex A): - Video codecs: MPEG-2 MPML/MPHL, MPEG-4 H.264 (5 variants), HEVC H.265 - JPEG XL trio (.110/.111/.112) - JPEG 2000 Part 2 multi-component (.92/.93) - JPEG Extended (.51), Deflated Image Frame (.5.1) - Encapsulated Uncompressed Explicit VR LE (.1.2.1.98) - Verified HTJP2K .202 present User Identity Type 5 (JWT) already in USER_IDENTITY_TYPES dict; ByteEnumField is not range-capped; secondary_field conditional correctly gates on type 2 only (PS3.7 D.3.3.7). Document PresentationDataValueItem endianness boundary: PDV header is Big Endian (PS3.8), DIMSE payload is Little Endian (PS3.7 §9.3); verified all DIMSE field classes use "<" struct format. https://claude.ai/code/session_01V6q2gwYcdiBS3aerxqj53m
2a. DICOMSOPClassCommonExtendedNegotiation (0x57): add sub_item_version
property that reads byte 2 from the DICOMVariableItem underlayer,
since PS3.7 D.3.3.6 uses that byte as version (not reserved).
Both 0x56 and 0x57 already had full packet classes and bindings.
2b. Range-based DIMSE status display: add DIMSE_STATUS_CODES dict,
dimse_status_repr() helper with fallback to 0xA/0xB/0xC range
descriptions, and DICOMStatusField subclass. All DIMSE response
status fields now use DICOMStatusField for human-readable output.
Added missing codes: 0xA700, 0xA701, 0xA801, 0xA900.
2c. UID padding verified correct: association items use StrLenField
(raw bytes, length from header), DIMSE commands use DICOMUIDField
(null-padded to even length). No changes needed.
2d. Unknown sub-item graceful skip: DICOMGenericItem already consumes
exactly underlayer.length bytes; added post_dissect log.info for
visibility when unknown types are encountered (PS3.8 Annex D.2).
Also shortened PresentationDataValueItem endianness comment to
one line per review feedback.
https://claude.ai/code/session_01V6q2gwYcdiBS3aerxqj53m
3a. Add 14 fragmentable / referenced / SMPTE Transfer Syntax UIDs:
JPIP Referenced (.94/.95/.204/.205), fragmentable video variants
(.100.1–.106.1), and SMPTE ST 2110 (.7.1–.7.3).
3b. Add SOP_CLASS_NAMES lookup dict with ~45 commonly negotiated
SOP Classes for display during association negotiation traces.
3c. Retired tag (0000,0001) tolerance: DICOMElementField.getfield
now skips elements whose tag doesn't match the expected one,
logging them at INFO level. Prevents misparsing when legacy
CommandLengthToEnd or other unexpected elements appear.
3d. A-ASSOCIATE-AC: verified called/calling_ae_title fields already
parse the reserved bytes 11-74 correctly; added comment.
https://claude.ai/code/session_01V6q2gwYcdiBS3aerxqj53m
- getfield: stop skipping when encountering a tag that sorts *after* the expected one, preventing cascading misparsing when a tag is simply absent from the stream. - Remove non-existent SOP UID .130 (duplicate of .128.1) and retired Storage Commitment Pull Model (.1.20.2). - Wire SOP_CLASS_NAMES into DICOMAbstractSyntax.mysummary so the lookup dict is actually used for display. https://claude.ai/code/session_01V6q2gwYcdiBS3aerxqj53m
UID 1.2.840.10008.1.2.5.1 does not exist in PS3.5. The only deflated Transfer Syntax is Deflated Explicit VR LE at .1.2.1.99, already present. https://claude.ai/code/session_01V6q2gwYcdiBS3aerxqj53m
- Add MR_IMAGE_STORAGE_SOP_CLASS_UID and SECONDARY_CAPTURE_SOP_CLASS_UID as top-level importable constants (previously only in SOP_CLASS_NAMES). - Replace one stray f-string with %-formatting to match the file's formatting convention. - Shorten C_MOVE_RQ docstring to satisfy flake8 max-line-length=88 (the scapy project lint config). - Add trailing newline. https://claude.ai/code/session_01PeJVWbwEKVpoxMySmdAcBy
Make the file structurally match secdev/scapy/master conventions so it reads like the rest of the contrib tree: - Shorten the module docstring to the upstream pattern (title + 1 line of scope + reference URL), dropping the ASCII stack diagram. - Flatten __all__ to one identifier per line; remove the group banner comments (# Constants, # PDU Classes, etc.) that upstream does not use. - Drop the per-constant trailing comments (e.g. "# retired") and the "# -- Core --", "# -- JPEG --" subgroup labels from the Transfer Syntax UID block, matching upstream's bare KEY = "value" style. - Reformat compact multi-key-per-line dicts (PDU_TYPES, ITEM_TYPES, DIMSE_COMMAND_FIELDS, DATA_SET_TYPES, PRIORITY_VALUES, USER_IDENTITY_TYPES, A_ASSOCIATE_RJ.RESULT_CODES / SOURCE_CODES / REASON_*, A_ABORT.SOURCE_CODES / REASON_PROVIDER) to one entry per line with blank lines around class-level dicts, as upstream does. - Remove the "# Layer Bindings for Variable Items" and "# TCP Port and PDU Type Bindings" banner comments above the bind_layers blocks. - Drop the "# Display-only lookup..." and "# Combined lookup dict..." explanatory comments above SOP_CLASS_NAMES and DIMSE_STATUS_CODES; the dict names are self-describing. - Add a 1-line docstring to every Packet/DIMSEPacket subclass that lacked one (DICOMApplicationContext, A_ASSOCIATE_RQ, the DIMSE command/response classes, etc.), matching the upstream pattern. - Convert the inline "# Bytes 11-42 / 43-74 are reserved..." note on A_ASSOCIATE_AC and "# PDV header is BE..." note on PresentationDataValueItem into proper class docstrings. - Replace the Sphinx-style multi-line :param: / :returns: docstrings on DICOMSocket.associate / c_echo / c_store with single-line summaries, matching upstream's DICOMSocket conventions, and add the missing one-liner upstream uses on DICOMSocket.sr1. Behaviour, public API and DICOM wire format are unchanged. flake8 passes with scapy's tox.ini config (max-line-length=88) and the full test/contrib/dicom.uts suite (89 tests) passes. https://claude.ai/code/session_01PeJVWbwEKVpoxMySmdAcBy
Replace the local dicom.py with the upstream secdev/scapy version, then cherry-pick only the additions our tests/features depend on: - Transfer Syntax UID constants (IMPLICIT_VR_*, EXPLICIT_VR_*, JPEG_*, RLE_*, HTJP2K_*, etc.) - MR_IMAGE_STORAGE_SOP_CLASS_UID, SECONDARY_CAPTURE_SOP_CLASS_UID - SOP_CLASS_NAMES lookup dict - DIMSE Status Codes (STATUS_* constants, DIMSE_STATUS_CODES dict, dimse_status_repr helper) - DICOMATField for N-GET-RQ attribute_identifier_list - C_CANCEL_RQ command - N-DIMSE classes (N_EVENT_REPORT/_GET/_SET/_ACTION/_CREATE/_DELETE RQ+RSP) Targeted modifications to upstream: - DICOMUIDField.i2repr uses errors="replace" for malformed bytes - C_MOVE_RQ reorders move_destination before priority (tag-order compliance per PS3.7 Section 6.3.1) - C_STORE_RQ adds optional Move Originator fields (0000,1030/1031) - DICOMMaximumLength.mysummary shows "unlimited" for max_pdu_length=0 Diff against upstream shrinks from 1877 to ~548 lines (445 added, 4 removed) and is now nearly purely additive. All 90 tests in test/contrib/dicom.uts pass. https://claude.ai/code/session_013vPex1WoZzEyp1yXCK31Wo
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4992 +/- ##
========================================
Coverage 80.28% 80.29%
========================================
Files 383 383
Lines 94739 94915 +176
========================================
+ Hits 76062 76210 +148
- Misses 18677 18705 +28
🚀 New features to boost your workflow:
|
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
3 participants
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.
Hi
I did the original Scapy PR for dicom 3 month ago...
This PR adds the following:
This enhancement should cover most of what this layer needs from NEMA PS3.7 and 3.8 spec