search

TheAMM / mpv_thumbnail_script

A Lua script to show preview thumbnails in mpv's OSC seekbar, sans external dependencies
GNU General Public License v3.0
679 stars 68 forks source link

Guard against some race conditions. Update osc to upstream. #54

Closed blankname closed 4 years ago

blankname commented 4 years ago

I can drop whatever commits you don't want and/or create a separate pull request for the osc update if you want me to.

I was investigating @Jerrk's comment on #47 and while working on that decided to fix the other similar issues I came across.

They all occurred occasionally while fast-forwarding through many short videos.

b82d752: image 75c9f65: image 66f87ac: image 1b67e22: image ccdddb7: image

cc:

46

19

https://github.com/TheAMM/mpv_thumbnail_script/issues/47#issuecomment-593175039

blankname commented 4 years ago

It's possible that this is just treating the symptoms and there's a better solution to the race conditions out there.

I believe it still improves the situation.

tonycpsu commented 4 years ago

I dunno, I've been running with this PR for a while and haven't had any issues even when skipping tracks / seeking quickly. If there's still a race condition, this vastly improves it over my previous attempt to fix it.

EDIT: Literally one minute after posting that, I did get a "thumbnailing failed" error that caused the OSC to disappear. Couldn't get the stdout/stderr dump, unfortunately.

tonycpsu commented 4 years ago

I think the previous error happened when I moved a file to another directory while it was being thumbnailed in the background.

blankname commented 4 years ago

Thanks for trying it out!

It looks like the error in my first screenshot could still happen: image

The error occurred when duration became nil in between the check in on_video_change and a later retrieval shortly afterwards in the get_thumbnail_count call in update_state. image

I've added a commit to fix that for now 6c88e7c. I will drop b82d752 (since 6c88e7c should be a more complete fix for that issue).

I think we need to try to reduce our calls to retrieve mpv properties (through some sort of caching like what you were doing in your pull) and make sure every property retrieval is checked for nil.

I'll try to look into it more later this weekend.

blankname commented 4 years ago

I created #54 with what I think is a more complete fix for these issues.

I haven't had a ton of time to test yet.

Hope to do so later in the weekend, but I'm out of time for now.