454 refactored Encoding/Decoding logic to expose DICOM encoding mappings for consumption #455
Conversation
…t singleVRs array.
…the same logic can be used elsewhere and hopefully it is clearer. Functionality should remain the same.
… makes more sense for it to be there.
…used by BufferStream but also can be used elsewhere. Added export of new DicomBufferCODEC class.
✅ Deploy Preview for dcmjs2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@pieper I went ahead and did some cleanup based on your comments in #454. In terms of the other issue (#373), I do not do anything here other than change the location of the logic in question which I partially move lower in the abstraction hierarchy. However, if I get some time in the near future and no one has addressed it, I might take a stab. I think that the reasonable approach is to find the specific character set at the level of the tree in the header and use that for that level only. That can get a bit complex than I have time at the moment and I will need to consult the standard for other potential cases. |
|
From a quick look I think the PR looks good 👍 I'd like another set of eyes on it though, so hopefully @wayfarer3130 can take a look too. |
|
@wayfarer3130 checking in if there is anything you think I should improve on in this PR. No rush, I know you have been very helpful in other PRs and very busy. I am just helping keep this PR in people's minds in case there is a chance to merge. Thank you! |
| "1.2.840.10008.5.1.4.1.1.77.1.5.4": "OphthalmicTomographyImage" | ||
| }; | ||
|
|
||
| DicomMetaDictionary.encodingMapping = { |
There was a problem hiding this comment.
Can you drop this externally as it's own file? There should also be an addEncoding method to externally add them.
just import the encoding mapping and assign to the dicom meta dictionary rather than increasing the overall contents of this file.
|
|
||
| DicomMetaDictionary.defaultEncoding = "latin1"; | ||
|
|
||
| DicomMetaDictionary.encapsulatedSyntaxes = [ |
There was a problem hiding this comment.
Same for this. I would suggest having several categories, and use a Set for this many values. The types:
Encapsulated
Video
Uncompressed
| if (!vr.isBinary() && singleVRs.indexOf(vr.type) == -1) { | ||
| if ( | ||
| !vr.isBinary() && | ||
| ValueRepresentation.singleVRs.indexOf(vr.type) === -1 |
There was a problem hiding this comment.
Use ValueRepresentation.singleVRs.has(vr.type)
with a set or map.
| * Facilitates the conversion of binary buffers from a DICOM encoding scheme to | ||
| * a web supported string encoding scheme and vice versa. | ||
| */ | ||
| class DicomBufferCODEC { |
There was a problem hiding this comment.
This should be it's own file.
Also, the encoder/decoder should be identical because there are cases that both reading and writing occur on a single dicom buffer.
Would like to see a small unit test on this.
The default for both should actually be latin1 until the specific character set gets written.
3 participants
Fixes #454.
Although, I did not find any bugs with dcmjs, some of the tooling in the library could be better exposed for consumption in other projects. I focus on exposing the DICOM buffer decoding here given an initial
SpecificCharacterSet. If translation is not necessary, a few convenience methods are provided to set the correct encoder or decoder on demand.This is a code cleanup / reorganization kind of PR.
Jest tests passed.
I did not add any new tests since there is coverage from the current tests and the one class I added is trivial.