Skip to content

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