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

Add fps_source option for #404 #516

Merged
merged 6 commits into from
Mar 30, 2017
Merged

Conversation

tburrows13
Copy link
Collaborator

In #404 Zulko suggested a fps_source parameter for VideoFileClip, to tell it to read fps instead of tbr if required.

I'm not sure how to implement the test (download the file), so if someone could let me know how, or do it yourself, that would be great!

And is it ok to declare the functions inside the main function? I did it like that so that they would be right before they are called for reference, but it might impact performance a little. Your decision.

@coveralls
Copy link

coveralls commented Mar 28, 2017

Coverage Status

Coverage decreased (-0.05%) to 48.634% when pulling e8505c3 on Gloin1313:fps-option into 6c2ca83 on Zulko:master.

@ghost
Copy link

ghost commented Mar 28, 2017

add the following to the top of your test files:

# must have to work on travis-ci
import sys
sys.path.append("tests")
import download_media

def test_download_media(capsys):
    with capsys.disabled():
       download_media.download()

then you can use any of the files that are contained in the download_media.py file. These will be downloaded and placed in your ./media directory. if you want an extra media file added, we can look at modifying the download_media.py script, and also adding the media file to the earney/moviepy-media repo.

Let me know if you have additional questions.

@tburrows13
Copy link
Collaborator Author

The only video you have is the "Big Buck Bunny" one, which I have downloaded to test, and it has the same fps as tbr, which makes it unsuitable for testing. Can we add a different one? One that was actually filmed on a camera would probably be best.

@ghost
Copy link

ghost commented Mar 28, 2017

yes, we can add more videos. Do you have one in mind?

@tburrows13
Copy link
Collaborator Author

I can't seem to find any videos on youtube where they are different. Maybe youtube uniforms them as they are uploaded?

@tburrows13
Copy link
Collaborator Author

tburrows13 commented Mar 29, 2017

Are there any other ways we can do it? e.g. load a video into Travis? Or the actual repo?

@ghost
Copy link

ghost commented Mar 29, 2017

we load a video up to earney/moviepy-media. When Travis runs the tests, the first test in the python file is a test that sees if all the media files have been downloaded. If not, they are downloaded to the media directory.

@tburrows13
Copy link
Collaborator Author

So is there any way for me to use my own files in the test without download_media having to get them from youtube?

@ghost
Copy link

ghost commented Mar 29, 2017

download_media can download any url, so if you have a location, we can download it. I created a repo just for media for this project called earney/moviepy_media, so that the media will not get moved like it does from sources that we do not control, etc.

@ghost
Copy link

ghost commented Mar 29, 2017

@gloin1313 I'm not aware of any other way of using our own files, but to use the download_media script I created. If you can find some other way, I'm interested.

@tburrows13
Copy link
Collaborator Author

tburrows13 commented Mar 30, 2017

@Earney Ok, I've uploaded a video to vimeo here: https://vimeo.com/210765693
Its not a slo-mo, but ffprobe tells me it has 10.51 fps, 90k tbr. Re-downloaded it from vimeo and it still has those settings, so it should work fine. Is it ok if you add this to earney/moviepy_media? Then when you've done that I'll write the test.
Thanks!

@ghost
Copy link

ghost commented Mar 30, 2017

its located at https://github.com/earney/moviepy_media/tree/master/tests/videos as fire2.mp4. Let me know if you need some additional help.

@coveralls
Copy link

coveralls commented Mar 30, 2017

Coverage Status

Coverage increased (+0.2%) to 48.883% when pulling 23d33eb on Gloin1313:fps-option into 6c2ca83 on Zulko:master.

@coveralls
Copy link

coveralls commented Mar 30, 2017

Coverage Status

Coverage increased (+0.2%) to 48.883% when pulling 23d33eb on Gloin1313:fps-option into 6c2ca83 on Zulko:master.

@ghost
Copy link

ghost commented Mar 30, 2017

Aweseome!.. @Gloin1313 I think this is ready to merge.. :)

@tburrows13
Copy link
Collaborator Author

Great! One thing I wasn't sure about; is defining a function inside of the function recommended? Do we agree readability over speed?

@ghost
Copy link

ghost commented Mar 30, 2017

yes, readability > speed

@tburrows13 tburrows13 merged commit 4e55f60 into Zulko:master Mar 30, 2017
@tburrows13 tburrows13 added the feature New addition to the API i.e. a new class, method or parameter. label Apr 17, 2017
@tburrows13 tburrows13 deleted the fps-option branch February 12, 2018 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition to the API i.e. a new class, method or parameter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants