Skip to content

Videoio: FFMpeg remove locks from VideoCapure/VideoWriter::open() to fix 20114#23601

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
cudawarped:videocapture_threading
May 19, 2023
Merged

Videoio: FFMpeg remove locks from VideoCapure/VideoWriter::open() to fix 20114#23601
asmorkalov merged 1 commit intoopencv:4.xfrom
cudawarped:videocapture_threading

Conversation

@cudawarped
Copy link
Copy Markdown
Contributor

@cudawarped cudawarped commented May 10, 2023 •
edited
Loading

Remove locks added in #9006 from VideoCapure/VideoWriter::open() when using the FFMpeg backend to fix #20114.

Locks are removed if OPENCV_FFMPEG_IS_THREAD_SAFE==true.

Should I add a test? If so it would need to be skipped on Windows to avoid build bot failures and it may be confusing when it fails on user's machines?

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

@cudawarped cudawarped force-pushed the videocapture_threading branch from 47ae518 to 17c30e6 Compare May 10, 2023 14:38
opencv-alalek
opencv-alalek reviewed May 10, 2023
View reviewed changes
@cudawarped cudawarped force-pushed the videocapture_threading branch from 17c30e6 to 6711bf8 Compare May 11, 2023 05:10
@cudawarped cudawarped requested a review from opencv-alalek May 11, 2023 05:55
@cudawarped
Copy link
Copy Markdown
Contributor Author

cudawarped commented May 12, 2023 •
edited
Loading

@mshabunin Should all the FFmpeg environmental variables be documented somewhere? e.g.

OPENCV_FFMPEG_CAPTURE_OPTIONS
OPENCV_FFMPEG_DECODE_ATTEMPTS
OPENCV_FFMPEG_STREAM_ATTEMPTS
OPENCV_FFMPEG_WRITER_OPTIONS
OPENCV_FFMPEG_THREADS
...

@cudawarped cudawarped force-pushed the videocapture_threading branch from 6711bf8 to 41b2acd Compare May 12, 2023 07:32
@mshabunin
Copy link
Copy Markdown
Contributor

mshabunin commented May 12, 2023

@cudawarped , yes, ideally all available environment variables should be documented. But we do not have such page yet. Some information is scattered through the wiki.

mshabunin
mshabunin reviewed May 16, 2023
View reviewed changes
modules/videoio/src/cap_ffmpeg_impl.hpp
}

static bool isThreadSafe() {
const bool threadSafe = utils::getConfigurationParameterBool("OPENCV_FFMPEG_IS_THREAD_SAFE", false);
Copy link
Copy Markdown
Contributor

@mshabunin mshabunin May 16, 2023

Choose a reason for hiding this comment

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

static maybe?

Copy link
Copy Markdown
Contributor Author

@cudawarped cudawarped May 17, 2023

Choose a reason for hiding this comment

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

Will this make any difference if the the scope of threadSafe is limited to isThreadSafe?

Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek May 17, 2023

Choose a reason for hiding this comment

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

It depends on what we need.

  • static - make variable with "global" lifetime and execute rhs expression once
  • no static - read configuration on every call. We should not do that for performance sensitive cases, but there is no performance problem in the current case.

@cudawarped cudawarped force-pushed the videocapture_threading branch from 41b2acd to 99ef35a Compare May 17, 2023 05:22
mshabunin
mshabunin approved these changes May 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mshabunin mshabunin left a comment

Choose a reason for hiding this comment

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

I tried to run it and it works well. And I have one more suggestion 🙂 Maybe we should rename environment variable to work the other way: OPENCV_FFMPEG_DISABLE_LOCKS (default 0) or OPENCV_FFMPEG_ENABLE_LOCKS (default 1), so that its name would directly state what it does.

Though this is just naming suggestion, overall the PR looks good to me.

@asmorkalov asmorkalov merged commit c946285 into opencv:4.x May 19, 2023
@asmorkalov asmorkalov mentioned this pull request May 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mshabunin mshabunin mshabunin approved these changes

@opencv-alalek opencv-alalek opencv-alalek approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Windows FFMPEG back end VideoCapture::open() blocks VideoCapture::open() and VideoWriter::open() in other threads

4 participants

@cudawarped @mshabunin @opencv-alalek @asmorkalov