dcmPreferVRFromDataDictionary and EVR_ox

[andreasb]

I'm not sure if this is expected behavior or a bug. I was setting dcmPreferVRFromDataDictionary to true to avoid common VR errors, and this works well for most tags, but I got a problem with waveform data, which has the VR OW, but was changed to EVR_ox due to that setting.
If then trying to read the data using getUint16Array, I get EC_IllegalCall, because the VR was not expected.
Actually in the code (dcitem.cc) the flag is used with this check:
if (dcmPreferVRFromDataDictionary.get() && (newEVR != EVR_UNKNOWN) && (newEVR != EVR_UNKNOWN2B)) {
...

I would have expected this also to check that ambiguous VRs that have one of the allowed values (e.g. OB/OW for EVR_ox or US/SS for EVR_xs etc.) would not be changed.
On the other hand, this would mean quite a few checks added, so I'm not sure if this is wanted.
Should I add these checks in my own code and set dcmPreferVRFromDataDictionary depending on that each time a tag is read, or is the usage of dcmPreferVRFromDataDictionary generally not recommended? Or should I make a PR to change the beforementioned line?

Thanks!

[Marco Eichelberg]

EVR_ox is used for attributes that may be either OB or OW in DICOM, such as WaveformData. The DCMTK parser actually looks at the value of WaveformBitsAllocated to determine whether OB or OW should be used.
You can either force WaveformData to OW by calling setVR(EVR_OW), or access the value with getUint8Array() in the case where the attribute is OB.

[andreasb]

Thanks Marco!

Yes, I understand that EVR_ox, EVR_xs etc. are used for ambiguous VRs. My problem is with the mentioned code in dcitem.cc, which resets the correct value for WaveformData (in this case OW) back to EVR_ox, if dcmPreferVRFromDataDictionary is true.
If I use either getUint16Array or getUint8Array afterwards, I get EC_IllegalCall:

Code: Select all

OFCondition DcmOtherByteOtherWord::getUint16Array(Uint16 *&wordVals)
{
    errorFlag = EC_Normal;
    if (getTag().getEVR() == EVR_OW || getTag().getEVR() == EVR_lt)
        wordVals = OFstatic_cast(Uint16 *, getValue());
    else
        errorFlag = EC_IllegalCall;
    return errorFlag;
}

getUint8Array does exactly the same (except the cast to Uint8* instead of Uint16*).

I would have expected that the usage of dcmPreferVRFromDataDictionary does not revert the previously set correct value, so that I do not have to handle EVR_ox and similar myself again (in this case by checking WaveformBitsAllocated myself).
Does that make my question more clear?

[andreasb]

To elaborate a bit more about the change I would like to have or make:
The easiest way would be to just ignore these ambiguous VRs if dcmPreferVRFromDataDictionary is set, e.g. something like:

Code: Select all

        if (dcmPreferVRFromDataDictionary.get() && (newEVR != EVR_UNKNOWN) && (newEVR != EVR_UNKNOWN2B))
        {
            if (newEVR != vr.getEVR())
            {
                if (vr.getEVR() != EVR_ox && vr.getEVR() != EVR_ox && vr.getEVR() != EVR_px && vr.getEVR() != EVR_xs && vr.getEVR() != EVR_lt)
                {
                    /* ignore explicit VR in dataset if tag is defined in data dictionary */
                    DCMDATA_DEBUG("DcmItem::readTagAndLength() ignoring explicit VR in data set ("
                        << vr.getVRName() << ") for element " << newTag
                        << ", using the one from data dictionary (" << newTag.getVRName() << ")");
                } else {
                    newTag.setVR(vr);
                }        
            }
        } else {
            /* set the VR which was read in the above created tag object */
            newTag.setVR(vr);
        }

This would just leave the VR for these values. There are more complicated possibilities, like checking if the current VR is allowed (e.g. OB or OW in the case of EVR_ox) and leave it in this case, otherwise setting EVR_ox, though I doubt that that makes much sense, if the code that defines the correct VR for ambiguous cases has already run.

Another possibility would be to adapt the check in getUint8Array and getUint16Array:

Code: Select all

OFCondition DcmOtherByteOtherWord::getUint16Array(Uint16 *&wordVals)
{
    errorFlag = EC_Normal;
    if (getTag().getEVR() == EVR_OW || getTag().getEVR() == EVR_lt || getTag().getEVR() == EVR_ox)
        wordVals = OFstatic_cast(Uint16 *, getValue());
    else
        errorFlag = EC_IllegalCall;
    return errorFlag;
}

The current code already checks for EVR_lt, so it may also check for EVR_ox, provided there is no reason why that would be wrong.
This would fix that concrete problem, but there may be similar problems elsewhere if the VR is reset to the ambiguous value.

[J. Riesmeier]

Your proposed changes would not solve all issues, in particular as "ox" represents either a sequence of 8 or 16 bit values. I would suggest to check whether calling DcmItem::checkAndUpdateVR() at an appropriate location (i.e. within the DCMTK) would be a suitable solution.

[andreasb]

Thanks, Jörg!
Sure, these would not solve all the problems, especially the second one, as I wrote.

I thought that the first proposal (not "correcting" correct VRs to ambiguous VRs) would at least avoid the problem, even if it would not add a correction for these VRs. I don't see any advantage of the current behavior, though I may miss something.

I'm not sure if I understood you correctly, though.
Are you suggesting that the current behavior of dcmtk is correct, and I have to call DcmItem::checkAndUpdateVR() in our own code, if I encounter an ambiguous VR (which I certainly can do, I just thought that this was already called before the correction code)?
Or should I patch dcmtk to handle the problem (and provide the patch or a PR)?

[J. Riesmeier]

I haven't checked the code in detail, but intuitively I would suggest to use the existing method DcmItem::checkAndUpdateVR(), which updates the VR in a few special cases such as Waveform Data, and call it after reading a DICOM dataset if option dcmPreferVRFromDataDictionary is enabled. I think currently it is only called when reading a DICOM dataset that is encoded with implicit VR.

[andreasb]

Thanks, I will have a look. Still not sure: do you mean that I call this in my own code, or that I patch dcmtk?

[J. Riesmeier]

As I wrote in a previous posting (see above), I would suggest to call this method within the DCMTK (i.e. somewhere in dcmdata). Whether you come up with a proposed patch or someone from the DCMTK team finds some time to do this depends on how urgent it is (for you)

[andreasb]

Thanks, that's what I thought you meant, but I wasn't sure
I'll do my own patch then and will propose it to you later. BTW, what is the preferred way for this: posting the patch here, or making a PR in GitHub?

[J. Riesmeier]

Unless the patch fixes a security issue (which is not the case here), it is up to you whether you want to submit the proposed patch by email (bugs/at/dcmtk/dot/org), here i the forum or as a PR at GitHub.

[andreasb]

Thanks!

[J. Riesmeier]

Fixed with commit 2b53c9d8d, which should be visible in the public git repository soon.

Also see GitHub PR 100.