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

Suggested changes for CUDA build #209

Merged
merged 8 commits into from
Nov 21, 2023
Merged

Conversation

Xosrov
Copy link
Contributor

@Xosrov Xosrov commented Nov 6, 2023

These changes were made mostly because the CUDA Dockerfile in the project, while successfully building FFmpeg, didn't recognize nvenc for me. I made some changes that made it work.

Main Changes:

  • Use NVIDIA CUDA+Ubuntu as the building stage image instead of Ubuntu base.
  • Build and use deviceQuery from CUDA Samples in cuda-ubuntu.dockerfile so compute capability can be detected automatically. Default values are still hard-coded inside the build script, and can be overridden by setting the CUDA_COMPUTE_CAPABILITY env variable.
  • Make use of CURRENT_PACKAGE_VERSION more so as to reduce redundancy.
  • Add -O2 to nvccflags. Since NVIDIA's own examples do this, I don't think anything is going to break.
  • Added more documentation to the README regarding these changes, and updated the example build output to match the the CUDA build and changes in general.

Other Changes:

  • Added libzmq support
  • Change download link for libsdl (It's not working for me for some reason) to use SDL's Github release page instead
  • Change download link for zlib, for the same reason, to use Madler's Github release page instead

The final image containing the binary is still around 650MB.

- Updated cuda-ubuntu Dockerfile to use NVIDIAs CUDA image instead
- Updated cuda-ubuntu Dockerfile to build NVIDIAs deviceQuery for
  automatic compute capability detection
- Updated build-ffmpeg to use automatic compute capability
- Added libzmq to build-ffmpeg
@markus-perl
Copy link
Owner

Hi Xosrov, Thank you for the pull request.

Currently the integration tests fail with the following error message:
0.301 CUDA compute capability could not be found... do you have CUDA access in docker?

It should be possible to build the image without an active Nvidia graphics card. Some people need to built the image on a different machine than where the image is later used.

Please update your pull request to pass the integration tests. Then I can merge the changes back to the master branch.

@Xosrov
Copy link
Contributor Author

Xosrov commented Nov 7, 2023

Hey Markus, your concerns are valid. I will have to add default values as fallback to prevent build failures.
I will get this done by the end of the week.

All important changes:
- Updated README
- Added "-O2" build option to nvccflags
- Make use of CURRENT_PACKAGE_VERSION more to reduce redundancies
This was done to avoid superfluous checks for cuda compute capability,
even though they are not needed
@Xosrov
Copy link
Contributor Author

Xosrov commented Nov 13, 2023

@markus-perl I've added the default values, but also made some changes to the overall build script. Please run the workflow and let me know what you think.

EDIT: I've updated the original pull request text with more details about the changes

@Xosrov Xosrov marked this pull request as draft November 13, 2023 11:41
@Xosrov Xosrov marked this pull request as ready for review November 19, 2023 10:58
@Xosrov
Copy link
Contributor Author

Xosrov commented Nov 19, 2023

@markus-perl The CI problems are fixed.
For OS X, the problem was with the libzmq package using deprecated functions, using a slightly older version fixed the problem.

For the CUDA Dockerfile, the problem was in the naming of the libraries. For example changing libnppc.so -> libnppc.so.12 fixed the issues. I'm not sure why this was having problems in Github CI, even though it worked for me?

Please take a look and let me know what you think. Thanks for the amazing project!

@markus-perl
Copy link
Owner

Thx for the work. I will merge the changes into the master branch.

@markus-perl markus-perl merged commit d94fc2f into markus-perl:master Nov 21, 2023
5 checks passed
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.

2 participants