Skip to content
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.

Sign up for GitHub

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

fix: get direct url pixel data should be optional for video #4152

Merged
merged 4 commits into from
Jun 5, 2024
Merged

fix: get direct url pixel data should be optional for video #4152

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6aeaba4
fix: 🐛 get direct url pixel data should be optional for video
igoroctaviano May 21, 2024
a710efc
Add CR changes
igoroctaviano May 23, 2024
be52b14
Refactor functions
igoroctaviano May 23, 2024
a98d8a6
Merge branch 'master' of github.com:ohif/Viewers into fix/get-direct-…
igoroctaviano May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 23 additions & 23 deletions extensions/default/src/utils/getDirectURL.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,31 +29,31 @@ const getDirectURL = (config, params) => {
}

const value = instance[tag];
if (!value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what value there is in increasing the indent level. The recommended approach for handling things is to return quickly at the top of the function, rather than nesting a function inside, as was done originally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see what you are doing, github made it non obvious that there was more code - cut things off below. What I think should be done is to separate this out into two functions, one of which constructs a URL, the other of which uses the value - and export both of them so that they can be used independently, or combined. Then getDirectURL becomes:

const value = instance[tag];
return getBulkdataValue(value) || createRenderedRetrieve(instance)

Then, you can leave the return immediate as is inside getBulkdataValue.
Also, you might add a note that it is incorrect to not have a value here, or rather it means that the value itself is supposed to be empty, but that some DICOMweb systems incorrectly return empty tag values.
Then, we should handle both:

TAG: null
and
TAG: {}

I'd like to see a unit test to check a couple of the various cases, since I think this logic is getting complex enough to warrant that.  

Sorry for the extra work, but by the time things get this complex, splitting it up makes it easier to figure out what one should expect to receive back.

return undefined;
}
if (value) {
if (value.DirectRetrieveURL) {
return value.DirectRetrieveURL;
}

if (value.DirectRetrieveURL) {
return value.DirectRetrieveURL;
}
if (value.InlineBinary) {
const blob = utils.b64toBlob(value.InlineBinary, defaultType);
value.DirectRetrieveURL = URL.createObjectURL(blob);
return value.DirectRetrieveURL;
}
if (!singlepart || (singlepart !== true && singlepart.indexOf(fetchPart) === -1)) {
if (value.retrieveBulkData) {
// Try the specified retrieve type.
const options = {
mediaType: defaultType,
};
return value.retrieveBulkData(options).then(arr => {
value.DirectRetrieveURL = URL.createObjectURL(new Blob([arr], { type: defaultType }));
return value.DirectRetrieveURL;
});
if (value.InlineBinary) {
const blob = utils.b64toBlob(value.InlineBinary, defaultType);
value.DirectRetrieveURL = URL.createObjectURL(blob);
return value.DirectRetrieveURL;
}

if (!singlepart || (singlepart !== true && singlepart.indexOf(fetchPart) === -1)) {
if (value.retrieveBulkData) {
// Try the specified retrieve type.
const options = {
mediaType: defaultType,
};
return value.retrieveBulkData(options).then(arr => {
value.DirectRetrieveURL = URL.createObjectURL(new Blob([arr], { type: defaultType }));
return value.DirectRetrieveURL;
});
}
console.warn('Unable to retrieve', tag, 'from', instance);
return undefined;
}
console.warn('Unable to retrieve', tag, 'from', instance);
return undefined;
}

const { StudyInstanceUID, SeriesInstanceUID, SOPInstanceUID } = instance;
Expand Down
Loading