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

Force frame rate of FFMpegFileWriter input #5453

Merged
merged 2 commits into from Nov 11, 2015
Merged

Conversation

auneri
Copy link
Contributor

@auneri auneri commented Nov 9, 2015

The output video file was observed to skip frames, when input frame rate does not match output (e.g., when fps=1, or interval=1000).

See https://www.ffmpeg.org/ffmpeg.html#Description for a description of the solution.

The output video file was observed to skip frames, when input frame rate does not match output (e.g., when fps=1, or interval=1000).

See https://www.ffmpeg.org/ffmpeg.html#Description for a description of the solution.
@dopplershift
Copy link
Contributor

Interesting. Any idea if that's a new option (I've never noticed it before)? My only concern would be that it would break on some other version of ffmpeg.

@auneri
Copy link
Contributor Author

auneri commented Nov 9, 2015

Good point, I've actually first observed the error with FFMPEG v2.6, then updated to v2.8.1, where I've validated the proposed solution.

I could double check on other versions, what is the lowest supported, or which versions should I cover?

@WeatherGod
Copy link
Member

On my CentOS6 machine, I see that the -r option is available and is documented to force a frame rate. Here is the version info:

$ ffmpeg -version
FFmpeg version 0.6.5, Copyright (c) 2000-2010 the FFmpeg developers
  built on Jan 29 2012 17:52:15 with gcc 4.4.5 20110214 (Red Hat 4.4.5-6)

@jenshnielsen
Copy link
Member

And with FFmpeg version 0.5.2, Copyright (c) 2000-2009 Fabrice Bellard, et al
of RHEL 5

ffmpeg -h
-r rate             set frame rate (Hz value, fraction or abbreviation)

@@ -446,7 +446,7 @@ class FFMpegFileWriter(FileMovieWriter, FFMpegBase):
def _args(self):
# Returns the command line parameters for subprocess to use
# ffmpeg to create a movie using a collection of temp images
return [self.bin_path(), '-i', self._base_temp_name(),
return [self.bin_path(), '-r', str(self.fps), '-i', self._base_temp_name(),
Copy link
Member

Choose a reason for hiding this comment

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

This line is now too long.

@dopplershift
Copy link
Contributor

Was wanting to double check when -r became an option for input. However, it looks like this feature has been in for almost 10 years: https://ffmpeg.org/pipermail/ffmpeg-devel/2005-August/000524.html

So, minus the pep8 issue, I'm 👍 on the change.

As an aside, I really wish there were a better way to do this than to constantly tinker with the ffmpeg command line incantation.

@jenshnielsen
Copy link
Member

It's also in the RHEL 5 man page (ffmpeg 0.5.2)

Is a similar fix needed for the pipe based writer?

@dopplershift
Copy link
Contributor

Looking at the pipe-based writer, I think it's already covered; the rate is set before the -i pipe:. (I'm not sure how the output rate was working in that case--maybe it just defaulted to the input rate?)

Ensure line length is <80.
@tacaswell
Copy link
Member

@dopplershift See #4416 😉

tacaswell added a commit that referenced this pull request Nov 11, 2015
FIX: Force frame rate of FFMpegFileWriter input
@tacaswell tacaswell merged commit 57d1343 into matplotlib:master Nov 11, 2015
@tacaswell
Copy link
Member

@auneri Thanks!

@efiring
Copy link
Member

efiring commented Sep 28, 2016

@tacaswell, @dopplershift, here is another one that has cost me hours of frustration. It is fixing a big bug, so I would like to backport it. In fact, from my reading of the avconv and ffmpeg docs (it applies to both), the second '-r' option is at best superfluous; the critical thing is to have the '-r' option specified before the '-i' option. Any '-r' option after the '-i' serves to drop or duplicate frames in an attempt to match the rate of the input.

@tacaswell
Copy link
Member

👍 on backporting.

efiring pushed a commit that referenced this pull request Sep 29, 2016
FIX: Force frame rate of FFMpegFileWriter input
@efiring
Copy link
Member

efiring commented Sep 29, 2016

Backported to v2.x as c0c65e3.

@QuLogic QuLogic added this to the 2.0 (style change major release) milestone Sep 29, 2016
@auneri
Copy link
Contributor Author

auneri commented Nov 22, 2016

I am failing to see this change (or any change that addresses this issue) in master or 2.x, am I missing something?

@efiring
Copy link
Member

efiring commented Nov 23, 2016

Good catch. This is very puzzling. Some changes--not just this one--seem to have gone missing. I suspect something went wrong in all the backwards and forwards between v2.x and master. Or maybe I simply fouled it up myself. I will try to track it down.

@efiring
Copy link
Member

efiring commented Nov 23, 2016

I'm the culprit. Somehow, not long after merging this, I got mixed up and removed the wrong '-r'--and even left a comment in its place. I will fix it now.

@efiring
Copy link
Member

efiring commented Nov 23, 2016

OK, it should be fixed in v2.x and master now. Thanks for catching that, @auneri.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants