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

Using length in sequences #20

Closed
ibaryshnikov opened this issue Oct 9, 2019 · 6 comments · Fixed by #22
Closed

Using length in sequences #20

ibaryshnikov opened this issue Oct 9, 2019 · 6 comments · Fixed by #22
Labels
bug This is a bug help wanted

Comments

@ibaryshnikov
Copy link
Contributor

There's a TODO
https://github.com/Enet4/dicom-rs/blob/master/object/src/mem.rs#L305
Is it clear how it should be implemented? It may fix #10

@Enet4
Copy link
Owner

Enet4 commented Oct 9, 2019

That relates to the case where the item start element provides the length of the subsequent object. Unless I am mistaken, there should still be an item delimiter at the end of this object, which means that the length does not have to be tracked for a program to parse each item in the sequence. If this is the case, nothing wrong should happen, Nope, there is no item delimiter at the end of this particular object, which would lead to the program thinking that all of the following elements are part of that item.

This doesn't seem to be related to #10, though. I will expand why on that issue.

@ibaryshnikov
Copy link
Contributor Author

Thanks for explanations! I also found some files to reproduce the behavior (DataSetSyntax(PrematureEnd) inside build_sequence)
folder: https://www.creatis.insa-lyon.fr/~jpr/PUBLIC/gdcm/gdcmSampleData/US_DataSet/Philips_US/
file: 2XV8RLGL_8b_RGB_PlanConfig0_uncompressed.dcm
transfer syntax: 1.2.840.10008.1.2.1\u{0}

@Enet4 Enet4 added bug This is a bug help wanted labels Oct 10, 2019
@ibaryshnikov
Copy link
Contributor Author

I checked the standard http://dicom.nema.org/medical/dicom/current/output/chtml/part05/sect_7.5.2.html
Sequence may contain Explicit Length or Undefinded Length. In case of Explicit Length there's no Sequence Delimitation Item in the end

@Enet4
Copy link
Owner

Enet4 commented Oct 24, 2019

Yes, I feared so. Thank you for pinpointing the part of the standard. Clearly we're against a bug which ought to be fixed with high priority.

@victorgabr
Copy link

I wish I could help with this issue, but my background is C++ (DCMTK) and Python (Pydicom). But as an idea, you could design this crate to allow the user to be responsible for providing particular information on edge cases. For example, TransferSyntaxUID, etc. The DICOM standard is huge, with a lot of deprecated/new specifications. It is quite difficult to cover all cases.

@Enet4
Copy link
Owner

Enet4 commented Nov 5, 2019

Just copying a reply from #21 for context.

Thank you for the suggestion. It is true that some flexibility on how the application handles edge cases should be desired. I just found a few TODOs mentioning this too. Adding a set of options on whether the parser should be pedantic or relaxed would indeed be useful.

In this case though, it's pretty much a bug, and I would say having it return an error consistently was as ideal as we could ask for. :)

After some inspection of the code, I think I'll be able to fix this without expending too much time. Expect a pull request soon-ish.

It is worth noting that two mechanisms need to be updated here, both related in some way: explicit item delimitation, and explicit sequence delimitation. And both are based on lengths in bytes, which means that the number of bytes already read needs to be tracked by the parser.

Enet4 added a commit that referenced this issue Nov 16, 2019
- keep track of sequence and item nesting in a stack
- check delimiters after parsing data value tokens
- add parser dataset test
Enet4 added a commit that referenced this issue Nov 16, 2019
- check for sequence/item endings after start if len = 0
- clarify TODO note regarding EOF
- more tests, uniformize common impl into helper fn
@Enet4 Enet4 closed this as completed in #22 Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is a bug help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants