po5 / thumbfast

High-performance on-the-fly thumbnailer script for mpv
Mozilla Public License 2.0
760 stars 34 forks source link

Draw and file handling rework #36

Closed christoph-heinrich closed 1 year ago

christoph-heinrich commented 1 year ago

This is a fundamental change to how drawing and file changes are handled. The thumbnail now follows the cursor better without any unnecessary draw calls. Only valid thumbnail files are now promoted to .bgra.

Before:

https://user-images.githubusercontent.com/8932183/193477097-7cd5b98d-ee1a-4eba-8ed6-95038f09244e.mp4

After:

https://user-images.githubusercontent.com/8932183/193477094-dbc08b65-ab85-4ed2-bb12-df3020d9fe19.mp4

Depends on #31

po5 commented 1 year ago

Wrong position/orientation on file change, both files have the same resolution and orientation.
While moving the cursor: Ah-le-le  Detective Conan Movie 01 - The Time Bombed Skyscraper (4K Digital Remastered) (BDRip 1080p 10bit FLAC TrueHD)  5959B33F mkv_000002 920 Cursor still: Ah-le-le  Detective Conan Movie 01 - The Time Bombed Skyscraper (Remastered) (BDRip 1080p HEVC FLAC TrueHD)  7FE69DA0 mkv_000000 626

christoph-heinrich commented 1 year ago

I found a little problem in info(), but I doubt it helps. Can't reproduce :/ After moving the cursor it's correct again?

Edit: Managed to reproduce now. Apparently it doesn't happen with spawn_first=yes.

po5 commented 1 year ago

It's correct once the first thumbnail from the new file loads. This is because we're within the same mpv instance, using the same bgra file. After switching to the new file it happily reuses the old thumbnail.

Removing thumbnail files at the start of file_load() fixes the file bleed issue. Dimensions are still wrong initially.

It's caused by the info call in file_load. --Well, no wonder. The dimensions are flipped in that call.

christoph-heinrich commented 1 year ago

Yeah, the rotation was a problem in #31, so I've fixed it there.

po5 commented 1 year ago

When switching files with the cursor on the timeline (not moving), it'd be nice if it generated the thumbnail without having to cross over to another thumbnail index when both files have the same resolution.
Seems to already work fine when resolutions are mismatched.
Not a blocker for merge, since master's behavior is more broken (bleeds previous thumb if resolutions match).

natural-harmonia-gropius commented 1 year ago

Move cursor on timeline, sometimes finfo is nil, and thumbnail is stuck. image

po5 commented 1 year ago

Are you on the latest commit? Line number doesn't match, and can't reproduce by manually setting finfo to nil.
Edit: Oh, there's a second finfo. Can reproduce when forcing it.
I wonder what would cause it to return nil, file not moved yet?

natural-harmonia-gropius commented 1 year ago

I rebased this commit to master. image

christoph-heinrich commented 1 year ago

I've never encountered the second info not being available, but this should fix it.

Showing the thumbnail at the correct time after a going to the next file is tricky, because the seek command gets sent before the process is done spawning. That is only a problem with spawn_first=yes, otherwise it should work now.

natural-harmonia-gropius commented 1 year ago

it'd be nice if it generated the thumbnail without having to cross over to another thumbnail index when both files have the same resolution.

I think this can be solved by adding the file path comparison to thumb() When switching to the next file, the timeline will disappear and trigger clear(), and when it reappears it will trigger thumb().

christoph-heinrich commented 1 year ago

That's why it works when spawn_first=no, but with spawn_first=yes the sub process gets started with a time of 0 and when thumb() sends the seek command, it hasn't finished starting yet and thus the command gets ignored and the thumbnail ends up showing the thumbnail from time 0 until you move your mouse enough.

natural-harmonia-gropius commented 1 year ago

That's why it works when spawn_first=no

It doesn't work for me, I need to move cursor to somewhere have different seek_time.

Edit: It works.

christoph-heinrich commented 1 year ago

I can sort of make it work with a workaround like that, but that's not a nice solution.

po5 commented 1 year ago

Works fine with spawn_first=no here. I have no more complaints, will merge if no one else objects in a day or so.

christoph-heinrich commented 1 year ago

This should be better then the timeout solution, but for some reason it's not 100% reliable for me :neutral_face:.

natural-harmonia-gropius commented 1 year ago

but for some reason it's not 100% reliable for me 😐.

I guess is if seek_time == last_seek_time then return end prevents file_timer:resume()

christoph-heinrich commented 1 year ago

No, I've added an output above the seek in the file handling function. It definitely does send the seek after receiving the first thumbnail from time 0, it just sometimes falls on deaf ears it seems (still had the output, even when it was ignored).

Funnily enough now after adding some more outputs it works 100% reliably and even after removing the print statements again, I still can't reproduce it anymore. :laughing:

Edit: Another fix would be to delay the initial spawn from spawn_first=yes a bit, because then it would get spawned in thumb() when hovering the timeline during file change. But that might be subject to a race condition. Maybe I can think of something better tomorrow.

po5 commented 1 year ago

There's also the option of not killing the thumbnailer process on file change, but you'll need to call loadfile and I couldn't get that to work reliably, plus I don't trust it so long as we handle things through shell commands.
The security implications of passing filenames that way are too great. Mentioning anyway for posterity.
I know there are scripts that use the playlist file feature for "safe" file loading, could work here.

christoph-heinrich commented 1 year ago

I've now also tried deleting the socket file and waiting for it to reappear, expecting the sub process to accept commands as soon as it's created. Turns out that is not the case and it takes a little longer then that, so this also doesn't work.

If we ever were to introduce script that runs in the slave, maybe creating a "socket ready" file from that script and then waiting for that file to appear would work. I'd expect everything to be initialized once lua scripts enter the event loop, and creating the file in the callback from a timeout of 0 seconds would create the file as soon as scripts enter the event loop.

po5 commented 1 year ago

Merged as 37e5054e6172b1404587b6301341615e34ba64f1