Skip to content

[ffmpeg] Add NVIDIA Codec feature for Windows and Linux#9171

Merged
dan-shaw merged 10 commits intomicrosoft:masterfrom
marcbertola:ffmpeg-nvcodec
Mar 24, 2020
Merged

[ffmpeg] Add NVIDIA Codec feature for Windows and Linux#9171
dan-shaw merged 10 commits intomicrosoft:masterfrom
marcbertola:ffmpeg-nvcodec

Conversation

@marcbertola
Copy link
Contributor

This is my first vcpkg pull request, I'm very receptive to feedback on how I can do this better!
This change exposes FFmpeg options to enable NVIDIA's hardware codecs.
./vcpkg install ffmpeg[nvcodec]

Changes:

  • Created a new feature for ffmpeg called nvcodec
  • Force the configure script to be executable
  • Make ffnvcodec visible to pkgconfig, that ffmpeg uses to find the headers.

Notes to ffnvcodec maintainer ( @NancyLi1013 ? ):

  • I rolled back the header version -- my Linux machine complained that 9.1 was too new for my GTX1060. I am concerned that setting the version too high may limit the target audience.
  • The existing nvcodec port contained an LGPL license, but I see no mention of it in the headers that it fetches. They don't mention any specific GPL-related restrictions so I used the common text from the headers to create a copyright file -- but I'm not sure it's my place to do so. These files should probably have been published with a proper license definition. and ideally, we may want to let the headers do the talking instead of guessing the license to use.

@marcbertola marcbertola changed the title Add NVIDIA Codec support for Windows, Linux and Linux for Tegra Add NVIDIA Codec support for Windows and Linux Dec 2, 2019
@PhoebeHui PhoebeHui requested a review from NancyLi1013 December 2, 2019 09:55
@grdowns grdowns changed the title Add NVIDIA Codec support for Windows and Linux [ffmpeg] Add NVIDIA Codec feature for Windows and Linux Dec 2, 2019
@fawdlstty
Copy link
Contributor

👍 great pull request

vcpkg_from_github(
OUT_SOURCE_PATH SOURCE_PATH
REPO FFmpeg/nv-codec-headers
REF n9.0.18.1
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to use commit id instead of tag name in REF place. Could you please update it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@NancyLi1013
Copy link
Contributor

@marcbertola thanks for this PR.
First, I make a simple description about the license for ffnvcodec .
Since I noticed that ffnvcodec is related with ffmpeg and I didn't find the description about the license any where. So I used license from ffmpeg. I am sorry about that I didn't clarify for this in my PR.
As for the version ffnvcodec roll back, I am not sure whether it is suitable. I need to further confirm.

@fawdlstty
Copy link
Contributor

I have tried to compile both versions successfully

@fawdlstty
Copy link
Contributor

I studied the corresponding relationship between cuda SDK and ffnvcodec, and found that ffnvcodec and cuda are subordinate, so ffnvcodec should be installed as a feature of cuda, and the ffnvcodec version should be dynamically determined according to the cuda SDK version during installation.
In addition, ffnvcodec has an ambiguous correspondence with ffmpeg. If one of them is a high version and the other is a low version, it will not compile.I'm not sure about the association yet, but if anyone can find a relationship, you can be prompted at compile time, for example cuda, and if ffmpeg is not compatible with ffnvcodec, you can be prompted for the range of ffmpeg version that needs to be installed.

@fawdlstty
Copy link
Contributor

congruent relationship:
cuda version
nvcodec version

@marcbertola
Copy link
Contributor Author

If I understand correctly, ffnvcodec is a set of headers that can be included to dynamically link to the NV codec functions without needing an import library/.dll or the .so while compiling.
The revision number seems to indicate the supported NVIDIA API level, which is probably tied to a driver version. When I used an ffnvcodec version that was too recent, it complained that my drivers were too old (i.e. they probably don't support the desired API level). I'm working on Linux these days, maybe the versions don't keep up as well. Not sure.

So yeah, I'm wondering what we should do with this. The vcpkg team probably has an opinion on which versions to prioritize: Use the latest and greatest, or support older legacy versions?

Regarding the 10.2 mismatch - @fawdlstty, have you had success with finding the right combination of ffnvcodec and ffmpeg versions for CUDA 10.2? I'm stuck on version 10.1 / 10.0 at the office right now, I haven't been able to try it yet.

@fawdlstty
Copy link
Contributor

fawdlstty commented Dec 12, 2019

I have use cuda 10.2 and ffmpeg n4.2-02d8c63f80, the two ffnvcodec versions compile and run successfully: n9.0.18.1-77b06e328b and n9.1.23.0-c646ab60a8

@fawdlstty
Copy link
Contributor

fawdlstty commented Dec 12, 2019

This creates a problem: the user's graphics driver must have the latest version installed.If you haven't updated your graphics driver in 2018, you can't use hard coding.Therefore, I think compatibility should be considered. If the developer needs to use cuda9.1, then VCPKG should provide ffnvcodec corresponding to it and indicate ffmpeg version

@NancyLi1013 NancyLi1013 self-assigned this Jan 14, 2020
@giladbau
Copy link
Contributor

Hi. I stumbled into this by looking for this exact solution.
Why is this not being merged? If the only required change is that seemingly minor request from @NancyLi1013, will you accept it from someone else?

@marcbertola
Copy link
Contributor Author

Unfortunately, there is more to it than the change request. I didn't have clarity on the following items:

  • Which policy to use for selecting the ffnvcodec version
  • The copyright file to bundle with ffnvcodec. The copyright file added by the maintainer says LGPL, but the headers provided by NVIDIA don't stipulate that anywhere as far as I can tell.
  • fawdlstty recently reported a regression (but I can't find it in the log above... Wondering if it got removed?)

I recently updated my Linux machine to cuda 10.2, so I'll take it for a spin again to see how far I get without modifying ffnvcodec - If I can leave it as is then the issues above go away.

I'll look over it today.

@marcbertola
Copy link
Contributor Author

Ok, my Linux PC is building with 9.1.23.1. Installing CUDA 10.2 on my Windows PC and I should be able to provide feedback shortly.

@fawdlstty
Copy link
Contributor

fawdlstty recently reported a regression

that's my question, it has nothing to do with this PR. I think this request can be merged as soon as possible, if there are special requirements can also modify the port to achieve custom requirements

@marcbertola
Copy link
Contributor Author

Ok, I should be able to post an update within the hour that uses the original ffnvcodec version (9.1.23), so it removes some of the uncertainty we had about rolling back the version in this PR.

@marcbertola
Copy link
Contributor Author

@NancyLi1013, your requested change has been implemented. I also changed to the ffnvcodec version that was originally used before the PR.
I was able to build successfully on my Linux and Windows machines, with dynamic targets, with CUDA 10.2 on both machines.

@NancyLi1013
Copy link
Contributor

Hi @marcbertola

I noticed ffmpeg has been updated.
Could you please update ffmpeg to the version in master branch first?
Please also bump the version in CONTROL file.

@marcbertola
Copy link
Contributor Author

Hi @NancyLi1013
The requested changes have been applied. Please let me know if there is anything else.

@NancyLi1013
Copy link
Contributor

Need to test features.

Copy link

@ZhengzhongSun ZhengzhongSun left a comment

Choose a reason for hiding this comment

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

Works for me! It's really helpful to everyone who uses the ffmpeg with nvenc.

@NancyLi1013
Copy link
Contributor

Sorry for the late reply.
All the feature test results for this port are as follows:

  • All features are passed on x86-windows triplet.

  • All features except for opencl, openssl, lzma (These three features are failed due to not found) , ffmpeg, ffprobe (These two features have been fixed in PR [ffmpeg] Fix build static error #10368) are passed on x64-windows-static triplet.

  • All features except for opencl, openssl, lzma (These three features are failed due to not found) , ffmpeg, ffprobe (These two features have been fixed in PR [ffmpeg] Fix build static error #10368) , vpx, x264 (These two features are only supported on Windows), nvcodec (has not tested) are passed on x64-linux triplet.

Note: Due to nvcodec depends on cuda , my Linux environment is broken about this. I try to install CUDA bud failed. So this feature has not been tested so far. This is also the reason for the late reply.

If you can make sure that the new feature nvcodec can work well on Linux, personally, there is no problem about this PR now.

Thanks again for your efforts and patience.

@marcbertola
Copy link
Contributor Author

I use the feature actively on Linux for my experiments. It works for my purposes (hw acceleratiob for h264 and hevc). Not sure if there are some formal things to check apart from that.

My triplet is the x64 linux one, but modified to be dynamic instead of static.

@NancyLi1013
Copy link
Contributor

Since vcpkg supports static on Linux. It might be better to make sure this feature work in this case.

@marcbertola
Copy link
Contributor Author

marcbertola commented Mar 10, 2020 via email

@marcbertola
Copy link
Contributor Author

marcbertola commented Mar 13, 2020

@NancyLi1013
I have successfully compiled under x64-linux (static), with CUDA 10.2.
Using the ffmpeg binary under /packages/ffmpeg_x64-linux/bin (not under installed because of the reason you stated in the last post) I have successfully transcoded a file using the hardware accelerated codec (H264 and HEVC).

IMHO, I think this confirms the Linux build.

Sorry for the delay, been distracted by the world events :\

@marcbertola
Copy link
Contributor Author

What is the process now to get this merged? Do I need to do anything else?

Copy link
Contributor

@dan-shaw dan-shaw left a comment

Choose a reason for hiding this comment

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

Wow this PR has been open for a while. Let's get this merged. It looks like a change we want, made some comments below.

@dan-shaw dan-shaw added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. waiting for response and removed needs-feature-review info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Mar 18, 2020
@marcbertola
Copy link
Contributor Author

OK, I submitted the comment with the tag and now instruct FFmpeg to find the nvcodec headers in the installed directory.
I have also rebuilt and performed a summary encode/decode test using the hardware codecs (h264_nvenc and hevc_nvenc) on Ubuntu 18.04 with CUDA 10.2 (x64-linux and my own x64-linux-dynamic). The new changes don't actually change the data being compiled, so I think it's fair to assume @NancyLi1013 's tests on Windows still hold, above.

Copy link
Contributor

@dan-shaw dan-shaw left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, just one more question

# Copy ffnvcodec.pc to the location in the MSYS environment where pkgconfig
# expects to find it.
mkdir -p /usr/lib/pkgconfig
cp ${CURRENT_PACKAGES_DIR}/lib/pkgconfig/ffnvcodec.pc /usr/lib/pkgconfig
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this copy? We are installing packages config in the packages dir for ffnvcodec and ffmpeg is getting it via PKG_CONFIG_PATH

Copy link
Contributor Author

@marcbertola marcbertola Mar 20, 2020

Choose a reason for hiding this comment

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

Hm. This was a long time ago. It think the idea is as follows:

  • The FFmpeg portfile invokes build.sh in MSYS
  • build.sh which in turn calls FFmpeg's configure, which calls pkg_config
  • pkg_config looks for a .pc file for ffnvcodec in the default location.

This last step is the reason why we copy the file to the root /usr/lib/pkgconfig

Now, I guess that if we do away with this copy, we remove a potential build problem. I can think of a situation where the user would decide to purge/reset the msys environment. The ffnvcodec package would still be installed, but the ffmpeg build would fail because the .pc file is no longer in the msys environment.

My question for you is: does vcpkg launch msys with the right PKG_CONFIG_PATH? if so, then yeah, I think we can remove these lines.

I'll give it a try. I'll start with a fresh checkout, remove the offending lines, and try again. I'll let you know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If make eventually accepts the PREFIX parameter, and it can run under Windows, then I think we may even be able to dispense with the indirection via the build.sh script in ffnvcodec.

Copy link
Contributor Author

@marcbertola marcbertola Mar 20, 2020

Choose a reason for hiding this comment

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

Ok, I ran into some problems, sorry for any confused back and forth email you may have received.
Contrary to my original impressions, PKG_CONFIG_PATH is set in MSYS, but it is a windows path (ex: C:\blah\blah), which doesn't work in MSYS. Once it was converted using cygpath, it works.
So: yes, these lines can be removed, but PKG_CONFIG_PATH given to MSYS should be converted using cygpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have submitted a workaround for the cygpath issue along with the removal of the lines.
I used the -p option on cygpath in case it contains multiple directories.

@dan-shaw dan-shaw added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed waiting for response labels Mar 23, 2020
@dan-shaw
Copy link
Contributor

LGTM, thanks for all those changes. Currently, we are fixing some CI problems before merging this.

@dan-shaw dan-shaw merged commit 78a2116 into microsoft:master Mar 24, 2020
@dan-shaw
Copy link
Contributor

@marcbertola Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants