feat: Generate all thumbnails#125
Conversation
|
|
||
| _resolveBulkDataPath(studyUID, seriesUID, bulkDataURI, frameNumber) { | ||
| const frameSuffix = frameNumber ? `/${frameNumber}` : ''; | ||
| if (/^https?:\/\//i.test(bulkDataURI)) { |
There was a problem hiding this comment.
This test regex seems to be done a lot. Is it worth centralizing somewhere?
jbocce
left a comment
jbocce
left a comment
There was a problem hiding this comment.
Here are some requests from Claude. I think the first one is the most important to address.
- HttpDicomWebReader._resolveBulkDataPath ignores instanceUID for instance-relative ./frames URIs (HttpDicomWebReader.mjs:55-65). The file reader distinguishes ./instances//frames (series-relative) from ./frames (instance-relative, needs instanceUID); the HTTP reader treats every ./-prefixed URI as series-relative. A server emitting BulkDataURI: "./frames" will resolve
to studies//series//frames/ instead of studies//series//instances//frames/ — 404 or wrong frame. Match the file reader's rule. - Redundant double upload in studiesMain.mjs:31-34: uploadDeploy is awaited twice in a row. This appears pre-existing (also in master) and is not fixed by this PR — worth fixing while you're in the file.
- thumbnailMain.mjs:831 HTTP-output guard fires after the more specific check: (outputDicomdir || dicomdir).startsWith('http') rejects an HTTP outputDicomdir, but the message "must be a file path" is misleading since s3:// is also accepted. Reword to "must be a local path or s3:// URI" and place it before the http-input check for clarity.
- No Accept headers on QIDO/WADO-RS fetches (HttpDicomWebReader._fetch). DICOMweb-strict servers may return 406 or wrong representations. Set Accept: application/dicom+json for QIDO paths and multipart/related; type="application/octet-stream" (or the appropriate transfer-syntax variant) for bulk frames.
- HTTP path has no retry / timeout / concurrency control. --all-thumbnails against a remote DICOMweb peer issues serial fetches with no backoff — slow on success, blocking on a single hang. Consider an AbortSignal with AbortSignal.timeout(...) per request, and Promise.all/p-limit for instance-level work.
|
@rleisti - I hvae this change to allow uploading the thumbnails in bulk to S3 or creating or re-creating them locally. It was for some OHIF test work, but I don't have a reviewer on the OHIF side who is familiar with this. Could you review the changes? I'd like to address the resolve bulkdata path as a separate issue, since the changes that were being directly suggested don't quite work without doing things in a different way. |
rleisti
left a comment
rleisti
left a comment
There was a problem hiding this comment.
Added some initial comments: I have not gone through the entire review yet.
| const shouldExclude = Array.from(excludePatterns).some( | ||
| pattern => entryRelativePath.indexOf(pattern) !== -1 | ||
| ); | ||
| const shouldInclude = |
There was a problem hiding this comment.
In studyMainSingle, options.include is set to ['thumbnail']. If collectFiles is called for the directory studies/<UID> then it seems this code would skip any sub-directories that not include the substring 'thumbnail' (like 'series'), cause it to skip series thumbnails.
| await commonMain(this, 'root', uploadOptions, uploadDeploy.bind(null, studyDirectory)); | ||
| console.log('Storing studyUID', studyUID); | ||
| await commonMain(this, 'root', options, uploadDeploy.bind(null, studyDirectory)); | ||
| await commonMain(this, 'root', uploadOptions, uploadDeploy.bind(null, studyDirectory)); |
There was a problem hiding this comment.
commonMain is being called twice with identical arguments: is this intended?
3 participants
Adds ability to generate thumbnails for selected queries from DICOMweb or file system.