-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fixes #248 issue with VideoFileClip() not reading all frames #251
Conversation
Why -1 ? |
The issue I was facing is that moviepy was not reading the data of large gifs correctly. Looking at the code, the line I changed used to read the first element of a list. I found out that for large gifs, the data is being read incrementally and added to the list. So the element with the "full" length of large gifs was actually the last element. -1 is just the last element of a list. |
@aldilaff can you change this so that it reads something like:
|
@aldilaff I'll give you a few more days to respond, if not, I'll create another pull request that makes the necessary changes I have suggested, and will close this PR. |
Changes Unknown when pulling 69e51c1 on aldilaff:master into ** on Zulko:master**. |
@Earney My apologies for the delay. Updated. |
Maybe there should be a comment in there explaining why, or at least linking to this PR? |
@achalddave Gloin1313 is probably right.. Can you add the following comment to your submission? #for large GIFS the "full" length is presented as the last element in the output. Thanks! |
moviepy/video/io/ffmpeg_reader.py
Outdated
if is_GIF: | ||
line = [l for l in lines if keyword in l][-1] | ||
else: | ||
line = [l for l in lines if keyword in l][0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of writing out the line =
twice, we could probably replace L264-L267 with something like:
index = -1 if is_GIF else 0
line = [l for l in lines if keyword in l][index]
or even:
line = [l for l in lines if keyword in l][-1 if is_GIF else 0]
Granted, there is a lot we could refactor in there to get more DRY, but you get the point!
Thanks for the PR, btw!
@achalddave thanks for updating this.. We will try to merge it ASAP.. |
Hmm, I assume I'm being mentioned here due to a typo? I think you want @aldilaff :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes Unknown when pulling f156c95 on aldilaff:master into ** on Zulko:master**. |
No description provided.