dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
291 stars 110 forks source link

fix: 🐛 update invalid VR type for LUTData #343

Open bmoix opened 1 year ago

bmoix commented 1 year ago

Update the Value Representation (VR) for the LUTData tag. It was defined as lt instead of LT.

The method ValueRepresentation.createByTypeString(type) from ValueRepresentation.js couldn't find the right VR class from the VRInstances object, and it was creating an UnknownValue object. This was causing a crash when reading a file with that tag.

Context

Invalid vr type lt - using UN
(node:28115) UnhandledPromiseRejectionWarning: TypeError: First argument to DataView constructor must be an ArrayBuffer
    at new DataView (<anonymous>)
    at new BufferStream (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:203740)
    at ReadBufferStream._createSuperInternal (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:128580)
    at new ReadBufferStream (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:210562)
    at UnknownValue.writeBytes (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:221335)
    at Tag.write (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:246135)
    at /tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:1105876
    at Array.forEach (<anonymous>)
    at Function.write (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:1105724)
    at SequenceOfItems.writeBytes (/tmp/.mount_RadiobfjpWia/resources/app.asar/index.js:2:235261)
(node:28115) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 3)

Tests

All unit tests pass :heavy_check_mark:

Test Suites: 10 passed, 10 total
Tests:       52 passed, 52 total
Snapshots:   0 total
Time:        5.638 s
Ran all test suites.
pieper commented 1 year ago

Thanks for the report and the proposed fix 👍

Question though: shouldn't the VR be US or OW?

https://dicom.innolitics.com/ciods/nm-image/voi-lut/00283010/00283006

bmoix commented 1 year ago

Thanks for taking the time to review this @pieper

Yes, you are right!

In the case that a tag can have more than one VR, what is the best approach? Is using the first one that appears in the tag definition, US in this particular case, good enough? I can see that in dictionary.js you can only assign one VR for each tag.

pieper commented 1 year ago

That's a good question - I think the right answer is that whoever creates the element needs to provide the _vrMap like is done here.