fix: use array of mediatypes and transferSyntax when calling a get xhrRequest for image loading #477
Conversation
…rRequest for image loading
❌ Deploy Preview for cornerstone-wado-image-loader failed.
|
|
@sedghi can you please review this? thanks! |
|
@GitanjaliChhetri leaving this to you, but it should be ready, and just merge it |
|
|
||
| assertMediaTypeIsValid(mediaType); | ||
|
|
||
| let fieldValue = `multipart/related; type="${mediaType}"`; |
There was a problem hiding this comment.
this should not have type="${mediaType}" it should be type=${mediaType}. Double checking with @wayfarer3130
There was a problem hiding this comment.
Hello @sedghi,
Referring to azure dicom docs, type shoud be in double quotes. Otherwise, 406 response status will be returned.
There was a problem hiding this comment.
According to the DICOM standard it depends on the version, and according to the original RFC for multipart related, the quotes are NOT permitted, although every example in the document shows them. Including them drops the speed by approximately a half because it requires a full extra CORS request, assuming the CORS is used. Not including htem breaks Azure, so the DICOM committee has agreed to a statement that clients need to have an option to fallback from one version to the other. For cornerstone, that is determined by a configuration parameter on the specific back end request, so that you can include them when configured to do so, and omit them otherwise.
|
so if I understand this, this will create a giant accept header something like
is this true? I need another set of eyes to review this since this can be a bottleneck in the performance |
|
This might be related to this as well cornerstonejs/cornerstone3D#232 |
|
@sedghi If I understand correctly, it's OK to have one or more comma-separated mediatypes. And origin server will ignore all mediatypes that are not valid or not supported.
|
|
For the retrieve request, rather than generating a long query URL, you can retrieve just the specific request type you want by using the AvailableTransferSyntaxUID to map to a single accept header. If the back end supports it, you can avoid decoding the multipart/related by requesting single part. The single part request implicitly doesn't use the quotes, so it has no CORS issues. |
Re IDC OHIF/Viewers#2934
Related to OHIF/Viewers#2934 (comment)
This implements the get image in xhrRequuests using an array of mediaTypes and TransferSyntax (analogous to SLIM viewer https://github.com/herrmannlab/dicom-microscopy-viewer/blob/83739f1ba11840df7f4487cdf8015b7450ee419d/src/pyramid.js#L436-L491 ), instead of always requesting as this default:
Requesting images with type=application/octet-stream; transfer-syntax=* as default can create several issues. For example as in OHIF/Viewers#2934, the 8 bit colored jpeg will be decoded server side (including color corrections), but then the returned mime and transfer syntax is 1.2.840.10008.1.2 instead of 1.2.840.10008.1.2.4.50 (because the array is already decoded by the server), but at this point cornerstoneWADOLoader willl apply again color corrections (which is based on the photometric Interpretation of the image frame).
With this PR, type=application/octet-stream; transfer-syntax=* will have lower priority and used only as last resource.
For example, in the case of 8 bit colored jpeg, the images will be donwloaded in jpeg format and always decoded (and color transformations applied) client-side.