fix: render thumbnails on uosc side

[po5]

I've isolated what causes the occasional thumbnail/thumbnail border desync.
It's not a performance problem of either thumbfast or uosc.
It's simply because the two don't update at the same time.

I had anticipated this and provided the thumbfast-render script message for those who wanted to implement it.
Turns out it was unusable because mpv wouldn't let you pass the arguments it expects, and the clear command would remove the overlay straight away (undesirable in script render mode).

With these issues fixed, I've implemented it (in a hacky way) in uosc.
What this does is move all thumbnail and border rendering to render().
That way, the overlay is always updated at the same time (or as close to it as mpv will allow) as its border.
This seems to fix any lags between the two elements on my end, be it when moving really fast or when clearing the thumbnail.

Make sure you are on the latest thumbfast commit.

Is this something you would consider doing, or are you fine with the current slightly inaccurate thumbnails with a simpler implementation?

[christoph-heinrich]

If we end up going down this route it would make sense to send the updated time to thumbfast in a Timeline:on_mouse_move() instead of doing it in Timeline:render() to get slightly faster updates. thumbnail_render() would also become part of Thumbnail:render() I assume, although timing wise it would make sense to have overlay-add as close to the osd update as possible.

[po5]

Rebased on master and cleaned up commits.
I've been using this and haven't noticed desyncs.

Waiting for ideas on how to implement this in a less silly way. @tomasklaen

[hooke007]

Fine, I didn't notice I was using this PR before.
It would randamly happen only on Windows. po5/thumbfast#57 (comment)

[tomasklaen]

So if I understand it correctly, when we send mp.commandv('script-message-to', 'thumbfast', 'thumb', ... to thumbfast from Timeline:render(), there's a delay between us sending it and thumbfast executing overlay-add that causes desync?

In any case, I can see how requesting thumbnail inside Timeline:on_mouse_move() can speed stuff up. So here's my API proposal:

Thumbfast informs uosc about thumbnail dimensions via thumbfast-info on file load, just like it does now.

Timeline:on_mouse_move() sends thumbfast reqest {timestamp}, to which thumbfast responds with thumbfast-path {path} message back. This message handler then calls Timeline:set_thumbnail(path), which does:

function Timeline:set_thumbnail(path)
  self.thumbnail_path = path
  request_render()
end

And inside Timeline:render() if self.thumbnail_path exists, we use it along with thumbnail dimensions from thumbfast-info to do overlay-add.

Timeline:clear_thumbnail() would than set path to nil, clear the overlay, and send thumbfast clear message to delete the file.

This way we don't have to pass dimensions, coordinates, and other stuff back and forth and parse json on every thumbnail change. We just need the path to the thumbnail.

Does this sound good? Or am I missing something?

[christoph-heinrich]

The dimension of the thumbnail can change over time. In theory every thumbnail can have a different dimension (we calculate the real thumbnail dimension each time a file got generated), so the size certainly has to be part of set_thumbnail.

[tomasklaen]

99.9% time it'll be the same no? So just send thumbfast-info before thumbfast-path when it changes, and we're good.

[christoph-heinrich]

Sure, I just don't see the upside in having two separate messages for path and dimensions.
It's not like that necessitates json parsing, as script messages support multiple parameters.

function(width, height, path)
    width = tonumber(width)
    height = tonumber(height)
    ...
end

The path parameter can be optional. When using client rendering it sends the path, otherwise it only sends the dimensions.

That would be a breaking change to the API, but since there hasn't been a release yet, I don't think anyone expects a stable API yet.

[po5]

I don't understand this proposition, since the thumbnail path is unchanging for the current instance.

The thing we do need to update is accurate dimensions, and it's required that uosc knows the exact size of the thumbnail, else in the case that it thinks the thumbnail resolution is higher than it actually is, overlay-add will hang the entire player and need a restart.

The reason it currently uses a single JSON argument instead of multiple string ones is to not burden clients with more state management logic than is necessary. That's less relevant in a script that uses the full render control feature, I suppose.

If the idea of skipping JSON parsing is from a performance concern, thumbfast-info is called much less frequently (close to only when needed, like thumbnail resolution update) since po5/thumbfast@bb68b79.

We can get rid of the thumbfast-render message, but because we'll no longer know when an updated thumbnail is available, we'll have to constantly call overlay-add, reading the file over and over whenever the mouse is on the thumbnail (even when not moving, since we need to catch slow updates/wait for accurate seek to finish).
I've pushed a very lazy commit if you want to try that route.

[tomasklaen]

Ah sorry, I assumed the path changes to prevent some potential caching. Too much web dev in my life.

So, how often do dimensions change? Is there some noise where thumbnails differ from the previous by 1px or something?

If there's no noise and thumbnail dimensions change happens only on new files, or weird videos that change size sometimes, than I guess the best would be to just have these messages:

Receiving:

• thumbfast reqest {timestamp}

Sending:

• thumbfast-info {path} {width} {height} - when any of these 3 values changes, or nil, nil, nil when thumbnailing is not available at the moment
• thumbfast-new - when new thumbnail is generated to {path} after thumbfast reqest {timestamp}

I think that's all that UI needs to implement thumbnails. Any issues with this? For example, does mpv guarantee that the order of received messages matches the order in which they were sent? It'd be bad if new arrived before info, in which case we'd need

[christoph-heinrich]

does mpv guarantee that the order of received messages matches the order in which they were sent?

I did some testing on this a while back and the messages were always received in the correct order, but I don't think there is any guarantee for that in the documentation, so that may change in the future.

Afaik po5 would like to implement caching at some point, so the path will always change when a different thumbnail should be displayed. Sending a thumbfast-info {path} {width} {height} whenever a new thumbnail is available is all we really need. That works fine with caching, with dimension changes and doesn't rely on message ordering.

[tomasklaen]

All right, but since it's not just info, but also a call to render now, lets name it something like:

thumbfast-thumbnail {path|nil} {width|nil} {height|nil}

[tomasklaen]

@po5 so where do you stand on this? Is this API something you'd implement?

Thumbfast receiving:

• reqest {timestamp}

Thumbfast sending:

• thumbfast-thumbnail {path} {width} {height}

Thumbnail cleaning can than happen in the background on file close.

[po5]

I am worried about how easy it may be to crash the player with the overlay managed by the UI, and how related bugfixes may have to be implemented in each UI script instead of just thumbfast.
There will still be latency between when thumbfast sends its message and when the UI script receives it, which can lead to the UI's overlay-add failing and crashing if the file is being written to.

I've only had the player crash once under normal conditions with this really naive implementation despite much fooling around on random videos, but it's enough for me to believe that thumbfast's check_new_thumb() must be performed in the UI script's context.

-- in thumbfast.lua draw()
mp.commandv("script-message-to", "uosc", "thumbfast-thumbnail", options.thumbnail..".bgra", display_w, display_h, (4*display_w))

-- in uosc.lua
mp.register_script_message('thumbfast-thumbnail', function(path, width, height, stride)
	mp.command_native({"overlay-add", 12, 0, 0, path, 0, "bgra", width, height, stride})
end)

In reality a UI script wouldn't be calling overlay-add instantly, it would store the information for the next render, so the real latency would be greater.
I recently upgraded my machine and found it very hard to reproduce the initial issue of laggy backgrounds, wrapping the thumbfast render side in an add_timeout is a good way to emulate it, if you're in the same position.
After adding a timeout around the thumbfast-thumbnail message handler too I was able to reliably reproduce the crash when switching between two specific files on my system. I know the thumbnail path and info can be cleared on file change in the UI to avoid it, but the underlying issue exists for all attempts to display a thumbnail.

The solution that doesn't involve somehow injecting code from thumbfast into the UI would be to write each thumbnail to its own file, and only tell the UI about thumbnails after they've passed the check_new_thumb() sanity check.
Ideally with some sort of reception acknowledgement from the UI script for the timestamp/id of the last displayed thumbnail, so that thumbfast can do safe cleanup.

I think this is the less dumb approach, it does introduce some latency to displaying the thumbnail (script-message dispatch + UI script render loop), but that's necessary if we want synced ASS and overlay updates.
This model may also let us simplify some things in thumbfast, since we'll know the true lifetime of each thumbnail file.
This would also come with free caching, if we leave a customizable grace period before deletion. Or keep thumbnails until they get explicitly hidden, we know the user isn't looking through them. Cleaning thumbs after an unclean shutdown is trickier but that's a problem to think about later.

An idea I had very early on was to have UI script send thumbfast its desired border styling (any arbitrary ASS), which would solve our issue but it introduces friction and may not fit all use cases.
The border would be synced to the thumbnail, but the lot would still be desynced from the UI script's elements.
Shouldn't matter in most cases but I think it's ugly.

---

I propose this API.
thumbfast receives:
thumbfast-request <timestamp:float> <overlay_id:int>
<overlay_id> is the overlay id you intend to use. New requests for the same overlay_id let the last pending request finish and gets rid of unfulfilled requests. No hard relation to the overlay id, but I think its effect is clearer this way.
thumbfast-info already gave you the user's preferred overlay_id for the primary thumbnail.
A script is free to use separate IDs for each thumbnail to display multiple (think a gallery script).

thumbfast replies:
thumbfast-thumbnail <timestamp:float> <overlay_id:int> <path:str> <width:int> <height:int> <stride:int>
Multiple scripts can request thumbnails (think a gallery script, separate from the osc), the reply is broadcast to all scripts. You should check if the overlay_id is yours.
Upon receiving thumbfast-thumbnail, the UI script stores the info in state (Is thumbfast-info still necessary? We could merge the two now.) and calls overlay-add with those values at the appropriate time.

When a thumbnail needs to be hidden, the UI script calls overlay-remove then notifies thumbfast with:
thumbfast-discard <overlay_id:int>
In the background, thumbnail files will be cleared based on user configuration.

This requires minimal additions to existing scripts.
How can this be improved? thumbfast-info could go away.
This may be my highest word count on a Github comment.

[christoph-heinrich]

The problem I see with that is that unless you wait between sending seek commands until the new file is written out, there is no way of knowing to which timestamp that new file belongs. I suspect that waiting between requests will make the thumbnail generation noticeably slower, so that seems rather counterproductive for timeline thumbnails.

Also if thumbfast develops in to a more general thumbnail provider it would make sense to differentiate between fast and exact seeks in the request, because both can be useful depending on the application.

Regarding cleanup I don't think it's a problem to keep all thumbnail files around until mpv exits, because those files are pretty small anyway. As long as those thumbnails get stored in a ram disk (like they already are afaik) there is no need to deal with unclean shutdowns.

[N-R-K]

ram disk (like they already are afaik)

Not all distros have /tmp set to ram-disk by default (gentoo openrc, for example does not). Also thumbfast allows user to configure the thumbnail path, so I don't think it'd be wise to leave those files hanging around.

[tomasklaen]

I propose this API.

Seems fine to me. And yes, thumbfast-info is not needed anymore.

[christoph-heinrich]

Idea for dealing with unclean shutdowns:

thumbfast stores its thumbnails in a directory with the current process id.
On startup we get a list of all currently running mpv processes and their process ids (e.g. the ps command on linux, don't know about other platforms), and if there is a directory with a process id that does not belong to a currently running mpv instance, it gets deleted.