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

Change vid may cause resize and disable, Fix it. #25

Merged
merged 4 commits into from
Nov 1, 2022
Merged

Change vid may cause resize and disable, Fix it. #25

merged 4 commits into from
Nov 1, 2022

Conversation

natural-harmonia-gropius
Copy link
Contributor

Fix #21

@natural-harmonia-gropius natural-harmonia-gropius changed the title Change vid may cause resize, also disabled. Change vid may cause resize and disable, Fix it. Sep 24, 2022
@hooke007
Copy link
Contributor

would break when cycling the vid
Snipaste_2022-09-24_12-05-55

@natural-harmonia-gropius
Copy link
Contributor Author

would break when cycling the vid

fixed

@hooke007
Copy link
Contributor

But... would be easier to be freezed when cycling the vid

@natural-harmonia-gropius
Copy link
Contributor Author

But... would be easier to be freezed when cycling the vid

Should have same behavior as change video-rotate with master branch.

@hooke007
Copy link
Contributor

Should have same behavior as change video-rotate with master branch.

Not here.

thumbfast.lua Outdated Show resolved Hide resolved
@hooke007
Copy link
Contributor

hooke007 commented Sep 25, 2022

8e68d25

No.

default.mp4

@natural-harmonia-gropius
Copy link
Contributor Author

As I said above, this PR has nothing to do with freezing. All it does is restart the child process when it should be restarted.

The problem is in the logic of the spwan(), which has not been touched yet. So there must be a freezing problem with video-rotate.

Piplup.Step.MV.-.mpv.2022-09-25.11-08-05_Trim.mp4

@hooke007
Copy link
Contributor

hooke007 commented Sep 25, 2022

No this issue in the current master. 7b32934

default.mp4

You ignored my comment here #25 (comment)
It's not the same problem.

@natural-harmonia-gropius
Copy link
Contributor Author

Because set vid no in the master did not do anything.
In your video on the black screen shows thumbnails, and this is what to fix.
image

@natural-harmonia-gropius
Copy link
Contributor Author

natural-harmonia-gropius commented Oct 6, 2022

TODO:

  • nil passed condition in sync_changes() It doesn't
  • add debounce to watch_changes(), only execute the last request.

@natural-harmonia-gropius
Copy link
Contributor Author

Sadly, debounce didn't fix the freezing of the video due to rotation, and I can't reproduce the freezing due to cycle vid

@christoph-heinrich
Copy link
Contributor

We could use mp.register_idle() and then queue up changes in watch_changes() that then get sent after all changes were gathered.

@natural-harmonia-gropius
Copy link
Contributor Author

It is called too frequently.

image
image

@dyphire
Copy link
Contributor

dyphire commented Oct 7, 2022

Sadly, debounce didn't fix the freezing of the video due to rotation, and I can't reproduce the freezing due to cycle vid

In my test, no matter whether the vid is switched or not, it is easier to freeze than the master branch.
Ah, I forgot to test the latest push. Freezing does not occur after retesting.
But It will not generate thumbnails after cycle vid: set vid no,set vid 1.

@natural-harmonia-gropius
Copy link
Contributor Author

I don't quite understand why this is so, Change vid has always worked for me.
https://user-images.githubusercontent.com/50797982/194635139-aa3ce4ac-8165-49ba-a9a2-53f0e4e97aa6.mp4

And I think maybe it makes more sense to add debounce to spwan.

@hooke007
Copy link
Contributor

hooke007 commented Oct 7, 2022

Change vid has always worked for me.

Just using a normal video to test.

Another issue happened here.
Snipaste_2022-10-07_20-26-15

@natural-harmonia-gropius
Copy link
Contributor Author

natural-harmonia-gropius commented Oct 8, 2022

Just using a normal video to test.

Also works fine for me.

Another issue happened here.

I can see same error when I set rotate.

All the problems are due to the fact that the child processes and also the file system are unobservable asynchronous processes and we can only guess that it completes

@natural-harmonia-gropius
Copy link
Contributor Author

@hooke007 Does it still freeze under your test? If not, then is ready for review.

@hooke007
Copy link
Contributor

Does it still freeze under your test?

Nopo. But the msg #25 (comment) is too noisy. Did a larger timeout would help?

@natural-harmonia-gropius
Copy link
Contributor Author

the msg is too noisy

No idea until #11 (comment) is fixed.

Did a larger timeout would help?

I don't think so.

@po5
Copy link
Owner

po5 commented Oct 30, 2022

@hooke007 Can only reproduce when hovering the timeline during file change, and that issue is present on master.
Actually I'm on the uosc PR version, disregard that as I haven't tested with a script that uses the regular API.

I will look at this PR in detail tomorrow.

thumbfast.lua Outdated Show resolved Hide resolved
@natural-harmonia-gropius
Copy link
Contributor Author

effective_w, effective_h = 0, 0 is removed, add disabled = not (w and h) or ... for vid=0.

thumbfast.lua Outdated Show resolved Hide resolved
@po5 po5 merged commit 3469ff8 into po5:master Nov 1, 2022
@hooke007
Copy link
Contributor

hooke007 commented Nov 1, 2022

Freezing issue happened again with the latest commit...

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.

More needs to do when vid changes
5 participants