| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
Add jxl (JPEG XL) codec support #26379
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! The codec support will be definitely useful to many users.
|
I tried the patch with Ubuntu 24.04 (libjxl-dev:amd64 (0.7.0-10.2ubuntu6)) and observe the following issues:
|
|
I do not get any of the errors above, obviously (tested on Windows and Ubuntu 22.04), but I think I fixed them for your system as well, except for the last error which I do not see anything wrong, the function is defined in I see you used a very old version of the library. I used |
|
Cmake error on all platforms: |
This pull request expects to build
libjxl v0.7.0 had been released at 2022, so it is slightly old. And vckpkg supports v0.11.0/ latest release, And we can limit minimum version of libjxl for api and/or header compatibilty reason. |
…version handling and improve status messages
…de JPEGXL_FOUND check
|
fixed both, pls review again |
|
Test |
|
I can not reproduce the error, tried on Ubuntu 22.04 and Windows 10. |
|
There is some CMake issue on Mac. Looks like we have some instance installed there. See CI logs: |
|
It seems the version.h file does not exists even though the search location seems correct. I am not interested in supporting all ancient versions of the jxl library. |
|
Pls feel free to take this PR farther. I feel I can not help anymore with all the opencv integration details. |
|
I tried with 0.7.0 (from apt) and 0.11.0 (from vcpkg).
( Umm... it may be need to continue changes, so does new pull request from my repos need ? ) |
|
( Umm... it may be need to continue changes, so does new pull request from my repos need ? ) - yes. We will see CI status |
OK, thank you for your reply, I'll make new pull request from my repos and fix at tomorrow. |
|
Thank you all for helping we this. I have the sens we are very close to the merge now, but it seems there is still a build check failing, but not clear what. Is it related to JXL interface code so that I can try to help? |
|
Hi, could you open "Annotations" in the top of test results, please ?
New commit runs CI again, so it will be help about this problem. And I added |
|
Umm... it seems that another trouble is happend on CI server (windows-1) .
https://pullrequest.opencv.org/buildbot/builders/precommit_windows64/builds/107630/steps/Fetch%20opencv%20opencv/logs/stdio |
| if(NOT OPENCV_SKIP_JPEGXL_FIND_PACKAGE) | ||
| find_package(PkgConfig QUIET) | ||
| if (PkgConfig_FOUND) | ||
| pkg_check_modules(PC_JPEGXL QUIET jxl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried building standalone libjxl and it uses libjxl name for pkg-config, so this line fails to find it. I understand that this file is taken from another project, but perhaps we could tune it slightly for our needs.
Another thing in my opinion is that it mixes pkg-config and separate find_* calls, it could result in mixing incompatible library versions - some found by pkg-config, others from system root. Maybe it would be better to add two blocks:
# 1. try pkg-config
pkg_check_modules(...)
if(FOUND)
# create targets using pc
# success
end()
# 2. manual detection
find_path(...)
find_library(...)
...
if(..all components were found...)
# create targets using found components
# success
endif()| return true; | ||
| break; | ||
| } | ||
| case JXL_DEC_FULL_IMAGE: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that this event will occure before JXL_DEC_BASIC_INFO? I didn't find this guarantee in the libjxl documentation.
| #]=======================================================================] | ||
|
|
||
| if(NOT OPENCV_SKIP_JPEGXL_FIND_PACKAGE) | ||
| find_package(PkgConfig QUIET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is the same call in the root cmake-file:
Line 710 in 6873bde
| find_package(PkgConfig QUIET) |
We don't need to repeat it here.
|
I cannot commit into your repos directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 tested manually with Ubuntu 24.04 and apt-provided jpegxl.
| if (!pimg->isContinuous()) | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log warning at least.
| case CV_8U: | ||
| col = cv::Scalar(124, 76, 42, 192); | ||
| th = 3; // = 255 / 100 (1%); | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add assert to default branch.
| case CV_8U: | ||
| col = cv::Scalar(124, 76, 42, 192); | ||
| th = 3; // = 255 / 100 (1%); | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add assert to default branch.
Add jxl (JPEG XL) codec support opencv#26379 ### Pull Request Readiness Checklist Related CI and Docker changes: - opencv/ci-gha-workflow#190 - opencv-infrastructure/opencv-gha-dockerfile#44 See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work opencv#20178 - [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake
Add jxl (JPEG XL) codec support opencv#26379 ### Pull Request Readiness Checklist Related CI and Docker changes: - opencv/ci-gha-workflow#190 - opencv-infrastructure/opencv-gha-dockerfile#44 See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work opencv#20178 - [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake
Pull Request Readiness Checklist
Related CI and Docker changes:
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.