Skip to content
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

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

Closed
wants to merge 7 commits into from

Conversation

blankname
Copy link

@blankname blankname commented Mar 4, 2020

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
#47 (comment)

@blankname
Copy link
Author

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
Copy link

tonycpsu commented Mar 7, 2020

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
Copy link

tonycpsu commented Mar 7, 2020

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

@blankname
Copy link
Author

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
Copy link
Author

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.

@blankname blankname closed this Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants