search

dcmjs-org / dcmjs

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

Lots of 'Invalid vr type... ' log messages in dcmjs release 0.29.11 #368

Open jbocce opened 8 months ago

jbocce commented 8 months ago

Using OHIF...

  1. Open a browser window and open the browser's console window.
  2. Launch the following study in OHIF's dev version https://viewer-dev.ohif.org/viewer/?StudyInstanceUIDs=2.16.840.1.114362.1.11972228.22789312658.616067305.306.2
  3. Notice the many logged error messages in the console. They typically look like the following... 
    ValueRepresentation.js:262 Invalid vr type xs - using US
value   @   ValueRepresentation.js:262
set @   ValueRepresentation.js:40
(anonymous) @   DicomMetaDictionary.js:160
value   @   DicomMetaDictionary.js:117
d   @   index.js:346
(anonymous) @   index.js:395
(anonymous) @   index.js:439
Promise.then (async)        
(anonymous) @   index.js:438
_retrieveSeriesMetadataAsync    @   index.js:437
await in _retrieveSeriesMetadataAsync (async)       
metadata    @   index.js:193
(anonymous) @   Mode.tsx:67
(anonymous) @   Mode.tsx:66
(anonymous) @   Mode.tsx:388
(anonymous) @   Mode.tsx:400
Ul  @   react-dom.production.min.js:262
t.unstable_runWithPriority  @   scheduler.production.min.js:18
qa  @   react-dom.production.min.js:122
Nl  @   react-dom.production.min.js:261
(anonymous) @   react-dom.production.min.js:261
x   @   scheduler.production.min.js:16
M.port1.onmessage   @   scheduler.production.min.js:12
  4. Previous versions (namely 0.29.5) did not have this.

Not sure what is causing it. Please advise.

jbocce commented 8 months ago

FYI @pieper

sedghi commented 8 months ago

Sorry for tagging you Steve, we meant to see if @dmlambo has any comment on this

pieper commented 8 months ago

Sorry for tagging you Steve, we meant to see if @dmlambo has any comment on this

Thanks, I'm very happy for someone else to investigate this. 👍

For background:

When I click on one of the error message I get to this code:

https://github.com/dcmjs-org/dcmjs/blob/f0dc1995f3e1d986b7e668dc4780181176e90ba6/src/ValueRepresentation.js#L253-L271

Which reference this issue thread for background:

https://github.com/dgobbi/vtk-dicom/issues/38

But I don't know why the latest changes would trigger more of those cases. Can you maybe bisect between 0.29.5 and 0.29.11 to see exactly which commit leads to the extra logging?

https://github.com/dcmjs-org/dcmjs/commit/f0dc1995f3e1d986b7e668dc4780181176e90ba6

dmlambo commented 8 months ago

Ah, yeah that would be my change -- it's a symptom of variable VR types, which I just found out were possible: SmallestImagePixelValue, for example.

What my change introduces is the ability to access values differently than their backing data structure, to allow PN (PersonName) type values to return dicom+json when json stringifying, and formatted strings when writing part10 dcm. As part of that change I obtain the ValueRepresentation class for the VR type and call into a function which can add the required accessors. This actually only happens with PN currently (and may indeed forever stay that way).

The good news is this doesn't affect anything; it's safe to ignore it for the time being. The bad news is the dicom standard is a bit messier than I originally thought. I'll have a look at supressing this accessor step for unknown VRs.