search

dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
300 stars 112 forks source link

Other double as explicit value representation #399

Closed craigberry1 closed 3 months ago

craigberry1 commented 3 months ago

Add reserved 2 byte padding to OtherDouble read logic and read value length as 32 bit integer per spec.

Specifically section 7.1.2 that states: for VRs of AE, AS, AT, CS, DA, DS, DT, FL, FD, IS, LO, LT, PN, SH, SL, SS, ST, TM, UI, UL and US the Value Length Field is the 16-bit unsigned integer following the two byte VR Field. The value of the Value Length Field shall equal the length of the Value Field.

Meaning OD should be treated as: for all other VRs the 16 bits following the two byte VR Field are reserved for use by later versions of the DICOM Standard. These reserved bytes shall be set to 0000H and shall not be used or decoded. The Value Length Field is a 32-bit unsigned integer.

craigberry1 commented 3 months ago

Not doing this causes data elements after the first OD tag to be corrupted. This means reading and writing a file with OD will not produce a valid DICOM file:

image

netlify[bot] commented 3 months ago

Deploy Preview for dcmjs2 ready!

Name Link
Latest commit ec823b81c16135c1b6a5991e81e7beb82e3480c8
Latest deploy log https://app.netlify.com/sites/dcmjs2/deploys/66ba844dad15cf0008d2212e
Deploy Preview https://deploy-preview-399--dcmjs2.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

pieper commented 3 months ago

Thanks for working on this 👍

Even though it's a small file, let's put it in as a data release asset for consistency. Can you update the test, remove the dcm file and update the PR?

https://github.com/dcmjs-org/data/releases/tag/od-encoding-data

craigberry1 commented 3 months ago

Thanks for working on this 👍

Even though it's a small file, let's put it in as a data release asset for consistency. Can you update the test, remove the dcm file and update the PR?

https://github.com/dcmjs-org/data/releases/tag/od-encoding-data

Done 👍

github-actions[bot] commented 3 months ago

:tada: This PR is included in version 0.34.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: