dcmjs-org / dcmjs

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

feat: added metadata key to return #285

Closed Ouwen closed 2 years ago

Ouwen commented 2 years ago

Adding dcmjs to cornerstoneWADO requires knowledge of some metadata on a given tag. Current progress here: https://github.com/Ouwen/cornerstoneWADOImageLoader/tree/feat_added_dcmjs

Specifically replicating this line: https://github.com/Ouwen/cornerstoneWADOImageLoader/blob/939052f26d128d15e7d3edc4900984359b1da3ab/src/imageLoader/wadouri/getPixelData.js#L12

Which is determined if the pixel data tag is of a length 0xfffffff

pieper commented 2 years ago

Can you provide some more context for this? Maybe file an issue describing what this issue is and how why it needs to be fixed. Then this PR can be tagged as a fix for the issue.

Ouwen commented 2 years ago

Weird that the recorded length changes when written out. @pieper seems like some tag lengths change from 2 to 4 when being rewritten. Since length was never enforced, past tests didn't throw this error.

Not sure if this is intended behavior.

pieper commented 2 years ago

That sounds wrong. Intended behavior is lossless read/write.

Ouwen commented 2 years ago

I’ll do a check on the values in buffer format, but seems like the length changes even without my line changes.

Ouwen commented 2 years ago

Can you provide some more context for this? Maybe file an issue describing what this issue is and how why it needs to be fixed. Then this PR can be tagged as a fix for the issue.

Was on my phone and didn't see this. Yep I can make an issue about this! https://github.com/dcmjs-org/dcmjs/issues/286

Also, I did try an arraybuffer check on current master and it seems like some weird padding occurs during writing. The data might still be lossless, but the underlying buffer is not the same.

Ouwen commented 2 years ago

@pieper seems like the length is shortened for DS value representations. Seems like what happens is that the original dicom has "60.0" in string format, but this gets converted to Number(60.0) => 60.

Then during the writing the string is written as "60" and not "60.0"

Ouwen commented 2 years ago

Seems like both are valid dicoms according to NEMA

https://dicom.nema.org/dicom/2013/output/chtml/part05/sect_6.2.html A string of characters representing either a fixed point number or a floating point number. A fixed point number shall contain only the characters 0-9 with an optional leading "+" or "-" and an optional "." to mark the decimal point. A floating point number shall be conveyed as defined in ANSI X3.9, with an "E" or "e" to indicate the start of the exponent. Decimal Strings may be padded with leading or trailing spaces. Embedded spaces are not allowed.

Ouwen commented 2 years ago

I am probably going to close this PR as the original purpose was to determine if pixeldata was encapsulated. I'm not sure if there is an advantage to checking if the length is unknown 0xfffffff as in dicomParser (https://github.com/cornerstonejs/dicomParser/blob/141fbff6a55e8573a1a2c3251bbd9409d3fa375d/src/readDicomElementExplicit.js#L67-L69) vs checking the transfer syntax. But I will probably move forward with the transfer syntax lookup method.

https://github.com/dcmjs-org/dcmjs/blob/a0a0fd59cd67890f182b9dc1bb73609f7822b5d2/src/DicomMessage.js#L12-L30

Let me know if it is worth putting the DS read/write as a known issue or if it's a nofix.

pieper commented 2 years ago

Let me know if it is worth putting the DS read/write as a known issue or if it's a nofix.

Yes, there's been discussion of the issue of going back and forth between ascii and float before and we should try to make that lossless (see https://github.com/dcmjs-org/dcmjs/issues/175). I was thinking of keeping an extra field like we have now for _vrMap to keep the original string when reading so the Value is still a number but when writing out we can use the original string if it's still equal to (that is, a valid dicom encoding of) the Value. I understand this is what is done in pydicom.