dcmjs icon indicating copy to clipboard operation
dcmjs copied to clipboard

fix: 🐛 update invalid VR type for LUTData

Open bmoix opened this issue 2 years ago • 3 comments

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.

bmoix avatar Mar 29 '23 11:03 bmoix

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

pieper avatar Mar 29 '23 18:03 pieper

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.

bmoix avatar Apr 03 '23 12:04 bmoix

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.

pieper avatar Apr 06 '23 21:04 pieper