Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cornerstone parser library? #4

Open
DanielHeath opened this issue Jan 3, 2020 · 8 comments
Open

Cornerstone parser library? #4

DanielHeath opened this issue Jan 3, 2020 · 8 comments

Comments

@DanielHeath
Copy link
Contributor

I'll need to pull in cornerstone's dicomParser library ( https://github.com/cornerstonejs/dicomParser ) to use https://github.com/cornerstonejs/cornerstoneWADOImageLoader .

Would we get any substantial simplifications by using it here as well, or should we just parse twice?

@plwp
Copy link
Contributor

plwp commented Sep 1, 2020

It would fix #8, which is definitely a problem in the parsing library.

It seems to be much better maintained so it'd be a generally good idea to migrate anyway, also less code is better.

Anon.ts and Validator.ts would need to be updated to the new interface but I wouldn't think it'd be super complicated?

@DanielHeath
Copy link
Contributor Author

Looks like dicom-parser has no distinction between the 'meta' data and the rest (see https://github.com/cornerstonejs/dicomParser/blob/82573d94342e12b4bae4fcd2e93f91bb061474cf/src/parseDicom.js#L149 ).

Do we need a way to figure out which ones belong in the meta and which belong in the p10 datastream when writing?

@plwp
Copy link
Contributor

plwp commented Sep 1, 2020

Dicom doesn’t really make a distinction between metadata and data, it’s all just tags.

@DanielHeath
Copy link
Contributor Author

This repo has got a dicom dictionary which records what the expected format of all the tags are.

dicomParser doesn't, because it only unpacks elements on-demand and you have to ask it for the right type when you ask it to do the unpack. Instead, it stores the offset in the original stream and how to unpack it.

This is awkward when it comes to replacing a field with a new value (especially if the new value has a different length to the original).

So, converting it to use dicomParser will require a bit of fiddling.

One alternative might be to use dicomParser to pull out the image data, which is the only part that's failing to parse, but that seems pretty unsatisfying; the underlying bug seems likely to affect something else at some point.

@DanielHeath
Copy link
Contributor Author

DanielHeath commented Sep 1, 2020 via email

@plwp
Copy link
Contributor

plwp commented Sep 1, 2020

Looks like you’re right there’s a bunch of tags that sit at the start of the file.

http://dicom.nema.org/dicom/2013/output/chtml/part10/chapter_7.html

I guess we’d just have to whitelist the 0002 tags if that’s the case? They should probably be whitelisted anyway to be fair.

@DanielHeath
Copy link
Contributor Author

I had a try at swapping in the pixel data from dicom-parser (that is, keeping the existing parse, also reading the tag data via dicom-parser, then comparing them).

Based on that, it appears that the read is happening fine.

Based on instrumenting the writer, the write is also happening fine.

This leaves me marginally confused (perhaps some subtlety of how buffers are implemented is the culprit?)

@plwp
Copy link
Contributor

plwp commented Sep 13, 2020

I see what you mean about the fiddling to make it work...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants