Add dicom visual navigation | ⚠️ Changes removed from main

[jcfr]

⚠️ Changes originally introduced through this pull-request have been removed from main by force pushing. Corresponding changes are superseded by #1165 ⚠️

Rational:

• Commit introducing the updates was lacking of a descriptive commit message
• Feature and follow-up fixes should be cleanly integrated using Squash & Merge

[jcfr]

@Punzo While reviewing the changes I ended up creating a pull request and marked it as draft.

[Punzo]

@Punzo While reviewing the changes I ended up creating a pull request and marked it as draft.

no problem!

btw: I am still actively working on it.

Here it is my current To Do list:

• add a storage listener class in CTK (ctkDICOMStorageListener).
• binaries in CTK with an example of the visual dicom browser running without Slicer (Folder selector for the database and ctkDICOMVisualBrowserWidget).
• PR ready for review

[jcfr]

Add dicomweb query and retrieve (We would need to implement a new C++ library).

cc: @tbirdso

[lassoan]

Ideally, DICOMweb query/retrieve would use a DICOM toolkit, such as DCMTK, but unfortunately it does not seem like DCMTK will ever have DICOMweb networking. We could look for other C++ implementations, or implement using CTK's qRestAPI, or worst case call a Python implementation (dicomweb-client) from C++.

[Punzo]

Ideally, DICOMweb query/retrieve would use a DICOM toolkit, such as DCMTK, but unfortunately it does not seem like DCMTK will ever have DICOMweb networking. We could look for other C++ implementations, or implement using CTK's qRestAPI, or worst case call a Python implementation (dicomweb-client) from C++.

for future reference:
https://github.com/MITK/MITK/tree/master/Modules/DICOMweb

[pieper]

FYI @Punzo I'm investigating DICOMweb loading too, so we can touch base to compare notes at some point. Still working on some experimental code.

[Punzo]

FYI @Punzo I'm investigating DICOMweb loading too, so we can touch base to compare notes at some point. Still working on some experimental code.

Ciao Steve, ok that would be great!

I did not have the time to investigate DICOMweb in C++ yet and I will focus next days on the first 4 points in my To Do list, but I would be glad to connect soon. Let's update soon.

[Punzo]

@Punzo While reviewing the changes I ended up creating a pull request and marked it as draft.

no problem!

btw: I am still actively working on it.

Here it is my current To Do list:

• add a storage listener class in CTK (ctkDICOMStorageListener).
• binaries in CTK with an example of the visual dicom browser running without Slicer (Folder selector for the database and ctkDICOMVisualBrowserWidget).
• PR ready for review

@jcfr @pieper @lassoan this PR is ready for a first review
pinging also @nolden @cpinter

PR content description

The operations are executed by a task pool manager in separate threads with a priority queue.
The current supported operations are:

• Filtering and navigation with thumbnails of local database and servers results
• Import from file system to local database
• Query/Retrieve from servers (DIMSE C-GET/C-MOVE )
• Storage listener
• Send (emits only a signal for the moment, requires external implementation)
• Remove (only from local database, not from server)
• Metadata exploration

UML diagram:

Testing in CTK Application:

I have added a ctkApplication called ctkDICOMVisualBrowser for testing (need to set cmake options examples and DICOM on).

Testing by running visual dicom browser from Slicer python console:

import os

server = ctk.ctkDICOMServer()
server.connectionName = "ConnectionName"
server.callingAETitle = "callingAETitle"
server.calledAETitle = "calledAETitle"
server.host = "host"
server.port = 11112
server.retrieveProtocol = ctk.ctkDICOMServer.CGET

DICOMDatabase = qt.QSettings().value(slicer.dicomDatabaseDirectorySettingsKey)
if not os.path.isdir(DICOMDatabase):
  os.makedirs(DICOMDatabase)  

browser = ctk.ctkDICOMVisualBrowserWidget()
browser.addServer(server)
browser.setDatabaseDirectory(DICOMDatabase)
browser.filteringPatientID = 1
browser.resize(slicer.util.mainWindow().size)
browser.show()
browser.onQueryPatient()

Testing taskPool tasks from the python console (e.g. query and retrieve)

server = ctk.ctkDICOMServer()
server.connectionName = "ConnectionName"
server.callingAETitle = "callingAETitle"
server.calledAETitle = "calledAETitle"
server.host = "host"
server.port = 11112
server.retrieveProtocol = ctk.ctkDICOMServer.CGET

taskPool = ctk.ctkDICOMTaskPool()
taskPool.setDicomDatabase(slicer.dicomDatabase)
taskPool.addServer(server)

nDays = 30
endDate = qt.QDate().currentDate()
startDate = endDate.addDays(-nDays);
parameters = {
  "ID": "CT",
  #"Name": "name",
  #"Study": "description",
  #"Series": "description",
  #"Modalities": ["CT", "MR"],
  #"StartDate": startDate.toString("yyyyMMdd"),
  #"EndDate": endDate.toString("yyyyMMdd")
}

taskPool.setFilters(parameters)
# Use one of the following methods: all these methods run background processes!!!!
#taskPool.queryPatients()
#taskPool.queryStudies('patientID')
#taskPool.querySeries('patientID', 'studyInstanceUID')
#taskPool.queryInstances('patientID', 'studyInstanceUID', 'seriesInstanceUID')
#taskPool.retrieveStudy('patientID', 'studyInstanceUID')
#taskPool.retrieveSeries('patientID','studyInstanceUID', 'seriesInstanceUID')
#taskPool.retrieveSOPInstance('patientID', 'studyInstanceUID', 'seriesInstanceUID', 'SOPInstanceUID')

Future ENH:

• Integrate in Slicer (probably as an additional experimental option in the DICOM module)
• Implementing send (i.e. adding ctkDICOMSendTask and ctkDICOMSend with underlining DIMSE DcmStorageSCU)
• add DICOMweb

To Do: cleaning design

At the moment the classes are too dependent on each other:

• the updates are done by sending signals with raw pointers ctkDICOMTaskResults (queued connections for slots). These raw pointers are created and owned by the Tasks and the data are needed both from the indexer and the UI.
• the role of deleting the tasks is on the UI.
• we need to wait the UI updates before deleting the tasks.

An improved design with a clean and simple memory managment should:

• destroying the task as soon as possible -> e.g. for retrieve, add the inserter operation directly in the task and delete the task as soon as the indexing is finished.
• UI updates should be done by sending simple strings (PatientID, studyInstanceUID, etc...) from the tasks to the TaskPool and from the TaskPool to the UI (direct connections for slots).
• the UI access the data for the update directly from the local dicom database using the information strings (PatientID, studyInstanceUID, etc...).
• task dependencies (e.g., retrieve with C-MOVE / C-GET with a proxy server) logic should not be hardcoded in the TaskPool. We need a class that collects the tasks and put them in a queue and prints to a file (JSON?). Then the TaskPool simply run the QRunnables. This would allow also resuming tasks if the UI crash or gets restarted etc....

[Punzo]

@Punzo Please rebase on latest master. There are some conflicting changes in ctkDICOMDatabase.cxx.

Done!

[lassoan]

Query/retrieve from dicomserver.co.uk (see server information here) works well using Slicer's old DICOM networking feature. However, it does not work with the visual DICOM browser.

I've tried query/retrieve from dicomserver.co.uk and no progress information was shown, no error message was displayed, , nothing showed up in the browser. We really need some feedback about what's going on with the query. https://dicomserver.co.uk/logs/ showed that connection was established and some information was exchanged.

Verification failed, too. A popup was displayed, but would be nicer to have a "Verification" column in the "Servers" table (unknown, in progress, success, failure).

[lassoan]

After about half hour of investigation, I've realized that although I've fixed the port number of dicomserver.co.uk, I did not click Apply changes. To avoid this, the following could help: if when there are pending changes, then the modified field and the Apply changes button could be VERY visibly highlighted.

After Apply button, some patients are retrieved!! Good! However, studies only appeared for a few patients.

[Punzo]

After about half hour of investigation, I've realized that although I've fixed the port number of dicomserver.co.uk, I did not click Apply changes. To avoid this, the following could help: if when there are pending changes, then the modified field and the Apply changes button could be VERY visibly highlighted.

Ok, I will make this a priority on Monday.

Do you think that setting the text to bold and the cell selected (i.e. different background) would be enough?

After Apply button, some patients are retrieved!! Good! However, studies only appeared for a few patients.

Uhm, ok I will have a look on Monday. Can you tell me what is the right port for dicomserver.co.uk? So I will also correct the default.

[lassoan]

dicomserver.co.uk - port 104 worked for me:

More information on the server and its capabilities:

https://dicomserver.co.uk/

[lassoan]

Do you think that setting the text to bold and the cell selected (i.e. different background) would be enough?

Using the same yellow background as in the filter/query parameter highlighting could work. I don't think bold is necessary.

Few other things:

• Style of "Apply changes" and "Discard changes" should be the same as Add/verify/remove host. Maybe it would be more clear if Apply/Discard changes buttons would be in each section (to avoid cluttering the GUI, we could make the buttons show up only when there are pending changes)
• Error reporting is inconsistent. Most often I don't see any errors (just nothing shows up), but once an error message came up as a button. But then I could not remove that message. A nice solution would be to have a "Job list" button, which could show an error notification (red or yellow circle) on it when one of the jobs failed. Clicking on it the button would show the job list (it can be a popup or a collapsing section in the GUI) that would show all the jobs (in-progress and completed) along with its status (it can be just simple list, details shown as tooltip or popup; with an option to show all jobs or only in-progress jobs, a button to clear the completed jobs, a button to cancel the selected jobs, and a button to retry the selected jobs).

• In the ctkDICOMVisualBrowser application, the folder selector does not reflect the actual DICOM database location (it seems that the button is not updated from application settings at startup)
• When hitting the search button, the patient list always jumps to the first patient, which is quite annoying, as I keep clicking it to see if new studies have become available for the current patient on the server. Current patient selection should not be lost if Search button is pressed (if the current patient is still selected by the search criteria).
• By default show all local content.
• Error message like "bc2343432-234241frfd-2342341s job failed to fetch data" is quite annoying. It should tell something like "Fetching data for patient John Doe [patientid] from server MedicalConnections failed. Response not received for 30 seconds. Click here to see more details." (clicking could open the job list with that task highlighted)
• In settings, servers should be in a "Servers" collapsible section, the same way as "Storage" (instead of the "Servers" label)
• Display patient name in "Patient information" section (while there are many patient names listed above, it is not that clear which one is actually selected; it is also not possible to select and copy-paste that name). Left-align field names and values (for example, "ID:" label is on the left - which is good; but then the actual ID value is floating somewhere in the middle of the screen).

[Punzo]

RoadMap (after item 4, the items are not listed in priority):

• 1) Apply windows build fix
• 2) Fix missing studies when querying https://dicomserver.co.uk/ (port 104)
• 3) merge PR
• 4) Integrate Visual DICOM Browser in Slicer as an experimental option (off in default)

Logic:

• 5) add a writing lock static variable in the ctkDICOMDatabase class
• 6) smart managing of the inserts and memory when retrieving a series. i.e. we could have a customizable framesBatchLimit variable.
• 7) add infrastructure to set the maximum number of concurrent workers per server (now it is just per type of job).
• 8) the retry time delay value should be higher and exponential with a factor 2/3 (with some randomness), i.e. each subsequent retry should have a larger delay. Current default parameter is a fixed 100ms (not enough for a server to recover). Instead of using the Maximum number of retries (current default is 3), we should use the maximum waiting time (the one defined in the GUI in the server settings) -> timout warning in the UI (check the string if it is clear).
• 9) Automatic prefetch of the full series (now the current behaviour) could be disabled and the retrieve could it be done only when the selected series are manually loaded.

Settings UI:

• 10) servers should be in a "Servers" collapsible section, the same way as "Storage" (instead of the "Servers" label)
• 11) Verify host: change response string. C-ECHO connection verified/failed (if failed error). Add a "Verification" column in the "Servers" table (unknown, in progress, success, failure). Should the verification be run by the scheduler? at the moment it is run in the main thread.
• 12) server settings should be more human readeable in the qt settings file.
• 13) Change in the UI the column names Proxy and Protocol with additional Retrieve (Retrieve Proxy etc....)
• 14) Add debug option to activate the output from DCMTK. Also we should have a look as to stream the DCMTK output. It would be nice also to have the output separated for each request.
• 15) Style of "Apply changes" and "Discard changes" should be the same as Add/verify/remove host. Maybe it would be more clear if Apply/Discard changes buttons would be in each section (to avoid cluttering the GUI, we could make the buttons show up only when there are pending changes). Also apply warning yellow background to modified fields.

visual browser UI:

• 16) When hitting the search button, the patient list always jumps to the first patient, which is quite annoying, as I keep clicking it to see if new studies have become available for the current patient on the server. Current patient selection should not be lost if Search button is pressed (if the current patient is still selected by the search criteria).
• 17) By default show all local content.
• 18) Display patient name in "Patient information" section (while there are many patient names listed above, it is not that clear which one is actually selected; it is also not possible to select and copy-paste that name). Left-align field names and values (for example, "ID:" label is on the left - which is good; but then the actual ID value is floating somewhere in the middle of the screen).
• 19) right click delete name in the UI should be: delete from local database
• 20) Thumbnails: add transparent and four directions in the draw text in the screenshots. Remove N.frames and put the third dimension on the rowXcolumns (e.g. 100x100x5). Elide the series description and put the full text in tooltip.
• 21) Error reporting is inconsistent. Most often I don't see any errors (just nothing shows up), but once an error message came up as a button. But then I could not remove that message. A nice solution would be to have a "Job list" button, which could show an error notification (red or yellow circle) on it when one of the jobs failed. Clicking on it the button would show the job list (it can be a popup or a collapsing section in the GUI) that would show all the jobs (in-progress and completed) along with its status (it can be just simple list, details shown as tooltip or popup; with an option to show all jobs or only in-progress jobs, a button to clear the completed jobs, a button to cancel the selected jobs, and a button to retry the selected jobs).
• 22) Error message like "bc2343432-234241frfd-2342341s job failed to fetch data" is quite annoying. It should tell something like "Fetching data for patient John Doe [patientid] from server MedicalConnections failed. Response not received for 30 seconds. Click here to see more details." (clicking could open the job list with that task highlighted)
• Hitting enter in a search field should run the query (same as clicking the search button)
• The logic behind "Study" and "Series" textboxes in "Patient search" section is not clear. If I type there something then still all the patients are shown, just studies or series list is empty. When the study filter is set then patients that don't have any matching studies should not show up. Similarly, those patients and studies shold not show up that don't have any series that matches the filter criteria.

ctkDICOMVisualBrowser application:

• 23) the folder selector does not reflect the actual DICOM database location (it seems that the button is not updated from application settings at startup)

[Punzo]

@lassoan (1) and (2) done. Please have a look if this can be merged now. I will start (4) meanwhile

[lassoan]

Based on review with @pieper and @lassoan and testing on Linux and Windows on multiple servers, this is now ready to be merged.

[lassoan]

This is now merged. @Punzo thank you very much for your awesome work!

Please add the list of issues/enhancements to a new issue in this repository.

[Punzo]

• implementing send (i.e. adding ctkDICOMSendJob, ctkDICOMSendWorker and ctkDICOMSend with underlining DIMSE DcmStorageSCU)
• add DICOMweb
• handle jobs queue in the scheduler by file (so we can restart the jobs/workers at application restart). We should add UI to visualize the jobs queue and status.
• add data streaming from visual brower series widgets to Slicer volume nodes.

Thanks for reviewing/testing and merging!

[Punzo]

Please add the list of issues/enhancements to a new issue in this repository.

Done, see #1162

[jcfr]

I suggest we revert CTK master to the hash before the integration and use squash & merge along with providing a descriptive commit message outlining the feature integrated, currently there is only the title ENH: Add visual DICOM browser

[lassoan]

I was thinking about this today, too. There is a follow-up PR that I'll test and if that works well then we should squash that with all these commits.