Convert a dataset IR 192 (UTF-8)?

[thomas-tribus]

I've been going over the Discussions and Issues and went through the code. But still I'd figure I make sure, so I hope you don't mind me asking.

When users add dicom data to my system I want to make sure it is ISO_IR 192 (UTF-8).

Is changing the character set of the DicomObject really all it takes (because the data was already converted upon parsing)?

dcm_file.put_element(InMemElement::new(
        tags::SPECIFIC_CHARACTER_SET,
        VR::CS,
        ISO_IR_192,
    ));

[thomas-tribus]

To partly answer my own question: It is not enough. It works on mosts datasets I tried, but for datasets that contain sequences it fails because the sequence length changes when the character set changes. I'm trying to write a function now that does the conversion.

[Enet4]

Thank you for the question! The DICOM parser in crate dicom_parser is prepared to decode DICOM data sets at different levels of textual data preservation, but the current DICOM object implementation relies exclusively on preserving text as Rust strings. That is, textual values are decoded into UTF-8 strings on the spot, which in theory means that replacing the Specific Character Set would seamlessly transcode text accordingly.

With that said, the problem that you stumbled upon is definitely bug in the data set writing function, which was caused by retaining specified lengths in data set sequences, even after modifying the specific character set. I envision 3 directions moving forward:

• the data set writer (for the current in-memory DICOM object implementation) can be changed to always write data set sequences with an undefined length;
• the objects could have some kind of dirty flag to indicate that the data set writer should make SQ lengths undefined;
• in the long term we could try a DICOM object implementation which preserves text to the byte: all values are kept in its byte form as originally encoded, text would be decoded on demand with eventual caching, and transcoding text would be done through a separate method. AFAIUI this is the approach done by dcm4che.

Anther slightly related discussion: #401

[thomas-tribus]

I decided to try and write a new AttributeAction, just to test this approach.

AttributeAction::InvalidateLength => {
    if let Some(e) = self.entries.get_mut(&tag) {
        let vr = e.vr();
        if vr == VR::SQ {
            self.len = Length::UNDEFINED;
        }
    }
    Ok(())
}

I combined this with the a snippet I found in #415, which as I read it should select all paths.

Unfortunately, the resulting dicom is not correct according to dcmtk:
W: DcmSequenceOfItems: Parse error in sequence (0032,1064), found (204f,4f4d) instead of sequence delimiter (fffe,e0dd)

It seems I don't trigger the code to emit the sequence delimiters.

Am I missing something here?

Full example:

use anyhow::Context;
use dicom::core::ops::{AttributeAction, AttributeOp, AttributeSelector, AttributeSelectorStep};
use dicom::core::{DicomValue, VR};
use dicom::dictionary_std::tags;
use dicom::object::mem::InMemElement;
use dicom::object::DefaultDicomObject;
use dicom_core::ops::ApplyOp;
use std::env;

fn main() {
    let args: Vec<String> = env::args().collect();
    let input_file_name = &args[1];

    let mut input = DefaultDicomObject::open_file(input_file_name).expect("can read file");
    //
    // let mut output = DefaultDicomObject::new_empty_with_meta(input.meta().clone());
    input.put_element(InMemElement::new(
        tags::SPECIFIC_CHARACTER_SET,
        VR::CS,
        "ISO_IR 192",
    ));
    convert(&mut input).expect("good things");
    input.write_to_file("tst.dcm").expect("success");
}

pub fn convert(input: &mut DefaultDicomObject) -> Result<(), anyhow::Error> {
    let entries = get_all_entries(input)?;
    for entry in entries {
        let op = AttributeOp::new(entry, AttributeAction::InvalidateLength);
        if let Err(e) = input.apply(op) {
            println!("Entry failed {e:?}");
        }
    }
    Ok(())
}

fn get_all_entries(file: &DefaultDicomObject) -> Result<Vec<AttributeSelector>, anyhow::Error> {
    let mut all_paths = Vec::<Vec<AttributeSelectorStep>>::new();
    file.iter()
        .for_each(|element| process_entry(&element, Vec::new(), &mut all_paths));
    let all_paths = all_paths
        .into_iter()
        .map(|e| AttributeSelector::new(e))
        .collect::<Option<Vec<_>>>()
        .context("Read error")?;
    Ok(all_paths)
}

fn process_entry(
    de: &InMemElement,
    mut path: Vec<AttributeSelectorStep>,
    all_paths: &mut Vec<Vec<AttributeSelectorStep>>,
) {
    let tag = de.header().tag;

    match de.value() {
        DicomValue::Primitive(_v) => {
            // // Push the full path
            path.push(AttributeSelectorStep::Tag(tag));
            all_paths.push(path);
        }
        DicomValue::Sequence(v) => {
            let mut path = path.clone();
            for (num, ds) in v.items().iter().enumerate() {
                path.push(AttributeSelectorStep::Nested {
                    tag,
                    item: num as u32,
                });
                for el in ds.iter() {
                    process_entry(el, path.clone(), all_paths);
                }
            }
        }
        DicomValue::PixelSequence(_v) => return,
    }
}

[Enet4]

Ah, I was close to mentioning that there already are a few places where the sequence length is reset to undefined (#364). We just overlooked the case where changing the specific character set would affect the real length of data set sequences.

An operation to change the character set could maybe just live as an independent method, because the Operations API currently applies to a specific attribute, and there is no way to point at the root data set, nor to select multiple elements.
In addition, it would be possible to tweak the current ApplyOp implementation so as to reset defined lengths when the target attribute is Specific Character Set. Setting a flag would still be cheaper than traversing through nested data sets and clearing the length of those with textual data elements.

[Enet4]

As for that code snippet suggesting an InvalidateLength, there is a small mistake: it should probably be e.len = . Then any parent element should be subjected to the same treatment. But perhaps we are better off with an approach that handles invalid lengths automatically.

[thomas-tribus]

Ah yes, I see how the Operations API might not be the best place. A separate function makes a lot of sense, even if all it does is set a flag.

As for that code snippet suggesting an InvalidateLength, there is a small mistake: it should probably be e.len =

Thanks for the correction. Sadly, even then it still doesn't work for me. Also it isn't really the best approach I think. But just FYI, this is what I tried.

e.header.len = Length::UNDEFINED;

So that leaves us (me) with the option of setting a flag. Can you give me any hints on where to start?

[thomas-tribus]

@Enet4 I created a draft PR for pathfinding how a flag could work: https://github.com/Enet4/dicom-rs/pull/456/files

It adds a (still failing...) unit tests. Curious to see if this is how you envisioned it too.

The way I wrote the test assumes a change to the way tokens are generated. I think the advantage of that would be that all downstream code consuming tokens would work as is.

I haven't found the best way to change token generation, so I figured I'd await some feedback first. Curious what you think.

[thomas-tribus]

Closing this discussion as this issue is now resolved. (#456)