Align opencv image codec option default values to upstream defaults #24814

pasbi · 2024-08-04T13:04:52Z

The OpenCV projects sets the default of this value to true, there is no reason to change this. If anything, not enabling this by default can lead to unexpected fails at runtime which can be hard to debug.

Summary

Changes to recipe: lib/opencv

Motivation

The OpenCV projects sets the default of this value to true, there is no reason to change this.
If anything, not enabling this by default can lead to unexpected fails at runtime which can be hard to debug.
See #24813.

Details

Changing the default value for option with_imgcodec_pfm from False to True.

Read the contributing guidelines
Checked that this PR is not a duplicate: list of PRs by recipe
Tested locally with at least one configuration using a recent version of Conan

The OpenCV projects sets the default of this value to true, there is no reason to change this. If anything, not enabling this by default can lead to unexpected fails at runtime which can be hard to debug.

CLAassistant · 2024-08-04T13:04:57Z

All committers have signed the CLA.

valgur · 2024-08-04T13:33:59Z

Should probably enable the other imgcodecs as well if they are enabled by default in OpenCV (I have not checked).

pasbi · 2024-08-04T15:49:16Z

Should probably enable the other imgcodecs as well if they are enabled by default in OpenCV (I have not checked).

I thought about that, too.
Unfortunately, it's not so simple and I'm afraid I need help to do that.
Some options aren't translated one-to-one from CMake to conan.
E.g., whether WITH_OPENEXR is enabled by default depends on the platform and more.
In conan you cannot set the with_jpeg2000-option to both jasper and openjpeg at the same time, however, CMake enables both WITH_JASPER and WITH_OPENJPEG by default.

Here is a quickly compiled overview (didn't double check it, take with care).

conan option	conan default	OpenCV default
with_avif	False	OFF
with_jpeg	"libjpeg"	WITH_JPEG=ON
with_png	True	ON
with_tiff	True	ON
with_jpeg2000	"jasper"	WITH_JASPER=ON, WITH_OPENJPEG=ON
with_openexr	True	((WIN32 OR ANDROID OR APPLE) OR BUILD_OPENEXR) OR NOT CMAKE_CROSSCOMPILING
with_webp	True	ON
with_gdal	False	OFF
with_gdcm	False	OFF
with_imgcodec_hdr	False	ON
with_imgcodec_pfm	False	ON
with_imgcodec_pxm	False	ON
with_imgcodec_sunraster	False	ON
with_msmf	True	NOT MINGW
with_msmf_dxva	True	WITH_MSMF

My suggestion is to update

with_imgcodec_hdr => True
with_imgcodec_pfm => True
with_imgcodec_pxm => True
with_imgcodec_sunraster => True

and leave the other options alone.
Looks like @SpaceIm should know best what to do.

SpaceIm · 2024-08-05T04:45:26Z

In conan you cannot set the with_jpeg2000-option to both jasper and openjpeg at the same time, however, CMake enables both WITH_JASPER and WITH_OPENJPEG by default.

Indeed, these 2 options are enabled by default in CMakeLists of OpenCV, BUT in cmake/OpenCVFindLibsGrfmt.cmake, there is a logic where openjpeg has precedence over jasper when both options are enabled (jasper branch is disabled when openjpeg is found).
So technically openjpeg should be the default value in conanfile of opencv. But in the meantime, before opencv 4.3.0, jasper was the default jpeg2000 backend (openjpeg was not supported by opencv actually).

What should be done:

default value of with_jpeg2000: openjpeg
in config_options(): set with_jpeg2000 to jasper if version < 4.3.0
in validate(): raise if with_jpeg2000 == openjpeg and version < 4.3.0

SpaceIm · 2024-08-05T04:53:16Z

And yes regarding all these with_imgcodec options, I don't see any reason to not change their default value to True. There default values in conanfile come from #6802

pasbi · 2024-08-05T09:04:22Z

@SpaceIm please have a look again.

jcar87 · 2024-08-05T10:44:32Z

Thanks @pasbi for noticing - indeed if these have no additional dependencies and they are enabled by default, perhaps the original PR that introduced the options was too conservative by setting them to False, although preserving the previous behaviour is also very valid.

Running this on CI.

I appreciate everyone's help on the matter and the detail discussion - however please consider that suggesting additional changes unrelated to the issue to fix does make reviewing more difficult for us which can cause delays.

The description says "Changing the default value for option with_imgcodec_pfm from False to True.", and seeing the changeset, it goes beyond that in ways that requires us to validate and understand the reasoning further. Anything related to the handling of jpeg2000 is unrelated to the original issue - and can delay reviewing.

recipes/opencv/4.x/conanfile.py

Co-authored-by: Martin Valgur <martin.valgur@gmail.com>

pasbi · 2024-08-05T11:29:48Z

@jcar87 thanks for answering.

My intention was just to update the default of with_imgcodec_pfm.
Though I think it makes sense to apply the other suggested changes in the same PR.

Merging this PR is not urgent for me, so I'm fine.
I have updated the PR title to reflect the more thorough changes.

jcar87 · 2024-08-05T14:30:38Z

@jcar87 thanks for answering.

My intention was just to update the default of with_imgcodec_pfm. Though I think it makes sense to apply the other suggested changes in the same PR.

Merging this PR is not urgent for me, so I'm fine. I have updated the PR title to reflect the more thorough changes.

Thanks for updating the title!
The original PR as it was, was very well motivated and the issue mentioned in the description #24813 is enough for us to double check in the OpenCV repository that this is enabled by default and that it does not have additional dependencies - so that on its own is easy to review.

But it becomes more difficult when we review the "files changed" tab - and there are changes that touch other options not mentioned in the description, and also changes the default value for a dependency depending on the version (which we need to validate separately).

They are valid changes by all means - but given the high volume of requests and the size of the backlog - those changes become secondary to the issue you have reported and the jpeg changes, altogether unrelated. Not your fault at all - we are trying to go through the backlog while at the same time we also need support from the community to ensure the PR descriptions have all the context needed for us for a quick review.

conan-center-bot · 2024-08-05T15:27:30Z

Conan v1 pipeline ✔️

All green in build 4 (8e6566cc84a9f8bb500944b4e59e395bcff23c63):

opencv/4.5.5:
All packages built successfully! (All logs)
opencv/4.9.0:
All packages built successfully! (All logs)
opencv/4.10.0:
All packages built successfully! (All logs)
opencv/4.8.1:
All packages built successfully! (All logs)
opencv/4.1.2:
All packages built successfully! (All logs)
opencv/4.5.3:
All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 4 (8e6566cc84a9f8bb500944b4e59e395bcff23c63):

opencv/4.10.0:
All packages built successfully! (All logs)
opencv/4.8.1:
All packages built successfully! (All logs)
opencv/4.9.0:
All packages built successfully! (All logs)
opencv/4.5.5:
All packages built successfully! (All logs)
opencv/4.5.3:
All packages built successfully! (All logs)
opencv/4.1.2:
All packages built successfully! (All logs)

github-actions · 2024-08-05T15:27:56Z

Hooks produced the following warnings for commit 8e6566c

opencv/4.5.5@#2d1655f8fc10bb3b9b0b7dd9d24bcb3c

post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio455d.dll' links to system library 'mf' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio455d.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio455d.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio455d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio455.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio455.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio455.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio455.dll' links to system library 'mf' but it is not in cpp_info.system_libs.

opencv/4.9.0@#3593ca7d1cdd0d449dfb9e5898f05a68

post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio490d.dll' links to system library 'mf' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio490d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio490d.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio490d.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio490.dll' links to system library 'mf' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio490.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio490.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio490.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.

opencv/4.10.0@#2e6d30371a742880ea9167a012697646

post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio4100d.dll' links to system library 'mf' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio4100d.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio4100d.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio4100d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio4100.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio4100.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio4100.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio4100.dll' links to system library 'mf' but it is not in cpp_info.system_libs.

opencv/4.8.1@#206beb6aefe57048135a68cd92e40ee8

post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio481.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio481.dll' links to system library 'mf' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio481.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio481.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio481d.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio481d.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio481d.dll' links to system library 'mf' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio481d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.

opencv/4.1.2@#b3ea5a040ac87445ec6b445c8c7c00e9

post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio412d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio412d.dll' links to system library 'mf' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio412d.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio412.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio412.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio412.dll' links to system library 'mf' but it is not in cpp_info.system_libs.

opencv/4.5.3@#e904666fbbb4a065bb741e53cbaa1484

post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio453d.dll' links to system library 'mf' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio453d.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio453d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio453d.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio453.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio453.dll' links to system library 'dxgi' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio453.dll' links to system library 'mf' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\opencv_videoio453.dll' links to system library 'mfreadwrite' but it is not in cpp_info.system_libs.

AbrilRBS

Thanks!

mologie · 2024-08-21T14:01:24Z

Unfortunately this PR breaks building OpenCV on iOS completely, because option with_jpeg2000 is accessed unconditionally on line 1218, but is deleted when the target is iOS, which this crashes in verify(). I will open a PR to address this shortly.

jcar87 · 2024-08-21T14:15:34Z

Unfortunately this PR breaks building OpenCV on iOS completely, because option with_jpeg2000 is accessed unconditionally on line 1218, but is deleted when the target is iOS, which this crashes in verify(). I will open a PR to address this shortly.

Thanks @mologie - we will be more careful in the future, should have followed the gut instinct of just solving the problem that was reported, rather than additional things that had nothing to do with it :(

mologie · 2024-08-21T14:21:56Z

No worries, thanks for helping out with Conan either way, was a quick fix after all o/

Our CI at @authenticvision happens to catch iOS-specific bugs in Conan recipes all the time since there unfortunately is no official CI that builds for mobile systems, which makes these bugs quite hard to spot during review.

default with_imgcodec_pfm to True

10dd111

The OpenCV projects sets the default of this value to true, there is no reason to change this. If anything, not enabling this by default can lead to unexpected fails at runtime which can be hard to debug.

conan-center-bot added the User-approval pending label Aug 4, 2024

This comment has been minimized.

Sign in to view

pasbi mentioned this pull request Aug 4, 2024

[!!] Access request #4

Open

pasbi added 2 commits August 5, 2024 11:02

set default value of with_imgcodec_* to True

3aa0788

with_jpeg2000 defaults to jasper if <4.3.0 else openjpeg

438ab64

This comment has been minimized.

Sign in to view

uilianries mentioned this pull request Aug 5, 2024

[bot] Update authorized users list (2024-08-05) #24823

Merged

valgur reviewed Aug 5, 2024

View reviewed changes

recipes/opencv/4.x/conanfile.py Outdated Show resolved Hide resolved

Update recipes/opencv/4.x/conanfile.py

8e6566c

Co-authored-by: Martin Valgur <martin.valgur@gmail.com>

pasbi changed the title ~~default with_imgcodec_pfm to True~~ Align opencv image codec options to upstream defaults Aug 5, 2024

pasbi changed the title ~~Align opencv image codec options to upstream defaults~~ Align opencv image codec option default values to upstream defaults Aug 5, 2024

conan-center-bot removed the User-approval pending label Aug 5, 2024

AbrilRBS assigned jcar87 Aug 5, 2024

AbrilRBS approved these changes Aug 5, 2024

View reviewed changes

SpaceIm approved these changes Aug 5, 2024

View reviewed changes

jcar87 mentioned this pull request Aug 6, 2024

[question] Rationale of default_options #24813

Closed

jcar87 approved these changes Aug 8, 2024

View reviewed changes

conan-center-bot merged commit 05abdad into conan-io:master Aug 9, 2024
27 checks passed

pasbi deleted the pasbi-with_imgcodec_pfm-to-True branch August 9, 2024 22:07

mologie mentioned this pull request Aug 21, 2024

opencv/4.x: fix building for iOS #25004

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align opencv image codec option default values to upstream defaults #24814

Align opencv image codec option default values to upstream defaults #24814

pasbi commented Aug 4, 2024

CLAassistant commented Aug 4, 2024 •

edited

Loading

This comment has been minimized.

valgur commented Aug 4, 2024

pasbi commented Aug 4, 2024 •

edited

Loading

SpaceIm commented Aug 5, 2024 •

edited

Loading

SpaceIm commented Aug 5, 2024 •

edited

Loading

pasbi commented Aug 5, 2024

This comment has been minimized.

jcar87 commented Aug 5, 2024

pasbi commented Aug 5, 2024 •

edited

Loading

jcar87 commented Aug 5, 2024

conan-center-bot commented Aug 5, 2024

github-actions bot commented Aug 5, 2024

AbrilRBS left a comment

mologie commented Aug 21, 2024

jcar87 commented Aug 21, 2024

mologie commented Aug 21, 2024

Align opencv image codec option default values to upstream defaults #24814

Align opencv image codec option default values to upstream defaults #24814

Conversation

pasbi commented Aug 4, 2024

Summary

Motivation

Details

CLAassistant commented Aug 4, 2024 • edited Loading

This comment has been minimized.

valgur commented Aug 4, 2024

pasbi commented Aug 4, 2024 • edited Loading

SpaceIm commented Aug 5, 2024 • edited Loading

SpaceIm commented Aug 5, 2024 • edited Loading

pasbi commented Aug 5, 2024

This comment has been minimized.

jcar87 commented Aug 5, 2024

pasbi commented Aug 5, 2024 • edited Loading

jcar87 commented Aug 5, 2024

conan-center-bot commented Aug 5, 2024

Conan v1 pipeline ✔️

Conan v2 pipeline ✔️

github-actions bot commented Aug 5, 2024

AbrilRBS left a comment

Choose a reason for hiding this comment

mologie commented Aug 21, 2024

jcar87 commented Aug 21, 2024

mologie commented Aug 21, 2024

CLAassistant commented Aug 4, 2024 •

edited

Loading

pasbi commented Aug 4, 2024 •

edited

Loading

SpaceIm commented Aug 5, 2024 •

edited

Loading

SpaceIm commented Aug 5, 2024 •

edited

Loading

pasbi commented Aug 5, 2024 •

edited

Loading