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

Feat: Add Loop property #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

CodeYan01
Copy link

Makes it easier to toggle looping, and prevents flickering that would be caused by calling TkinterVideo.play() after the "<>" event.

Fixes #32

@CodeYan01
Copy link
Author

I only made it a property (with get/setter) for consistency with other functions like keep_aspect and set_resampling_method having setter functions that don't really do anything else. But having a getter too imo is important if you're gonna have a setter, so I made it a Python property for completeness

@PaulleDemon
Copy link
Owner

Hi, I am currently will look into this as soon as I am free

Copy link

@Brandon4466 Brandon4466 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a REQUIRED change. Not because of the reason though. This is required because the original documented way to loop causes a massive memory leak. Just replaying in that way creates a new instance in memory and does not release the old instance from memory and Python's GC does not catch and drop it. After 3-4 mins a 3 second video fills up 2GB+ of RAM. This change here fixes the memory leak because it uses the same instance. This isn't the perfect implementation, but may be the best possible. Implementing in this way causes the video to be unable to be changed unless you destroy the widget entirely and have it reestablished which is inconvenient. This is because it will continue loop forever, not looking for changes without ever breaking (Line 194+195). This does NOT stop the flickering and jittering. I don't know if there is a better solution to implement this given how tkinter is constructed. If you wrap the load and play lines in a function and thread that using threading that DOES get rid of the flickering, however it does not get rid of the stuttering every ~1sec.

@PaulleDemon
Copy link
Owner

Ok, I am looking into this. @Brandon4466 can you help me understand which part is not being Garbage collected? I will try to implement this ASAP

@CodeYan01
Copy link
Author

@Brandon4466 were you suggesting changes to this PR? Because on github it shows that you are suggesting changes but I don't see anything new that you changed.

@PaulleDemon PaulleDemon mentioned this pull request Jun 11, 2023
Copy link
Contributor

@havetc havetc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two very minor suggestions, to have more consistency in the code base

Documentation.md Outdated Show resolved Hide resolved
tkVideoPlayer/tkvideoplayer.py Outdated Show resolved Hide resolved
Makes it easier to toggle looping, and prevents flickering that would be
caused by calling `TkinterVideo.play()` after the "<<Ended>>" event.
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.

Flickering in Loop Example
5 participants