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 ways to composite video clips by using PIL #1157

Closed
wants to merge 0 commits into from

Conversation

ODtian
Copy link

@ODtian ODtian commented Apr 23, 2020

This is mainly due to the performance issues on CompositeVideoClip, by changing dependency to PIL, it becomes 25 times faster.

@tburrows13
Copy link
Collaborator

Hello! Thank you for making this contribution, it looks like it will be quite useful. Let me know if you need a hand with anything. In order to pass the code format check, you need to run pip install black and then black -t py36 .. That will auto format the code so that everything is formatted the same and you don't need to worry about it when you're writing it.

@tburrows13 tburrows13 added the enhancement Positive change that does not change the API, i.e. improved performance, using less memory etc. label Apr 23, 2020
Copy link
Author

@ODtian ODtian left a comment

Choose a reason for hiding this comment

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

weird, I'm sorry

@coveralls
Copy link

coveralls commented Apr 23, 2020

Coverage Status

Coverage decreased (-0.6%) to 66.777% when pulling 9b0ee3e on ODtian:master into 3b25a76 on Zulko:master.

@ODtian
Copy link
Author

ODtian commented Apr 23, 2020

All done. But notice never nesting the CompositeVideoClips, it will slow down the speed.

Copy link

@amirothman amirothman left a comment

Choose a reason for hiding this comment

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

Seems good, have some minor suggestions.

return np.minimum(1, picture + self.blit_on(np.zeros(framesize), t))
# ? I'm not so sure about the function of this code, need information
# if self.ismask and picture.max():
# return np.minimum(1, picture + self.blit_on(np.zeros(framesize), t))

Choose a reason for hiding this comment

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

do we need this?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your view, I annotated it since I have no idea about these code, but it still pass all tests anyway. Seems ok to delete them.

@Garett-MacGowan
Copy link

Hey, I was just wondering if this code is still in the works. Has someone had the chance to look at the test_subtitles failure? Any ideas on when this code might get merged in, if at all? Thanks for all the efforts.

@ODtian
Copy link
Author

ODtian commented Aug 4, 2020

@Garett-MacGowan I also noticed that the subtitle test failed. I tested it on my pc, but I don’t remember it a bit. If you have time, can you test it manually?

@Garett-MacGowan
Copy link

I should have a little bit of time this week. I'll try and test it manually and come to a resolution.

@keikoro
Copy link
Collaborator

keikoro commented Oct 3, 2020

@Garett-MacGowan Just checking in here, did you eventually find time to test this?

@tburrows13 tburrows13 self-assigned this Oct 4, 2020
@tburrows13
Copy link
Collaborator

Linking #1145 and also a bug fix that may need to be applied here as well: https://www.reddit.com/r/moviepy/comments/i1z3rq/rendering_a_10_minute_video_takes_hours/g03gu6m/

This PR is on my todo list to get finalised and merged.

@tburrows13
Copy link
Collaborator

Ah sorry, I messed this PR up whilst trying to fix the merge conflicts. It seems that I can't fix it without proper write permissions to ODtian:master so I'll create a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Positive change that does not change the API, i.e. improved performance, using less memory etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants