VideoCapture segfault fix for #22126 by danopdev · Pull Request #22137 · opencv/opencv

danopdev · 2022-06-20T19:06:41Z

resolves #22126

Pull Request Readiness Checklist

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
[ x ] 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

alalek · 2022-06-22T17:33:42Z

wants to merge 7 commits

Please rebase to have 1 commit only.

alalek

~~Please run reproducer locally first. Fix is not valid.~~ My bad

danopdev · 2022-06-23T13:07:13Z

I used this code for testing:

#include <opencv2/videoio.hpp>

using namespace cv;

int main() {
    VideoCapture vc = VideoCapture("cv2-crash.mkv");
    Mat frame;

    while(vc.read(frame)) {
        double value = vc.get(cv::CAP_PROP_POS_MSEC);
        printf("pos: %f\n", value);
    }

    vc.release();

    return 0;
}

The video is the one attached to the issue (#22126).

Before the patch (original 4.x) when I run the application it ends with:

pos: 163659.000000
pos: 163692.000000
pos: 163725.000000
pos: 163759.000000
pos: 163792.000000
pos: 163959.000000
pos: 163992.000000
Segmentation fault

After the patch:

pos: 179258.000000
pos: 179358.000000
pos: 179458.000000
pos: 179558.000000
pos: 179658.000000
pos: 179758.000000
pos: 0.000000
pos: 0.000000

(no segfault).

I also save all frames as jpeg and checked that they are ok: not distorted or cropped.

@alalek: Can you please provide me a link to a video that don't work so I can test it ?

NOTE: I rebased the code to have only one commit.

alalek · 2022-06-23T15:01:18Z

~~Check content of returned frame data (use video from the issue). Frame data is corrupted.~~

danopdev · 2022-06-23T15:26:07Z

I saved the frames a JPEG before and after the patch:

index: 00000 => the very first frame (1024x576 with both sources)
index: 03413 => the last frame before the segfault (894x504 with both sources)
index: 03414 => the very first frame after the segfault with the old source (894x504)
index: 03621 => the last video frame (640x360)

Sure the frame size changes (and it does with the original code too) but this is how the video was created.

before_00000

before_03413

after_00000

after_03413

after_03414

after_03621

alalek

Looks like it works. Thank you!

As a bug fix this patch should go into 3.4 branch first.
We will merge changes from 3.4 into 4.x regularly (weekly/bi-weekly).

Please:

change "base" branch of this PR: 4.x => 3.4 (use "Edit" button near PR title)
rebase your commits from 4.x onto 3.4 branch. For example:
git rebase -i --onto upstream/3.4 upstream/4.x
(check list of your commits, save and quit (Esc + "wq" + Enter)
where upstream is configured by following this GitHub guide and fetched (git fetch upstream).
push rebased commits into source branch of your fork (with --force option)

Note: no needs to re-open PR, apply changes "inplace".

alalek · 2022-06-23T20:13:27Z

modules/videoio/src/cap_ffmpeg_impl.hpp

            img_convert_ctx,
            sw_picture->data,
            sw_picture->linesize,
-            0, context->coded_height,


Lets use sw_picture->height here.

According to ffmpeg sample code: https://github.com/FFmpeg/FFmpeg/blob/n4.4.2/fftools/ffplay.c#L925-L926

BTW, from the same samples code there is no usage of coded_width/coded_height values with modern FFmpeg.
Perhaps we need to update sws_getCachedContext() call too (probably later, and align with "n0,8" ffmpeg version).

Actually I tried 3.4 as it is and it works like a charm. No segfault.
Even the issue #22145 is not present.
Maybe there are some specific 4.x changes that brakes it.

Code is different, but there is the same problem on 3.4: https://github.com/opencv/opencv/blob/3.4.18/modules/videoio/src/cap_ffmpeg_impl.hpp#L1409-L1411

Actually 3.4 always use "video_st->codec" but 4.x uses "context" and sometimes it mixes both like:

if( img_convert_ctx == NULL || frame.width != video_st->CV_FFMPEG_CODEC_FIELD->width || frame.height != video_st->CV_FFMPEG_CODEC_FIELD->height || frame.data == NULL ) { // Some sws_scale optimizations have some assumptions about alignment of data/step/width/height // Also we use coded_width/height to workaround problem with legacy ffmpeg versions (like n0.8) int buffer_width = context->coded_width, buffer_height = context->coded_height;

May be the issue is there, but for sure it's related to 4.x (3.4 is OK).

My bad. I don't know why but from time to time it pass without crashing.
But 3.4 can crash too.
I'll update the PR.

PR ready for 3.4 (builds and run correctly)

alalek · 2022-06-25T03:50:54Z

modules/videoio/src/cap_ffmpeg_impl.hpp

            picture->data,
            picture->linesize,
-            0, video_st->codec->coded_height,
+            0, video_st->codec->height,


Again, please use picture->height here.

According to ffmpeg sample code: https://github.com/FFmpeg/FFmpeg/blob/n4.4.2/fftools/ffplay.c#L925-L926

You are right, we use picture->data and picture->linesize so it make sense.
Done (build and run OK)

alalek

Thank you!

chercy1 · 2023-02-07T07:19:55Z

resolves #22126

Pull Request Readiness Checklist

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

[ x ] 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

Can you explain the reason for this problem? What caused cv2.VideoCapture segfaults?

danopdev mentioned this pull request Jun 20, 2022

cv2.VideoCapture segfaults #22126

Closed

asmorkalov requested a review from alalek June 21, 2022 06:14

asmorkalov changed the title ~~Issues 22126~~ VideoCapture segfault fix for #22126 Jun 21, 2022

asmorkalov added the category: video label Jun 21, 2022

alalek requested changes Jun 22, 2022

View reviewed changes

danopdev force-pushed the issues-22126 branch from 90d16a1 to b482aee Compare June 23, 2022 12:53

alalek reviewed Jun 23, 2022

View reviewed changes

danopdev force-pushed the issues-22126 branch from b482aee to 3bdd56e Compare June 24, 2022 18:24

danopdev changed the base branch from 4.x to 3.4 June 24, 2022 18:39

alalek reviewed Jun 25, 2022

View reviewed changes

issues-22126

2366f2c

danopdev force-pushed the issues-22126 branch from 3bdd56e to 2366f2c Compare June 25, 2022 13:14

alalek approved these changes Jun 25, 2022

View reviewed changes

opencv-pushbot merged commit 397e9bc into opencv:3.4 Jun 25, 2022

alalek mentioned this pull request Jun 26, 2022

(4.x) Merge 3.4 #22161

Merged

alalek mentioned this pull request Aug 21, 2022

(5.x) Merge 4.x #22408

Merged

Uh oh!

Conversation

danopdev commented Jun 20, 2022 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

alalek commented Jun 22, 2022

Uh oh!

alalek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danopdev commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danopdev commented Jun 23, 2022

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

chercy1 commented Feb 7, 2023

Pull Request Readiness Checklist

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danopdev commented Jun 20, 2022 •

edited by alalek

Loading

alalek left a comment •

edited

Loading

danopdev commented Jun 23, 2022 •

edited

Loading

alalek commented Jun 23, 2022 •

edited

Loading