Skip to content

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

Merged
Lestropie merged 8 commits intomasterfrom
allow_DICOM_header_read_with_unsupported_transfer_syntax
Feb 10, 2025
Merged

DICOM: add null imageIO handler to allow parsing of DICOM data even with unsupported transfer syntax#2767
Lestropie merged 8 commits intomasterfrom
allow_DICOM_header_read_with_unsupported_transfer_syntax

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 previously approved these changes Jan 23, 2024
@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
daljit46
daljit46 previously approved these changes Feb 7, 2025
@Lestropie Lestropie added this pull request to the merge queue Feb 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 9, 2025
@Lestropie Lestropie dismissed stale reviews from daljit46 and themself via 6b9957a February 9, 2025 23:28
@Lestropie Lestropie self-requested a review February 9, 2025 23:28
@Lestropie Lestropie enabled auto-merge February 9, 2025 23:29
@Lestropie Lestropie requested a review from daljit46 February 9, 2025 23:29
@Lestropie Lestropie added this pull request to the merge queue Feb 10, 2025
Merged via the queue into master with commit 5c3d62d Feb 10, 2025
5 checks passed
@Lestropie Lestropie deleted the allow_DICOM_header_read_with_unsupported_transfer_syntax branch February 10, 2025 11:41
@Lestropie Lestropie restored the allow_DICOM_header_read_with_unsupported_transfer_syntax branch August 26, 2025 08:11
@Lestropie Lestropie deleted the allow_DICOM_header_read_with_unsupported_transfer_syntax branch August 27, 2025 00:02
@Lestropie Lestropie mentioned this pull request Sep 2, 2025
Lestropie added a commit that referenced this pull request Sep 3, 2025
- #2968: Absent
- #3049: Absent
- #2767: Partially absent
- # 3027: Almost all absent (the new tests were in place but that was all)
- #3047: Absent
- #3071: Absent
- #3011: Fill in gaps of changes that were applied to #3011 prior to its derivation from #2917.
- #2609: Fixed a couple of small omissions.
- #2602: Bits and pieces missing.
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.

4 participants