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

Integrated libx264 for higher quality mp4 output #1564

Merged
merged 9 commits into from Sep 29, 2015
Merged

Conversation

chaosphere2112
Copy link
Contributor

Fixes #1118, and gives us much nicer quality animation output.

@chaosphere2112 chaosphere2112 added this to the 2.4 milestone Sep 22, 2015
@durack1
Copy link
Member

durack1 commented Sep 22, 2015

@chaosphere2112 nice! How much nicer are the outputs? And are they also smaller?

@doutriaux1
Copy link
Contributor

@chaosphere2112 looks like you're missing some files.

@doutriaux1
Copy link
Contributor

@chaosphere2112 you might as well use cdat_confiugre_step for x264 as well.

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 I just did a dumb thing (removed the ffmpeg_configure_step.cmake.in and x264_configure_step.cmake.in without removing the configure_file command, despite them not being in use). Also forgot to update the ffmpeg() call on canvas to actually take advantage of this stuff.

@durack1 A lot better. And, yes, smaller as well.

Codec Size Quality
mpeg2 2 MB Low
mpeg2 6.3 MB High
h264 3.6 MB High

@doutriaux1
Copy link
Contributor

@chaosphere2112 yasm stuff

@durack1
Copy link
Member

durack1 commented Sep 23, 2015

@chaosphere2112 great - thanks for doing this.. I also note that the h264/*.mp4 encoded files play directly in any modern browser, whereas the mpeg2/*.mp4 files can be painful with codecs (and don't just play in FF41).. This is a great jump forward!

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 On the system requirements page we list YASM as a requirement for Ubuntu, though neither of the other platforms. Hm... I can just wrap the configure args to check if YASM is installed and disable it otherwise?

cmd += ' -r %s ' % rate
if bitrate is not None:
cmd += ' -b:v %sk' % bitrate
cmd += " -pix_fmt yuv420p "
Copy link
Member

Choose a reason for hiding this comment

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

@chaosphere2112 are h264 encoded files now the new default for the x.ffmpeg() and the x.animation() functions (where x = vcs.init())?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, FFMPEG defaults to using h264 when libx264 is installed. That's the reason for the -pix_fmt yuv420p option (they default to a pixel format that quicktime doesn't support but most other players do, so I went with the lowest common denominator).

@chaosphere2112
Copy link
Contributor Author

Let's hang off a sec on merging this, @doutriaux1; it turns out Cisco provides a binary version of H264 that is properly licensed for anyone to use, so I'm going to try linking against that with ffmpeg and see what happens.

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 also there's all of the build failures, but that's probably not my fault.

@aashish24
Copy link
Contributor

Looks like we are running into CMake version issue. This is new but I don't know why we didn't see this before.

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 Rather than porting to OpenH264's source, I'm content just sticking with libx264. It's better supported by FFMPEG, and we already made it work here. I'll push a fresh commit to restart these... not sure why they all failed.

@durack1
Copy link
Member

durack1 commented Sep 23, 2015

@chaosphere2112 there are a bunch of master tweaks to get cmake working and other things, a rebase might be useful to pull all these little tweaks and get your branch passing all the buildbot tests..

@doutriaux1
Copy link
Contributor

@chaosphere2112 looks like Ubuntu is hang. Will try on my machine before giving it the final go. Did you try under Ubuntu?

@durack1
Copy link
Member

durack1 commented Sep 24, 2015

@chaosphere2112 to fix travis-ci/push you'll need to merge in #1565, it seems garant has a standing issue with vcs_test_click_info this has repeated in a couple of standing PRs #1540, #1560, #1557

@doutriaux1
Copy link
Contributor

@durack1 I believe this PR is already in master so if the bot started after it went in there is no need for @chaosphere2112 to merge it in, the bots automatically merge master in.

@doutriaux1
Copy link
Contributor

ooops @durack1 I take it back... the bots WILL merge it in from master, but travis is not a bot...

@doutriaux1
Copy link
Contributor

@durack1 I think the click_info is an artefact of the baselines being updated for the resize merge and that code actually not making it into garant because it is not a bot.

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 @durack1 Rebased off of master, hopefully we get a clean build this time 😄

@durack1
Copy link
Member

durack1 commented Sep 24, 2015

@doutriaux1 @chaosphere2112 seems like garant just isn't getting up to date source.. Everything else is passing, I wonder if this issue is the same problem with vcs_test_click_info which keeps occurring, just on the one machine (garant)?

@chaosphere2112
Copy link
Contributor Author

@durack1 Dunno; I can't seem to get to the cdash page to see the results.

@durack1 durack1 mentioned this pull request Sep 24, 2015
@durack1
Copy link
Member

durack1 commented Sep 24, 2015

@chaosphere2112 try safari, FF is getting blocked by the lab..

@jbeezley
Copy link
Contributor

What are your issues with the cdash page? We have been reconfiguring SSL on all of our servers. If there are specific problems, I can try to work them out with our sysadmins.

@durack1
Copy link
Member

durack1 commented Sep 24, 2015

@jbeezley it's likely an issue specific to our side (and firewall, and browser - safari works!), @doutriaux1 has an open ticket with the network folks to stop blocking our connections to https://open.cdash.org

@chaosphere2112
Copy link
Contributor Author

@durack1 @jbeezley I'm unable to get to it from any of my web browsers (or wget).

--2015-09-24 10:34:33--  http://open.cdash.org/
Resolving open.cdash.org... 66.194.253.26
Connecting to open.cdash.org|66.194.253.26|:80... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://open.cdash.org/ [following]
--2015-09-24 10:34:34--  https://open.cdash.org/
Connecting to open.cdash.org|66.194.253.26|:443... connected.
Unable to establish SSL connection.

Odds are solid you guys upgraded to an SSL version that's too new for our firewall (same thing happened to http://djangoproject.com a while back).

Amusingly, it just happened when I tried to connect to https://OpenSSL.org

Here's the openssl s_client errors:

CONNECTED(00000003)
60321:error:14077410:SSL routines:SSL23_GET_SERVER_HELLO:sslv3 alert handshake failure:/SourceCache/OpenSSL098/OpenSSL098-52.6.4/src/ssl/s23_clnt.c:593:

@chaosphere2112
Copy link
Contributor Author

Indeed! You don't see me trying to port our build to anything else 😄

@durack1
Copy link
Member

durack1 commented Sep 25, 2015

@chaosphere2112 want to refresh/rebase this PR again so those tests get re-issued? I think we're close to an all green! The "Update branch" button/link below might just do it for you..

@chaosphere2112
Copy link
Contributor Author

@durack1 Doing so now; fingers crossed 🎱

@doutriaux1
Copy link
Contributor

@aashish24 @sankhesh I get this:

CMake Warning (dev) at CMake/cdat_modules/ffmpeg_external.cmake:13 (set):
  Policy CMP0053 is not set: Simplify variable reference and escape sequence
  evaluation.  Run "cmake --help-policy CMP0053" for policy details.  Use the
  cmake_policy command to set the policy and suppress this warning.

  For input:

    '--disable-yasm^^--enable-gpl^^--enable-libx264^^--extra-cxxflags=@ffmpeg_source@^^--enable-shared^^--enable-zlib'

  the old evaluation rules produce:

    '--disable-yasm^^--enable-gpl^^--enable-libx264^^--extra-cxxflags=/home/doutriaux1/build_x64/build/FFMPEG^^--enable-shared^^--enable-zlib'

  but the new evaluation rules produce:

    '--disable-yasm^^--enable-gpl^^--enable-libx264^^--extra-cxxflags=@ffmpeg_source@^^--enable-shared^^--enable-zlib'

  Using the old result for compatibility since the policy is not set.
Call Stack (most recent call first):
  CMakeLists.txt:671 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

We can fix this in another branch later.

@doutriaux1
Copy link
Contributor

@chaosphere2112 on ubuntu I get:

   Command failed: 1
   '/usr/local/bin/cmake' '-DINSTALL_DIR=/home/doutriaux1/build_x64/install/Externals' '-DWORKING_DIR=/home/doutriaux1/build_x64/build/X264' '-DCONFIGURE_ARGS=--disable-asm;--enable-shared' '-DBASH_CONFIGURE=ON' '-P' '/home/doutriaux1/build_x64/CMake/cdat_configure_step.cmake'
   From /home/doutriaux1/build_x64/X264-prefix/src/X264-stamp/X264-configure-out.log

  -------------------------------------------------

  -------------------------------------------------

   From /home/doutriaux1/build_x64/X264-prefix/src/X264-stamp/X264-configure-err.log

  -------------------------------------------------

  [INFO] ADDITIONAL CFLAGS

  CONFIGURE_ARGS IS --disable-asm;--enable-shared

  LD_ARGS IS
  /home/doutriaux1/build_x64/install/lib:/home/doutriaux1/build_x64/install/Externals/lib:/home/doutriaux1/build_x64/install/Externals/lib64:/home/doutriaux1/build_x64/install/Externals/lib/paraview-.:/lgm/uvcdat/2015-09-16/lib:/lgm/uvcdat/2015-09-16/Externals/lib:


  CFLAGS : -I/home/doutriaux1/build_x64/install/Externals/include
  -I/home/doutriaux1/build_x64/install/Externals/lib/libffi-3.1/include

  bash: configure: No such file or directory

  Config Errors detected:





  CMake Error at
  /home/doutriaux1/build_x64/CMake/cdat_configure_step.cmake:25 (message):

    Error in config

I think it needs ./configure

Will try it now.

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 I have seen the first warning message; I think all that needs to be done is convert the @ffmpeg_source@ to ${ffmpeg_source}. As for the second... that's weird. I wonder if the PWD isn't getting set up properly?

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 I'm not having any trouble building libx264 or ffmpeg, but I do run into problems with pip. My ubuntu VM needs a pile of updates, so I'll run those and try again.

@chaosphere2112
Copy link
Contributor Author

Weird. Mine tried to install pip before setuptools. Worked when I tried again. Building as we speak.

@doutriaux1
Copy link
Contributor

@chaosphere2112 can you please check that pip_deps refers setuptools_pkg

@chaosphere2112
Copy link
Contributor Author

It does, which is why I found it odd.

@doutriaux1
Copy link
Contributor

@chaosphere2112 configure is not in the source path:
it is at:

/home/doutriaux1/build_x64/build/X264/x264-snapshot-20150921-2245-stable/

one extra directory.

@doutriaux1
Copy link
Contributor

@chaosphere2112 on my ubuntu I have this extra directory. But not on the bots... I have no idea why. @aashish24 @sankhesh could it be a cmake version difference?

@chaosphere2112
Copy link
Contributor Author

@doutriaux1 libx264 and ffmpeg both built fine on my ubuntu VM (14.04), though I think I'm missing some prereqs that are breaking the rest of the build. Unfortunately, that VM is on the computer I don't have a property pass for, so I'll have to finish debugging it tomorrow.

@chaosphere2112
Copy link
Contributor Author

(that's before your commit)

@doutriaux1
Copy link
Contributor

@chaosphere2112 don't know what's going on here. It's not crucial for RC1 (but is for 2.4) so if the other 2 PR pass I will tag RC1 tonight. Unless you get a push that fixes the bots. I won't worry about my machine for now if all bots pass your thing.

@doutriaux1
Copy link
Contributor

@chaosphere2112 let's try to fix this early tomorrow morning so we can tag rc1

doutriaux1 added a commit that referenced this pull request Sep 29, 2015
Integrated libx264 for higher quality mp4 output
@doutriaux1 doutriaux1 merged commit b4b469f into master Sep 29, 2015
@doutriaux1 doutriaux1 deleted the add_libx264 branch September 29, 2015 19:57
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

5 participants