senko / python-video-converter

Python Video Converter (ffmpeg wrapper)
468 stars 191 forks source link

Improve x264, vorbis and theora codecs #23

Closed tahajahangir closed 11 years ago

tahajahangir commented 11 years ago

Added quality option for x264, vorbis and theroa codecs and preset option for x264.

Also removed confusing unnecessary x264 options. (It's better to use presets) From http://ffmpeg.org/trac/ffmpeg/wiki/x264EncodingGuide:

You can overwrite default preset settings with the x264opts option or by using the libx264 private options (see libx264 AVOptions in ffmpeg -h full). This is not recommended unless you know what you are doing. The presets were created by the x264 developers and tweaking values to get a better output is usually a waste of time.

senko commented 11 years ago

Looks good in theory, I hate that voodoo thing. I'm concerned about two potential problems:

Suggestions or other feedback welcome, if more people are happy with this change, I'll be easily convinced. @Kami?

tahajahangir commented 11 years ago

Removing those options doesn't break anything. Many of them are default options (like -flags +loop , -rc_eq 'blurCplx^(1-qComp)' , ...) and remaining options only affect speed and quality of encoding (not saving format).

Kami commented 11 years ago

I do think that moving towards presets is the right thing to do for the x264 codec.

As far as the backward incompatibility goes - it should be fine as long as we bump the major version and document this change in the changelog

Kami commented 11 years ago

@tahajahangir I noticed your commit c92071c841d20e2ad8f124dcdc764bb3e6197c22 fixed the codec name, but the tests didn't fail in the previous commit which contained a wrong codec name.

We need to add a test case to catch failures like this.

Kami commented 11 years ago

@tahajahangir I've based a pull request on your branch and added a test for vp8 codec there - https://github.com/tahajahangir/python-video-converter/pull/1

Kami commented 11 years ago

Never mind, I see you already merged my branch.

tahajahangir commented 11 years ago

:D

senko commented 11 years ago

I haven't used these options before so I'll defer to your expertise :)

Thanks guys for the patches, there were some conflicts in test.py with the previous pull req so I resolved & merged manually.