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

Fix rotated videos #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix rotated videos #41

wants to merge 1 commit into from

Conversation

po5
Copy link

@po5 po5 commented May 3, 2023

Closes #4

@NicolaSmaniotto
Copy link
Collaborator

This does not solve the issue for me, the thumbnail has the correct aspect ratio, but the image is still glitched.

Here is the result with the video in #4 (comment).

Screenshot_20230713_121649

@po5
Copy link
Author

po5 commented Jul 13, 2023

Weird, can you try it in thumbfast with a compatible UI? We do the same thing there and it seems to work.
Let me know if the PR is badly implemented or if thumbfast's fix itself is not sufficient.

@NicolaSmaniotto

This comment was marked as outdated.

@NicolaSmaniotto
Copy link
Collaborator

In any case, even if I'd like to investigate this further, it solves the issue sometimes and it's still a good enough reason to merge.

Comment on lines +239 to +240
local rotate = mp.get_property_number("video-params/rotate")
if rotate ~= nil and rotate % 180 == 90 then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
local rotate = mp.get_property_number("video-params/rotate")
if rotate ~= nil and rotate % 180 == 90 then
local rotate = mp.get_property_number("video-params/rotate", 0)
if rotate % 180 == 90 then

It's cleaner to have a default and avoid checking for nil.

Copy link
Author

Choose a reason for hiding this comment

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

This is indeed what happens with my test file if the thumbnails are generated before the video is rotated

Some insight from thumbfast, if we're missing rotate info on the first run, we queue another check in 0.05s (in effect the next even loop).
This is obviously hacky, it's a leftover of when thumbfast queried properties when needed instead of watching for changes and caching the value.
I need to rewrite this to call info() within watch_changes() when video-params/rotate changes, that way it's no longer racy. Though this logic may not be needed at all anymore as we delay announcing until video-out-params is available.

We should do a similar thing in mpv_thumbnail_script, return nil in get_thumbnail_size() until video-params/rotate is available (is it guaranteed to eventually exist?). There is already a check for video-dec-params/dw, video-dec-params/dh. The issue may go away if we make the check more robust.

@NicolaSmaniotto
Copy link
Collaborator

I was wrong, the test video I made created thumbnails that were too regular and hid the glitches. Here is another, the problem is still present:

New test file
untitled.mp4

Generated thumbnail:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vertical video aspect ratios are broken
2 participants