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

VideoPlayer: ffmpeg, fix calculating aspect ratio for 3d modes #10727

Merged
merged 1 commit into from Oct 20, 2016

Conversation

@FernetMenta
Copy link
Member

commented Oct 19, 2016

@afedchin

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

thanks. will test this tonight.

@@ -833,7 +833,13 @@ bool CDVDVideoCodecFFmpeg::GetPictureCommon(DVDVideoPicture* pDvdVideoPicture)
aspect_ratio = av_q2d(pixel_aspect) * pDvdVideoPicture->iWidth / pDvdVideoPicture->iHeight;

if (aspect_ratio <= 0.0)
{

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 19, 2016

Member

If aspect is calculated already then all code below will not work. so the correction doesn't work also.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 19, 2016

Author Member

this block is only executed if codec does not provide AR

@afedchin

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

I can't test with all my samples, but seems with this an anamorph video is displayed wrong.
I'll try to explain:

  1. for anamorph videos (1920x1080) we should use codec aspect without changes.
  2. for non anamorph videos (1920x2160 or 3840x1080) we should change aspect to *2 or /2
@afedchin

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

@afedchin

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

with this afedchin@3989890 I have correct aspect for anamorph and full stereo. as I said above I will test with more samples tonight.

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2016

your sample does not have stereo_mode set by ffmpeg. stereo_mode is set by CStereoscopicsManager::DetectStereoModeByString.
this is all rather hacky and a can of worms ...
we have to keep things clear and separated.

@afedchin

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

but we support both stereo flags from a container and file naming convention.

@FernetMenta FernetMenta force-pushed the FernetMenta:aspect branch from d3614f2 to 9b7e1ef Oct 19, 2016
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2016

but the one form file naming must not be used to determine aspect ratio. PR is updated.

@afedchin

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

this https://www.dropbox.com/s/an8v07r1t3k0e9h/3d.Kodi.test.mkv?dl=0 sample (anamorph stereo) still not work as expected.

@FernetMenta FernetMenta force-pushed the FernetMenta:aspect branch from 9b7e1ef to 643b552 Oct 19, 2016
@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2016

@afedchin I looked into ffmpeg code and noticed that AR already observes stereo modes. All samples I got work with this change.

@afedchin

This comment has been minimized.

Copy link
Member

commented Oct 19, 2016

after quick test seems it returns pre ffmpeg 3.x behavior

@FernetMenta

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2016

great, thanks

jenkins build this please

@FernetMenta FernetMenta merged commit 8b6a24f into xbmc:master Oct 20, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins4kodi You did a great job. Have a cookie.
Details
@FernetMenta FernetMenta deleted the FernetMenta:aspect branch Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.