Repository navigation
Fix/cstore implicit vr little endian - #60
Open
luis-godinho wants to merge 2 commits into
Open
luis-godinho wants to merge 2 commits into
luis-godinho wants to merge 2 commits into
Conversation
normalizeToPs310 created DicomOutputStream with the negotiated transfer
syntax, but writeFileMetaInformation requires Explicit VR Little Endian
(PS3.10). With Implicit VR LE this threw IllegalStateException ('Sta6')
and aborted the association.
Always create the stream in Explicit VR LE; DicomOutputStream.writeDataset
writes the FMI in Explicit VR LE then switches to the transfer syntax
declared in the FMI before writing the dataset, so the dataset keeps its
original transfer syntax.
Adds a test covering Implicit VR LE C-STORE (regression for issue #56).
…ssue #56) DimseCStoreSCPIntegrationTest starts an embedded DimseCStoreServer (SCP) and connects as a DICOM SCU over TCP, sending a C-STORE-RQ with Implicit VR Little Endian transfer syntax. Verifies the association completes and the server returns C-STORE-RSP with status Success. Reproduces the exact scenario from issue #56: a SCU sending an object in Implicit VR Little Endian (1.2.840.10008.1.2) via DIMSE C-STORE.
Enet4
reviewed
Oct 23, 2026
Enet4
left a comment
Enet4
left a comment
There was a problem hiding this comment.
I could not do a complete review, but I found at least one thing worth improving.
Comment on lines
324
to
+328
| try (var fos = new java.io.FileOutputStream(tmpFile.toFile()); | ||
| DicomOutputStream dos = new DicomOutputStream(fos, transferSyntaxUid)) { | ||
| // File Meta Information must always be written in Explicit VR Little Endian | ||
| // (PS3.10). DicomOutputStream.writeDataset(fmi, attrs) writes the FMI in Explicit VR LE, | ||
| // then switches to the transfer syntax declared in the FMI before writing the dataset. | ||
| DicomOutputStream dos = new DicomOutputStream(fos, UID.ExplicitVRLittleEndian)) { |
There was a problem hiding this comment.
Since we have a (temporary) File at this level, the use of the constructor overload receiving a File is preferred, which makes less room for misuse. Note how this also provides buffered writing.
try (var dos = new DicomOuputStream(tmpFile.toFile())) {In any case I admit that a DicomOutputStream(OutputStream) overload would have been useful here. The sequence of constructing a DicomOutputStream with any TS UID other than Explicit VR, and then calling writeDataset with a non-null file meta information group will always fail. I sent this suggestion upstream to the dcm4che project.
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.
Description
CStoreService.normalizeToPs310created theDicomOutputStreamwith the negotiated transfer syntax. But PS3.10 requires File Meta Information (FMI) to always be written in Explicit VR Little Endian. With Implicit VR LE,writeFileMetaInformation()threwIllegalStateExceptionbefore the dataset was written.This PR aims to address issue #56
Fix
Always create the stream in Explicit VR LE.
DicomOutputStream.writeDataset(fmi, attrs)writes the FMI correctly, then switches to the transfer syntax declared in the FMI before writing the dataset — so the image data keeps its original transfer syntax.Tests added
CStoreServiceTest.returnsSuccessForImplicitVRLittleEndian— unit test exercising the fix path.DimseCStoreSCPIntegrationTest— end-to-end network test: embedded SCP server + real SCU client over TCP sendingC-STORE-RQwith Implicit VR LE; verifiesC-STORE-RSPreturnsSuccess.