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.

Sign up for GitHub

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

Jump to bottom

Add experimental HTJ2K TransferSyntax for reading to GDCM #5020

Merged
merged 7 commits into from
Feb 10, 2025
Merged

Add experimental HTJ2K TransferSyntax for reading to GDCM #5020

merged 7 commits into from
Feb 10, 2025

Conversation

blowekamp
Copy link
Member

@blowekamp blowekamp commented Dec 6, 2024 •
edited
Loading

Relates to #3983

The experimental support for HTJ2K has been merged upstream into GDCM. Update ITK to the latest GDCM release tag include this feature.

Adding it to ITK will allow the ITK community to better test the new feature.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:IO Issues affecting the IO module area:ThirdParty Issues affecting the ThirdParty module labels Dec 6, 2024
dzenanz
dzenanz approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Some of this should eventually go into GDCM upstream?

@dzenanz dzenanz mentioned this pull request Dec 6, 2024
Merged
7 tasks
@blowekamp
Copy link
Member Author

Some of this should eventually go into GDCM upstream?

Yes this should definitely go upstream. This effort was less than expected to get something functional, but there are likely details in the DICOM specs that I am not aware of that may may it more complicated for complete decoding support.

@thewtex
Copy link
Member

thewtex commented Dec 9, 2024

Great!!

issakomi
issakomi reviewed Dec 12, 2024
View reviewed changes
...es/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmTransferSyntax.cxx Outdated
TSField == MPEG4AVCH264BDcompatibleHighProfileLevel4_1
TSField == MPEG4AVCH264BDcompatibleHighProfileLevel4_1 ||
TSField == HTJ2KLossless ||
TSField == HTJ2KRPCLLossless
Copy link
Member

Choose a reason for hiding this comment

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

E.g. IsLossless() will return false for HTJ2KLossless, isn't it? Something is wrong here.

P.S. Just FYI, I don't plan to step into this stuff (I don't use GDCM / GDCMIO for my app), sorry.

@issakomi
Copy link
Member

Yes this should definitely go upstream.

AFAIK, upstream GDCM has internal OpenJPEG version 2.3, decoding of HTJ2K requires 2.5. Just FYI.

@blowekamp
Copy link
Member Author

Draft of GDCM changes submitted upstream: malaterre/GDCM#187

blowekamp and others added 7 commits February 7, 2025 08:56
@blowekamp
ENH: Remove GDCM's OpenJPEG source
a6dd845 
ITK's thirdparty library is used.
@blowekamp
ENH: Modify GDCM script for v3.0.25
9c4fa9f 
This update include the experimental HTJ2K codec, and completes
removal openjpeg source code.
@blowekamp
GDCM 2025-02-07 (41c9bd75)
d54614b 
Code extracted from:

    https://github.com/malaterre/GDCM.git

at commit 41c9bd758000da78b02a71569fb7dfd2d8a86007 (v3.0.25).
@blowekamp
Merge branch 'upstream-GDCM' into gdcm_update_htj2k
5b934cf 
# By GDCM Upstream
* upstream-GDCM:
  GDCM 2025-02-07 (41c9bd75)
@blowekamp
BUG: Fix GDCM for system OpenJPEG
40b16ac
@blowekamp
ENH: Add HTJ2K to GDCM ImageIO Compression types.
cd9047d 
Clarify documentation for support compression types for writing.
@blowekamp
ENH: Test GDCM HTJ2K reading
8066dc4 
Added test images from:
https://www.aliza-dicom-viewer.com/download/datasets

These may be a little large.
@github-actions github-actions bot removed type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:IO Issues affecting the IO module area:ThirdParty Issues affecting the ThirdParty module labels Feb 7, 2025
@blowekamp
Copy link
Member Author

@bradking This PR updates a third party library and removes a directory from the third party library. To do this I first removed the directory in ITK in a commit, updated the UpdateFromUpstream.sh script to remove then directory, them proceed as normal. Is this the correct process?

@blowekamp blowekamp marked this pull request as ready for review February 7, 2025 14:29
@blowekamp blowekamp requested a review from zivy February 7, 2025 14:30
@bradking
Copy link
Member

bradking commented Feb 7, 2025

@blowekamp yes, and the history structure LGTM. Good work.

Note that removing the directory from ITK first may not have been necessary, as the new import commit records removals that would be merged too. It won't hurt anything though.

@blowekamp blowekamp changed the title Prototype Add HTJ2K TransferSyntax to GDCM Add experimental HTJ2K TransferSyntax for reading to GDCM Feb 7, 2025
dzenanz
dzenanz reviewed Feb 7, 2025
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Mostly looks good.

zivy
zivy approved these changes Feb 7, 2025
View reviewed changes
Copy link
Member

@zivy zivy left a comment

Choose a reason for hiding this comment

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

LGTM

@dzenanz
Copy link
Member

dzenanz commented Feb 10, 2025

Should this be merged, Brad?

@blowekamp
Copy link
Member Author

Should this be merged, Brad?

Sure.

@dzenanz dzenanz merged commit ea9a3c9 into InsightSoftwareConsortium:master Feb 10, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@issakomi issakomi issakomi left review comments

@zivy zivy zivy approved these changes

@dzenanz dzenanz dzenanz approved these changes

@thewtex thewtex Awaiting requested review from thewtex

@malaterre malaterre Awaiting requested review from malaterre

@hjmjohnson hjmjohnson Awaiting requested review from hjmjohnson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@blowekamp @thewtex @issakomi @bradking @dzenanz @zivy