Skip to content

Reenable filesystem for ios builds#20128

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
kikaxa:master
Jun 21, 2021
Merged

Reenable filesystem for ios builds#20128
opencv-pushbot merged 1 commit intoopencv:3.4from
kikaxa:master

Conversation

@kikaxa
Copy link
Copy Markdown
Contributor

@kikaxa kikaxa commented May 20, 2021

As per comment #20010 (comment)

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 other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to 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

# if (defined(TARGET_OS_OSX) && TARGET_OS_OSX) || (!defined(TARGET_OS_OSX) && !TARGET_OS_IPHONE)
# define OPENCV_HAVE_FILESYSTEM_SUPPORT 1 // OSX only
# if (defined(TARGET_OS_OSX) && TARGET_OS_OSX) || (defined(TARGET_OS_IOS) && TARGET_OS_IOS)
# define OPENCV_HAVE_FILESYSTEM_SUPPORT 1 // OSX, iOS only
Copy link
Copy Markdown
Member

@alalek alalek May 22, 2021

Choose a reason for hiding this comment

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

This condition is from 3.4 branch: https://github.com/opencv/opencv/blame/3.4/modules/core/include/opencv2/core/utils/filesystem.private.hpp#L19

relates #10018 (before any of 4.x releases)


Which functionality do you want to use on iOS?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We found ::fs:: , and (strangely) some parts of videoio not working.

Opencv builds and works fine in our (limited) test cases for ios, macos when OPENCV_HAVE_FILESYSTEM_SUPPORT=1.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

::fs::

This API should be considered as internal.
There is no goal to support FS utilities through Computer Vision Library.
Some of these utilities are available through C++17 filesystem features, consider using them instead: https://en.cppreference.com/w/cpp/filesystem


some parts of videoio not working

Do you mean a problem similar to #19910?

Copy link
Copy Markdown
Contributor Author

@kikaxa kikaxa May 26, 2021

Choose a reason for hiding this comment

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

::fs:: This API should be considered as internal.

I agree. I have checked and we do not use ::fs on ios.
As a note, the whole namespace is listed and documented in the documentation, without any mentions of being internal.

We use cv::glob() (core/utils.hpp included by default with the core module) extensively, i have checked the PR and it modifies this function as well

videoio

No, we had an explicit assert/error mentioning the filesystem being disabled. The only place i found skimming through the module is loadPlugin()/getPluginCandidates()
We call cv::VideoCapture/open()

@asmorkalov asmorkalov requested a review from alalek June 10, 2021 07:42
@alalek
Copy link
Copy Markdown
Member

alalek commented Jun 20, 2021

Rebased on 3.4 branch


disabled in #20010

this note is removed as incorrect

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