Roadmap list of issues/enhancements for Visual DICOM Browser

Punzo commented 10 months ago

RoadMap (items are not listed in priority):

long-termENH:

[ ] implementing send in C++ in CTK (i.e. adding ctkDICOMSendJob, ctkDICOMSendWorker and ctkDICOMSend with underlining DIMSE DcmStorageSCU). This would allow to use the background/parallel operations infrastructure for SEND as well.
[ ] add DICOMweb
[ ] handle jobs queue in the scheduler by file (so we can restart the jobs/workers at application restart)
[ ] add data streaming from visual browser series widgets to Slicer volume nodes.
[ ] add support for DICOM frame set (reference https://discourse.slicer.org/t/new-frame-set-table-in-the-dicom-database/35012)

UI customization:

[ ] visual DICOM browser (and in general CTK) uses Qpalette to change colors dynamically. This does not go well when customizing CTK/Slicer by styleSheet. Probably we should use the styleSheet approach everywhere. For example, the background yellow warning on filters does not work when applying a custom styleSheet. Discuss with Sam for a good design. Ideally the solution should: 1) use one unique style file containing the stylesheet for all the CTK classes 2) change color of qt widgets in CTK dynamically with the Property Selector, e.g.: https://wiki.qt.io/Dynamic_Properties_and_Stylesheets 3) custom Slicer apps should be able to change the style by simply rewriting and reloading the style file

Issues and minor ENH:

Logic:

[ ] add a writing lock static variable in the ctkDICOMDatabase class
[ ] smart managing of the inserts and memory when retrieving a series. i.e. we could have a customizable framesBatchLimit variable.
[ ] add infrastructure to set the maximum number of concurrent workers per server (now it is just per type of job).
[ ] 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).
[ ] 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.
[x] While waiting for download of a complete sequence, we can still do some user actions, but not all. I suspect that it is because we pump the event loop with app->processEvents(), but that might be unintentional. We might want to call processEvents with flags that disable user interaction, or even better, show a model popup with a "Cancel" button.
[x] Stopping/Deleting many jobs (>100) makes the scheduler hanging for ~2 sec. This because the deleting is done one job at the time and then a signal is emitted to queue new jobs. In Addition this methods are thread safe by a mutex which is expensive. Probably the best action would be to delete all the jobs togheter in only 1 call.
[x] The SCU release association (DCTMK) should be protected by a mutex, because the method in DMTCK is not thread safe and in past we had crashes for it. I had also to force the release of the association at cancel operation of worker for query and retrieve. This because the DCTMK cancel operation can fail (but returns always good() == true) and in some cases we can have eternal zombies workers. The release is done in the hanlde overriden methods of SCU.

Settings:

[x] servers should be in a "Servers" collapsible section, the same way as "Storage" (instead of the "Servers" label)
[x] Add debug option to activate the output from DCMTK (global logger to terminal).
[x] Style of "Apply changes" and "Discard changes" should be the same as Add/verify/remove host.
[x] Verify host: change response string. C-ECHO connection verified/failed. Add a "Verification" column in the "Servers" table (unknown, in progress, success, failure). C-ECHO should be run in background by the scheduler (need to implement related job and worker classes).
[x] server settings should be more human readeable in the qt settings file.
[x] Change in the UI the column names Proxy and Protocol with additional Retrieve (Retrieve Proxy etc....)
[x] Apply warning yellow background to modified fields.
[x] MedicalConnection server node, should have the Query/Retrieve/Storage checkboxes to be checked as default.
[x] After clicking Apply, the row selection should be preserved
[x] Hide restore defaults button (it can be shown only for testing)
[x] Verify host should work on the current modified server settings (or it should be disabled until the user apply or discard the changes).
[x] MedicalConnection server node, should actually NOT have the Query/Retrieve/Storage checkboxes to be checked as default.
[x] verification column should be updated only from jobs which started after clicking the verify button.
[x] Closing the application while echo workers are running make it crash. steps: run a verify server with wrong parameters and a timeout of 30 sec. The destructor of the scheduler will wait only 10 sec (hardcoded). The destructor has to wait for all jobs closing properly (echo job will not terminate before the server timeout parameter).

Visual browser UI: Patient selection:

[x] By default show all local content.
[x] 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).
[x] Hitting enter in a search field should run the query (same as clicking the search button)
[x] 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).
[x] Patient selection is lost when showing/hiding the browser in 3D Slicer
[x] Having a shortcut to browser previous/next patient (Ctrl-tab/Ctrl-shift-tab) would be useful when working with a research PACS (where we are not find/explore data, without necessarily knowing patient name/id).
[x] study sorting by date, sometimes does not work. Verify the sorting.
[x] at least happened once: after pressing search, download of series started. Switched to a different patient, hourglass was displayed for a while and then the application crashed.
[x] When changing patient if there are TCIA datasets imported from local disk and there is the test server enabled (medical connection), the UI gets stuck and there is this message logged many times: Failed to find patient with PatientsName= and PatientID=
[x] visual dicom browser does not have drop action (neither the old browser). In Slicer dropping a dicom folder will use the python code in DICOM.py and then use the old browser to import. If Slicer is using the new browser should import with the visual one.
[x] All the DICOM import from local disk were broken for the visual dicom browser when using it in Slicer. The issue was that several methods on the DICOM module UI were using directly the old browser. Refactored to call methods of the class DICOMbrowser.py. Then it is the DICOMbrowser that take care of calling methods from the two browsers.
[x] importing multiple folders for the visual dicom browser is broken
[x] when importing in Slicer this logging should not happen:
```
D: DcmDataset::read() TransferSyntax="Little Endian Explicit"
D: DcmMetaInfo::checkAndReadPreamble() TransferSyntax="Little Endian Explicit"
```
This happens when detailed logging in DICOM settings is checked. It is logging printed by DCMTK. Detailed logging is already as default set to false.
[x] We need to be able to select the enabled severs on a patient basis.
[x] when importing local files, will be nice to select the tab with the imported patient. If multiple, the last one

Filtering:

[x] when doing a search without filter (i.e. show all local database), if there is nothing in the local database, the yellow warning background fo the filter gets bugged (always on, even if doing a new search)
[x] Filter at the moment is both query and filter the local database (this could be confusing). Add an advanced menu under the query button to disable the query/retrieve.
[x] Filter local database could leave studies or patients empty (these should be filtered out). See also next two issues for more detailed info.
[x] I don't get some updates (study list remains empty) when setting filter criteria to "Everything from last year". I see "ctkDICOMQuery: the number of responses of the query task at patients level surpassed the maximum value of permitted results (i.e. 25)." message in the log - can it be the root cause? Or maybe just that this filter does not filter out patients. It is confusing that some filter boxes filter patients, while others let all patients displayed, even those that don't have matching studies/series/modality/date.
[x] 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.
[ ] Patients list as tabs in the tab control of the Patient widgets is not optimal. We should use the old widget from the ctkDICOMBrowser (first one, only the patient list). We could have an option to switch between them.

Thumbnails && series widgets:

[x] on right click menu we will need an addittion action: force redownload. (for series and studies)
[ ] additional feature for force download (add checkable settings, default False): if any study is 3 hours old (use DICOM study date), force redownload every time we use query (not for local search).
[ ] additional feature for force download: enable monitoring (right click action) for a time (configurable). If any series or instance in series gets uploaded in PACs, in this case, will be automatically fetched and the UI updated (some UI icons to give the warning that it is updating). For series, we should check also if new instances have been added in the PACs to the series.
[ ] additonal feature: add a check (when clicking the query/retrieve lense button) to a folder containing dicom files, if there are files import them and clean the folder. Create a lock file in the dicom folder and we put there the PID of the session of the user, when is accessing the DICOM files/deleting them. Check also if using a watcher we can automatically do the import. The local folder should be a "local connection", like a server. So we could add as many we want in the settings and everything will be automatic in the infrastructure (not adding hard coded specil cases, but everything will be independednt), it is just a new "protocol" comunications.
[x] right click delete name in the UI should be: delete from local database
[x] text: add transparent and improve shadows. Remove N.frames and put it as the third dimension on the rowXcolumns (e.g. 100x100x5). Elide the series description and put the full text in tooltip.
[x] thumbnails are quite low quality, which is visible when the image contains text or simple graphics - if there is an option to use higher-order interpolation and it is not much slower then it would be nice to switch to that
[ ] rendering for Segmentation DICOM object does not work with the ctk thumbnail class.
[x] rendering of thumbnail can be expensive and currently is done in the main thread. It would be nice to do it in background (i.e. create ctkDICOMThumbnailJob and ctkDICOMThumbnailWorker classes)
[x] Selection in series list uses blue background. If the thumbnail image has rectangular shape then the blue text that is overlaid on the thumbnail is not readable (blue text on blue background). Maybe improving the drop shadow would fix it (to make it 4 directions instead of just 1, maybe larger offset).
[x] Wait and load selected series sometimes did not worked and the UI got stuck.
[ ] loading a series in visual dicom browser does not have the old advanced/examine UI
[x] after deselecting there is a white box around thumbnails in dark mode in Slicer
[x] on high resolution monitor, most likley the user has scaling not at 100%. This creates issues with the thumbnails text rendering.
[x] Study description should go in the ctk group title

Error report:

[x] Implement Job list status UI. See next two issues for more info.
[x] 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).
[x] 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)
[x] we need to stream the DCMTK logging per each job to the Job list UI, this can be implemented by instantiating an Appender with a custom ErrorHandler to the SCU logger: DCM_dcmnetLogger (name: "dcmtk.dcmnet")
[ ] when loading a series, if it is not supported (or it is supported with an extension), we should fire a popup warning.
[x] big warning push button for failed jobs should be removed. We should have warning/error icons at patient/study/series widgets which should refer to the job in some way.
[x] then we could use the same icon on patient/study/series widget to know if we are waiting for the query (loading icon)
[x] also a refresh button to refresh the query for patient/study/series widgets (if everything was success, this can be useful to query new changes in the server)
[x] clearing jobs push buttons should be outside from filter group box
[x] clear completed push button, should also remove the current "cancel" one
[x] status: if the user stop the jobs, status should be "user stopped"
[x] status: if the job is stopped because it failed, it should be "attempt failed" (if it will retry it) / "failed" (if it is the last attempt)
[x] When a request fails then it leaves 4 entries in the log: 3x failed attempts + 1x final failed attempt. This is too much noise (and if the user wants to retry he would need to pay attention not to retry all 4 entries, only 1). Could you change filtering settings so that the 3 failed attempts are not displayed by default? The simplest would be to only display the 3 failed attempts only if “Show completed” is enabled in filtering settings.
[x] Jobs and Settings group box should be encapsulated in one
[x] Having also a splitter between the main patients navigation widget and Jobs/Settings groups would be ideal.
[x] clicking a row in the job list should open the corrispettive patient widget in the main navigation widget.
[x] Details:1) do not put the separator if only one job is selected 2) separator should be either ----- or ==== (check DCMTK and use a different one) 3) do not put spaces before ":"

ctkDICOMVisualBrowser application:

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

Reference PR:

Punzo commented 10 months ago

Huge amount of DICOM communication log is printed on the console. It can cause very significant delays, so the amount should be configurable (we can use DICOM/detailedLogging application settings value).

@lassoan this should already work, please try to disable DICOM/detailedLogging in the Slicer settings.

Also, the messages should not be dumped to the console but it should be shown in the job list (maybe some messages, e.g., warnings/errors could be also logged in the application log)

yes I agree, this is also described in other points.

Punzo commented 9 months ago

For reference: Error report is addressed in the following PR:

https://github.com/commontk/CTK/pull/1184.

jcfr commented 9 months ago

[ ] Finalize and publish discourse post: https://hackmd.io/SFrAGE4CS8uSkI2YvDRKRg?both

Punzo commented 9 months ago

[ ] Finalize and publish discourse post: https://hackmd.io/SFrAGE4CS8uSkI2YvDRKRg?both

I have done my last edit. @lassoan it would be nice if you can review (specifically the intial sentence regarding the "patient exploration tool").

Punzo commented 9 months ago

@lassoan, thanks for the text edits and I have applied yout comments in https://hackmd.io/SFrAGE4CS8uSkI2YvDRKRg?both.

I am going to post it now on the Slicer forum

@jcfr can you remove or replace the images and videos (see below for the new ones) in https://github.com/commontk/CTK/pull/1165#issue-2078249761, please? thanks in advance

ScreenshotVisualDICOMBrowser ScreenshotSettings

https://github.com/commontk/CTK/assets/7985338/a0e362e7-858a-4e47-9298-8d00ae16a23b

VisualDICOMBrowserUML

lassoan commented 9 months ago

I am going to post it now on the Slicer forum

Is the feature included in today's Slicer Preview Release? If not then please wait. Also, for biggest impact, it may worth making the announcement on Tuesday (many people are not at their computers in the weekend; Monday is often too busy). It would also allow us to do some internal testing with people we ask to have a look (Steve, Rudolf Bumm, ...).

Punzo commented 9 months ago

Is the feature included in today's Slicer Preview Release? If not then please wait.

yes!

Also, for biggest impact, it may worth making the announcement on Tuesday (many people are not at their computers in the weekend; Monday is often too busy). It would also allow us to do some internal testing with people we ask to have a look (Steve, Rudolf Bumm, ...).

ops sorry, I should have waited, I have already posted https://discourse.slicer.org/t/new-feature-visual-dicom-browser/33874 . I can delete it if you think it would be better.

lassoan commented 9 months ago

OK, no problem. There is some time before the weekend, so hopefully it will catch people's attention and they can find some time to try it and give feedback.

jcfr commented 9 months ago

We can also add a follow post on the topic on Tuesday to bring it to the top of the list. Something along those lines:

Raw:

Now that the feature has been available :sparkles: for a few day through the Slicer `Preview` package, let us know if you have any comments or suggestions.

Thanks again for your time :pray:

Rendered:

Now that the feature has been available :sparkles: for a few day through the Slicer Preview package, let us know if you have any comments or suggestions.

Thanks again for your time :pray:

Punzo commented 9 months ago

We can also add a follow post on the topic on Tuesday to bring it to the top of the list. Something along those lines:

Raw:
Now that the feature has been available :sparkles: for a few day through the Slicer `Preview` package, let us know if you have any comments or suggestions.

Thanks again for your time :pray:
Rendered:

Now that the feature has been available ✨ for a few day through the Slicer Preview package, let us know if you have any comments or suggestions.

Thanks again for your time 🙏

@jcfr @lassoan I was thinking that we may wait for the merge of https://github.com/commontk/CTK/pull/1184 (job list) to bring the post up to the list. Let me know.

Punzo commented 9 months ago

for reference:

current dev CTK branch https://github.com/Punzo/CTK/tree/visualDICOMBrowserENH

current dev Slicer branch https://github.com/Punzo/Slicer/tree/visualDICOMBrowserENH

mauigna06 commented 5 months ago

Could you please do the one below?

Study description should go in the ctk group title

Punzo commented 5 months ago

Could you please do the one below?

Study description should go in the ctk group title

sure!

Punzo commented 5 months ago

Could you please do the one below?

Study description should go in the ctk group title

Done in https://github.com/commontk/CTK/pull/1206/commits/7dca0f3fa0d12b4730f9027caa5a7225785eeef2

lassoan commented 5 months ago

I've been testing this and ran into an issue:

configure https://www.dicomserver.co.uk/ server
query using "a" in patient name
browse tabs, find a tab where there are a few normal-sized CT or MR scans
double-click on one of them => ERROR: it never loads
checked the job list, all jobs are cancelled
click the show/hide DICOM browser button to hide the browser and then click again to show it => ERROR: crash in ctkDICOMVisualBrowserWidgetPrivate::retrieveSeries() due to seriesItemWidget pointer is valid, but the memory position it points to contains a deleted object (dangling pointer)

Punzo commented 5 months ago

I've been testing this and ran into an issue:

configure https://www.dicomserver.co.uk/ server

query using "a" in patient name

browse tabs, find a tab where there are a few normal-sized CT or MR scans

double-click on one of them => ERROR: it never loads

checked the job list, all jobs are cancelled

click the show/hide DICOM browser button to hide the browser and then click again to show it => ERROR: crash in ctkDICOMVisualBrowserWidgetPrivate::retrieveSeries() due to seriesItemWidget pointer is valid, but the memory position it points to contains a deleted object (dangling pointer)

ok, thanks for reporting. I will look into it now.

Punzo commented 5 months ago

I've been testing this and ran into an issue:

configure https://www.dicomserver.co.uk/ server

query using "a" in patient name

browse tabs, find a tab where there are a few normal-sized CT or MR scans

double-click on one of them => ERROR: it never loads

checked the job list, all jobs are cancelled

click the show/hide DICOM browser button to hide the browser and then click again to show it => ERROR: crash in ctkDICOMVisualBrowserWidgetPrivate::retrieveSeries() due to seriesItemWidget pointer is valid, but the memory position it points to contains a deleted object (dangling pointer)

ok, thanks for reporting. I will look into it now.

@lassoan fixed in https://github.com/commontk/CTK/pull/1206/commits/967de75d2653ad685d03fcee28a8f429a63dc0a5

There were few issues: 1) Not all the series widget status were handled properly in that specific logic -> causing hanging 2) some jobs were not stopped or restarted properly 3) missing a progress ApplicationModal dialog, I was giving the illusion that the user could freely click on the UI during this step. That was not possible since allowing the user to interact with the UI at this step would be possible only when we implement the streaming from series widget to slicer volumes. Therefore I have added a progress ApplicationModal dialog with cancel button.

I have implemented also this item from the roadmap (the issue was connected to this): While waiting for download of a complete sequence, we can still do some user actions, but not all. I suspect that it is because we pump the event loop with app->processEvents(), but that might be unintentional. We might want to call processEvents with flags that disable user interaction, or even better, show a model popup with a "Cancel" button.

please feel free to retest latest version! thanks for testing!

Punzo commented 5 months ago

please feel free to retest latest version! thanks for testing!

please note that I have just done a force push on last commit (https://github.com/commontk/CTK/pull/1206/commits/648f261a10e1a2d29c37d9a4e43249a84d4e6856)

lassoan commented 5 months ago

Thank you, I'm testing it now.

lassoan commented 5 months ago

Thanks a lot for the fixes, there are no more crashes or hangs!

But. When I click the "Show DICOM database" button to hide the browser then it's get hidden in a 1 second, good. When I click the "Show DICOM database" button to show the browser then it takes about 1 minute for the browser to appear, with this call stack when I stop at random times:

It seems that thumbnails are regenerated every time I show the browser. Probably in release mode it would not be so bad, but still, it is a lot of unnecessary work.

It is possible that these thumbnails are regenerated because thumbnail generation failed. I see these on the console:

E: no pixel data found in DICOM dataset
Rendering of DICOM image failed for thumbnail failed: Missing attribute
E: no pixel data found in DICOM dataset
Rendering of DICOM image failed for thumbnail failed: Missing attribute
E: no pixel data found in DICOM dataset
Rendering of DICOM image failed for thumbnail failed: Missing attribute
E: no pixel data found in DICOM dataset
Rendering of DICOM image failed for thumbnail failed: Missing attribute
E: no pixel data found in DICOM dataset
Rendering of DICOM image failed for thumbnail failed: Missing attribute
W: DcmItem: Length of element (01f1,1055) is not a multiple of 8 (VR=FD)
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result
W: processing MR image ... applying modality transform may create unexpected result

It also seems that these generated thumbnail are not saved to disk.

I've also noticed that the thumbnails are generated on the main thread. Since thumbnail generation can be really slow (it takes 10 seconds for this DICOM SEG) it would be nice to do it in background threads.

Punzo commented 5 months ago

@lassoan I have addressed the main issues with the thumbnails generation in https://github.com/commontk/CTK/pull/1206/commits/f59aef0e4b620f31919902209baeaf4794fcbd5b, plus few minor issues :

PERF: Implement thumbnail storage to disk to avoid re-rendering after series widget destruction.
PERF: Disable SEG rendering in thumbnails due to limited support and potential performance issues. Replace with a blank thumbnail featuring a document SVG.
BUG: Resolve issue hanging job termination when status is 'Queued'.
BUG: Address issue with FreezeJobsScheduling during shutdown.
BUG: Correct retry functionality when status icon on series widget is clicked.

Still need to work on the background thumbnail creation - ran out of time. But don't worry, it's on the task list for this issue.. i.e.:

rendering of thumbnail can be expensive and currently is done in the main thread. It would be nice to do it in background (i.e. create ctkDICOMThumbnailJob and ctkDICOMThumbnailWorker classes)

Feel free to test again, Thanks!

lassoan commented 5 months ago

Few minor things to improve on the very latest DICOM browser improvements:

showing DICOM browser after it was hidden still takes about 5 seconds in debug mode (showing 25 patients from dicomserver.co.uk) - could we just show/hide the widget without rebuilding?
when I double-click on an image to load it and it stops retrieve of other series, then I get a bunch of "user stopped" jobs - OK. But then when I select all and retry then those "user stopped" jobs never go away. They seem to trigger some new jobs, but if those are successful then those stopped jobs still remain. It would be better to move them to a queued or in-progress state (they could maintain the entire job history, with all preivous errors, retries, etc.)
Maybe not an error, but those "user-stopped" jobs have various error, such as DUL Finite State Machine Error: No action defined, state 7 event 10 or Failed receiving DIMSE command: 0006:0205 DIMSE Caller passed in an illegal association but nothing indicates that the user cancelled these jobs (other than the Status: user-stopped). Is this normal?
When in 3D Slicer the DICOM browser is opened the first time then a window pops up for a fraction of a second and then disappears. This could be probably prevented by adding the window to some layout when it is created and first shown (or only show it when it is actually needed).

Punzo commented 4 months ago

Few minor things to improve on the very latest DICOM browser improvements:

showing DICOM browser after it was hidden still takes about 5 seconds in debug mode (showing 25 patients from dicomserver.co.uk) - could we just show/hide the widget without rebuilding?

when I double-click on an image to load it and it stops retrieve of other series, then I get a bunch of "user stopped" jobs - OK. But then when I select all and retry then those "user stopped" jobs never go away. They seem to trigger some new jobs, but if those are successful then those stopped jobs still remain. It would be better to move them to a queued or in-progress state (they could maintain the entire job history, with all preivous errors, retries, etc.)

Maybe not an error, but those "user-stopped" jobs have various error, such as DUL Finite State Machine Error: No action defined, state 7 event 10 or Failed receiving DIMSE command: 0006:0205 DIMSE Caller passed in an illegal association but nothing indicates that the user cancelled these jobs (other than the Status: user-stopped). Is this normal?

When in 3D Slicer the DICOM browser is opened the first time then a window pops up for a fraction of a second and then disappears. This could be probably prevented by adding the window to some layout when it is created and first shown (or only show it when it is actually needed).

I have addressed the latter and opened a PR for Slicer to update CTK version. The first 3 I will address them whne I will be back from vacation.

Punzo commented 2 months ago

when I double-click on an image to load it and it stops retrieve of other series, then I get a bunch of "user stopped" jobs - OK. But then when I select all and retry then those "user stopped" jobs never go away. They seem to trigger some new jobs, but if those are successful then those stopped jobs still remain. It would be better to move them to a queued or in-progress state (they could maintain the entire job history, with all preivous errors, retries, etc.)

Maybe not an error, but those "user-stopped" jobs have various error, such as DUL Finite State Machine Error: No action defined, state 7 event 10 or Failed receiving DIMSE command: 0006:0205 DIMSE Caller passed in an illegal association but nothing indicates that the user cancelled these jobs (other than the Status: user-stopped). Is this normal?

Done in https://github.com/Punzo/CTK/commit/0c80562f7c48e5d6c3592aeb94bd4a4a916d34ab I will open a PR in CTK when I will have more ENH done

Punzo commented 2 months ago

showing DICOM browser after it was hidden still takes about 5 seconds in debug mode (showing 25 patients from dicomserver.co.uk) - could we just show/hide the widget without rebuilding?

I could not reproduce the timing issue, I have 30 patients from dicomserver.co.uk and the show/hide does not lag. Using debug mode and detailed logging in DICOM settings:

https://github.com/user-attachments/assets/d5626ea5-777c-481c-87cc-d109eda42692

In fact, when showing the visual DICOM browser widget, Slicer calls only self.dicomVisualBrowser.onShowPatients(), which do not perfom any query/retrieve and builds only the UI of the current selected patient widget. In addition for the series widgets of the selected patient, it uses the stored png files. Maybe there was an image giving issues to the thumbnail generator, which is still done in the main thread. It would be nice to get such series, if you manage to reproduce again the issue.

Anyway, in Slicer, it did not make sense to rebuild the UI of the visual dicom browser everytime. So now, I build it only once. Done in: https://github.com/Punzo/CTK/commit/6bc59dc1e19ef5ff918779377a87da345013d3b6 https://github.com/Punzo/Slicer/compare/Slicer%3ASlicer%3Amain...visualDICOMBrowserENH

Punzo commented 2 months ago

rendering of thumbnail can be expensive and currently is done in the main thread. It would be nice to do it in background (i.e. create ctkDICOMThumbnailJob and ctkDICOMThumbnailWorker classes)

Done in https://github.com/Punzo/CTK/commit/b9bc53df26c18ff79d85260f71514b6c83c0fb3e

this should also fix the 5 sec issue of the previous post:

showing DICOM browser after it was hidden still takes about 5 seconds in debug mode (showing 25 patients from dicomserver.co.uk) - could we just show/hide the widget without rebuilding?

Punzo commented 2 months ago

current PRs: https://github.com/commontk/CTK/pull/1217 and https://github.com/Slicer/Slicer/pull/7912

ENH: Refactor and clean up manual retry infrastructure ENH: Add job status logging to the detail logging window ENH: Enable thumbnail generation in worker processes BUG: Fix missing update of server settings UI BUG: Add missing Python wrapping methods for ctkJobDetail ENH: Enhance job logging for better traceability PERF: Optimize UI updates by directly calling component slots

To Do: Fix performance issues when allocating hundreds of jobs. In this case the: QMutexLocker locker(&this->QueueMutex); is now the bottleneck.

Strategy 1: Investigate all the current QMutexLocker. Are all really needed?

Strategy 2: substitute QMutexLocker with QReadLocker and QWriteLocker.

Strategy 3: Use tbb::concurrent_hash_map instead of a QMap if TBB is available (#ifdef). If not

Strategy 4 (only if the other strategies fail): We could drop the QThreadPool and instatiating QThreads by ourselves. Each QThread would have his own jobs/worker map (for each job there is a worker) and we would not need to lock anything. In this case, the scheduler would need to handle the load balacing of the threads.

Punzo commented 2 months ago

current PRs: #1217 and Slicer/Slicer#7912

ENH: Refactor and clean up manual retry infrastructure ENH: Add job status logging to the detail logging window ENH: Enable thumbnail generation in worker processes BUG: Fix missing update of server settings UI BUG: Add missing Python wrapping methods for ctkJobDetail ENH: Enhance job logging for better traceability PERF: Optimize UI updates by directly calling component slots

To Do: Fix performance issues when allocating hundreds of jobs. In this case the: QMutexLocker locker(&this->QueueMutex); is now the bottleneck.

Strategy 1: Investigate all the current QMutexLocker. Are all really needed?

Strategy 2: substitute QMutexLocker with QReadLocker and QWriteLocker.

Strategy 3: Use tbb::concurrent_hash_map instead of a QMap if TBB is available (#ifdef). If not

Strategy 4 (only if the other strategies fail): We could drop the QThreadPool and instatiating QThreads by ourselves. Each QThread would have his own jobs/worker map (for each job there is a worker) and we would not need to lock anything. In this case, the scheduler would need to handle the load balacing of the threads.

Fixed in https://github.com/commontk/CTK/pull/1218/commits/b204aa2f187c3b572a1b966b68c3408d6f90f116 by cleaning loops inside the mutex and using the QReadLocker/QWriteLocker instead of QMutexLocker

Tested with a patient with 33 studies and ~200 series

commontk / CTK

Roadmap list of issues/enhancements for Visual DICOM Browser #1162

Reference PR: