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

Update to use CharLS 2.0 #18

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

Conversation

sanjayankur31
Copy link

Various fixes to use CharLS version 2.0

(Squashable, but I've kept different commits to make the changes easier to see).

@jriesmeier
Copy link
Member

jriesmeier commented Aug 7, 2019

Thank you for the patch. As far as I can see, the CharLS 2.0 library itself is missing, right? So, how did you replace the DCMTK-specific version of CharLS 1.0 (contained in "dcmtk/dcmjpls") by CharLS 2.0?

If you've used the Debian version of DCMTK's source code, you might also interested in the following feature request: https://support.dcmtk.org/redmine/issues/344

@sanjayankur31
Copy link
Author

sanjayankur31 commented Aug 7, 2019

I'm afraid I haven't updated the bundled version of CharLS since the Fedora package does not use the bundled version. We've got patches downstream that let us use the system version of CharLS 2.0, and that's what I've tested against.

So, this PR only updates the dcmtk code to use the new CharLS API. It does not update the bundled version of CharLS (and the associated cmake files).

@jriesmeier
Copy link
Member

OK, thank you for further explanations. So, this means we cannot apply your patch before we've switched to CharLS 2.0 (which will not happen soon because version 2.0 requires C++11 or C++14).

@sanjayankur31
Copy link
Author

sanjayankur31 commented Aug 7, 2019

Sure, that's really the dev team's decision. We try not to carry different versions of libraries in Fedora, and since other projects that also depend on dcmtk (like gdcm) have already moved to CharLS 2.0, we had to look into moving dcmtk to CharLS 2.0 also.

We're using these patches on Fedora now, so it suggests that dcmtk works fine with the new CharLS version. Fedora already uses the default standard for gcc 9, which is C++14 (unless the dcmtk CMake files force a different version---I haven't checked this recently).

@jriesmeier
Copy link
Member

Thank you for confirming this. I've added a note to the above-mentioned feature request #344.
Debian people still seem to have problems with newer versions of the CharLS library.

@sanjayankur31
Copy link
Author

sanjayankur31 commented Aug 7, 2019

Thanks---it's quite possible that we see the same bug but just haven't run into it yet. Perhaps it's worth reporting the regression to CharLS so they may have a look? (Sorry, I don't have an account on the debian tracker, and I can't figure out how to get an account on the dcmtk tracker).

As soon as we have the build, I'll be able to get a reproducer ready. Still waiting for our patches to be merged downstream.

@jriesmeier
Copy link
Member

Maybe, you should contact Mathieu Malaterre (@malaterre) regarding this issue. He is also the author of gdcm.

@sanjayankur31
Copy link
Author

Maybe, you should contact Mathieu Malaterre (@malaterre) regarding this issue. He is also the author of gdcm.

Could the dcmtk devs please work with them? We're downstream and while we can look at API updates etc., we don't know the source well enough to take on actual development and bug fixing.

@jriesmeier
Copy link
Member

According to Feature #344:

The implementation strategy for DCMTK decided by the team will be to keep the internal CharLS version available in DCMTK (for platforms where CharLS is not available as an external library or compilers that do not support C++11/C++14) but provide an option to compile DCMTK either with the internal or with an external CharLS library. For the external CharLS library, we will primarily aim at supporting CharLS 2.x. This does require several modification of the build system, though, and these will only be implemented after DCMTK 3.6.5.

@sanjayankur31
Copy link
Author

sanjayankur31 commented Aug 21, 2019 via email

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

Successfully merging this pull request may close these issues.

2 participants