Skip to content

allow codec to be none for old versions of ffmpeg #117

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

Merged
merged 3 commits into from
Feb 3, 2015

Conversation

aestrivex
Copy link
Contributor

The save_movie() function calls ffmpeg with the -c argument which breaks very old versions of ffmpeg such as versions which can be found on old installations of RHEL/CentOS. This PR allows the omission of the -c argument when the codec kwarg is None.

if codec is None:
cmd = ['ffmpeg', '-i', frame_fmt, '-r', str(framerate), dst]
else:
cmd = ['ffmpeg', '-i', frame_fmt, '-r', str(framerate), '-c', codec, dst]
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be more DRY to do something like:

cmd = ['ffmpeg', '-i', frame_fmt, '-r', str(framerate)]
if codec is not None:
    cmd += ['-c', codec]
cmd += [dst]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done that way.

@larsoner
Copy link
Contributor

larsoner commented Feb 3, 2015

I assume you've tested this and it works for your code?

@aestrivex
Copy link
Contributor Author

Yes it works, it is very slow to update though for brains with a large number of vertices.

I tried it on fsaverage5 and it was quite a lot faster.

larsoner added a commit that referenced this pull request Feb 3, 2015
allow codec to be none for old versions of ffmpeg
@larsoner larsoner merged commit b27896e into nipy:master Feb 3, 2015
@aestrivex aestrivex deleted the allow_no_codec_ffmpeg_compat branch February 3, 2015 22:39
@larsoner
Copy link
Contributor

larsoner commented Feb 3, 2015

I wonder where the bottleneck is... feel free to open an improvement PR if you find a better way. We spent a bit of time getting the API right, but changing the under-the-hood mechanics shouldn't be too bad from an API standpoint. Maybe MoviePy would speed things up.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.16%) to 72.43% when pulling f47af04 on aestrivex:allow_no_codec_ffmpeg_compat into 7d7373b on nipy:master.

@aestrivex
Copy link
Contributor Author

Part of the bottleneck may be in repeatedly saving images, but I think a far larger bottleneck is in the large amount of interpolation done for a large amount of vertices (which the user can adjust currently).

I have some other code (which is not trivially portable to non X systems) which captures movies from mayavi by using ffmpeg to do screen capture rather than repeatedly taking screenshots. I tried both methods, and found that doing the direct screen capture does save a lot of overhead. My implementation might have been inefficient though, I will try it here and see if it does any better.

@larsoner
Copy link
Contributor

larsoner commented Feb 3, 2015

We do want to keep compatibility with other platforms, though, so I'm not sure that would end up working.

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.

3 participants