Fix UN VR setting during naturalization/denaturalization

[andreidubov]

Hello, I encountered several issues with saving the dataset to a file. This is also related to this PR #352 . Some of them:

Value exceeds max length, vr: CS, value: [object ArrayBuffer], length: undefined
value.toExponential is not a function

After investigation I found that the source file has UN VR for some tags. And the main problem is that after the naturalizeDataset process we lose UN VR for these tags and in the denaturalizeDataset process we get not UN VR, but VR according to the dictionary. New VR does not match the value and therefore we get the following errors when trying to write.

Tag with UN VR in file

This tag un dict after DicomMessage.readFile

This tag in dataset after DicomMetaDictionary.naturalizeDataset

This tag after DicomMetaDictionary.denaturalizeDataset (DS VR instaed of UN)

Solution: Saving the UN VR to the _vrMap in dataset and use it in the denaturalizeDataset to restore the original UN VR. Based on #352 PR. If the VR is UN and does not match the VR in dictionary for this tag, then parse the tag's value according to the VR from the dictionary. This logic is controlled by the parseUnknownVr flag, which is disabled by default.

[jimOnAir]

@pieper, @PantelisGeorgiadis could you please have a look on this?

[pieper]

I see that the logic of this fix could be okay, but the underlying issue seems to be an incorrect dicom instance so I'm a little hesitant to put in patches to deal with every bad data case. In this case ExposureIndex is supposed to be a DS, so having it as an UN is probably not legal, but may show up in practice.

https://dicom.innolitics.com/ciods/digital-x-ray-image/x-ray-acquisition-dose/00181411

That said, this change probably makes everything more robust and shouldn't break anything. Maybe others could chime in. @wayfarer3130 or @PantelisGeorgiadis ?

[jimOnAir]

Thanks for review. It is the same case I had with KPacs. It changes VR of all unknown tags to UN. At least this change allows us to save images to file as it was received. Previously there were errors while trying to process ArrayBuffer as number or other type.

[pieper]

Why doesn't kpacs know about ExposureIndex if it's in the standard? Maybe it needs an updated data dictionary?

[jimOnAir]

It doesn't know. And all unknown tags are coded with UN VR. KPacs is not supported for at least decade. But some organisations are still using it.

[pieper]

I'm not saying this is not important, but workarounds for out of date and unsupported external software should be isolated from the main logic of the package. The reason for this is that many dicom toolkits have historically become cluttered with code that was only needed for data of a particular nonstandard type. Sometimes these workaround are not well commented or rely on data that can't be shared for privacy reasons or can't be reproduced because they require some commercial installation. For these reasons I'd like the library itself to focus mainly on very standard dicom and move special case handling somewhere else.

In this case I'd suggest adding a very specific filter for code that is known to require interoperability with kpacs where it applies a specific standardization step converting UN to DS for this tag (and maybe makes similar fixes for other tags with known VRs).

[jimOnAir]

The issue is not only with converting DS VR to UN but for any VR absent in KPacs or other PACS dictionaries. UN VR is a part of standard:

• An existing application that does not support that new Attribute (and encounters it) could convert the VR to UN. https://dicom.nema.org/
• The issues of whether or not the Data Element may be copied, and what VR to use if copying, do not arise when converting a Data Set from Implicit VR Little Endian Transfer Syntax, since the VR would not be present to be unrecognized, and if the Data Element VR is not known from a data dictionary, then UN would be used. https://www.dicomstandard.org/

Currently, if we are trying to write to a file a previously parsed dataset with UN it fails with an error because writing is based on a dictionary and not the original dataset

[PantelisGeorgiadis]

Hello all! Sorry for being silent all that time! Well, dcmjs-dimse was designed having simplicity in mind while trying to abstract the calling code from “advanced DICOM internals”. However, as it is getting more adopted, it is obvious that users need more flexibility so they can handle more complex and demanding cases.

In that direction, we have created this PR https://github.com/PantelisGeorgiadis/dcmjs-dimse/pull/41, a long time ago (!!!), that it allows the reception of DICOM content to Writable streams. It actually allows the receiving code to create an object inherited from Writable, that will receive the incoming bytes from the network (in chunks), giving the freedom to perform any operation on them before the cStoreRequest event is called. A major use case is to perform streaming to disk while receiving big instances.

There are still a few technical issues to be solved (e.g. handling of drain stream event) but I believe that it fits your needs, since it will allow you to access the incoming bytes and apply a “custom-parsing” logic. Thoughts?

[andreidubov]

Hello everyone. I changed the logic based on #352 PR. Now If the VR is UN and does not match the VR in dictionary for this tag, then parse the tag's value according to the VR from the dictionary (we will have the correct value corresponding to VR from the dictionary and there will be no errors when writing). Also I added parseUnknownVr option which is disabled by default. Maybe this solution would be better. What do you think?

[andreidubov]

Hi @pieper, @wayfarer3130 could you please have a look on this?

[pieper]

Can you clarify if you think that KPACS behavior is actually correct according to the dicom standard? If it doesn't know about the tag because it's dictionary is out of date is it correct for it to set the VR to UN? And if that's the case, then do we know for sure that since we have a more recent dictionary then parsing the value with the correct VR is the supposed to work? If that's the case then I don't think we need to have a flag for this as it should always be done (with error checks).

If on the other hand this is invalid DICOM then I think something like what @PantelisGeorgiadis suggested is more appropriate.

[andreidubov]

As @jimOnAir wrote here, it is correct to set the VR to UN if this tag is not in application dictionary.

• An existing application that does not support that new Attribute (and encounters it) could convert the VR to UN. https://dicom.nema.org/

• It also says that we can parse an element with UN VR if we know its VR from our dictionary: If at some point an application knows the actual VR for a Data Element of VR UN (e.g., has its own applicable data dictionary), it can assume that the Value Field of the Data Element is encoded in Little Endian byte ordering with Implicit VR encoding, irrespective of the current Transfer Syntax. https://dicom.nema.org/

[pieper]

@wayfarer3130 @PantelisGeorgiadis do you agree this PR now represent standard behavior? Do you see any cases where it will won't be backwards compatible?

@andreidubov @jimOnAir thanks for bearing with the process.

One more request: we try to keep binary files out of the repo. Can you redo the test to download the file from here: https://github.com/dcmjs-org/data/releases/tag/unknown-VR

You can copy how it's done in other tests.

[wayfarer3130]

@wayfarer3130 @PantelisGeorgiadis do you agree this PR now represent standard behavior? Do you see any cases where it will won't be backwards compatible?

@andreidubov @jimOnAir thanks for bearing with the process.

One more request: we try to keep binary files out of the repo. Can you redo the test to download the file from here: https://github.com/dcmjs-org/data/releases/tag/unknown-VR

You can copy how it's done in other tests.

Yes, I agree that it now looks like it is standards compliant

[pieper]

Thanks @wayfarer3130

@andreidubov if you update the test I'll be fine with merging this.

[PantelisGeorgiadis]

Thank you @andreidubov and @jimOnAir for working on this! I agree that it is fine merging this, after updating the test.

However, now that I'm thinking of it, in order to update dcmjs-dimse with the release that will include this fix, we need to fix the unit tests that are going to fail because of the https://github.com/dcmjs-org/dcmjs/pull/364. @pieper, @wayfarer3130, I know it is too late now, but it would be great if we could switch the PN tag interpretation between object and string behind a parsing option. My bad for not catching it when this was still under review.

[andreidubov]

Thanks guys, sorry for the wait, test updated.

[pieper]

Thanks for updating the test 👍

I see there's an unrelated test failing, so I created an issue for it https://github.com/dcmjs-org/dcmjs/issues/380

[github-actions[bot]]

:tada: This PR is included in version 0.30.2 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[pieper]

@PantelisGeorgiadis do you want to create a new issue related to the PN changes you mentioned?

[PantelisGeorgiadis]

Thank you Steve! I create one here: https://github.com/dcmjs-org/dcmjs/issues/381.