feat(MetadataLoading) : enable metadata loading from images #3573
Conversation
…er no compatible data source
This commit allow OHIF to check when needed if the displaySet are reconstructible. This allow loading metadata after the initial viewport init and still display the MBR
✅ Deploy Preview for ohif-platform-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
so this is only for the dicomJSON right? since for the dicomweb there is no extra call to the PACS and the metadata provider already have all the necessary information from the |
|
Maybe we missed something in the recent changes, but in the previous versions, OHIF was doing a WADO-RS Metadata request before retrieving the instances, so we thought this could be useful as well for the dicomweb mode, as long as you know the studies, series and instances UIDs. |
|
I see. which ideally should not happen. But I will give my in-depth comments today/tomorrow Thanks! |
| @@ -95,6 +95,29 @@ function _getInstanceByImageId(imageId) { | |||
| } | |||
| } | |||
|
|
|||
| function _updateInstance(StudyInstanceUID, SeriesInstanceUID, SOPInstanceUID, metadata) { | |||
There was a problem hiding this comment.
I prefer _updateMetadataForInstance for consistency with next method
| callback: displaySet => { | ||
| const { | ||
| value: isReconstructable, | ||
| missingFrames: missingFrames, | ||
| } = isDisplaySetReconstructable(displaySet.instances); | ||
|
|
||
| return isReconstructable && missingFrames == 0 |
There was a problem hiding this comment.
I think an idea would be to fire and event in the dicomMetadataStore when an instance is updated and maybe recalculate the displaySet.isReconstructable in an event handler?
This will most likely break the dicomweb or if not break would slow it down
| // We need to force this flag to false in case the client wants to load the metadata from the image | ||
| // directly with automaticallyLoadDicomMetadata | ||
| imageMetadataLoaded: false |
There was a problem hiding this comment.
how about only make it false if only series/studies/sop UID are provided. For other use cases where we have the full metadata in the JSON this doesn't makes sense to be false and later in the provider it checks and reads again etc.
There was a problem hiding this comment.
If you follow the suggestions further in this, then you can figure out when to load appropriately just by which loader it is calling, so there wouldn't be any setting here at all.
| import dicomImageLoader from '@cornerstonejs/dicom-image-loader'; | ||
| import dcmjs from 'dcmjs'; |
There was a problem hiding this comment.
I thought these are new dependencies but seems like we already have them although I prefer if we can find a way to not call dicomImageLoader in the provider here, any thoughts?
There was a problem hiding this comment.
What I would suggest is having a lazy load function to call that if it is defined in the metadata, it can be called to replace the metadata with a full load version of it - which then removes the function from the metadata. That way, different providers can provide different versions of the function, some of which might do the dcmjs thing, and others might load JSON metadata, while others might just read bulkdata information which is missing.
| // is set. | ||
| // To make your data source compatible with this you'll have to explicitly | ||
| // set the instance.imageMetadataLoaded to false | ||
| if (this.automaticallyLoadDicomMetadata && (instance.imageMetadataLoaded === false)) { |
There was a problem hiding this comment.
as I said above, I prefer not to call the dicomImageLoader here, but even with calling it here, I don't think we need a new flag. We can derive the flag status from whether it has all necessary metadata (we should create a schema for that somewhere to validate againts, that also helps people to know what metadata is necessary) and if not we automatically switch to the dicomloader+dcmjs how about that?
There was a problem hiding this comment.
@wayfarer3130 any thought on where to call the dicomImageLoader + dcmjs other than here? (ideally they should not be here)
| @@ -22,6 +22,9 @@ window.config = { | |||
| // above, the number of requests can be go a lot higher. | |||
| prefetch: 25, | |||
| }, | |||
| // Allow Metadataprovider to parse dicom images as it retrieves it | |||
| // Only works with dicom json datasource for now | |||
| automaticallyLoadDicomMetadata: true, | |||
There was a problem hiding this comment.
Should the JSON itself specify this information? That way a JSON can say whether it already has full metadata or not. Something that just specifies SOP UID's would say no, while those that have enough for display might still load it automatically, but not delay processing in order to load it?
|
can you give this a look please? @moalsad |
I agree that dicomImageLoader + dcmjs should be placed somewhere else more specific to the (JSON) data source. |
|
Replaced by #3935 |
Context
The goal of this PR is to allow the MetadataProvider to read the metadata of dicom images directly without making an extra WADO-RS Metadata request to the PACS.
Internaly it uses dataSetCacheManager of dicomImageLoader to access the DICOM dataset. Then dcmjs parse the dataset and add the metadata to the MetadataStore.
This PR allow the DicomJSON source to provide only the image UID + instance number without the need of putting all the image metadata into the final .json.
You can enable or disable this behavior with the variable automaticallyLoadDicomMetadata in the config file.
Changes & Results
Testing
This can be tested using a regular JSON DataSource where only SeriesInstanceUID, StudyInstanceUID, SOPClassUID and InstanceNumber must be present.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment