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

PR: Add PyTorch FFmpeg to wheel and conda distributions #2596

Merged
merged 40 commits into from
Oct 6, 2020

Conversation

andfoy
Copy link
Contributor

@andfoy andfoy commented Aug 19, 2020

This PR enables the inclusion of FFmpeg LGPL binaries on wheel distributions and also updates the conda recipe dependencies to include FFmpeg as well

Fixes #2365

@andfoy
Copy link
Contributor Author

andfoy commented Aug 19, 2020

@fmassa, it seems that the decoder backend is broken in Windows, is this expected?

@fmassa
Copy link
Member

fmassa commented Aug 20, 2020

@andfoy this is not expected, and I believe we at some point tested the Windows compilation of the decoder

@fmassa
Copy link
Member

fmassa commented Aug 21, 2020

@andfoy what do you think about the following: you disable Windows for FFmpeg for now, revert the changes that are specific to trying to fix Windows, and we open a new issue to track fixing Windows for this and add the Windows label, so that @peterjc123 and colleagues can have a look at it?

@andfoy
Copy link
Contributor Author

andfoy commented Aug 21, 2020

@fmassa, I managed to fix the Windows compilation issues, right now we're having test failures on both Linux and Windows

@fmassa
Copy link
Member

fmassa commented Aug 21, 2020

Does it pass locally for you? Maybe the machine is running out of memory? Can you ssh to the machine to debug?

@andfoy
Copy link
Contributor Author

andfoy commented Aug 21, 2020

Does it pass locally for you?

No, they fail and segfault as well, I don't know if the errors are related to the ffmpeg binaries that av uses vs ours, I'll try again with gdb and see where it fails

@andfoy
Copy link
Contributor Author

andfoy commented Sep 11, 2020

It seems that --disable--mmx flag when compilling FFmpeg fixes the segfaults on the tests

@fmassa
Copy link
Member

fmassa commented Sep 14, 2020

It seems that --disable--mmx flag when compilling FFmpeg fixes the segfaults on the tests

So this means that we would need to ship ffmpeg without MMX optimizations? Do you have an idea if it would make the binaries much slower than the ones we currently have for some machines?

@andfoy
Copy link
Contributor Author

andfoy commented Sep 14, 2020

So this means that we would need to ship FFmpeg without MMX optimizations? Do you have an idea if it would make the binaries much slower than the ones we currently have for some machines?

It seems that on modern architectures MMX was superseded by SSE/2/3/4 and AVX, so in principle, disabling MMX should only affect very old processors, as FFmeg has individual flags to disable SSE and AVX: https://github.com/FFmpeg/FFmpeg/blob/05c8d0bce64888c5312822fbc9cdb63934b86519/configure#L430

@fmassa
Copy link
Member

fmassa commented Sep 23, 2020

Interesting, test failures could seem related, although most of them are on test_io.py which uses pyav, so I'm not 100% sure what's happening here. Maybe pyav is picking a different ffmpeg version than the one it was compiled to use?

For the error in test_read_video_from_memory_scripted, there is an easy fix: replace the % for string formatting in

start pts: %d and end pts: %d""" % (

with the .format equivalent. There might have been a change in torchscript that made the % stop working.

@andfoy
Copy link
Contributor Author

andfoy commented Sep 23, 2020

The error here is that some tests are trying to use x264 and x264rgb, which are not available on our LGPL FFmpeg distribution. I tried to switch some of them to h264, but it fails when x264rgb is required

@andfoy
Copy link
Contributor Author

andfoy commented Sep 28, 2020

For some reason, tests pass when test_io.py is executed with pytest -v test/test_io.py

imagen

However, it fails when they are executed alongside all the testsuite (pytest -v test)

imagen

@fmassa
Copy link
Member

fmassa commented Sep 29, 2020

@andfoy if tests pass when only running on test_io, but not the full test suite, this might indicate that another test (test_video_reader?) is also loading ffmpeg from our binaries and affects test_io. To make sure that this is the case, can you run test_io and test_video_reader in the same run to double-check (although the order on which the tests are run might be important)

@andfoy
Copy link
Contributor Author

andfoy commented Oct 5, 2020

Found more information related to the test failure cases:

#2745 (comment)

@fmassa
Copy link
Member

fmassa commented Oct 6, 2020

Great, this is awesome, we will soon be unblocking this and @bjuncek PR I believe

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Let's merge this PR and see how it goes.

We can clean a few things up (following @peterjc123 comment) on a follow-up PR.

Also, ccing @malfet and @seemethere for visibility -- we are adding ffmpeg as a dependency for torchvision.

@fmassa fmassa merged commit 635406c into pytorch:master Oct 6, 2020
@andfoy andfoy deleted the distribute_ffmpeg branch October 6, 2020 18:27
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* Add PyTorch FFmpeg to wheel and conda distributions

* Try to install wget from conda

* Add yq flag on Mac

* Correct copy instructions

* Use cURL on Windows

* Call bzip2 directly due to msys2/MSYS2-packages#1548

* Copy ffmpeg binaries to system-wide directories

* Try to use std:c++17 on Windows

* Try to define ssize_t on Windows

* Use C++14

* Declare AVRational structs explicitly

* Initialize AVRational explicitly

* Replace macro to prevent errors on Windows

* Replace AV_TIME_BASE_Q

* Add library paths for video extension

* Force ffmpeg from pytorch channels?

* Fix clang style warnings

* Update CONDA_CHANNEL_FLAGS

* Fix clang style issues

* Update unittest

* Use FFmpeg 4.2

* Install correct version on Mac

* Pin av version to 8.0.0

* Fix string formatting issue

* Fix pip pinning

* Try with 8.0.1

* Use av 8.0.2

* Remove trailling whitespaces

* Disable test_io_opt.py

* Disable test_datasets_video_utils

Co-authored-by: Francisco Massa <[email protected]>
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* Add PyTorch FFmpeg to wheel and conda distributions

* Try to install wget from conda

* Add yq flag on Mac

* Correct copy instructions

* Use cURL on Windows

* Call bzip2 directly due to msys2/MSYS2-packages#1548

* Copy ffmpeg binaries to system-wide directories

* Try to use std:c++17 on Windows

* Try to define ssize_t on Windows

* Use C++14

* Declare AVRational structs explicitly

* Initialize AVRational explicitly

* Replace macro to prevent errors on Windows

* Replace AV_TIME_BASE_Q

* Add library paths for video extension

* Force ffmpeg from pytorch channels?

* Fix clang style warnings

* Update CONDA_CHANNEL_FLAGS

* Fix clang style issues

* Update unittest

* Use FFmpeg 4.2

* Install correct version on Mac

* Pin av version to 8.0.0

* Fix string formatting issue

* Fix pip pinning

* Try with 8.0.1

* Use av 8.0.2

* Remove trailling whitespaces

* Disable test_io_opt.py

* Disable test_datasets_video_utils

Co-authored-by: Francisco Massa <[email protected]>
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.

Make video_reader backend be available in the binaries
3 participants