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

Thumbnail: thumbfast integration #62

Merged
merged 12 commits into from
Nov 6, 2022
Merged

Thumbnail: thumbfast integration #62

merged 12 commits into from
Nov 6, 2022

Conversation

po5
Copy link
Contributor

@po5 po5 commented Oct 29, 2022

Closes #43.
This adds support for thumbfast.

Did this over a month ago but finally making a PR for it.
I don't expect this to be merged as-is, as I'm sure my unfamiliarity with the codebase led to bad choices. But it works.

Hope you can guide me to clean this up.

Preview

thumbfast-progressbar.webm

@torque
Copy link
Owner

torque commented Nov 4, 2022

Hey, thank you for this PR, this is a feature I long wanted to implement but never got to. I'll take a look at this soon.

Right now this just consists of a clamp function, which is more
semantically intuitive than the old min(max()) dance.
This setting has been removed, since there is a property that actually
updates properly and dynamically based on which display the mpv window
is on. I figured this property existed, but I never bothered actually
adding it until now.
Why not? Since the animation property isn't enforced by UIElement
itself, it is weird that it was mandatory for all subclasses.
Not sure why someone would have thumbfast installed and not want to
have thumbnails, but overconfigurability is the name of the game on
this crazy train and I for one do not intend to derail the tracks
Since progressbar manages the display scale internally, the thumbnails
would display at the wrong position on high DPI displays because the
positioning would get scaled by progressbar and then again by
thumbfast. This has been worked around by exposing the unscaled values
in the Window and Mouse singletons.

I also cleaned up the implementation a little bit by eliminating some
lurking lua-isms and also removing some boilerplate functionality that
this didn't really need because it doesn't actually do any traditional
ASS-based OSD drawing. I may try to add animation to this in the
future, but I don't know how well that will work. I guess it will be
an experiment if I ever get around to it.
@torque
Copy link
Owner

torque commented Nov 5, 2022

The only real functional issue I ran into while playing around with this was that it was behaving incorrectly on high-DPI displays. This is because progressbar does some internal management of the OSD on high-dpi displays and thumbfast does as well, so essentially the positioning of the thumbnail was getting scaled twice. I ended up going down a bit of a rabbit hole in fixing this (including finally addressing the extremely long-standing issue of progressbar not automatically updating its scaling when switching between displays with different DPIs, so thanks for the motivation to fix that). Essentially the fix was simple, I expose the non-scaled window/mouse coordinates to the thumbfast call now, but it required touching various parts.

Since that fix was a bit more invasive, I went ahead and committed those changes directly to your PR. I hope you don't mind. I did end up making minor changes to the Thumbnail class you added, primarily:

  1. I cleaned out some boilerplate (mostly the animation stuff that the integration wasn't using anyway, but that I had sort of implicitly baked into the object inheritance stupidly
  2. I adjusted some minor lua-isms to their more moonscripty counterparts (e.g. ~= to !=)
  3. I updated the way the Thumbnail object is created. Since the thumbnail information comes in asynchronously in a callback, it's a little goofy to deal with, but it seems to work ok in practice
  4. I added an option to overall enable/disable thumbnails in line with the way the other UI widgets can be disabled

Thanks for submitting this PR and for developing thumbfast, it's really nice to have this functionality. With these changes, I'm ready to merge this, but if you're interested in doing any testing or other work on your end, let me know and I'll hold off for a bit.

src/Thumbnail.moon Outdated Show resolved Hide resolved
@po5
Copy link
Contributor Author

po5 commented Nov 5, 2022

Thanks for the hidpi stuff, I don't have such a display and wouldn't know how to fake it for testing.

The last commit 7297ab0 broke the timeline for me.
Top title still shows up fine. Works fine when disabled=true in thumbnail state (e.g. audio-only file where thumbfast doesn't want to run).
Sorry for the awful quality. Previous commit works.

mpvprogressbar.webm

Did some digging, it's because we can receive multiple thumbfast-info calls (when swiching videos, or if the aspect ratio changes mid-video).
Adding more than one Thumbnail element is what breaks it. Keep in mind we do need to update our thumbfast object to position everything correctly. Can't just ignore all calls after the first.

Currently most integrations draw a border around the thumbnail, to make it fit in better than just a floating image.
Is this something you would want to have?

@torque
Copy link
Owner

torque commented Nov 6, 2022

Did some digging, it's because we can receive multiple thumbfast-info calls (when swiching videos, or if the aspect ratio changes mid-video). Adding more than one Thumbnail element is what breaks it. Keep in mind we do need to update our thumbfast object to position everything correctly. Can't just ignore all calls after the first.

Hmm, yeah I was worried about something like this happening, but I actually did not encounter problems when testing with it. I did some simple checking with a hybrid playlist of one network video and one local video, but I didn't check multiple local videos in a row. Thanks for figuring out the cause, it should be simple to make it so that successive invocation of the thumbfast-info call updates the existing object rather than trying to create a new one.

edit: I wasn't actually able to reproduce the problem you were having even with somewhat more in-depth testing, but I made the change anyway since it should be a strict improvement regardless. I am curious if you still have the same problem after the latest update.

Currently most integrations draw a border around the thumbnail, to make it fit in better than just a floating image. Is this something you would want to have?

Yeah, I had this in mind as I tweaked the Thumbnail class to make it simpler to add a drawing underneath it, but I didn't actually end up implementing any drawing. I honestly don't think that the bare thumbnail feels terribly out of place with the general minimalism of this script, though there are obviously cases where having a border to differentiate it from the background would be good (e.g. a video/scene that is a flat color). I mentally put that in the "to do later" bucket along with experimenting with animating the thumbnail into place (which probably won't look great for layering reasons, among others). If it's something you'd like to work on though, I'd be happy to provide some guidance.

torque and others added 4 commits November 6, 2022 00:24
This gets emitted for new videos or aspect ratio changes etc, so let's
not jam a ton of thumbnail elements all over the place if we don't
need to.
This was having interesting effects when switching between network
files and local files (which leave `demuxer-cache-state` empty). This
may end up doing extra work since it doesn't cache the value, but I
expect the cache comparison would actually be more expensive than just
repeatedly clobbering the value when there's nothing to draw.
@po5
Copy link
Contributor Author

po5 commented Nov 6, 2022

It wouldn't draw the thumbnail when switching files with the cursor over the timeline but not moving. That's fixed.
The untold promise is that thumbfast will broadcast at least one info on file load, so this works.
Also accounts for the case where thumbnail dimensions may change for the already displayed frame, such as a script-message that lets the user adjust the thumbnail size, if I decide to add that feature later.

Also discovered #63 in the process.

Looks good to merge now.

For animating the thumbnail, the big problem is that image overlays can only be positioned with a precision of 1px, floats get truncated. Not well suited for smooth animations.

Like HoverTime, this is attached to the mouse position, so it doesn't
make sense to display it when the keyboard is used to show the UI.
This was a bit of an oversight on how this was redrawn previously. For
example changing between two videos with the same resolution but
different duration, the time hovered would not update until the mouse
moved.

In theory it's possible for the duration to change while a video is
playing I guess (but normally this would be for streamed video, for
which we don't present the hover time at all) so that's another edge
case potentially handled.
@torque torque merged commit 7459d8a into torque:master Nov 6, 2022
@torque
Copy link
Owner

torque commented Nov 6, 2022

Thanks again for the contribution!

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.

Thumbnails
2 participants