Ignore missing metadata group length field #269
Conversation
There was a problem hiding this comment.
Thanks for the contribution! Overall this seems reasonable to me. Left a couple comments below.
In terms of tests, I think it would be good to
- Add a Parse test that runs over the test files with this option enabled (sanity check). See . I'll prob turn these into parametrized tests that run over the options too
Line 113 in f77ffde
- Consider adding a readHeader test in read_test.go that could be more granular and test correctness (e.g. if the length element is indeed missing, and possibly error cases), though this may be a bit more challenging to bootstrap. We could consider using writeElement to help bootstrap a buffer for the test possibly.
read.go
Outdated
| } | ||
| tg := tag.Tag{} | ||
| buff := bytes.NewBuffer(tag_bytes) | ||
| binary.Read(buff, binary.LittleEndian, &tg) |
There was a problem hiding this comment.
I'm not sure what would happen here if in the future an extra field were added to the tag struct in the future? It also sounds like unexported fields may lead to Read panicing (if we were to add on in the future)
It's a bit more verbose, but we may want to consider something like
if err := binary.Read(buff, binary.LittleEndian, &tg.Group); err != nil {
// handle err
}
if err := binary.Read(buff, binary.LittleEndian, &tg.Element); err != nil {
//handle err
}
Also don't forget to handle the errors on binary.Read as well :).
WDYT?
There was a problem hiding this comment.
Certainly I can change it. I guess your point is that tag could change in a way that makes it incompatible with direct mapping from the DICOM header...
There was a problem hiding this comment.
Yep, structs can/do change and if it did, it could lead to an unintentional breakage here. If the tests are good enough it's possible that regression would be protected, but it seems safer to do the slightly more verbose way to me for now!
There was a problem hiding this comment.
Could we make this update? Thanks!
There was a problem hiding this comment.
Ah, not sure if this update was applied (it's not showing up for me). basically instead of reading into the tag.Tag directly, reading each field of the tag
if err := binary.Read(buff, binary.LittleEndian, &tg.Group); err != nil {
// handle err
}
if err := binary.Read(buff, binary.LittleEndian, &tg.Element); err != nil {
//handle err
}
|
I've updated the unit tests. I created 2 functions to generate fake header data (one with the 0x2,0x00 element/tag and another without). I am only testing readHeader for this. Let me know what you think. |
read.go
Outdated
| } | ||
| tg := tag.Tag{} | ||
| buff := bytes.NewBuffer(tag_bytes) | ||
| binary.Read(buff, binary.LittleEndian, &tg) |
There was a problem hiding this comment.
Could we make this update? Thanks!
|
Hello wanted to make sure that there was no pending action on my part and that you can see the changes I submitted per your latest review comments. Please let me know. Regards. |
|
All good on your end! I will take a look today or later this week! Apologies for the delay, have been quite busy over the past weeks! |
read.go
Outdated
| } | ||
| tg := tag.Tag{} | ||
| buff := bytes.NewBuffer(tag_bytes) | ||
| binary.Read(buff, binary.LittleEndian, &tg) |
There was a problem hiding this comment.
Ah, not sure if this update was applied (it's not showing up for me). basically instead of reading into the tag.Tag directly, reading each field of the tag
if err := binary.Read(buff, binary.LittleEndian, &tg.Group); err != nil {
// handle err
}
if err := binary.Read(buff, binary.LittleEndian, &tg.Element); err != nil {
//handle err
}
The proposed change adds an additional option flag to the parser which indicates it to ignore the MetaInformationGroupLength element (0x0002, 0x0000). There are many vendors which are still producing DICOM with this tag missing. Other toolkits like pydicom give a warning and dcmtk just reads these files (e.g. their dcmconv utility will fix the file and add the tag but it changes other tags such as ImplementationVersionName).
The idea behind the proposed change is to allow the file to be parsed even when missing this tag and it essentially does not use the PushLimit, PopLimit utilities; but rather peeks into every 4 bytes and reads all the element if it is in group 2. It will stop once it finishes reading group 2 tags. I tested with 2 different ultrasound vendor datasets.
Please let me know of any additional artifacts you would want in order to accept this (e.g. tests)