search

QIICR / dcmqi

dcmqi (DICOM for Quantitative Imaging) is a free, open source C++ library for conversion between imaging research formats and the standard DICOM representation for image analysis results
https://qiicr.gitbook.io/dcmqi-guide/
BSD 3-Clause "New" or "Revised" License
232 stars 62 forks source link

Crash converting fractional segmentation frame to ITK #419

Open lorteddie opened 3 years ago

lorteddie commented 3 years ago

While handling fractional frames in ImageSEGConverter::dcmSegmentation2itkimage the original frame is copied. This actually is a shallow copy so both frames point to the same pixel buffer. Deleting the shallow copy deletes the buffer. Afterwards the original frame is deleted, due to the changes in #417, causing a double delete which most likely results in a crash.

The shallow frame copy is actually unnecessary, a deletion of the 'working' frame is only necessary if it is an unpacked binary frame.

fedorov commented 3 years ago

Thank you for testing this and contributing a patch. I don't think dcmqi was used much with fractional segmentations. Would also be great to have a regression test for a fractional seg!

lorteddie commented 3 years ago

Not sure if I find some time to go there any time soon. But I can give some feedback: We are currently using dcmSegmentation2itkimage to convert SEG objects, binary and fractional. In general this works fine, couldn't find issues converting or parsing issues. There should definitely be an overload taking a DcmSegmentation object though. Problem here is the current interface requires us to parse the DcmSegmentation object twice. We decided against using itkimage2dcmSegmentation because it is too limiting with its interface and the hard coded values behind it. Had to write that ourselves. And some of those limitations even proceed to DCMTK, like for example the CodeSequenceMacro which is missing half the attributes. Also the SEG standard is ridiculously limiting, for example the stupid amount of single item sequences which are a pain ...

fedorov commented 3 years ago

Thanks for the feedback and your contribution!

We decided against using itkimage2dcmSegmentation because it is too limiting with its interface and the hard coded values behind it. Had to write that ourselves. And some of those limitations even proceed to DCMTK, like for example the CodeSequenceMacro which is missing half the attributes.

Can you elaborate a bit on this? You mean various type 1C and 3 in the below that you would want to populate?

image

lorteddie commented 3 years ago

That list is represented as class CodeSequenceMacro in DCMTK. It only supports CodeValue, CodingSchemeDesignator, CodingSchemeVersion, CodeMeaning. All other attribute cannot be read, written or set. This means, you cannot use codes longer than 16 characters since LongCodeValue is not supported, you cannot use links as code since URNCodeValue is not supported and you cannot use ContextGroups. And since we SegmentedPropertyTypeCodeSequence to add coded values we are forced to use EquivalentCodeSequence because SegmentedPropertyTypeCodeSequence is a sequence of one item. EquivalentCodeSequence is also not supported.

We want to use a private designator to specify our own codes which can be quite long. We also want to group these codes to basically tell what a code is, give it a type so to say. For that we thought to use Context Groups. So we give each coded value a ContextUID specifying its type and also a MappingResourceUID to tell the type context group is from us. To be honest I don't know if this is how context groups work or can/should be used but it seems like the only way to add coded meta information to a SEG object.

fedorov commented 3 years ago

@lorteddie that is interesting, no one ever raised this point before. I think it should not be difficult to add support for those items to DCMTK. One potential concern is that the features of this code sequence that you plan to use are not used in any of the implementations I know, but I can definitely understand the motivation for using URNCodeValue. As a background, in dcmqi and in adding support to DCMTK we tried to keep the list of attributes to the minimum to reduce complexity and hopefully improve interoperability in practice. In the use cases we encountered previously, the codes available in DCM/SCT/RadLex are usually sufficient.

Would you be able to share some examples that motivated you to pursue the approach with URNCodeValue?

Would be interesting what David Clunie @dclunie thinks about this.

lorteddie commented 3 years ago

My company takes part in the Racoon project in Germany. The DICOM SEG part is that multiple software systems generate, collect and communicate segmentations and SEG was chosen as communication method. Thing is that it's not actually only segmentations but also some meta information for each segmentation / segment. To encode such information the SEG standard didn't give us too many options. Basically we discussed using a bunch of private tags, or what we ended up with is to use the SegmentedPropertyTypeCodeSequence since it seemed like the best fit.

To add basically arbitrary meta information we need to use a private scheme designator since SCT/SRT etc. do not include codes for what we want to encode. This info can for example be the user who last modified the segmentation and what tool/algorithm did they use. In addition to that we want to also list the algorithm and user who created the segmentation. SegmentAlgorithmType is not sufficient because we need the creator and the editor algorithm, these are implementation details so no public codes. Also the usernames are necessary, also no public codes. But especially for the usernames we needed to tell that the coded value is actually a username so we needed some kind of type system. For that we thought of using context groups since a group of types inside a single private designator is basically a type. We currently use CodeValue and LongCodeValue depending on the code length. We are not using URNCodeValue (yet).

michaelonken commented 3 years ago

Hi,

that list is represented as class CodeSequenceMacro in DCMTK. It only supports CodeValue, CodingSchemeDesignator, CodingSchemeVersion, CodeMeaning. All other attribute cannot be read, written or set. This means, you cannot use codes longer than 16 characters since LongCodeValue is not supported, you cannot use links as code since URNCodeValue is not supported and you cannot use ContextGroups.

since end of 2019 the CodeSequenceMacro in DCMTK actually supports URN Code Value as well as Long Code Value, both, for reading and writing.

What DCMTK build does dcmqi use right now?

Context Group information as well as other "advanced" attributes are not supported but they are very rarely seen in practice. Most of the time, whenever people need a private code that does not exist anywhere else, they use a private Coding Scheme Designator (must start with "99", e.g. "99RACOON") and then invent any Code Value (or Long/URN Code Value) they like to use. That's it.

However, if you are sure that you need further attributes in the Code Sequence Macro, send me a patch and I'll merge it into DCMTK (or tell me you need it and bring some time...).

Best regards, Michael

fedorov commented 3 years ago

@lorteddie this is very interesting. I really appreciate all the details you provided.

@dclunie and I discussed this, and there are several options how to handle this properly. We would be very interested to get on a call with you to discuss those options, and how we could proceed.

Your use case and needs are very valid, but we believe the approach you are following is not the right approach, and we would really like to make an effort to help you develop a correct approach. Can you connect with me by email andrey.fedorov@gmail.com, if you would prefer to keep this conversation off of this publicly visible ticket?

pieper commented 3 years ago

If you take this conversation off the public ticket please report back what you decide - it sounds like an important use case and I look forward to seeing the best practice guidelines and example seg instances.

lorteddie commented 3 years ago

We are currently using DCMTK 3.6.5, URNCodeValue and LongCodeValue are available in 3.6.6 so we will update soon I guess, thanks for the info.

I don't mind keeping this public, our (my company) goal is to get a decent working solution ready and we would definitely prefer a solution which is standard conform and good practice. I'm very interested in the possible solutions you discussed and we can discuss them here or on a call. Just tell me what you prefer and I send you an email to get in contact.

fedorov commented 3 years ago

@lorteddie excellent! I would prefer to discuss on a call, and then summarize the decisions in this thread. I just think that would be more productive. Please follow up by email!

fedorov commented 3 years ago

To summarize the decisions made during the call today, here are the two main needs that Tobias had, and the decisions how they should be properly addressed while writing DICOM SEG:

@michaelonken do you have any concerns about the proposed approach?

dclunie commented 3 years ago

Here is a proposed CP to effect the approach discussed:

cp_dac579_PerSegmentAlgorithmAndCreator.pdf