suyashkumar / dicom

⚡High Performance DICOM Medical Image Parser in Go.
MIT License
933 stars 136 forks source link

add EncapsulatedDocumentLength #271

Open flicaflow opened 1 year ago

flicaflow commented 1 year ago

This PR adds the EncapsulatedDocumentLength tag which is needed to implement Encapsulated PDF IOD.

Note: I added the necessary value in the tag package manually. I tried to use the python script but that resulted in a broken package. Please advise if there is a better way doing it.

suyashkumar commented 1 year ago

Thanks for the contribution! This part of the library could definitely use some help haha. Ideally we would keep it so these tag definitions match exactly what the generate script produces, so I'll look into that if there's an issue (and maybe we can upgrade it a little).

Probably we should update the list of tags as well, or even better, maybe we can use the JSON parsed standard from Innolitics as the baseline (note the tag you're asking for is indeed included there).

Something that came to mind--do you think it would be helpful in the interim to have an "add custom tag" API so that users could add custom tag data to the global tagDict that would be used when parsing dicoms downstream in that program?

EDIT: this reminds me of #147 and draft #169 (thank you @bpeake-illuscio)! I will carve out some time in the next 2 weeks to circle back to those get a refined solution merged!

flicaflow commented 1 year ago

I had basically the same thought just didn't wanted to make opinionated changes which are not mergable. I would basically do the following:

I was also thinking about the AddCustom tag API. That is not only important because of missing official tags but also because the concept of private tags exist. You can basically just invent tags with uneven group numbers. And that is important if you want to store proprietary data e.g. with the RawData IOD.

I can give the code generation a shot, no promises when this will happen and no hard feelings if you wanna do it yourself ;)

suyashkumar commented 1 year ago

Indeed. Note that private tags won't break parsing even in today's code, but having them in the tagDict is helpful for parsing implicit transfer syntax private tags more semantically (instead of treating them as raw elements).

On the code generation, that's in line with what we've been thinking! As I mentioned check out #147 and draft PR #169. #169 actually does quite a bit already (thanks to @bpeake-illuscio @peake100) including rewriting the codegen in Go and using the innolitics dump. My plan was to go through that PR, and use it as a starting point to get something mergable and probably collaborate with @bpeake-illuscio if they have time or add commits to that work myself to get it in shape to merge. It's possible we could use some help on this, and would be happy to reach out if so!

In the meantime, perhaps we can think about something like tag.RegisterCustom/tag.AddCustom? I suppose this will require knowledge of what custom tags one would like to register in advance of parsing a dicom, but worst case scenario it'll fall to the fallback of reading it as UnknownVR which defaults to readString, essentially holding onto the bytes as a string (I need to double check how that roundtrips also). It should also probably be easier to later try to interpret those bytes as other valueTypes in the future too.