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

[wayfarer3130]

I agree with @pieper that this should be a separate step that should be configurable - maybe as an option going into the parse step, but also as a stand alone filter that works on any JSON DICOMweb metadata instance. That filter should then have some way to register named fixes and a way to disable fixes individually.

[andreidubov]

Could you please clarify this point, I want to make sure that I understood you correctly. Do you mean that I should add an option(flag) to naturalizeDataset/denaturalizeDataset and depending on this option(flag) apply or skip for this logic?

[pieper]

I was thinking you could have a special-purpose piece of code at the application level that is coded based on the unique information that it will be accepting data from a source (kpacs) that has known limitations in terms of generating standard dicom objects. I.e. in this case you know that certain tag values will have incorrect values. This special-purpose code should read the instance and apply whatever fixes are needed to make the instance standards-compliant and then send it to the dcmjs code.

We may want to provide a standard place for this kind of special-purpose code to happen. Perhaps in dcmjs-dimse there could be a hook to apply a standardizing transform to the incoming instance data. If there end up being very common ones they could be bundled as add-on packages (e.g. a set of transforms for kpacs, ones for non-standard microCT scanners, etc).

[jimOnAir]

We don't have access to this low-level data on the application level as the application provides C-Store service only thing we have from dcmjs-dimse is Dataset without any VR information. dcmjs-dimse relies on dcmjs while reading the Dataset. In this case, we don't know if something is wrong until we have read the data.
What you propose is rewriting parts of dcmjs and dcmjs-dimse that are receiving binary data over the network and naturalizing the dataset.
The other thing is that we don't have access to the KPacs dictionary and can't compare it to dcmjs Dictionary to find out which tags can have the wrong VRs.

[pieper]

From what I understand if dcmjs-dimse is processing a c-store you should be able to access the instance as it is received. From there you can read it as pure dicom json model and you can look at each of the tags in the instance and see if they have the non-standard form that leads to the issue based on the dicom dictionary and your knowledge of the deficiencies with kpacs. (That is, if you see that a tag with a know standard VR has been set to UN by kpacs). If you see an issue, you can modify the instances and save them back before further processing.

[andreidubov]

At the application level in CStore we do not have DICOM json, we only have naturalized Dataset. At this level, there is no way how we could get the original VR for tag.

DICOM json object is present inside dsmjs-dmse in the buffer/file reading functions.

If we talk about saving the original UN VR and saving it to a file without getting an error (as suggested in PR). I think it’s impossible to do without changes in dcmjs. The denaturalizeDataset takes the VR based only on the value in the dictionary, which leads to a mismatch between the VR and the value. If we talk about converting UN VR value to VR from the dictionary, it looks like a more difficult task to handle all possible VRs. @PantelisGeorgiadis do you have any ideas how this issue could be solved at the dcmjs-dimse level?

[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 #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 #380

[github-actions]

🎉 This PR is included in version 0.30.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[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: #381.