Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FFmpeg cap consider rotation metadata 3.4 #17489

Merged

Conversation

Lapshin
Copy link

@Lapshin Lapshin commented Jun 6, 2020

Pull Request Readiness Checklist

Relates to #15499

Duplicates commit of #17452

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 OpenCV (BSD) 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
allow_multiple_commits=1

Copy link
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

return;
}
int angle = ffmpegCapture->rotation_angle;
if(!ffmpegCapture->rotation_auto || (angle % 360) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

OpenCV build for Windows used FFmpeg through dedicated wrapper DLL and internal fields are not visible. You have to use getProperty method to get values.

@Lapshin
Copy link
Author

Lapshin commented Jun 8, 2020

Is it allowed to change the declaration of cvRetrieveFrame_FFMPEG and CvRetrieveFrame_Plugin?

So, now it able to build, but some tests fail. I do not know where I can get testing video samples and how to run the tests.
Maybe I should also change tests because now image rotates automatically...

Please give me the introduction info about this

@asmorkalov
Copy link
Contributor

@Lapshin Thanks for the fix. I cherry-picked test for the new rotation test from #17500 and pushed to your branch. I'll check the tests status on CI and let you know.

@asmorkalov
Copy link
Contributor

i checked FFmpeg version and it looks like the rotation metadata is not supported by libav in Ubuntu 14.04. It's used for 3.4 testing. It looks like we need another branch inside FFmpeg code or skip the test for old systems. I'll check and come up with solution in mean time.

@Lapshin
Copy link
Author

Lapshin commented Jun 12, 2020

Hi @asmorkalov,

You can delegate it to me if you have not enough time

@asmorkalov
Copy link
Contributor

@Lapshin Sorry for delay. I investigated FFmpeg API and options to support libAV on Ubuntu 14.04. It looks like public API for ROTATIONMATRIX appeared in FFmpeg (libAV) in 2014 and is not present in the old Ubuntu. I'm discussing options with alalek and team members and return back in mean time.

@asmorkalov
Copy link
Contributor

@Lapshin @alalek I pushed extra checks to test and code itself to handle old FFmpeg correctly. Please take a look and comment.

@asmorkalov asmorkalov force-pushed the ffmpeg_cap_consider_rotation_metadata_3.4 branch from 32912e8 to 7423983 Compare June 22, 2020 16:00
@Lapshin
Copy link
Author

Lapshin commented Jun 23, 2020

Your changes are good for me
But I can't understand why the same tests fail on windows

@asmorkalov
Copy link
Contributor

Ci build for Windows does not re-build FFmpeg warpper each time, but use old version. We added extra fields to classes and it leads to binary incompatibly. I'm looking on the issue right now.

@asmorkalov asmorkalov self-requested a review July 13, 2020 11:31
modules/videoio/src/cap_ffmpeg.cpp Outdated Show resolved Hide resolved
@asmorkalov
Copy link
Contributor

@Lapshin I moved rotation step to cv::VideoCapture implementation for FFmpeg and restored FFmpeg wrapper interface for binary compatibility on Windows. Please check the latest version on your side.

@Lapshin
Copy link
Author

Lapshin commented Jul 20, 2020

Thank you, @asmorkalov. You did it :)

@asmorkalov asmorkalov self-requested a review July 20, 2020 16:01
@asmorkalov
Copy link
Contributor

@alalek The patch is ready for integration with squash. I fixed Windows test failure and related warnings. Windows builds will report "not supported" till FFmpeg wrapper rebuild.

@asmorkalov asmorkalov changed the title Ffmpeg cap consider rotation metadata 3.4 FFmpeg cap consider rotation metadata 3.4 Jul 21, 2020
@asmorkalov
Copy link
Contributor

@mshabunin Could you look at it.

@mshabunin mshabunin self-assigned this Jul 22, 2020
@mshabunin
Copy link
Contributor

@asmorkalov , could you please squash commits into two to preserve authors.

Alexey Lapshin and others added 2 commits July 23, 2020 09:27
- Add VideoCapture camera orientation property for mp4 videos with camera orientation meta.
- Add auto rotation for 90, 180, 270 degrees using cv::rotate
- Added test for automated rotation for  MP4 videos with metadata
- Fix 180 degrees rotation bug
- Moved rotation logic to cv::VideoCapture implementation for FFmpeg and restore binary compatibility with FFmpeg wrapper.
@asmorkalov asmorkalov force-pushed the ffmpeg_cap_consider_rotation_metadata_3.4 branch from 52c8ca3 to 7ed37b3 Compare July 23, 2020 06:34
@asmorkalov
Copy link
Contributor

@mshabunin Done.

@opencv-pushbot opencv-pushbot merged commit 5bfa43f into opencv:3.4 Jul 23, 2020
@mshabunin
Copy link
Contributor

@asmorkalov , thank you. Do we need to port this feature to master branch?

@asmorkalov
Copy link
Contributor

Yes, I expect that it can be merged as is with regular 3.4->master merge. Please let me know, if manual port is needed. I'll do it.

@mshabunin mshabunin added the port into 4.x is needed Label for maintainers. Authors of PR can ignore this label Jul 23, 2020
@alalek alalek mentioned this pull request Jul 28, 2020
return;
}

cv::rotate(mat, mat, flag);
Copy link
Member

Choose a reason for hiding this comment

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

cv::Mat / cv::rotate is not available for FFmpeg wrapper on 3.4 branch.
Build of Windows wrapper is broken

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.

None yet

5 participants