po5 / thumbfast

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

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

Closed natural-harmonia-gropius closed 1 year ago

natural-harmonia-gropius commented 2 years ago

Fix https://github.com/po5/thumbfast/issues/21

hooke007 commented 2 years ago

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

natural-harmonia-gropius commented 2 years ago

would break when cycling the vid

fixed

hooke007 commented 2 years ago

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

natural-harmonia-gropius commented 2 years ago

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

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

hooke007 commented 2 years ago

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

Not here.

hooke007 commented 2 years ago

https://github.com/po5/thumbfast/commit/8e68d254ab91ace210fac097cbd572c33e7655b7

No.

https://user-images.githubusercontent.com/41094733/192126281-a6af4637-c1f3-4662-a4bc-62d9b95ef3da.mp4

natural-harmonia-gropius commented 2 years ago

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.

https://user-images.githubusercontent.com/50797982/192126498-782d8116-98f4-47b3-ae6c-9cb7271339c2.mp4

hooke007 commented 2 years ago

No this issue in the current master. https://github.com/po5/thumbfast/commit/7b32934a72435d596b4aeeb87aa77f23650cbad3

https://user-images.githubusercontent.com/41094733/192126738-6b8ffde0-32b1-4059-826c-5acafc748a6c.mp4

You ignored my comment here https://github.com/po5/thumbfast/pull/25#issuecomment-1256965657 It's not the same problem.

natural-harmonia-gropius commented 2 years ago

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 commented 1 year ago

TODO:

natural-harmonia-gropius commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

It is called too frequently.

image image

dyphire commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

hooke007 commented 1 year ago

Does it still freeze under your test?

Nopo. But the msg https://github.com/po5/thumbfast/pull/25#issuecomment-1272013284 is too noisy. Did a larger timeout would help?

natural-harmonia-gropius commented 1 year ago

the msg is too noisy

No idea until https://github.com/po5/thumbfast/issues/11#issuecomment-1295865590 is fixed.

Did a larger timeout would help?

I don't think so.

po5 commented 1 year ago

@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.

natural-harmonia-gropius commented 1 year ago

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

hooke007 commented 1 year ago

Freezing issue happened again with the latest commit...