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

fixes #248 issue with VideoFileClip() not reading all frames #251

Merged
3 commits merged into from
Apr 13, 2017

Conversation

aldilaff
Copy link
Contributor

No description provided.

@MicahRamirez
Copy link

Why -1 ?

@aldilaff
Copy link
Contributor Author

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.

@ghost
Copy link

ghost commented Mar 15, 2017

@aldilaff can you change this so that it reads something like:

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]

@ghost
Copy link

ghost commented Apr 11, 2017

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

@coveralls
Copy link

coveralls commented Apr 13, 2017

Coverage Status

Changes Unknown when pulling 69e51c1 on aldilaff:master into ** on Zulko:master**.

@aldilaff
Copy link
Contributor Author

@Earney My apologies for the delay. Updated.

@tburrows13
Copy link
Collaborator

Maybe there should be a comment in there explaining why, or at least linking to this PR?

@ghost
Copy link

ghost commented Apr 13, 2017

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

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]
Copy link
Collaborator

@mbeacom mbeacom Apr 13, 2017

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!

@mbeacom mbeacom added the bug Issues that report (apparent) bugs. label Apr 13, 2017
@ghost
Copy link

ghost commented Apr 13, 2017

@achalddave thanks for updating this.. We will try to merge it ASAP..

@mbeacom mbeacom added this to the Release v0.2.3.3 milestone Apr 13, 2017
@achalddave
Copy link
Contributor

Hmm, I assume I'm being mentioned here due to a typo? I think you want @aldilaff :)

Copy link
Collaborator

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

:shipit: LGTM

@coveralls
Copy link

coveralls commented Apr 13, 2017

Coverage Status

Changes Unknown when pulling f156c95 on aldilaff:master into ** on Zulko:master**.

@mbeacom mbeacom added mergable video Related to VideoClip and related classes, or handling of video in general. LGTM Passed inspection by maintainers. labels Apr 13, 2017
@ghost ghost merged commit d41a40e into Zulko:master Apr 13, 2017
@tburrows13 tburrows13 added bug-fix For PRs and issues solving bugs. and removed bug Issues that report (apparent) bugs. LGTM Passed inspection by maintainers. mergable labels Jul 8, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix For PRs and issues solving bugs. video Related to VideoClip and related classes, or handling of video in general.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants