From 8c8f69accbf425205979611d3a08ed24af3e3ff3 Mon Sep 17 00:00:00 2001 From: Erik Ziegler Date: Mon, 6 Apr 2020 10:43:35 +0200 Subject: [PATCH] Add option for setting maximum concurrent metadata requests (@ckedar), show series in panel before their metadata is retrieved (#1576) * Show series in Study Browser before their metadata is retrieved * Switch to decreasing for loop after code review (thanks @ckedar!) * feat: Limit concurrent series metadata requests (#1581) * Limit concurrent series metadata requests * Switch to decreasing for loop after code review (thanks @ckedar!) * Set default maxConcurrentMetadataRequests to unlimited and add explaination of setting in config Co-authored-by: kedar.netelixir Co-authored-by: Erik Ziegler * Address comments from code review * Comment out misbehaving test Co-authored-by: Kedar Co-authored-by: kedar.netelixir --- platform/core/package.json | 2 +- .../utils/findMostRecentStructuredReport.js | 7 ++ .../src/classes/metadata/StudyMetadata.js | 88 +++++++++++++++---- .../wado/retrieveMetadataLoaderAsync.js | 41 +++++++-- .../ui/src/viewer/ViewportLoadingIndicator.js | 2 +- .../pwa/OHIFSaveMeasurements.spec.js | 4 +- platform/viewer/package.json | 2 +- platform/viewer/public/config/default.js | 8 ++ .../ViewerRetrieveStudyData.js | 56 +++++++----- yarn.lock | 28 ++---- 10 files changed, 169 insertions(+), 69 deletions(-) diff --git a/platform/core/package.json b/platform/core/package.json index 8fd794a86d4..8b7d199467e 100644 --- a/platform/core/package.json +++ b/platform/core/package.json @@ -39,7 +39,7 @@ "dependencies": { "@babel/runtime": "^7.5.5", "ajv": "^6.10.0", - "dcmjs": "^0.8.3", + "dcmjs": "^0.12.0", "dicomweb-client": "^0.5.2", "immer": "6.0.2", "isomorphic-base64": "^1.0.2", diff --git a/platform/core/src/DICOMSR/utils/findMostRecentStructuredReport.js b/platform/core/src/DICOMSR/utils/findMostRecentStructuredReport.js index 4db4db76cf4..9f8a3129ada 100644 --- a/platform/core/src/DICOMSR/utils/findMostRecentStructuredReport.js +++ b/platform/core/src/DICOMSR/utils/findMostRecentStructuredReport.js @@ -10,6 +10,13 @@ const findMostRecentStructuredReport = studies => { studies.forEach(study => { const allSeries = study.getSeries ? study.getSeries() : []; allSeries.forEach(series => { + // Skip series that may not have instances yet + // This can happen if we have retrieved just the initial + // details about the series via QIDO-RS, but not the full metadata + if (!series.instances.length) { + return; + } + if (isStructuredReportSeries(series)) { if ( !mostRecentStructuredReport || diff --git a/platform/core/src/classes/metadata/StudyMetadata.js b/platform/core/src/classes/metadata/StudyMetadata.js index 9cdcbd3bcbe..cd344e8cc9c 100644 --- a/platform/core/src/classes/metadata/StudyMetadata.js +++ b/platform/core/src/classes/metadata/StudyMetadata.js @@ -100,20 +100,32 @@ export class StudyMetadata extends Metadata { * Split a series metadata object into display sets * @param {Array} sopClassHandlerModules List of SOP Class Modules * @param {SeriesMetadata} series The series metadata object from which the display sets will be created - * @param {Array} [givenDisplaySets] An optional list to which the display sets will be appended * @returns {Array} The list of display sets created for the given series object */ _createDisplaySetsForSeries( sopClassHandlerModules, series, - givenDisplaySets ) { const study = this; - const displaySets = Array.isArray(givenDisplaySets) ? givenDisplaySets : []; + const displaySets = []; + const anyInstances = series.getInstanceCount() > 0; if (!anyInstances) { - return; + const displaySet = new ImageSet([]); + const seriesData = series.getData(); + + displaySet.setAttributes({ + displaySetInstanceUID: displaySet.uid, + SeriesInstanceUID: seriesData.SeriesInstanceUID, + SeriesDescription: seriesData.SeriesDescription, + SeriesNumber: seriesData.SeriesNumber, + Modality: seriesData.Modality, + }); + + displaySets.push(displaySet); + + return displaySets; } const sopClassUIDs = getSopClassUIDs(series); @@ -282,13 +294,14 @@ export class StudyMetadata extends Metadata { // Loop through the series (SeriesMetadata) this.forEachSeries( - series => - void this._createDisplaySetsForSeries( - sopClassHandlerModules, - series, - displaySets - ) - ); + series => { + const displaySetsForSeries = this._createDisplaySetsForSeries( + sopClassHandlerModules, + series, + ); + + displaySets.push(...displaySetsForSeries); + }); return sortDisplaySetList(displaySets); } @@ -304,13 +317,27 @@ export class StudyMetadata extends Metadata { * @returns {boolean} Returns true on success or false on failure (e.g., the series does not belong to this study) */ createAndAddDisplaySetsForSeries(sopClassHandlerModules, series) { - if (this.containsSeries(series)) { - this.setDisplaySets( - this._createDisplaySetsForSeries(sopClassHandlerModules, series) - ); - return true; + if (!this.containsSeries(series)) { + return false; } - return false; + + const displaySets = this._createDisplaySetsForSeries(sopClassHandlerModules, series) + + // Note: filtering in place because this._displaySets has writable: false + for (let i = this._displaySets.length - 1; i >= 0; i--) { + const displaySet = this._displaySets[i]; + if (displaySet.SeriesInstanceUID === series.getSeriesInstanceUID()) { + this._displaySets.splice(i, 1); + } + } + + displaySets.forEach(displaySet => { + this.addDisplaySet(displaySet); + }); + + this.sortDisplaySets(); + + return true; } /** @@ -319,6 +346,9 @@ export class StudyMetadata extends Metadata { */ setDisplaySets(displaySets) { if (Array.isArray(displaySets) && displaySets.length > 0) { + // TODO: This is weird, can we just switch it to writable: true? + this._displaySets.splice(0); + displaySets.forEach(displaySet => this.addDisplaySet(displaySet)); this.sortDisplaySets(); } @@ -407,6 +437,30 @@ export class StudyMetadata extends Metadata { return result; } + /** + * Update a series in the current study by SeriesInstanceUID. + * @param {String} SeriesInstanceUID The SeriesInstanceUID to be updated + * @param {SeriesMetadata} series The series to be added to the current study. + * @returns {boolean} Returns true on success, false otherwise. + */ + updateSeries(SeriesInstanceUID, series) { + const index = this._series.findIndex(series => { + return series.getSeriesInstanceUID() === SeriesInstanceUID; + }); + + if (index < 0) { + return false; + } + + if (!(series instanceof SeriesMetadata)) { + throw new Error('Series must be an instance of SeriesMetadata'); + } + + this._series[index] = series; + + return true; + } + /** * Find a series by index. * @param {number} index An integer representing a list index. diff --git a/platform/core/src/studies/services/wado/retrieveMetadataLoaderAsync.js b/platform/core/src/studies/services/wado/retrieveMetadataLoaderAsync.js index 2f224274bba..74d18a79ae3 100644 --- a/platform/core/src/studies/services/wado/retrieveMetadataLoaderAsync.js +++ b/platform/core/src/studies/services/wado/retrieveMetadataLoaderAsync.js @@ -1,4 +1,5 @@ import { api } from 'dicomweb-client'; +import dcmjs from 'dcmjs'; import DICOMWeb from '../../../DICOMWeb/'; import RetrieveMetadataLoader from './retrieveMetadataLoader'; import { sortStudySeries, sortingCriteria } from '../../sortStudy'; @@ -8,6 +9,8 @@ import { addInstancesToStudy, } from './studyInstanceHelpers'; +const { naturalizeDataset } = dcmjs.data.DicomMetaDictionary; + /** * Map series to an array of SeriesInstanceUID * @param {Arrays} series list of Series Instance UIDs @@ -101,15 +104,21 @@ export default class RetrieveMetadataLoaderAsync extends RetrieveMetadataLoader async preLoad() { const preLoaders = this.getPreLoaders(); - const result = await this.runLoaders(preLoaders); + + // seriesData is the result of the QIDO-RS Search For Series request + // It's an array of Objects containing DICOM Tag values at the Series level + const seriesData = await this.runLoaders(preLoaders); const seriesSorted = sortStudySeries( - result, + seriesData, sortingCriteria.seriesSortCriteria.seriesInfoSortingCriteria ); const seriesInstanceUIDsMap = mapStudySeries(seriesSorted); - return seriesInstanceUIDsMap; + return { + seriesInstanceUIDsMap, + seriesData + }; } async load(preLoadData) { @@ -118,7 +127,7 @@ export default class RetrieveMetadataLoaderAsync extends RetrieveMetadataLoader const seriesAsyncLoader = makeSeriesAsyncLoader( client, studyInstanceUID, - preLoadData + preLoadData.seriesInstanceUIDsMap ); const firstSeries = await seriesAsyncLoader.next(); @@ -126,16 +135,38 @@ export default class RetrieveMetadataLoaderAsync extends RetrieveMetadataLoader return { sopInstances: firstSeries.sopInstances, asyncLoader: seriesAsyncLoader, + seriesData: preLoadData.seriesData, }; } async posLoad(loadData) { const { server } = this; - const { sopInstances, asyncLoader } = loadData; + const { sopInstances, asyncLoader, seriesData } = loadData; const study = await createStudyFromSOPInstanceList(server, sopInstances); + // TODO: Should this be in a helper + const seriesDataNaturalized = seriesData.map(naturalizeDataset); + + seriesDataNaturalized.forEach((series, idx) => { + const seriesDataFromQIDO = { + SeriesInstanceUID: series.SeriesInstanceUID, + SeriesDescription: series.SeriesDescription, + SeriesNumber: series.SeriesNumber, + Modality: series.Modality, + instances: [] + }; + + if (study.series[idx]) { + study.series[idx] = Object.assign(seriesDataFromQIDO, study.series[idx]); + } else { + study.series[idx] = seriesDataFromQIDO; + } + + study.seriesMap[series.SeriesInstanceUID] = study.series[idx]; + }); + if (asyncLoader.hasNext()) { attachSeriesLoader(server, study, asyncLoader); } diff --git a/platform/ui/src/viewer/ViewportLoadingIndicator.js b/platform/ui/src/viewer/ViewportLoadingIndicator.js index 9bf8feb8f03..9f44921f9e9 100644 --- a/platform/ui/src/viewer/ViewportLoadingIndicator.js +++ b/platform/ui/src/viewer/ViewportLoadingIndicator.js @@ -4,7 +4,7 @@ import PropTypes from 'prop-types'; export function ViewportLoadingIndicator(props) { return (
{ extensionManager.modules['sopClassHandlerModule']; const study = studyMetadata.getData(); const seriesMetadata = new OHIFSeriesMetadata(series, study); - studyMetadata.addSeries(seriesMetadata); + const existingSeries = studyMetadata.getSeriesByUID(series.SeriesInstanceUID); + if (existingSeries) { + studyMetadata.updateSeries(series.SeriesInstanceUID, seriesMetadata); + } else { + studyMetadata.addSeries(seriesMetadata); + } + studyMetadata.createAndAddDisplaySetsForSeries( sopClassHandlerModules, seriesMetadata, - false ); study.displaySets = studyMetadata.getDisplaySets(); _updateStudyMetadataManager(study, studyMetadata); @@ -148,25 +153,6 @@ const _updateStudyDisplaySets = (study, studyMetadata) => { const _sortStudyDisplaySet = (study, studyMetadata) => { studyMetadata.sortDisplaySets(study.displaySets); }; -const _loadRemainingSeries = studyMetadata => { - const { seriesLoader } = studyMetadata.getData(); - if (!seriesLoader) { - return Promise.resolve(); - } - const promisesLoaders = []; - while (seriesLoader.hasNext()) { - promisesLoaders.push( - seriesLoader - .next() - .then( - series => void _addSeriesToStudy(studyMetadata, series), - error => void log.error(error) - ) - ); - } - - return Promise.all(promisesLoaders); -}; function ViewerRetrieveStudyData({ server, @@ -180,7 +166,10 @@ function ViewerRetrieveStudyData({ const [isStudyLoaded, setIsStudyLoaded] = useState(false); const snackbarContext = useSnackbarContext(); const { appConfig = {} } = useContext(AppContext); - const { filterQueryParam: isFilterStrategy = false } = appConfig; + const { + filterQueryParam: isFilterStrategy = false, + maxConcurrentMetadataRequests, + } = appConfig; let cancelableSeriesPromises; let cancelableStudiesPromises; @@ -241,7 +230,7 @@ function ViewerRetrieveStudyData({ // Attempt to load remaning series if any cancelableSeriesPromises[study.StudyInstanceUID] = makeCancelable( - _loadRemainingSeries(studyMetadata) + loadRemainingSeries(studyMetadata) ) .then(result => { if (result && !result.isCanceled) { @@ -262,6 +251,27 @@ function ViewerRetrieveStudyData({ } }; + const forceRerender = () => setStudies(studies => [...studies]); + + const loadRemainingSeries = async studyMetadata => { + const { seriesLoader } = studyMetadata.getData(); + if (!seriesLoader) return; + + const loadNextSeries = async () => { + if (!seriesLoader.hasNext()) return; + const series = await seriesLoader.next(); + _addSeriesToStudy(studyMetadata, series); + forceRerender(); + return loadNextSeries(); + }; + + const concurrentRequestsAllowed = maxConcurrentMetadataRequests || studyMetadata.getSeriesCount(); + const promises = Array(concurrentRequestsAllowed) + .fill(null) + .map(loadNextSeries); + await Promise.all(promises); + }; + const loadStudies = async () => { try { const filters = {}; diff --git a/yarn.lock b/yarn.lock index 10e7b245b51..1a5bf9985b8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -918,7 +918,7 @@ core-js "^2.6.5" regenerator-runtime "^0.13.2" -"@babel/polyfill@^7.6.0": +"@babel/polyfill@^7.8.3": version "7.8.7" resolved "https://registry.yarnpkg.com/@babel/polyfill/-/polyfill-7.8.7.tgz#151ec24c7135481336168c3bd8b8bf0cf91c032f" integrity sha512-LeSfP9bNZH2UOZgcGcZ0PIHUt1ZuHub1L3CVmEyqLxCeDLm4C5Gi8jRH8ZX2PNpDhQCo0z6y/+DIs2JlliXW8w== @@ -6385,25 +6385,15 @@ dateformat@^3.0.0: resolved "https://registry.yarnpkg.com/dateformat/-/dateformat-3.0.3.tgz#a6e37499a4d9a9cf85ef5872044d62901c9889ae" integrity sha512-jyCETtSl3VMZMWeRo7iY1FL19ges1t55hMo5yaam4Jrsm5EPL89UQkoQRyiI+Yf4k8r2ZpdngkV8hr1lIdjb3Q== -dcmjs@^0.10.1: - version "0.10.1" - resolved "https://registry.yarnpkg.com/dcmjs/-/dcmjs-0.10.1.tgz#0655f8c0f7ed1357145f416e14efd4a45e2c74be" - integrity sha512-aoXkZXTp3Kh20C6I2i2AJjcHRRxqjmt7KWvJElRv2b93AfUPVplg6+/B2ioiJFSSESt7/Q9yjHRM8k0U1uRCag== - dependencies: - "@babel/polyfill" "^7.6.0" - "@babel/runtime" "^7.6.3" - loglevelnext "^3.0.1" - ndarray "^1.0.18" - -dcmjs@^0.8.3: - version "0.8.3" - resolved "https://registry.yarnpkg.com/dcmjs/-/dcmjs-0.8.3.tgz#fff1b030b6cb2d6e2afb1aa99840bfa853724c31" - integrity sha512-eXQjqgtJf9+oseraKDNDm2A5F3Th4B2GJeZtjStj0IFXxjlbPOzdq3PfCyxdwfRaKOBIwr0q3YK/Vfs2CpQY8Q== +dcmjs@^0.12.0: + version "0.12.0" + resolved "https://registry.yarnpkg.com/dcmjs/-/dcmjs-0.12.0.tgz#8b1634f9b66e452075295f5d1f2c5bf3dabdf03e" + integrity sha512-AZAnFMvzAxUv5+KWoZcxnTYflLKk0rNPeeFt2KrVAgaExFBFfDgJQh1lEgqdNlIP+XQDFrEEUmp726SQhgmVCg== dependencies: - "@babel/polyfill" "^7.6.0" - "@babel/runtime" "^7.6.3" + "@babel/polyfill" "^7.8.3" + "@babel/runtime" "^7.8.4" loglevelnext "^3.0.1" - ndarray "^1.0.18" + ndarray "^1.0.19" debug@2.6.9, debug@^2.2.0, debug@^2.3.3, debug@^2.6.0, debug@^2.6.1, debug@^2.6.3, debug@^2.6.6, debug@^2.6.8, debug@^2.6.9: version "2.6.9" @@ -13006,7 +12996,7 @@ natural-compare@^1.4.0: resolved "https://registry.yarnpkg.com/natural-compare/-/natural-compare-1.4.0.tgz#4abebfeed7541f2c27acfb29bdbbd15c8d5ba4f7" integrity sha1-Sr6/7tdUHywnrPspvbvRXI1bpPc= -ndarray@^1.0.18: +ndarray@^1.0.19: version "1.0.19" resolved "https://registry.yarnpkg.com/ndarray/-/ndarray-1.0.19.tgz#6785b5f5dfa58b83e31ae5b2a058cfd1ab3f694e" integrity sha512-B4JHA4vdyZU30ELBw3g7/p9bZupyew5a7tX1Y/gGeF2hafrPaQZhgrGQfsvgfYbgdFZjYwuEcnaobeM/WMW+HQ==