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

DICOM: add null imageIO handler to allow parsing of DICOM data even with unsupported transfer syntax #2767

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jdtournier
Copy link
Member

This at least allow mrinfo to run. Any operation that attempts to access the voxel data will of course fail.

…ith unsupported transfer syntax

This at least allow mrinfo to run. Any operation that attempts to access
the voxel data will of course fail.
@jdtournier jdtournier requested a review from a team December 4, 2023 14:45
@jdtournier jdtournier self-assigned this Dec 4, 2023
@jdtournier
Copy link
Member Author

For info: the motivation for this is to at least allow mrinfo to report the contents of a DICOM dataset even if the voxel data are stored in a compressed format that we can't currently handle (e.g. JPEG)

Lestropie
Lestropie previously approved these changes Jan 9, 2024
Copy link
Member

@Lestropie Lestropie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had wanted to test this on some local data with unsupported transfer syntax, but am now having trouble finding any. I do plan on generating some at some point using the local scanner in order to evaluate scope of current support and prospect of better support (eg. #2675) and to augment the testing suite, but have not yet found the time to do it.

I nevertheless checked the code at the time and was happy with it (over and above aef44bb). Only other thing to consider would be version-matching the documentation hyperlink, but that's not requisite for the PR.

@Lestropie Lestropie added this pull request to the merge queue Jan 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2024
@Lestropie
Copy link
Member

Actually, in retrospect, the documentation shows mrinfo failing due to unsupported transfer syntax, whereas with this change, mrinfo will succeed but others will fail. So that example should ideally be updated.

@Lestropie
Copy link
Member

Found some unsupported data. Works as intended at my end, though the user feedback is perhaps sub-optimal:

  • mrinfo issues the full warning, but then reports header information anyway, without any specific message about being able to read the header but not the intensity data;
  • Any other command first issues the same warning, then reports as error "No suitable handler to access data". This is because the same Exception class is thrown regardless of whether a handler is incapable of accessing an image vs. there was some issue in the appropriate handler reading the image. Transitioning to STL standard exceptions (which we've discussed, but is not a C++17 thing, and doesn't yet have its own Issue) would potentially facilitate disambiguation here. Not a dealbreaker though.

Lestropie
Lestropie previously approved these changes Jan 23, 2024
Copy link
Member

@Lestropie Lestropie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

57c0579 resolves 2 of the 3 concerns I raised about documentation & terminal messages. I think that improves self-consistency and maybe decodes what's going on a little bit for any users who manage to encounter it.

@Lestropie Lestropie requested a review from a team March 5, 2024 05:15
@Lestropie
Copy link
Member

This requires review by someone other than myself or @jdtournier as we can't approve our own changes to master.

@Lestropie Lestropie added this to the 3.0.5 updates milestone Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants