-
Notifications
You must be signed in to change notification settings - Fork 47
fix: instance transfer syntax UID is required for decoding images #108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
dbbf4c8
fcd997d
8d2cc15
2efe1ae
66c1aee
2973d67
b74f11e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| version: "3" | ||
| services: | ||
| ldap: | ||
| image: dcm4che/slapd-dcm4chee:2.4.44-13.3 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,4 @@ | ||
| import { multipartEncode, multipartDecode } from './message.js'; | ||
|
|
||
|
|
||
| import { multipartEncode, multipartDecode, addHeaders } from './message.js'; | ||
|
|
||
| function isObject(obj) { | ||
| return typeof obj === 'object' && obj !== null; | ||
|
|
@@ -225,7 +223,7 @@ class DICOMwebClient { | |
| let requestInstance = request.instance ? request.instance : new XMLHttpRequest(); | ||
|
|
||
| requestInstance.open(method, url, true); | ||
| if ('responseType' in request) { | ||
| if (request.responseType) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excludes null/undefined which was causing issues. |
||
| requestInstance.responseType = request.responseType; | ||
| } | ||
|
|
||
|
|
@@ -256,12 +254,16 @@ class DICOMwebClient { | |
| requestInstance.onreadystatechange = () => { | ||
| if (requestInstance.readyState === 4) { | ||
| if (requestInstance.status === 200) { | ||
| const contentType = requestInstance.getResponseHeader('Content-Type'); | ||
| const contentType = requestInstance.getResponseHeader( | ||
| 'Content-Type', | ||
| ); | ||
| const headers = requestInstance.getAllResponseHeaders(); | ||
| // Automatically distinguishes between multipart and singlepart in an array buffer, and | ||
| // converts them into a consistent type. | ||
| if (contentType && contentType.indexOf('multipart') !== -1) { | ||
| resolve(multipartDecode(requestInstance.response)); | ||
| } else if (requestInstance.responseType === 'arraybuffer') { | ||
| addHeaders(requestInstance.response, headers); | ||
| resolve([requestInstance.response]); | ||
| } else { | ||
| resolve(requestInstance.response); | ||
|
|
@@ -295,10 +297,8 @@ class DICOMwebClient { | |
| }; | ||
|
|
||
| // Event triggered while download progresses | ||
| if ('progressCallback' in request) { | ||
| if (typeof request.progressCallback === 'function') { | ||
| requestInstance.onprogress = request.progressCallback; | ||
| } | ||
| if (typeof request.progressCallback === 'function') { | ||
| requestInstance.onprogress = request.progressCallback; | ||
| } | ||
|
|
||
| if (requestHooks && areValidRequestHooks(requestHooks)) { | ||
|
|
@@ -311,13 +311,11 @@ class DICOMwebClient { | |
| } | ||
|
|
||
| // Add withCredentials to request if needed | ||
| if ('withCredentials' in request) { | ||
| if (request.withCredentials) { | ||
| requestInstance.withCredentials = true; | ||
| } | ||
| if (request.withCredentials) { | ||
| requestInstance.withCredentials = true; | ||
| } | ||
|
|
||
| if ('data' in request) { | ||
| if (request.data) { | ||
| requestInstance.send(request.data); | ||
| } else { | ||
| requestInstance.send(); | ||
|
|
@@ -594,6 +592,7 @@ class DICOMwebClient { | |
| 'image/gif', | ||
| 'image/png', | ||
| 'image/jp2', | ||
| 'image/*', | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. image/* is a valid type, and I was trying to get the generic retrieve working, but it doesn't seem to be allowed for dcm4chee |
||
| ]; | ||
| } else { | ||
| supportedMediaTypes = { | ||
|
|
@@ -608,6 +607,7 @@ class DICOMwebClient { | |
| '1.2.840.10008.1.2.4.91': ['image/jp2'], | ||
| '1.2.840.10008.1.2.4.92': ['image/jpx'], | ||
| '1.2.840.10008.1.2.4.93': ['image/jpx'], | ||
| '*': ['image/*'], | ||
| }; | ||
|
|
||
| if (byteRange) { | ||
|
|
@@ -961,7 +961,7 @@ class DICOMwebClient { | |
| }); | ||
|
|
||
| if( !fieldValueParts.length ) { | ||
| throw new Error(`No acceptable media types found among ${JSON.stringify(mediaTypes)}`); | ||
| throw new Error(`No acceptable media types found among ${JSON.stringify(mediaTypes)} testing against ${JSON.stringify(acceptableMediaTypes)}`); | ||
| } | ||
|
|
||
| return fieldValueParts.join(', '); | ||
|
|
@@ -1227,7 +1227,7 @@ class DICOMwebClient { | |
| debugLog(`retrieve metadata of instance ${options.sopInstanceUID}`); | ||
| const url = `${this.wadoURL}/studies/${options.studyInstanceUID}/series/${options.seriesInstanceUID}/instances/${options.sopInstanceUID}/metadata`; | ||
|
|
||
| const request = getRequestOptions(options.request) | ||
| const request = getRequestOptions(options.request); | ||
| return this._httpGetApplicationJson(url, {}, request); | ||
| } | ||
|
|
||
|
|
@@ -1276,6 +1276,7 @@ class DICOMwebClient { | |
| const { mediaTypes } = options; | ||
|
|
||
| const request = getRequestOptions(options.request) | ||
| request.responseType = 'arraybuffer'; | ||
|
|
||
| if (!mediaTypes) { | ||
| return this._httpGetMultipartApplicationOctetStream( | ||
|
|
@@ -1317,7 +1318,6 @@ class DICOMwebClient { | |
| supportedMediaTypes, | ||
| ), | ||
| }; | ||
| request.responseType = 'arraybuffer'; | ||
| return this._httpGet(url, headers, request); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,7 @@ function identifyBoundary(header) { | |
| const parts = header.split('\r\n'); | ||
|
|
||
| for (let i = 0; i < parts.length; i++) { | ||
| if (parts[i].substr(0, 2) === '--') { | ||
| if (parts[i].substring(0, 2) === '--') { | ||
| return parts[i]; | ||
| } | ||
| } | ||
|
|
@@ -179,16 +179,72 @@ function multipartEncode( | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Splits the header string into parts and extracts the simple contentType | ||
| * and transferSyntaxUID, assigning them, plus the headers map into the destination object. | ||
| * | ||
| * @param {*} destination | ||
| * @param {string} headerString | ||
| */ | ||
| function addHeaders(destination, headerString) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allows for retrieving the actual transfer syntax used in the image encoding. This is allowed by Part 18, but unfortunately not required there. |
||
| if (!headerString) { | ||
| return; | ||
| } | ||
| const headerLines = headerString.split('\r\n').filter(Boolean); | ||
| const headers = new Map(); | ||
| let transferSyntaxUID = null, | ||
| contentType = null; | ||
|
|
||
| for (const line of headerLines) { | ||
| const colon = line.indexOf(':'); | ||
| if (colon === -1) { | ||
| continue; | ||
| } | ||
| const name = line.substring(0, colon).toLowerCase(); | ||
| const value = line.substring(colon + 1).trim(); | ||
| if (headers.has(name)) { | ||
| headers.get(name).push(value); | ||
| } else { | ||
| headers.set(name, [value]); | ||
| } | ||
| if (name === 'content-type') { | ||
| const endSimpleType = value.indexOf(';'); | ||
| contentType = value.substring( | ||
| 0, | ||
| endSimpleType === -1 ? value.length : endSimpleType, | ||
| ); | ||
| const transferSyntaxStart = value.indexOf('transfer-syntax='); | ||
| if (transferSyntaxStart !== -1) { | ||
| const endTsuid = value.indexOf(';', transferSyntaxStart); | ||
| transferSyntaxUID = value.substring( | ||
| transferSyntaxStart + 16, | ||
| endTsuid === -1 ? value.length : endTsuid, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Object.defineProperty(destination, 'headers', { value: headers }); | ||
| Object.defineProperty(destination, 'contentType', { value: contentType }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having at least the contentType allows one to figure out what the encoding is close enough to allow testing for which decoder to use. |
||
| Object.defineProperty(destination, 'transferSyntaxUID', { | ||
| value: transferSyntaxUID, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Decode a Multipart encoded ArrayBuffer and return the components as an Array. | ||
| * | ||
| * @param {ArrayBuffer} response Data encoded as a 'multipart/related' message | ||
| * @returns {Array} The content | ||
| * @returns {Uint8Array[]} The content as an array of Uint8Array | ||
| * Each item shall have a contentType value, and a transferSyntaxUID if available, | ||
| * as well as the headers Map. See parseHeaders for output. | ||
| * | ||
| */ | ||
| function multipartDecode(response) { | ||
| // Use the raw data if it is provided in an appropriate format | ||
| const message = ArrayBuffer.isView(response) ? response : new Uint8Array(response); | ||
|
|
||
| const message = ArrayBuffer.isView(response) | ||
| ? response | ||
| : new Uint8Array(response); | ||
| /* Set a maximum length to search for the header boundaries, otherwise | ||
| findToken can run for a long time | ||
| */ | ||
|
|
@@ -211,6 +267,8 @@ function multipartDecode(response) { | |
| const boundaryLength = boundary.length; | ||
| const components = []; | ||
|
|
||
| const headers = header.substring(boundary.length + 2); | ||
|
|
||
| let offset = boundaryLength; | ||
|
|
||
| // Loop until we cannot find any more boundaries | ||
|
|
@@ -240,6 +298,8 @@ function multipartDecode(response) { | |
| // Extract data from response message, excluding "\r\n" | ||
| const spacingLength = 2; | ||
| const data = response.slice(offset, boundaryIndex - spacingLength); | ||
| // TODO - extract header data on a per frame basis. | ||
| addHeaders(data, headers); | ||
|
|
||
| // Add the data to the array of results | ||
| components.push(data); | ||
|
|
@@ -261,4 +321,5 @@ export { | |
| multipartEncode, | ||
| multipartDecode, | ||
| guid, | ||
| addHeaders, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ let dwc = new DICOMwebClient.api.DICOMwebClient({ | |
| url: 'http://localhost:8008/dcm4chee-arc/aets/DCM4CHEE/rs', | ||
| retrieveRendered: false, | ||
| }); | ||
|
|
||
| describe('dicomweb.api.DICOMwebClient', function() { | ||
| // | ||
| // Note: you can add the following for debugging tests locally | ||
|
|
@@ -124,10 +125,17 @@ describe('dicomweb.api.DICOMwebClient', function() { | |
| sopInstanceUID: | ||
| '1.3.6.1.4.1.14519.5.2.1.2744.7002.325971588264730726076978589153', | ||
| frameNumbers: '1', | ||
| // The next line should work, but the server side is broken | ||
| // mediaTypes: [ {mediaType: 'image/*' }], | ||
| }; | ||
|
|
||
| const frames = dwc.retrieveInstance(options); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This didn't even use the await, so it was essentially an empty test, without checking if the response was valid. |
||
| }); | ||
| const frames = await dwc.retrieveInstanceFrames(options); | ||
| expect(frames instanceof Array).toBe(true); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now the test at least checks that the response has an image frame, and for the given server, tests for uncompressed. |
||
| expect(frames.length).toBe(1); | ||
| expect(frames[0].contentType).toBe("application/octet-stream"); | ||
| // The next line is the correct value for servers supporting image/* | ||
| //expect(frames[0].transferSyntaxUID).toBe('1.2.3'); | ||
| }, 15000); | ||
|
|
||
| it('should retrieve a single instance', async function() { | ||
| // from sample.dcm | ||
|
|
@@ -173,10 +181,9 @@ describe('dicomweb.api.DICOMwebClient', function() { | |
| const options = { | ||
| studyInstanceUID: '999.999.3859744', | ||
| seriesInstanceUID: '999.999.94827453', | ||
| sopInstanceUID: '999.999.133.1996.1.1800.1.6.25', | ||
| }; | ||
|
|
||
| const metadata = await dwc.retrieveInstanceMetadata(options); | ||
| const metadata = await dwc.retrieveSeriesMetadata(options); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since instance metadata is tested elsewhere, this tests just makes sure series metadata also works. |
||
|
|
||
| // TODO: Check why metadata is an array of objects, not just an object | ||
| const bulkDataOptions = { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some fixes for the retrieveInstanceFrames, needed since I added an actual integration test for it, and that wasn't running until I added the fixes.