search

Enet4 / dicom-rs

Rust implementation of the DICOM standard
https://dicom-rs.github.io
Apache License 2.0
403 stars 75 forks source link

findscu not working when dataset is included in C-FIND-RSP #522

Closed naterichman closed 5 days ago

naterichman commented 2 weeks ago

I've been dipping my toes into learning dimse by trying to write my own custom connector to a real pacs system, mostly using the code from findscu as a template. I found that my code wasn't working and I tried dicom-findscu itself and that also had strange behavior, whereas DCMTKs findscu returned immediately.

(the pacs is not my own system so I'm redacting everything just to be safe)

$ dicom-findscu --calling-ae-title <calling-ae-title> --called-ae-title <called-ae-title> -S -q AccessionNumber="<redacted>" -q SeriesInstanceUID <ip>:<port> -v
2024-06-14T23:12:43.614726Z  INFO dicom_findscu: Establishing association with '<ip>:<port>'...
2024-06-14T23:12:43.685326Z  INFO dicom_findscu: Association established
2024-06-14T23:12:43.685372Z DEBUG dicom_findscu: Transfer Syntax: Explicit VR Little Endian
2024-06-14T23:12:43.685436Z DEBUG dicom_findscu: Sending query (128 B)...
2024-06-14T23:12:43.685476Z DEBUG dicom_findscu: Awaiting response...
Match #0 Response command:
(0000,0000) CommandGroupLength           UL (1,  4 bytes): 76
(0000,0002) AffectedSOPClassUID          UI (1, 28 bytes): "1.2.840.10008.5.1.4.1.2.1.1"
(0000,0100) CommandField                 US (1,  2 bytes): 32800
(0000,0120) MessageIDBeingRespondedTo    US (1,  2 bytes): 1
(0000,0800) CommandDataSetType           US (1,  2 bytes): 258
(0000,0900) Status                       US (1,  2 bytes): 65280
2024-06-14T23:12:43.812867Z DEBUG dicom_findscu: Operation pending: ff00
------------------------ Match #0 ------------------------
(0000,0000) CommandGroupLength           UN (1, 76 bytes): [00, 00, 02, 00, 1C, 00, 00, ...
# Just hangs here indefinitely

I ended up finding during debugging that the dataset was included in the same PDU as another PDataValue (I think I'm using these terms correctly), so I changed the code as follows:

diff --git a/findscu/src/main.rs b/findscu/src/main.rs
index 6df7325..0601968 100644
--- a/findscu/src/main.rs
+++ b/findscu/src/main.rs
@@ -300,14 +300,21 @@ fn run() -> Result<(), Error> {
                     }

                     // fetch DICOM data
-
-                    let dcm = {
+                    let dcm = if data.len() > 1 && data[1].value_type == PDataValueType::Data {
+                            InMemDicomObject::read_dataset_with_ts(
+                                &data[1].data[..],
+                                ts,
+                            )
+                            .whatever_context("Couldn't read dicom")?
+
+                    } else {
                         let mut rsp = scu.receive_pdata();
                         let mut response_data = Vec::new();
                         rsp.read_to_end(&mut response_data)
                             .whatever_context("Failed to read response data")?;

-                        InMemDicomObject::read_dataset_with_ts(&response_data[..], ts)
+                        InMemDicomObject::read_dataset_with_ts(
+                            &response_data[..], ts)
                             .whatever_context("Could not read response data set")?
                     };

And now the same dicom-findscu command returns as such:

2024-06-14T23:42:56.786730Z  INFO dicom_findscu: Establishing association with '<ip>:<port>'...
2024-06-14T23:42:56.854351Z  INFO dicom_findscu: Association established
2024-06-14T23:42:56.854476Z DEBUG dicom_findscu: Transfer Syntax: Explicit VR Little Endian
2024-06-14T23:42:56.854659Z DEBUG dicom_findscu: Sending query (126 B)...
2024-06-14T23:42:56.854730Z DEBUG dicom_findscu: Awaiting response...
Match #0 Response command:
(0000,0000) CommandGroupLength           UL (1,  4 bytes): 76
(0000,0002) AffectedSOPClassUID          UI (1, 28 bytes): "1.2.840.10008.5.1.4.1.2.2.1"
(0000,0100) CommandField                 US (1,  2 bytes): 32800
(0000,0120) MessageIDBeingRespondedTo    US (1,  2 bytes): 1
(0000,0800) CommandDataSetType           US (1,  2 bytes): 258
(0000,0900) Status                       US (1,  2 bytes): 65280
2024-06-14T23:42:56.979638Z DEBUG dicom_findscu: Operation pending: ff00
------------------------ Match #0 ------------------------
(0008,0050) AccessionNumber              SH (1,  8 bytes): "<redacted>"
(0008,0052) QueryRetrieveLevel           CS (1,  6 bytes): "STUDY"
(0008,0054) RetrieveAETitle              AE (1, 14 bytes): "<called>"
(0008,0056) InstanceAvailability         CS (1,  6 bytes): "ONLINE"
Match #1 Response command:
(0000,0000) CommandGroupLength           UL (1,  4 bytes): 76
(0000,0002) AffectedSOPClassUID          UI (1, 28 bytes): "1.2.840.10008.5.1.4.1.2.2.1"
(0000,0100) CommandField                 US (1,  2 bytes): 32800
(0000,0120) MessageIDBeingRespondedTo    US (1,  2 bytes): 1
(0000,0800) CommandDataSetType           US (1,  2 bytes): 257
(0000,0900) Status                       US (1,  2 bytes): 0
2024-06-14T23:42:56.981138Z DEBUG dicom_findscu: Matching is complete

This certainly looks better and noticeably the command doesn't hang indefinitely. I think it was hanging because the subsequent call to scu.receive_pdata() when the status was pending was consuming the next command that actually returned the status of 0.

There are still a couple issues with the output from this modified code:

  1. The SeriesInstanceUID is not present even though I requested it in the query (I also tried -q SeriesInstanceUID= and a couple other variants) even though it is returned when I use DCMTKs findscu (see below),
  2. The way its output makes it looks like there are multiple matches since it reports Match #1 command
  3. It seems like there could be other PDataValues in the response PDU which aren't being checked

Anyway, just wanted to report this, I'm not sure the best way to implement that change, I can't quite understand from the standard whether there are limits on the number of PDVs within a PDU or if there might be a different number for different statuses/mismatch of commands/datasets, etc. Seems like there can be a ton of combinations...

findscu for same dataset:

$ findscu  \
    --patient    \
    -aec <calling>\
    -aet <title> \
    -k 0008,0052="STUDY"\
    -k 0008,0050="<redacted>" \
    -k 0020,000D -k  -od /tmp/dicom   <ip> <port>
I: ---------------------------
I: Find Response: 1 (Pending)
I:
I: # Dicom-Data-Set
I: # Used TransferSyntax: Little Endian Explicit
I: (0008,0050) SH [<redacted>]                               #   8, 1 AccessionNumber
I: (0008,0052) CS [STUDY ]                                 #   6, 1 QueryRetrieveLevel
I: (0008,0054) AE [<title> ]                         #  14, 1 RetrieveAETitle
I: (0008,0056) CS [ONLINE]                                 #   6, 1 InstanceAvailability
I: (0020,000d) UI [<redacted>] #  44, 1 StudyInstanceUID
Enet4 commented 2 weeks ago

Thank you very much for reporting! It's hard to suggest the right approach here, but the the first erroneous output suggests that the Find SCU is reading a command data set with the wrong transfer syntax. I assumed that a command part of the response would always be in implicit VR LE, but maybe more erroneous assumptions are at play.

I'm not sure the best way to implement that change, I can't quite understand from the standard whether there are limits on the number of PDVs within a PDU or if there might be a different number for different statuses/mismatch of commands/datasets, etc. Seems like there can be a ton of combinations...

It's true that a PDU can contain multiple P-Data values. Though I would expect responses to be in command data set / (main) data set pairs, I'd better consult some references before saying anything else.

naterichman commented 2 weeks ago

Yea I've been trying to read up as much as I can from the standard and consult the pydicom codebase (since I'm familiar), The approach there seems to be similar to the PDataReader in that it stores a buffer and extends the buffer with each PDV, then returns a tuple of (<command set>, <data set>) which I think might be a decent approach to modifying PDataReader.

Also you can disregard my # 1 above

The SeriesInstanceUID is not present even though I requested it in the query (I also tried -q SeriesInstanceUID= and a couple other variants) even though it is returned when I use DCMTKs findscu (see below),

This was user error passing in the wrong QueryRetrieveLevel