osc: define and add support for "thumbnailer" api#17518
osc: define and add support for "thumbnailer" api#17518N-R-K wants to merge 1 commit intompv-player:masterfrom
Conversation
|
To test this PR using thumbfast, use this glue script: -- glue code to make thumbfast work with the "thumbnailer" api
-- ideally should be merged into thumbfast once finalized.
local msg = require('mp.msg')
local utils = require('mp.utils')
local thumbfast = {
width = 0, height = 0,
overlay_id = -1,
}
local state = {
showing = false,
ass_showing = false,
}
local ass_overlay = mp.create_osd_overlay("ass-events")
local script_name = mp.get_script_name()
local function ass_draw(data)
local res_x, res_y = mp.get_osd_size()
ass_overlay.res_x = res_x
ass_overlay.res_y = res_y
ass_overlay.data = data
ass_overlay:update()
state.ass_showing = true
end
local function thumb_draw(t)
local path = thumbfast.thumbnail and (thumbfast.thumbnail .. ".bgra")
if not path or not utils.file_info(path) then
return false
end
mp.command_native({
"overlay-add",
thumbfast.overlay_id, t.x, t.y,
path, 0, "bgra",
thumbfast.width, thumbfast.height, 4 * thumbfast.width,
t.w, t.h
})
state.showing = true
return true
end
local function thumb_clear()
if (state.showing) then
mp.commandv("script-message-to", "thumbfast", "clear")
mp.command_native({"overlay-remove", thumbfast.overlay_id})
state.showing = false
end
ass_draw("")
end
mp.register_script_message("thumbfast-info", function(json)
local data = utils.parse_json(json)
if (type(data) ~= "table" or not data.width or not data.height) then
msg.error("thumbfast-info: received json didn't produce a table with thumbnail information")
else
thumbfast = data
end
end)
mp.observe_property('user-data/osc/draw-preview', 'native', function(kind, t)
if t == nil then
thumb_clear()
elseif t.hover_sec and t.x and t.y and t.w and t.h then
mp.commandv(
"script-message-to", "thumbfast", "thumb",
t.hover_sec, "", "", script_name
)
if thumb_draw(t) then
ass_draw(t.ass)
elseif not state.showing then
ass_draw('')
end
else
msg.error("user-data/osc/draw-preview: malformed property")
end
end)(Older versions of the glue script can be found in the comment edit history if needed) |
|
come on bro, that is exactly what i was doing in my pull request #17510 i am creating if this is not hypocrisy this i don't know what it is you are depending on you are making a osc depend on 3rd party script, my code is way better than yours, as it reveals the hover-time so custom script can do everything on their own but your PR will be merged mine not, good luck in advance |
this adds a "standard" api for ui scripts and thumbnailers to communicate with each other, based on the simple thumbfast api [1]. the api works as follows: * If a thumbnailer script is active, it will set `user-data/thumbnailer` property with the width and height of the thumbnail. UI scripts can observe this property to know the thumbnail w/h along with whether thumbnailer is active or not. * To issue a thumbnail draw command, the UI script will set the property `user-data/osc/thumbnailer` with `hover_sec`, `x` and `y` field set. hover_sec is the position in seconds where the user is hovering. x and y are top-left coordinates to draw the thumbnail in. * To clear the thumbnail, the UI script will set the previously mentioned property to `nil`. a more ideal api would make it so that the thumbnailer script only generates the thumbnail and doesn't need to draw at all. but this is a decent enough api that allows arbitrary thumbnailers and ui scripts to communicate between each other and work together. this change has been tested with work with thumbfast (using the "thumbfast-glue" script [4]). and for demonstration that this api can be useful outside of osc, it has also been tested to work on mfpbar's thumbnailer branch [3]. the code to determine thumbnail x,y is based on the osc fork inside of thumbfast [2]. [1]: https://github.com/po5/thumbfast?tab=readme-ov-file#for-ui-developers-how-to-add-thumbfast-support-to-your-script [2]: https://github.com/po5/thumbfast/tree/vanilla-osc [3]: https://codeberg.org/NRK/mpv-toolbox/src/branch/thumbnailer/mfpbar [4]: mpv-player#17518 (comment)
width and height should be decided by the UI script, not thumbnailer. |
This was chosen based on current script convention (I've checked |
ghost
mentioned this pull request
No matter what the convention is, special
This can be done by the UI script to advertise the desired size in advance. The thumbnailer then observes the property and pregenerate thumbnail whenever it changes. This also allows the UI script to change thumbnail size dynamically at runtime. |
That is true, but I'm also thinking of cases where thumbnailer might be getting pregenerated thumbnails (e.g from disk cache) or from web sources. So it'd need to do resizing (not impossible but will require a tangible amount of change in the existing thumbnailer scripts). Also if a thumbnailer does store disk cache, the width/height will directly affect the size of the cache. So from a user perspective, doesn't it make more sense for me to define these values via the thumbnailer config instead? (EDIT: also thumbnail width/height value in osc user's config will just become no-op if user doesn't install a 3rd party thumbnailer. So won't that also cause confusion for users?) I don't oppose adding width/height to osc on a theoretical basis though. But it will require more work, both on osc side and on thumbnailer scripts. I'll wait before making any more drastic changes since @po5 might provide more insight on this. |
This can be solved with script-opts. I agree with @na-na-hi that w/h request should come from UI script. It can ignore the request, it should be treated as a hint, and UI script should be able to handle resizing on its own, well this is already done because overlay-add will scale if needed, but it's better to not generate 4k images to scale it to 200x200 with bilinear, because it will look really bad. |
The UI script in principle only cares about controlling display size. The actual size of the image and the displayed size can be different in
They only need to documented to require a compatible thumbnailer.
There will be no much more work on osc side, only user config option for thumbnail size. In both cases UI script needs to calculate x/y from the thumbnail size. On thumbnailer side it can do the minimum required by changing the |
|
Unless I'm missing something, this API is basically that a 3rd party thumbnail script sets a thumbnail size it wants to render, and in return the OSC updates the mouse-hover-timepos when needed and X/Y of where the script should render at. I think it can be useful, but it puts burden on the OSC to communicate configurability which the user might want. For instance, currently the user cannot configure how far the thumbnail is from the osc box, etc, or anything else really, other than the size - which they can configure with the thumbnail script. Here's an alternative suggesiton: Instead of the OSC telling the script where to put the image, and over time also other user options which the OSC would have to communicate to the script, how about the OSC just giving the script the data which is enough to makes the decision where to draw, and leave the actual decision making and user-configurability to the script? If I read the code correctly, the only input which the OSC takes into account, other than the thumbnail size, is the layout. It has fixed relative position per layout, and that's it. Right? So relay the layout to the script, and let them make a configurable decision themselves, and deal with the user options to control that? But they don't even need that. They can already know what the OSC layout is - it's a property they could read themselves. It's nicer is the OSC could report it though, so I have no problem with that. But if there's one thing which might be harder for the script to know, it's the bounding box of the OSC. So I think a cleaner API would be that instead of advertizing hover-timepos and X/Y, to advertize hover-timepos and OSC layout name and OSC bounding box. EDIT: and remove the requirement from the script to send its desired size, because the OSC doesn't care anymore about that. I think it's plenty info for any 3rd party script to make a very informed decision, while taking away the burden from the OSC to maintain all the thumbnail user configurability and act as a mediator between the user wants and that script. |
It is simpler said than done. What information need to relayed? Seek bar geometry, other elements near seek bar, anything that the UI does not want to be obscured? I think it is pretty clear that only the osc has enough knowledge to know where to best position the thumbnail, and for a third party thumbnailer to make decision it has to know basically all geometry information of individual osc UI elements, including all of the "avoid zones". Different UI scripts can have completely different UI paradigm, which makes it difficult to generalize them into a generic "layout" information to relay to thumbnailer scripts. For example, it might want to show thumbnail at a fixed position instead of following mouse on seek bar. |
|
Exactly. The "thumbnailer" script should be UI agnostic, else it's basically tied to the specific implementation of OSC/UI. UI script may decide, to put thumbnail in any place, any z-order, add some deadzones, fade-in/fade-out, don't even track mouse. Why do we expect thumbnailer to have all this knowledge? If you are tinkering with your own script, you can make all sort of assumptions and maybe mouse position is the only information you need. Upstream solution however should be extensible and designed in a way that is reusable. If we design and implement thumbnaling interface that makes sense, it will be adopted by other scripts. If we hack around because it works, everyone will ignore it and keep forking osc. |
|
It may be worthwhile to consider what po5 wrote in tomasklaen/uosc#353 (comment) (and the whole thread in general). Seems like a lot of effort is required to make sure that the thumbnailer and osc are synchronized on everything but the proposed API makes sense. |
Theoretically, absolutely. If we're defining an interface between two arbitrary entities which don't know anything about eachother, of course it's not enough, and of course the UI part should specify both size and position. The "service" side should have absolutely no say at all. Not even the image size, because how can it know what are the minimum and maximum sizes (and aspect ratios) which will play nicely with the UI? The existing interface in this PR makes exactly the same assumption (of some existing non-communicated knowledge), just on a smaller scale. It allows the script to request arbitrary image size and aspect, and in some cases the OSC will have no useful reply as to where it wants the script to place the image so that it ends up useful to the user (think too small or too big).. But in practice, the mpv interface doesn't change every two weeks or years and probably also decades, In the context of mpv, the bounding box (yes, geometry - a rect) would cover all those needs ever since mpv have came to be, and that's also unlikely to change in the forseeable future. Please don't say windows title (and if you insist, then advertize the title geometry as well). mpv (but not mplayer) had rectangular-ish UI forever. And with rectangular geometry assumption it's a trivial task to cover all needs. Also, it doesn't have to be either or. The UI side could still advertise a hint about the preferred size and position, but still let the script side together with the user go wild. So every such script could have an option "respect OSC size/position hint", which would work even if mpv changes the OSC shape, layout and geometry every minutes. But the rest of the time, geometry alone will be enough for the script to support many wild configurations which might be useful for some users, but have no place in a world where the OSC is the sole mediator between the user needs and the 3rd party script. |
I agree, but I already addressed this in #17518 (comment).
The PR is designing a generic interface between UI scripts and thumbnailers. Even if the layout of osc.lua does not change much, the same cannot be said for third party scripts. Even under the assumption that all osc UIs are "rectangular", the "layout" proposal still needs to advertise at least a bounding box (forming an "allowed zone") to the thumbnailer. Compared to my proposal where only a defined destination rectangle is sent, the API is not simpler. Meanwhile, it makes user config more complicated: since the UI script defines the "allowed zone" and the thumbnailer decides where to draw in the "allowed zone", now the user has to configure both the UI script and the thumbnailer and understand the interaction between these scripts to achieve a desired placement. While in my proposal, all configuration can be done in the UI script. It also makes the responsibility unclear between the UI and thumbnailer scripts, which eventually complicates the implementation. For example, the
The responsibility is unclear, as all situations are valid. Now consider one OSC uses first approach, while another OSC uses the second approach. To make the thumbnailer work with both scripts, it has to contain the implementation of both, which increases code complexity, making it difficult for the thumbnailer to work with all OSCs which have this loose interface. A cleaner, stricter interface would eliminate this complexity.
What is the advantage of ignoring OSC size/position hint here? And in practice, the UI script can restrict the "allowed zone" to exactly the same as the hint, so it can force the thumbnailer to respect the hint anyway. |
I was referring to how the PR looks now, where the external script specifies the size. If you agree that it's not ideal from a perspective of "OSC should be in control", then great! :)
Well, more like forming an "exclusion zone" (the OSC bounding box - because the OSC could be in the middle, which leaves two "allowed zones" - above or below it), but sure, similar enough semantics.
Not sure I'm seeing this proposal. Do you mean where the OSC should specify X/Y/W/H (exact position and size) instead of only X/Y ? Also, I never made claims about the simplicity of the API, although I obviously prefer simpler over more complex. See next about what I claim and towards which goals.
Not exactly, because if we take this approach, then it's not about how the OSC can lie to the script. It's about coordinating in a way which is beneficial to both sides - where the OSC advertizes where its located on screen, so that the script can respect that and draw the thumbnail without occluding the OSC. As for "what is the advantage", quite a lot IMO. The way I see it, the beauty of mpv with regards to extensions is that it allows 3rd party scripts to go wild. Use their imagination, and come up with new and useful things to do. It's a symbiosys where mpv gives 3rd party scripts a lot of tools at their disposal (all the client API commands/properties, and builtin scripting support), and also giving them a very long leash about what they're allowed or not allowed to do, without being too opinionated about it, while trying to balance that with not too many obvious foot-guns. The clients/scripts authors side is to use their imagination to come up with useful and beautiful mpv extensions. This has worked great so far, and many of the things we have today in mpv started as 3rd party scripts or inspired by such, like the builtin console, or the context menu, and even the OSC itself started its life an an external 3rd party script, and much of its original code and structure is still in The reason it could happen is because the line of responsiblity is largely well defined: mpv focuses on exposing quality info and tools, and clients use that to do useful things which the mpv maintainers never even considered possible. Everyone benefits. It's relatively easy for the mpv devs to expose good info and tools, 3rd party authors are happy to extend mpv imaginatively, and the rest of the users also benefit from this symbiosys. So back to the original question of "What is the advantage of ignoring OSC size/position hint here?" The potential value in that is that 3rd party authors can be more imaginative than mpv devs about where, how, and when to display the thumbnail, or anything else really. Hopefully they have enough info to not obstruct the OSC itself, because we're all in this boat together - mpv devs and 3rd party authors. We're not against eachother. We want to complement eachother. And in general, this separation of concerns also means less "dirty" maintenance load on mpv devs. It's very easy to expose the OSC position and maybe thumbnail size and position hints, while letting 3rd party scripts take it from there and go wild. It's much less fun to keep a tight leash over what the script can or can't do. First, because if it's too tight then 3rd party authors will try to circumvent that. But more importanly, because it means that the responsibility of managing every aspect of it, including user configuration, extensibility, etc, is now on the mpv-devs shoulders. The more control mpv asserts, the higher the burden of responsibility which comes with it, and it's an ongoing maintenance burden. mpv devs want to control thumbnail size and position? now it also has the responsibility to expose it as configurable. This includes size, relative position to something (to what?), how to scale it with the window size, whether to disable it under some conditions, etc, etc etc. Just see how many options the OSC has to realize that making UI configurable is a never-ending task, and a never-ending stream of dealing with users about it. So instead of trying to control everything as tightly as possible, I'm suggesting to figure out what is the line which reduces the maintenance burden on the mpv side in the long term, while still allowing scripts to be well informed and responsible so that they don't accidentally clash with things which are controlled by mpv - specifically the OSC. It's all about this balance and symbiosys. mpv focuses on providing good tools and info with a long leash, and script authors try to use this leash imaginatively but also responsibly. |
Here's one example. A 3rd party author decides to use this API to display a box which contains the subtitles/lyrics/conversation/chat which refers to the hover-timepos over the seek bar. They don't want it to move with the mouse. They want it to be on the top right, or top left, or bottom left. However possible and configurable but without obstructing the OSC itself, because that would suck for the user. So they only need the hover timepos and OSC bounding box so that they can play nice, but they don't care about the size/pos hint. Don't limit the functionality in advance only to what you can imagine now. Provide good tools/info, and let 3rd party authors take it from there. |
thumbnailer may not have an ability to scale thumbnails, when they are sourced externally or whatever. While UI most certainly have that ability. I don't know why this topic has spurn so heated discussion. Drawing thumbnails on top of existing UI in external scripts is a workaround forced by the OSC limitations. All scripts run in own event loops, they are not synchronized, nor they can block another script (well they can, but it's not the design we want). This naturally causes UI elements to be drawn at different ticks. Unless you actively wait for Thumbnail generator should be passive module, not active. It should wait for image request. It can preload if possible. Most common solution is just to quantize timeline in N ranges and generate thumbs for them. Not for every millisecond of movie, but I digress. It's the UI script that draws, decorate, animate the image. It can move the image, fade it and so on, to provide smooth interface, without asking for thumbnailer script permission or update. It makes the communication layer simple as only the image request/response is made, not dozen of parameters how it should be drawn or where. Of course nothing stands in way of making UI modular, where the thumbnail UI handler is separate from main UI script, but this modularity is more of internal design choice, not the external API interface, as again the UI thumbnail handler needs to have all sorts of internal information about the UI itself. |
Yes.
This is flawed reasoning. The same goal can also be achieved when OSC decides where to draw, so it will obviously not occlude the OSC. Not only the OSC has more information on this factor so the placement end result is better, but the thumbnailer script also does not need to worry about placement which makes it simpler to implement. The benefit over your proposal is clear.
The mpv core is not opinionated on this matter. The command and property interface is as generic as possible, and does not need to care too much about what clients do, because they are not the direct interface between user and player for normal usage. OSC on the other hand is not part of the mpv core. It is a mpv client which uses the above mechanism to achieve a specific goal. This goal IS opinionated: the goal of OSC is to provide the user a good UI to interact with the player. It needs to ensure consistency of UI elements that are part of it.
Yes, the generic mechanism provided by the mpv core works great so far. It says little about the interplay between scripts, which is the topic here.
Yes, a 3rd party OSC script can also be more imaginative than mpv devs about where, how, and when to display the thumbnail, or anything else really. The interface I proposed still allows 3rd party OSC scripts to "go wild" and do whatever they want about the thumbnail placement. mpv devs can also selectively implement the good ideas imagined by them into osc.lua. Whether the placement is done by the OSC or by the thumbnailer is not important here compared to the ultimate goal, which is to serve user needs. Thus, the real benefit analysis hinges on the technical aspects between the two solutions, which I argue that placement by OSC is superior.
I consider this the only technical argument against the placement by OSC solution, that it increases the burden of mpv developers if osc.lua has to implement lots of configurable options. However, keep in mind that 3rd party OSC scripts can "go wild" and implement what osc.lua is not supporting. This, as you said, "has worked great so far", with them doing "useful things which the mpv maintainers never even considered possible". The goal of osc.lua is to provide a good out-of-box experience for most users, without needing to satisify ALL user needs. The 3rd party scripts exist to fill the gap.
Here's one example. A 3rd party script A decides to use this API to display a box which contains the subtitles which refers to the hover-timepos over the seek bar. They don't want it to move with the mouse. They want it to be on the bottom right. Now a user installs these 3 scripts, hoping to see this information presented nicely. The user moves mouse to near the end of seekbar. What is shown is that the information from 3 scripts displaying on top of each other, which is not the user wants. This happens because all of these scripts compete within the same "allowed zone" without knowledge of each other. Instead, consider when the position is controlled by the OSC. The user specifies that contents from script A and B placed at bottom right, while C follows mouse. The OSC can then smartly allocate the zones for the scripts so that they are placed according to the placement instruction without obscuring each other. |
I have already answered this in #17518 (comment):
I think this is OK in concept for the UI script to handle drawing, but it requires image data sharing between the thumbnailer and the UI script, which is where this approach faces practical difficulty. Assuming the scripts are written in lua without calling native C functions, they would not be able to share This also requires the UI script to do manual resource management to free the resource when it is not needed, instead of being handled entirely by the thumbnailer. If the thumbnailer allocates the resources using native function calls, it is also responsible to keep them alive as needed, and the UI script is also reliant on using script message to the thumbnailer for freeing resources. All of these make the communication layer more complicated. |
|
Since we want the OSC to control the rendering has anyone considered just making the OSC itself generate the thumbnails through a builtin module (that other OSCs can import)? It shouldn't be slower with async commands, it would be easy to pass data to and from the module without sync issues and would be easy to enable for users without finding an external script.
Note that we don't want to downscale with |
I don't think it's heated at all. I think it's a great discussion. The comments are respectful and on topic, and there are different opinions which IMO is great for everyone. We didn't yet find something which everyone is happy with, but I still think it's a great discussion.
Correct, but this has other disadvantages, as described later in that post.
It's correct, but I disagree with this premise. My goal here is to allow 3rd party script to be imaginative with the builtin OSC, without having to provide their own OSC - which is exactly what they have to do now if they want thumbnails. This PR and discussion is about allowing third party scripts to do that without bringing their OSC with them.
I obviously agree. But I don't think this contradicts my suggestion in any way. Exposing the hover timestamp, osc bounding box, and if you really want to - also thumbnail pos/size hint already does all that, without meaningful additional effort currently or in the future, while still allowing 3rd paty scripts to go wild either with the builtin OSC or to bring their own OSC. The only difference from your proposal on a technical level, if I get it right, is whether we document the size/pos data as requirement or hint, and whether we also advertise the OSC bounding box, correct? If yes, do you consider it big enough difference to fight over it (insist on not advertising the box, and insist on calling size/pos a requirement) instead of satisfying both of us trivially? "strict" 3rd party thumnail scripts can still ignore the osc box and just use the size/info hint, but it gives so much more possiblities for other scripts to do really nice things responsibly with the builtin OSC. And on a non-technical level, I already explained my philosophy and goal. Good info which allows scripts to extend mpv responsibly. Without the osc box exposed, it makes it a lot harder for scripts to be responsible IMO. And yes, "bring your own OSC" is always an option, which is completely irrelevant to this discussion, because the sole goal of this PR is to allow 3rd parties to avoid exactly that.
You could use this kind of argument regardless of which interface you think is best, and regardless of this PR. There's also a responsibility of the user here. They should not enable two thumbnail scripts, or any two other scripts which end up degrading their experience. This is the value and price of freedom. Users can experiment and pick whatever works best for them, but also shoot their foot if they insist. mpv cannot control everything tightly enough to ensure that no two scripts can ever interfere with eachother. So I just don't think it's a valid argument. |
This is not inter-script communication issue, this is
This is normal software development. At least you don't depend of UI color implementation details. It is simple as giving two entry point, to get image and to free image. (same as create_osd_overlay() has remove()) This is also extensible to any number of solutions, because you can implement native cplugin that just gives memory address for you to use. The expectation from thumbnailer would be to prepare payload for use with overlay-add, but its UI script decision to call it.
I don't see it. Depending on the design, it can be as simple as |
Yes.
This PR is about making a generic interface between any OSC and thumbnailer scripts, and not specific to osc.lua. My argument in #17518 (comment) is still valid. The "OSC bounding box" interface implies an assumption on how the UI is laid out, which cannot serve as a generic interface for arbitrary UI scripts. I think what can be done is that for OSC scripts that satisify this "OSC bounding box" model, the box can be advertised, but it is purely optional, and for any script that does not advertise a bounding box the size/pos data becomes a requirement. And this is only after an investigation of all popular 3rd party OSC scripts that all of them fit into this model to warrant this becoming a generic interface.
All of them can be implemented in the OSC so the user can shoot their foot if they insist. It does not have to live in the thumbnailer. |
By drawing thumbnail in the thumbnailer script, this issue is avoided. It does not matter whether the issue is "inter-script communication issue" or not if it causes drawbacks.
It requires a bidirectional communication flow of request data -> receive data -> free data, 3 communication per thumbnail change, while drawing thumbnail in the thumbnailer script allows for one-way communication, 1 per thumbnail change. The inter-script communication creates delay as pointed out by tomasklaen/uosc#353 (comment). I agree that this solution is better when properly implemented, but this requires higher implemention complexity for both UI and thumbnailer scripts. |
I completely disagree. It's very specifically only about the builtin osc.lua. Anyone can bring their own osc with their own interface to a thumbnail or any other script. I see this PR as wanting to allow 3rd party thumbnailers to work with the builtin osc, without any claims or suggestions what other osc scripts should or shouldn't do. Obviously, if it proves useful, then third party OSCs might want to adopt it, but it's not mpv's job to tell them what to do.
We already agreed on that, and we can also agree that the builtin osc and all the others which we (I) know of are all rectangular, and always have been, and that's not very likely to change. Additionally, once there's a non-rectanrular osc, it can simply not advertise its bounding box. Or maybe yes advertise a bounding box even if it's a circle or a triangle or whatever else. It can be documented as "The OSC may advertise its bounding box if possible, to allow 3rd party scripts to integrate better if they want to add more content on screen. At this time, and probably as long as the builtin osc is rectangular, it does advertise it.". And this comment applies equally also to your point of view that this PR is about general interface between OSCs to whatever and not specific to our osc.lua at all. It's about being practical. I think it's silly to avoid something which is likely to be useful in a lot of contexts, including mpv itself in the past and likely future too, just because there might be an OSC which cannot or don't want to do it. If it can't, then it can't. It ends there. It's simply not suitable to this kind of integration, which is absolutely fine. |
Single direction communication will never be flexible. It's just a quick poc, see mum I can display image on my timeline. Also see, even if thumbnailer script is handling drawing on request, it still needs to be told when to call None of the above solutions are able to be synchronized correctly. Even if you make a script that basically wraps However, if the UI script has the data at hand, it can show/hide it at full framerate as it wants it, without any synchronization considerations. And only delay is that it needs to get the data from script (2 ticks), which I think shouldn't be longer than the extraction itself, so we already have inherit delay. If this is really a problem, thumbnailer script shouldn't be based on script-messages and instead have native interface. Maybe osc.lua should directly call helper binary from subprocess... |
If you implement passive model (much more complex) then active model is completely free. It's literally just an if statement once thumbnail has been acquired,
I haven't started working on this yet. So feel free to do so yourself and share your findings. |
|
i thought this pr was something serious, 91 comments, but i guess it is just giving time and position so other script can use to display thumbnails, so why are comments so twisted? |
|
I've created a separate discussion about the passive model to avoid side-tracking this PR: #17654. Feel free to continue the discussion about the passive model there andd/or share your findings (pinging @CogentRedTester, but invitation for discussion applies to everyone as well). As for this PR, I personally consider it ready for merge. If there are any technical issues with the PR, feel free to point them and I'm willing to work on it. But if there's still disagreement over whether this (active/simple) api should be merged at all or not, then that's not something I can do much about. |
this adds a "standard" api for ui scripts and thumbnailers to communicate with each other, based on the simple thumbfast api [1]. the api works as follows: * If a thumbnailer script is active, it will set `user-data/thumbnailer/enabled` property to true. * To issue a thumbnail draw command, the UI script will set the property `user-data/osc/thumbnailer` with `hover_sec`, `x`, `y`, `w` and `h` field set. hover_sec is the position in seconds where the user is hovering. x and y are top-left coordinates to draw the thumbnail in and `w` and `h` are width and height of the size to draw the thumbnail at (the actual backing thumbnail size may be different). x,y,w,h must be positive integers. * To clear the thumbnail, the UI script will set the previously mentioned property to `nil`. a more ideal api would make it so that the thumbnailer script only generates the thumbnail and doesn't need to draw at all. but this is a decent enough api that allows arbitrary thumbnailers and ui scripts to communicate between each other and work together. this change has been tested with work with thumbfast (using the "thumbfast-glue" script [4]). and for demonstration that this api can be useful outside of osc, it has also been tested to work on mfpbar's thumbnailer branch [3]. the code to determine thumbnail x,y is based on the osc fork inside of thumbfast [2]. [1]: https://github.com/po5/thumbfast?tab=readme-ov-file#for-ui-developers-how-to-add-thumbfast-support-to-your-script [2]: https://github.com/po5/thumbfast/tree/vanilla-osc [3]: https://codeberg.org/NRK/mpv-toolbox/src/branch/thumbnailer/mfpbar [4]: mpv-player#17518 (comment)
I don't know either, why we rejected #17510 instead of letting them improve it. All the discussion and we circle back to basic approach proposed in the original PR, just with xy instead of time position.
See the comment from @na-na-hi:
Is there a reason we need this stop gap solution merged now? Should the documentation say that this interface will be removed and shouldn't be used by scripts? Or do you intend to introduce multiple ways to do the same thing? I personally don't mind either way, and don't take this like bikeshedding, because for all I care we can have it as-is or not. The question however we should ask is:
|
I have responded to that already:
And to reiterate: passive model is more complex than the active one. So if you implement passive model, it'd be trivial to also implement the active one. So there wouldn't really be any additional burden on implementers due to having two models as opposed to just the passive one.
The api in this PR is almost a 1:1 mapping of the thumbfast api - which is already used by multiple ui scripts. You can see here that it was trivial to convert a ui script that uses thumbfast to use this thumbnailer api: https://codeberg.org/NRK/mpv-toolbox/compare/master...thumbnailer
No one "rejected" it. The author was just impossible to work with. |
Depends how you look at it, it creates confusion and duplication, unless you read 100 comments in this PR. I don't see why the basic feature like thumbnails needing multiple different models. I'm almost certain that once this basic model is merged, there will be zero incentive to work on anything else. I'm fine with that, but we should be clear about it. |
this adds a "standard" api for ui scripts and thumbnailers to communicate with each other, based on the simple thumbfast api [1]. the api works as follows: * If a thumbnailer script is active, it will set `user-data/mpv/thumbnailer/enabled` property to true. * To issue a thumbnail draw command, the UI script will set the property `user-data/mpv/thumbnailer/draw` with `hover_sec`, `x`, `y`, `w` and `h` field set. hover_sec is the position in seconds where the user is hovering. x and y are top-left coordinates to draw the thumbnail in and `w` and `h` are width and height of the size to draw the thumbnail at (the actual backing thumbnail size may be different). x,y,w,h must be positive integers. * To clear the thumbnail, the UI script will set the previously mentioned property to `nil`. a more ideal api would make it so that the thumbnailer script only generates the thumbnail and doesn't need to draw at all. but this is a decent enough api that allows arbitrary thumbnailers and ui scripts to communicate between each other and work together. this change has been tested with work with thumbfast (using the "thumbfast-glue" script [4]). and for demonstration that this api can be useful outside of osc, it has also been tested to work on mfpbar's thumbnailer branch [3]. the code to determine thumbnail x,y is based on the osc fork inside of thumbfast [2]. [1]: https://github.com/po5/thumbfast?tab=readme-ov-file#for-ui-developers-how-to-add-thumbfast-support-to-your-script [2]: https://github.com/po5/thumbfast/tree/vanilla-osc [3]: https://codeberg.org/NRK/mpv-toolbox/src/branch/thumbnailer/mfpbar [4]: mpv-player#17518 (comment)
|
It should be noted that I get a lot of flicker with this youtube thumbnail script, but not with thumbfast or mpv_thumbnail_script. I reproduced it on all of Xorg/Wayland/dmabuf-wayland AMD/Nvidia proprietary/Nouveau but neither N-R-K nor llyyr could reproduce it. |
Likewise I'm experiencing audio glitches when I scrub across the seek bar which, as in your case, I'm not experiencing on mpv_thumbnail_script and thumbfast's |
The audio glitch is an issue with the subprocess command itself. I was able to reproduce it with a dummy script that just calls subprocess with sleep 1 when I lower my alsa buffer size. @llyyr speculated this was due to @neeshy are you also experiencing visual flicker as well? Or just the audio glitch? |
|
Yes, flicker on the thumbnail but not the actual video. The flicker is only noticeable when scrubbing across large portions of the seek bar quickly enough. On my machine, this coincides with rising CPU temperatures and once my CPU temps max out I start getting the audio glitches that I mentioned. I assume this is caused by CPU throttling. Moving the cursor rapidly back and forth across a small width doesn't cause any apparent temperature increase or slowdown. Neither does moving the cursor slowly across the seek bar, however, after the cursor has traveled enough distance, I do notice one flicker per every 5 seconds or so on the seek bar. In this case though, the frequency of these flickers isn't enough to cause an issue. There's just a few milliseconds where no thumbnail is visible on the screen. Also, regardless of using |
I am still not seeing a definite answer to this:
And it is clear that the current script is not polished as multiple users have issues with it, while other 3rd party thumbnailers work perfectly for them. It would be better if you include your glue script instead, so users can use it with a mature 3rd party thumbnailer, and mpv can maintain a much smaller script instead of a 400+ line script. If there are any issues with 3rd party thumbnailers, it is better to improve them instead doing NIH here. |
I did answer it here #17518 (comment) and also mentioned in a previous comment that the initial reason was to have a working example within the repo. I'm assuming you mean to say that you're not convinced by the answer rather than not seeing it. In which case, I can drop the example thumbnailer, it's not essential for me.
There's zero reason to add a glue script here. It was only for meant for testing only. I'll just PR proper support for it to thumbfast directly once we can decide on the api. |
na-na-hi
left a comment
na-na-hi
left a comment
There was a problem hiding this comment.
Review under the assumption that the API is for mouse hovering timeline thumbnail only. I do not think the scope can be anything more under the current model.
The quoted post states the reasons why the script by itself is useful. It does not answer my question regrading maintenance burden and if a 3rd party thumbnailer can be used instead and thus eliminating the need for it. |
Correct. The passive model would be more flexible. But there's nothing inside mpv that'd make use of that flexibility yet (e.g storyboard, playlist thumbnail preview etc). I'm still not clear on what the consensus is w.r.t how involved mpv should be in this. Should it be as minimal as possible? Or should it be more ingrained and think about other 3rd party non-ui usecases not present in here? If it's the former then I consider the current API to be the minimal set of information that's required for ui and thumbnailer to work sensibly. If it's the latter, then feel free to move over to #17654 and/or open a PR. I personally won't be working on that in the near future. |
this adds a "standard" api for ui scripts and thumbnailers to communicate with each other, based on the simple thumbfast api [1]. the api works as follows: * If a thumbnailer script is active, it will set `user-data/mpv/thumbnailer/enabled` property to true. * To issue a thumbnail draw command, the UI script will set the property `user-data/mpv/thumbnailer/draw` with `hover_sec`, `x`, `y`, `w` and `h` field set. hover_sec is the position in seconds where the user is hovering. x and y are top-left coordinates to draw the thumbnail in and `w` and `h` are width and height of the size to draw the thumbnail at (the actual backing thumbnail size may be different). x,y,w,h must be positive integers. * To clear the thumbnail, the UI script will set the previously mentioned property to `nil`. a more ideal api would make it so that the thumbnailer script only generates the thumbnail and doesn't need to draw at all. but this is a decent enough api that allows arbitrary thumbnailers and ui scripts to communicate between each other and work together. this change has been tested with work with thumbfast (using the "thumbfast-glue" script [4]). and for demonstration that this api can be useful outside of osc, it has also been tested to work on mfpbar's thumbnailer branch [3]. the code to determine thumbnail x,y is based on the osc fork inside of thumbfast [2]. [1]: https://github.com/po5/thumbfast?tab=readme-ov-file#for-ui-developers-how-to-add-thumbfast-support-to-your-script [2]: https://github.com/po5/thumbfast/tree/vanilla-osc [3]: https://codeberg.org/NRK/mpv-toolbox/src/branch/thumbnailer/mfpbar [4]: mpv-player#17518 (comment)
this adds a "standard" api for ui scripts and thumbnailers to communicate with each other, based on the simple thumbfast api [1]. the api works as follows: * If a thumbnailer script is active, it will set `user-data/mpv/thumbnailer/enabled` property to true. * To issue a thumbnail draw command, the UI script will set the property `user-data/mpv/thumbnailer/draw` with `hover_sec`, `x`, `y`, `w` and `h` field set. hover_sec is the position in seconds where the user is hovering. x and y are top-left coordinates to draw the thumbnail in and `w` and `h` are width and height of the size to draw the thumbnail at (the actual backing thumbnail size may be different). x,y,w,h must be positive integers. * To clear the thumbnail, the UI script will set the previously mentioned property to `nil`. a more ideal api would make it so that the thumbnailer script only generates the thumbnail and doesn't need to draw at all. but this is a decent enough api that allows arbitrary thumbnailers and ui scripts to communicate between each other and work together. this change has been tested with work with thumbfast (using the "thumbfast-glue" script [4]). and for demonstration that this api can be useful outside of osc, it has also been tested to work on mfpbar's thumbnailer branch [3]. the code to determine thumbnail x,y is based on the osc fork inside of thumbfast [2]. [1]: https://github.com/po5/thumbfast?tab=readme-ov-file#for-ui-developers-how-to-add-thumbfast-support-to-your-script [2]: https://github.com/po5/thumbfast/tree/vanilla-osc [3]: https://codeberg.org/NRK/mpv-toolbox/src/branch/thumbnailer/mfpbar [4]: mpv-player#17518 (comment)
|
Updated to add an Also updated the glue code.
The glue code was doing some silly thing like overlay-removing right before overlay-adding again. It's also abusing thumbfast's api quite a bit by assuming that the returned thumbnail path doesn't change. I have a preliminary patch adding support in thumbfast directly which should work much better, but I need to review it properly first before sending a PR. |
this adds a "standard" api for ui scripts and thumbnailers to communicate with each other, based on the simple thumbfast api [1]. the api works as follows: * To issue a thumbnail draw command, the UI script will set the property `user-data/osc/draw-preview` with `hover_sec`, `x`, `y`, `w` and `h` field set. hover_sec is the position in seconds where the user is hovering. x and y are top-left coordinates to draw the thumbnail in and `w` and `h` are width and height of the size to draw the thumbnail at (the actual backing thumbnail size may be different). x,y,w,h must be positive integers. * To clear the thumbnail, the UI script will set the previously mentioned property to `nil`. a more ideal api would make it so that the thumbnailer script only generates the thumbnail and doesn't need to draw at all. but this is a decent enough api that allows arbitrary thumbnailers and ui scripts to communicate between each other and work together. this change has been tested with work with thumbfast (using the "thumbfast-glue" script [4]). and for demonstration that this api can be useful outside of osc, it has also been tested to work on mfpbar's thumbnailer branch [3]. the code to determine thumbnail x,y is based on the osc fork inside of thumbfast [2]. [1]: https://github.com/po5/thumbfast?tab=readme-ov-file#for-ui-developers-how-to-add-thumbfast-support-to-your-script [2]: https://github.com/po5/thumbfast/tree/vanilla-osc [3]: https://codeberg.org/NRK/mpv-toolbox/src/branch/thumbnailer/mfpbar [4]: mpv-player#17518 (comment)
this adds a "standard" api for ui scripts and thumbnailers to communicate with each other, based on the simple thumbfast api [1]. the api works as follows: * To issue a thumbnail draw command, the UI script will set the property `user-data/osc/draw-preview` with `hover_sec`, `x`, `y`, `w` and `h` field set. hover_sec is the position in seconds where the user is hovering. x and y are top-left coordinates to draw the thumbnail in and `w` and `h` are width and height of the size to draw the thumbnail at (the actual backing thumbnail size may be different). x,y,w,h must be positive integers. * To clear the thumbnail, the UI script will set the previously mentioned property to `nil`. a more ideal api would make it so that the thumbnailer script only generates the thumbnail and doesn't need to draw at all. but this is a decent enough api that allows arbitrary thumbnailers and ui scripts to communicate between each other and work together. this change has been tested with work with thumbfast (using the "thumbfast-glue" script [4]). and for demonstration that this api can be useful outside of osc, it has also been tested to work on mfpbar's thumbnailer branch [3]. the code to determine thumbnail x,y is based on the osc fork inside of thumbfast [2]. [1]: https://github.com/po5/thumbfast?tab=readme-ov-file#for-ui-developers-how-to-add-thumbfast-support-to-your-script [2]: https://github.com/po5/thumbfast/tree/vanilla-osc [3]: https://codeberg.org/NRK/mpv-toolbox/src/branch/thumbnailer/mfpbar [4]: mpv-player#17518 (comment)
11 participants
this adds a "standard" api for ui scripts and thumbnailers to communicate with each other, based on the simple thumbfast api 1.
the api works as follows:
user-data/thumbnailerproperty with the width and height of the thumbnail. UI scripts can observe this property to know the thumbnail w/h along with whether thumbnailer is active or not.user-data/osc/thumbnailerwithhover_sec,xandyfield set.nil.a more ideal api would make it so that the thumbnailer script only generates the thumbnail and doesn't need to draw at all. but this is a decent enough api that allows arbitrary thumbnailers and ui scripts to communicate between each other and work together.
this change has been tested with work with thumbfast (using the "thumbfast-glue" script below). and for demonstration that this api can be useful outside of osc, it has also been tested to work on mfpbar's thumbnailer branch 3.
the code to determine thumbnail x,y is based on the osc fork inside of thumbfast 2.