-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
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.
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 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
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. |
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. mpvprogressbar.webmDid some digging, it's because we can receive multiple Currently most integrations draw a border around the thumbnail, to make it fit in better than just a floating image. |
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 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.
Yeah, I had this in mind as I tweaked the |
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.
It wouldn't draw the thumbnail when switching files with the cursor over the timeline but not moving. That's fixed. 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.
Thanks again for the contribution! |
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