commontk / CTK

A set of common support code for medical imaging, surgical navigation, and related purposes.
https://commontk.org
Apache License 2.0
855 stars 490 forks source link

Add dicom visual navigation | :warning: Changes removed from main #1142

Closed jcfr closed 8 months ago

jcfr commented 1 year ago

:warning: Changes originally introduced through this pull-request have been removed from main by force pushing. Corresponding changes are superseded by https://github.com/commontk/CTK/pull/1165 :warning:

Rational:

jcfr commented 1 year ago

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

Punzo commented 1 year ago

@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:

ctkVisualDICOMBrowser

jcfr commented 1 year ago

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

cc: @tbirdso

lassoan commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 11 months ago

@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:

  • [x] add a storage listener class in CTK (ctkDICOMStorageListener).
  • [x] binaries in CTK with an example of the visual dicom browser running without Slicer (Folder selector for the database and ctkDICOMVisualBrowserWidget).
  • [x] 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:

UML diagram:

image

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:

To Do: cleaning design

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

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

Punzo commented 10 months ago

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....

Done in https://github.com/commontk/CTK/pull/1142/commits/f9119726c3e52628820ffc16e97c71c64252c5b3 1) Cleaned raw pointers with QSharedPointers. 2) Signals now do not send anymore pointers for updates. UI reads all the informations from the dicom database. 3) Logic and UI are now independent 4) each worker is now independent and it can start new jobs independently, e.g.: retry or proxy server. Workers are the executors, jobs are metadata objects handled in a queue in the scheduler class.

this is the current UML diagram: ctkVisualDICOMBrowser

Current PR content description

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

Future ENH:

Testing in CTK Application:

I have added a ctkApplication called ctkDICOMVisualBrowser for testing. It is needed to set cmake options as:

 CTK_BUILD_ALL                    OFF
 CTK_BUILD_ALL_APPS               OFF
 CTK_BUILD_ALL_LIBRARIES          OFF
 CTK_BUILD_ALL_PLUGINS            OFF
 CTK_BUILD_EXAMPLES               ON
 CTK_ENABLE_DICOM                 ON
 CTK_ENABLE_DICOMApplicationHos   OFF
 CTK_ENABLE_PluginFramework       OFF
 CTK_ENABLE_Python_Wrapping       OFF
 CTK_ENABLE_Widgets               ON

Then in CTK-build/bin, there is the binary ctkDICOMVisualBrowser.

Punzo commented 9 months ago

@lassoan @pieper @jcfr, this is ready for review. Any feedback on the design of the infrastructure classes, widget, and logic implementation would be greatly appreciated. For a description of the current status, please refer to my previous comment.

Please note that I still have a couple of small tasks pending (see below and the "To Do" comments in the code). I plan to address them in January upon my return from vacation, but they are minor enhancements to the operations logic (deep within the code) and should not hinder the review process:

A) Presently, the results from a retrieve operation are inserted at the end of the fetch. This leads to: i) Too many inserts for the RetrieveSOPInstance operation, resulting in slower processing. ii) Temporary saving of too many frames in memory for RetrieveSeries and RetrieveStudy operations, leading to higher memory consumption, especially for large series. iii) For the StorageListener, inserts are done every 1 second for all the fetched frames within that time, causing similar issues as (i) and (ii).

B) Concurrently inserting into the ctkDICOMDatabase from different threads poses an issue where the native lock (transaction + commit) mechanism does not function as expected. I tested this with other databases and encountered this problem only with the scheme in ctkDICOMDatabase. This could potentially lead to crashes (even when opening a database connection for each QThread).

Proposed solutions (requiring only a couple of development days, but I'll address them in January):

(A) For all operations, implement batch insertions whenever the number of fetched frames exceeds a configurable frameBatchLimit variable.

(B) Currently, ctkDICOMScheduler ensures that only one ctkDICOMInsertJob is executed at a time by the workers. We should also add a static Lock variable in the ctkDICOMDatabase to block any other CTK component or application from writing to the database if another writing process is ongoing.


Note/question for @jcfr: To run the test locally, I had to follow these steps: 1) I configured CTK in a CTK-Build folder as follows:

CTK_BUILD_ALL                    OFF
CTK_BUILD_ALL_APPS               OFF
CTK_BUILD_ALL_LIBRARIES          OFF
CTK_BUILD_ALL_PLUGINS            OFF
CTK_BUILD_EXAMPLES               ON
CTK_ENABLE_DICOM                 ON
CTK_ENABLE_DICOMApplicationHos   OFF
CTK_ENABLE_PluginFramework       OFF
CTK_ENABLE_Python_Wrapping       OFF
CTK_ENABLE_Widgets               ON

2) Ran make -j 20 in CTK-Build 3) Manually ran make install in CTK-Build to generate DCMTK binaries in CTK-Build/CMakeExternals/Install/bin 4) Added CTK-Build/CMakeExternals/Install/lib to my LD_LIBRARY_PATH environment variable 5) Then I could run the test in CTK-Build/CTK-build folder (ctest -j 20)

I might be missing something, but it would be helpful if the superbuild could automatically install DCMTK (eliminating the need for steps 3 and 4). Please let me know if this is a bug or if I am overlooking something.

pieper commented 9 months ago

This is quite a lot of code, but a quick readthrough didn't raise any serious concerns for me, although we really should do some cross platform testing with different network and server scenarios to see how all the concurrency holds up.

I think it would help me to have a guided tour of the class structure. We can do this in January, either on zoom or in person at project week. Do you have a preferred timeline for merging?

lassoan commented 9 months ago

@davide has been doing all development and testing on linux. I'll test on Windows. It would be great if @pieper you could test on macOS.

Punzo commented 9 months ago

This is quite a lot of code, but a quick readthrough didn't raise any serious concerns for me, although we really should do some cross platform testing with different network and server scenarios to see how all the concurrency holds up.

Ok thanks, for having already had a quick look. I agree, as Andras said I tested on Linux and mainly with Orthanc servers. It would be probably good to test also in environments with low bandwidth and high packet loss.

I think it would help me to have a guided tour of the class structure. We can do this in January, either on zoom or in person at project week. Do you have a preferred timeline for merging?

Sure, we can do a video call asap when we will be back from vacation in January. Ideally I would like to merge and integrate in Slicer (as an additional experimental feature in the DICOM module) before the project week. In this way we can use the Slicer preview during the slicer project week for testing, collecting feedback, etc.

pieper commented 9 months ago

Sounds great. I'll be on break until Jan 3, but happy to meet after that. Enjoy your time off : )

Punzo commented 9 months ago

Sounds great. I'll be on break until Jan 3, but happy to meet after that. Enjoy your time off : )

Have a nice Christmas vacation too (: !!!!

Punzo commented 9 months ago

Plan from meeting 05/01/2024:

To Do from 2023:

To Do from meeting 05/01/2024:

lassoan commented 9 months ago

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

Punzo commented 9 months ago

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

Done!

lassoan commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

image

More information on the server and its capabilities:

https://dicomserver.co.uk/

lassoan commented 9 months ago

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:

image

Punzo commented 9 months ago

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

Logic:

Settings UI:

visual browser UI:

ctkDICOMVisualBrowser application:

Punzo commented 9 months ago

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

lassoan commented 8 months ago

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

lassoan commented 8 months ago

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 commented 8 months ago
  • 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 commented 8 months ago

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

Done, see https://github.com/commontk/CTK/issues/1162

jcfr commented 8 months ago

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 commented 8 months ago

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.