Skip to content

Patch for B-Frame seeking problems#6558

Merged
opencv-pushbot merged 1 commit intoopencv:masterfrom
lletourn:master
Jun 30, 2016
Merged

Patch for B-Frame seeking problems#6558
opencv-pushbot merged 1 commit intoopencv:masterfrom
lletourn:master

Conversation

@lletourn
Copy link
Copy Markdown

@lletourn lletourn commented May 19, 2016

resolves #4890

What does this PR change?

This change resolved the long standing issue #4890

I added on the issue a way to reproduce the bug and detect the problem. Here is a patch that fixes it.

I tried this modified code on x264 encoded videos with and without b-frames
I tried it on an xvid avi
I tried it on a QTRLE mov

They all worked.

This being said, I checked the git log on the file and I don't see any corner cases that would explain why packet.pts was used instead of picture->pkt_pts. In theory containers without dts or without pts should work with this code.

I also admit that I don't understand the use of 'first_frame_number' (not a corner case either).
Using best_effort_timestamp to resolve the current frame number makes it so decrementing first_frame_number from dts_to_frame_number is not needed. Is there something I'm missing?

I tried this with opencv compiled against ffmpeg 2.8.6 and 2.8.7

Thanks

@lletourn
Copy link
Copy Markdown
Author

lletourn commented May 19, 2016

I just compiled against FFMPEG 3.0.2 and it also compiles so I don't understand which version was used by the automatic builder.

That method comes from 'libavutil/frame.h' and has been there since FFMPEG 1.1.5.
frame.h is included through 'libavcodec/avcodec.h' which is included by opencv. So I'm confused.

@alalek
Copy link
Copy Markdown
Member

alalek commented May 19, 2016

Ubuntu 14.04 uses "libav" and there is no method with this name.

Difference between libav and ffmpeg in the "micro" version number (0+ vs 100+):

#if LIBAVCODEC_VERSION_MICRO >= 100
    ... ffmpeg code ...
#else
    ... libav code ...
#endif

@lletourn
Copy link
Copy Markdown
Author

lletourn commented May 19, 2016

Ah of course. I'll modify the patch.

Thanks

@lletourn
Copy link
Copy Markdown
Author

lletourn commented May 24, 2016

Also should I submit a separate patch to have this propagated in the 3.X branch?
I guess I should have patched the branch first instead of master?

@vpisarev
Copy link
Copy Markdown
Contributor

vpisarev commented May 25, 2016

@lletourn, thanks for the patch! the master is the right branch to target. We won't patch already made releases, but your patch will go to OpenCV 3.2

@alalek
Copy link
Copy Markdown
Member

alalek commented May 25, 2016

@lletourn Could you please squash commits into one.

@lletourn
Copy link
Copy Markdown
Author

lletourn commented May 25, 2016

If I want to write a test but need to add a video in the test_data
repository how can I submit there? A pull request?

Thanks
Louis
On May 25, 2016 09:12, "Vadim Pisarevsky" notifications@github.com wrote:

@lletourn https://github.com/lletourn, thanks for the patch! the master
is the right branch to target. We won't patch already made releases, but
your patch will go to OpenCV 3.2

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6558 (comment)

@lletourn
Copy link
Copy Markdown
Author

lletourn commented Jun 2, 2016

@alalek Don't you have the option to squash commits when merging the pull request to the main repo?
It's usually there when you accept the pull request. Either 'merge commits' or 'squash and merge commits'.

@alalek
Copy link
Copy Markdown
Member

alalek commented Jun 2, 2016

We don't use GitHub "Merge" button because we use different merge commit message and more strict policies about patched integration.
Also we can't do it on our side only, because GitHub will not change PR state to "merged". Also useful links to PR from "squashed" commits will not be added.

@lletourn
Copy link
Copy Markdown
Author

lletourn commented Jun 2, 2016

Thanks for the explanation it makes a lot of sense. I squashed the commits and pushed.

@alalek
Copy link
Copy Markdown
Member

alalek commented Jun 30, 2016

Thanks! 👍

@opencv-pushbot opencv-pushbot merged commit c03d778 into opencv:master Jun 30, 2016
opencv-pushbot pushed a commit that referenced this pull request Jun 30, 2016
@alalek
Merge pull request #6558 from lletourn:master
5346870
@PhilLab
Copy link
Copy Markdown
Contributor

PhilLab commented Jun 30, 2016

Thanks for the fix 👍 !

@lletourn
Copy link
Copy Markdown
Author

lletourn commented Jun 30, 2016

My pleasure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@alalek alalek

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Erroneous frame seek in VideoCapture::set (CAP_PROP_POS_FRAMES/CAP_PROP_POS_MSEC)

6 participants

@lletourn @alalek @vpisarev @PhilLab @opencv-pushbot @louis-sportlogiq