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

Travis CI fails tests that require ffmpeg/libavconv #12366

Open
johnhawkinson opened this issue Mar 5, 2017 · 9 comments
Open

Travis CI fails tests that require ffmpeg/libavconv #12366

johnhawkinson opened this issue Mar 5, 2017 · 9 comments

Comments

@johnhawkinson
Copy link
Contributor

@johnhawkinson johnhawkinson commented Mar 5, 2017

Travis CI's run of ./devscripts/run_tests.sh fails for tests that require ffmpeg or avconv.
This is a problem, since we don't need more false negatives in CI tests.

For instance, #12364 added test_NBC_4 which just appeared to outright fail in https://travis-ci.org/rg3/youtube-dl/jobs/207841764:
test_NBC_4 (test.test_download.TestDownload) ... ERROR
and it's a little annoying to actually see why, because the error results are after the the 10,000 line limit, so downloading the log is required, but:

======================================================================
ERROR: test_NBC_4 (test.test_download.TestDownload)
----------------------------------------------------------------------
...
[NBC] 3477499: Downloading webpage
[NBC] 3477499: Downloading webpage
[ThePlatform] 3477499: Downloading SMIL data
[ThePlatform] 3477499: Downloading m3u8 information
[ThePlatform] 3477499: Downloading JSON metadata
[info] Writing video description metadata as JSON to: test_NBC_4_3477499.info.json
[debug] Invoking downloader on u'https://nbcmpx-vh.akamaihd.net/i/video/210/775/170228_3477499_SNL_Host_Octavia_Spencer_Finds_Studio_8H_mpx_,1696,1296,896,696,496,306,150,240,64,.mp4.csmil/index_0_av.m3u8?null=0&id=AgA0Lbl1QZ4vXfy2u1iPKxJobgnYEPJ8Zf2nNgnnTbfTccfd1jxvJUrmfsMusy17rO8dw5jcRO2lhw%3d%3d&hdntl=exp=1488783484~acl=%2fi%2fvideo%2f210%2f775%2f170228_3477499_SNL_Host_Octavia_Spencer_Finds_Studio_8H_mpx_*~data=hdntl~hmac=245d3f10800160dd0af94c6fcf17dd311cf3115c24154e866b6acf7ca5b894ca'
[download] Destination: test_NBC_4_3477499.mp4
ERROR: m3u8 download detected but ffmpeg or avconv could not be found. Please install one.

I don't know what all the reasons are or why ffmpeg isn't or can't be in the Travis container. But if it can't be, at least those tests should be marked SKIPPED or something instead of ERROR.

Thanks.

@yan12125
Copy link
Collaborator

@yan12125 yan12125 commented Mar 5, 2017

Current practice is marking those tests as skipped.

But if it can't be, at least those tests should be marked SKIPPED or something instead of ERROR.

The reason is simple: we are lacking developers. I've once spent some time on fixing those issues and soon found that fixing them took too much time and real issues were kept untouched. (When I fixed them all and wait some days, there would be such errors in other tests. I don't know why...)

Now, as the native HLS downloader works fine, I think it's time to enable HLS tests.

@johnhawkinson
Copy link
Contributor Author

@johnhawkinson johnhawkinson commented Mar 5, 2017

I'm not quite sure I follow…I understand there aren't the development resources to go and fix all the broken tests. But the test system should be set up so that new pull requests that add tests from contributors who follow the instructions don't generate false CI negatives. It doesn't seem like fixing the test harness to report SKIPPED here takes much development resource. (Although I think it would take more effort for you to critique a pull request if I did it than to just do it yourself.)

Now, as the native HLS downloader works fine, I think it's time to enable HLS tests.

OK, maybe the above is irrelevant if it's about to change.

@yan12125
Copy link
Collaborator

@yan12125 yan12125 commented Mar 5, 2017

But the test system should be set up so that new pull requests that add tests from contributors who follow the instructions don't generate false CI negatives

I guess #8496 is related to what you're referring to

@johnhawkinson
Copy link
Contributor Author

@johnhawkinson johnhawkinson commented Mar 5, 2017

I guess #8496 is related to what you're referring to

Not really. I mean if I add a test (following the instructions in CONTRIBUTING.md) and it passes when I run it locally, it should not then fail when Travis runs it, unless I have done something wrong.

As far as I can tell, I didn't do anything wrong — I followed the instructions for adding the test, but it turns out some kinds of tests always fail when Travis runs them (m3u8 downloads). So either the instructions are wrong (e.g. I should not have added an m3u8 test. Or I should have marked it with skip_download but that seems wrong), or the Travis installation is broken (i.e. it should have ffmpeg, or it should know to skip m3u8 downloads without reporting them as ERROR).

#8496 is really about detecting commits which cause regressions -- which cause existing tests to fail.
But my concern is that I shouldn't be adding tests that I know are going to fail.

@yan12125
Copy link
Collaborator

@yan12125 yan12125 commented Mar 5, 2017

Oh I guess I got you. #12362 is the key change. Before that patch, ffmpeg downloads won't stop until finished, so they often go beyond the control and block other tests. After that testing with ffmpeg is possible. A problem is that it's difficult to install ffmpeg on Travis CI. Both Precise (Ubuntu 12.04) and Trusty (Ubuntu 14.04) provide libav but not ffmpeg, while the former has lots of bugs. (#8622 (comment))

Now, as the native HLS downloader works fine, I think it's time to enable HLS tests.

I forgot to say some new tests already uses the native downloader (by specifying entry_protocol='m3u8_native' in _extract_m3u8_formats). The question is whether it should be the default or not. To sum up, there are two ways:

  1. Use the native downloader
  2. Mark tests relying on ffmpeg with 'skip_download': True if the native downloader can't handle it

IMO updating docs is enough. For me it's not a good idea to silently skip tests that should be handled correctly.

@johnhawkinson
Copy link
Contributor Author

@johnhawkinson johnhawkinson commented Mar 5, 2017

Thanks.

To sum up, there are two ways:

  1. Use the native downloader
  2. Mark tests relying on ffmpeg with 'skip_download': True if the native downloader can't handle it

This still leaves me a bit confused. Let's take the case of test_NBC_4: I don't think I should be adding entry_protocol='m3u8_native' to the test spec, because then the test won't accurately reflect what users are likely to do. Sure, it probably won't break anything, but that's not the standard: it won't be testing the IE the way it normally operates.

And I definitely shouldn't mark the test skip_download, because the test works fine when I run it locally, and there's real value in tests being usable to developers working on code, even if they don't work for Travis. Unless there is some easy way for developers to run local tests ignoring skip_download (obviously we could edit the test parameters...). I thought the purpose of skip_download was for tests where the metadata was the purpose of the test.

It seems to me there should be a parameter to the test harness to tell is to skip tests that it is not capable of running, and that should be enabled for m3u8 tests in today's world when run from Travis. Now, if in the near future this isn't going to be an issue because either Travis will have ffmpeg and the 10k limit works, or because the native downloader will be enabled, or for some other reason, then it's not worth the effort to add such a skip-m3u8-tests-hack.

Sounds like the native downloaded is coming soon enough. But if we're still here in 3 months, maybe we should revisit this.

@yan12125
Copy link
Collaborator

@yan12125 yan12125 commented Mar 5, 2017

I may be inaccurate. I mean specifying the native downloader in the extractor, not in tests.

Unless there is some easy way for developers to run local tests ignoring skip_download

Another good point. We need it. Ignoring 'skip' is also necessary.

By the way, I guess there won't be ffmpeg in Travis CI containers soon. Ubuntu switches back to ffmpeg since Xenial (16.04), and Travis CI doesn't have Xenial yet.

@ritiek
Copy link

@ritiek ritiek commented Jul 13, 2017

I had a similar problem when installing ffmpeg from apt-repository in Travis CI a while back. Tests using ffmpeg for conversion were failing for some reason.

However, building ffmpeg from source fixed it for me. Travis CI (for ffmpeg compiled with libmp3lame) as below:

before_install:
  - sudo apt-get -qq update
  - sudo apt-get -y install autoconf automake build-essential libass-dev libfreetype6-dev libtheora-dev libtool libva-dev libvdpau-dev libvorbis-dev libxcb1-dev libxcb-shm0-dev libxcb-xfixes0-dev pkg-config texinfo wget zlib1g-dev
  - sudo apt-get -y install yasm nasm libmp3lame-dev
  - mkdir ~/ffmpeg_sources
  - cd ~/ffmpeg_sources
  - wget http://ffmpeg.org/releases/ffmpeg-snapshot.tar.bz2
  - tar jxf ffmpeg-snapshot.tar.bz2
  - cd ffmpeg
  - PATH="$HOME/bin:$PATH" PKG_CONFIG_PATH="$HOME/ffmpeg_build/lib/pkgconfig" ./configure --prefix="$HOME/ffmpeg_build" --pkg-config-flags="--static" --extra-cflags="-I$HOME/ffmpeg_build/include" --extra-ldflags="-L$HOME/ffmpeg_build/lib" --bindir="$HOME/bin" --enable-libmp3lame
  - PATH="$HOME/bin:$PATH" make -s -j
  - make install
  - hash -r
  - cd $TRAVIS_BUILD_DIR

It takes quite a time to build though (around 10 mins) but I can confirm it is invoked correctly from the command-line and ffmpeg conversion tests were passing for me.

This might be helpful.

@ritiek
Copy link

@ritiek ritiek commented Jul 18, 2017

UPDATE:

I was just messing around and I managed to upload the compiled ffmpeg binary from Travis CI to the internet.

Now one can just download this same binary on Travis CI Precise without having to build it on every run. So basically, .travis.yml can be reduced to:

before_install:
  - sudo apt-get -qq update
  - sudo apt-get -y install autoconf automake build-essential libass-dev libfreetype6-dev libtheora-dev libtool libva-dev libvdpau-dev libvorbis-dev libxcb1-dev libxcb-shm0-dev libxcb-xfixes0-dev pkg-config texinfo wget zlib1g-dev
  - sudo apt-get -y install yasm nasm libmp3lame-dev
  - mkdir $HOME/bin
  - wget </path/to/ffmpeg/bin> -O $HOME/bin/ffmpeg
  - chmod 755 $HOME/bin/ffmpeg
  - cd $TRAVIS_BUILD_DIR

It only takes a few seconds to download the binary and one does not need to wait for it to compile. I tried it and can confirm it works for me.

You can build and upload your own ffmpeg binary (with custom parameters) by adding this line at the end to .travis.yml in my previous comment:

- curl --upload-file $HOME/bin/ffmpeg https://transfer.sh/ffmpeg

and checking the log for the resultant URL and replacing it with the one in the sample .travis.yml above, so you won't have to build it on every test run.

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.