Skip to content

Refine logic of parsing PNG version#24754

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
FantasqueX:refine-png-version-parsing
Dec 25, 2023
Merged

Refine logic of parsing PNG version#24754
asmorkalov merged 1 commit intoopencv:4.xfrom
FantasqueX:refine-png-version-parsing

Conversation

@FantasqueX
Copy link
Copy Markdown
Contributor

Currently, if PNG_FOUND, cmake scripts will check include and parse header while we can use PNG_VERSION_STRING conveniently. If BUILD_PNG, parse version from PNG_LIBPNG_VER_STRING directly is more convenient than parsing major, minor and patch and concatenate them.

The comment of png.h also supports this.

/* These should match the first 3 components of PNG_LIBPNG_VER_STRING: */

https://github.com/glennrp/libpng/blob/libpng16/png.h#L287

This patch also modifies ocv_parse_header_version macro to receive another parameter to make it more general.

The reason why changing PNG_VERSION to PNG_VERSION_STRING is to be consistent with cmake's FindPNG.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • 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
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@FantasqueX
Copy link
Copy Markdown
Contributor Author

I don't know why there is a cmake warning. How can I reproduce the warning?

@opencv-alalek
Copy link
Copy Markdown
Contributor

How can I reproduce the warning?

They are CI internal checks that build configuration is not changed by mistake.
A bit outdated repo is here: https://github.com/opencv-infrastructure/opencv-ci-scripts/blame/4.x/cmake_checks/precommit_linux64/POST_FINALIZE.cmake#L109
With this patch we lost HAVE_LIBPNG_PNG_H variable.
This variable is still used in the source code.

Need to investigate if could remove that from everywhere.

Currently, if `PNG_FOUND`, cmake scripts will check include and parse
header while we can use `PNG_VERSION_STRING` conveniently. If
`BUILD_PNG`, parse version from `PNG_LIBPNG_VER_STRING` directly is more
convenient than parsing major, minor and patch and concatenate them.

The comment of png.h also supports this.
```
/* These should match the first 3 components of PNG_LIBPNG_VER_STRING: */
```
https://github.com/glennrp/libpng/blob/libpng16/png.h#L287

This patch also modifies `ocv_parse_header_version` macro to receive
another parameter to make it more general.

The reason why changing `PNG_VERSION` to `PNG_VERSION_STRING` is to be
consistent with cmake's FindPNG.

This patch removes `HAVE_LIBPNG_PNG_H` variable because `PNG_INCLUDE_DIR`
is where to find png.h, etc according to
https://cmake.org/cmake/help/latest/module/FindPNG.html.

This patch also removes `PNG_PNG_INCLUDE_DIR` variable which is an
advanced variable used in cmake's FindPNG and is not used in opencv.
@FantasqueX FantasqueX force-pushed the refine-png-version-parsing branch from c8ef35f to 4546f40 Compare December 23, 2023 08:06
@FantasqueX
Copy link
Copy Markdown
Contributor Author

@opencv-alalek Thanks for the review. I think we can safely remove HAVE_LIBPNG_PNG_H . This variable is only used to decide how to include png.h #include<png.h> or #include<libpng/png.h>. However, according to cmake's FindPNG document https://cmake.org/cmake/help/latest/module/FindPNG.html, PNG_INCLUDE_DIR is where to find png.h. So, we can use #include <png.h> directly.

This patch removes PNG_PNG_INCLUDE_DIR as well. It is an advanced variable used in cmake's FindPNG and is not used in opencv.

Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you for contribution!

@opencv-alalek opencv-alalek self-assigned this Dec 25, 2023
@asmorkalov asmorkalov merged commit d9d4029 into opencv:4.x Dec 25, 2023
@FantasqueX FantasqueX deleted the refine-png-version-parsing branch December 26, 2023 05:48
@asmorkalov asmorkalov mentioned this pull request Jan 19, 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