search

ffmpeginteropx / FFmpegInteropX

FFmpeg decoding library for Windows 10 UWP and WinUI 3 Apps
Apache License 2.0
205 stars 52 forks source link

Allow using VCPKG to build native dependencies #429

Closed JunielKatarn closed 4 weeks ago

JunielKatarn commented 1 month ago

All libraries except x265 can now be built using VCPKG, which hugely simplifies the dependency build workflow and makes most submodules under Libs obsolete (safe to remove).

Sample VCPKG-BASED workflow:

  1. Install VCPKG dependencies (takes a while). 
    Set-Location $ffmpegInteropXClone\Libs\vcpkg
.\bootstrap-vcpkg.bat 
vcpkg.exe install --triplet x64-uwp bzip2 dav1d ffmpeg libiconv liblzma libxml2 openssl x264 zlib
  2. Open VS solution.
  3. Build VS solution.
    • Set FFmpegInteropXImportFromNuGet to false by either the MSBuild command line argument or adding file Directory.Build.props.user in the repository root with these contents: 
      <?xml version="1.0" encoding="utf-8"?>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <FFmpegInteropXImportFromNuGet>false</FFmpegInteropXImportFromNuGet>
  </PropertyGroup>
</Project>
brabebhin commented 4 weeks ago

Thank you for the great PR @JunielKatarn

Would it be possible to rebase this on the winui-build branch? That branch contains several changes to the build pipeline, it would be interesting to see if everything still holds.

It will likely be merged later on this year once we are happy with the preview releases, anything on master atm should be considered sort of obsolete.

lukasf commented 4 weeks ago

Yes, thanks as well for the contribution @JunielKatarn!

Right, as @brabebhin said, all current development is on the winui branch. This will be the 2.0 source code.

I have a few remarks:

Most importantly, please don't mix unrelated things into a single PR. I see three different things here, which are completely unrelated: 1. The actual VCPKG stuff, 2. Change of line encoding, 3. Assumed fix for mss reference. This makes it difficult to review the PR. Especailly the change of file encoding makes it very hard to review the actual code changes, which happen in the same files. But also, we might want some of the changes and we might not want others. If you mix them in one PR, we cannot easily take some and leave others. Generally, if you want to do changes to the lib, it would be best to start a discussion first, before doing the work of an actual PR. Then we can align and do PRs for those things deemed neccessary.

Now to the changes themselves:

  1. VCPKG: This is definitely an interesting approach! I have been looking into VCPKG as well at some point. But it comes with some major downsides: There are zero configuration options for the actual ffmpeg build (afaik). As you probably know, there are loads of options for the ffmpeg configure step. Do we want this lib or that, do we want a GPL build or an LGPL build? Do we want decoders, encoders, or both (which has major impact on the file size)? Etc etc. Our current system does allow us to set many of these options. This all goes away for a generic ffmpeg build, when switching to VCPKG. And in the end, with our current build system, the build is very simple as well, it's only one call and all the things happen automatically, all the hard stuff is done by the scripts. Still, I think we could very well add the VCPKG build as a second option. It is good that you added parameter switches to turn this on and off, so people can choose one or the other. Thinking about it, maybe it would even be possible to combine the two approaches: First, use VCPKG to build the lib dependencies, then use a customized ffmpeg build script, linking ffmpeg with the VCPKG lib output. This would combine the best of the two worlds.

  2. Line endings: We use the default setting of git for windows, which is "auto". It converts line endings on checkout to CRLF and on checkin to GIT it turns back to LF. It seems that you are using a different settings, which seems to make all files you touch changed. Now next time one of us changes a thing in these files, the whole file will appear changed as well, turned back to original encoding. This is not good. Please use auto crlf setting like this, when working on our lib:

    git config --global core.autocrlf true

Edit: I just read that it is also possible to set this locally, when run from repo folder, then it only affects the current repo:

git config auto.crlf false

  1. mss reference: We have added an automatic reference handling some time back. The mss instance is created on first access of one of the related properties or methods (e.g. get MediaPlaybackItem). I am pretty sure you won't see any NullReferenceException. The MediaPlaybackItem itself then has a strong reference to the MSS, and the MSS has a strong reference to "us" (the FFmpegMediaSource). So as long as media is playing, the FFmpegMediaSource is kept in memory. No more need for devs to store it in a field to prevent it from being collected, which was a very frequent cause of trouble and devs asking for support. If you think that you have found a bug, please let us know (with a way how to reproduce). But I am pretty confident that this works well.
JunielKatarn commented 4 weeks ago

Thanks.

I will split the PR into separate ones and re-target to the new branch.

Regarding autoClRf, I'll open a discussion.

I very strongly believe it's unnecessary and leads to unneeded EOL mismatches.

Regarding the MSS reference, if this library is used without using a playbackItem (valid use case I'm actually running into) then lifetime and ownership becomes an issue.

It is true VCPKG is not easy (yet still possible) to configure.

Note, the aim of this PR is to enable it as an opt-in mechanism it would not disrupt the current dependency management method by default.

softworkz commented 4 weeks ago

I have strong reservations regarding the use of VCPKG for building FFmpegInteropX for a plentitude of reasons:

So, @JunielKatarn - thanks a lot for your effort, but I'm afraid to say that in my opinion, we should stay away from VCPKG by all means.

lukasf commented 4 weeks ago

Regarding the MSS reference, if this library is used without using a playbackItem (valid use case I'm actually running into) then lifetime and ownership becomes an issue.

If this also happens on the winui branch, please open an issue with some details of your use case. I am sure we will find a fix for that. The change you proposed here, when used in a playback scenario, would actually lead to a circular reference with no way to release it.

softworkz commented 4 weeks ago

Regarding AUTOCRLF

Just forget about that. In the first place, the idea is plausible to say that "it shouldn't be needed". From that same idea I had tried to switch it off in three different projects over the years and it never worked out. I always ended up reverting back to AUTOCRLF.

IIRC, the problems are:

My takeaway: This is creating trouble, trouble and more trouble. Not worth it in any way and there's no advantage at all.

softworkz commented 4 weeks ago

To illustrate how important dependency versions are and how important it can be that the configure script is being run to adjust the build accordingly, here's a contribution I had made to ffmpeg a while ago which was about the libx264 dependency: https://github.com/ffmpeg/ffmpeg/commit/dd5a0302d5286e66ca03a57770537d5afa4c7775

brabebhin commented 4 weeks ago

As long as we use and target Windows we should probably use autocrlf = true.

brabebhin commented 4 weeks ago

Risk of Interference VCPKG is an intrusive pest! Enabling it is an all-or-nothing decision. Once it is enabled, it pulls in ALL LIB PATHS OF ALL LOADED VCPKG packages - you cannot control this any further. So, what happened to me is that it took me a whole day to figure out why I wasn't able to build FFmpegInteropX - just in the normal way and I didn't even think about VCPKG. It was just enabled in VS because I had used it before for something different. And I had downloaded a VCPKG package of one of the ffmpeg dependencies one or two years ago. This causes the build of FFmpegInteropX-ffmpeg to fail, because Visual Studio automatically adds all include paths of all VCPKG packages before the include paths in the actual project/solution. So it included a header file from the VCPK package (wrong version) instead of the actual one from the submodule

This sounds like a big problem, especially if we want to incorporate new versions of the libs. You technically don't know which lib it linked if they are binary compatible. I used vcpkg in a previous project and it seemed nice but only used it for one thing and never bothered again. It is nice and it would be very interesting to see of it can work but we will need to do some very good testing. I am guessing this global cache thingy flow works well for serverless contained build agents but not on dev machines. Sounds like something Microsoft would do.

softworkz commented 4 weeks ago

This sounds like a big problem, especially if we want to incorporate new versions of the libs. You technically don't know which lib it linked if they are binary compatible.

Binary compatibility is not a problem, because you would be using the same headers to compile both, which means they'll be binary compatible. But you don't know whether they're logically compatible and whether their dependencies are compatible. Too often, a specific combination of versions (or version ranges) is needed in order to work well together.

I used vcpkg in a previous project and it seemed nice but only used it for one thing and never bothered again.

Yes, same for me. But that previous project was ffmpeg related, so I had versions of dependencies as VCPKG packages installed which were different from the ones needed for FFmpegInteropX-ffmpeg. And that alone caused the build of FFmpegInteropX to fail. I had to globally disable VCPKG in VS (and researched half a day for a less drastic option - without success).

I am guessing this global cache thingy flow works well for serverless contained build agents but not on dev machines.

I think it would need a script which uninstalls all VCPKGS before the build and then installs the right ones. But this can take quite some time and after that, any other VCPKG builds you might have would fail.

To me it seems that the whole VCPKG system is primarily for allowing people to easily build things with build systems which are incompatible to VS/VC, filling such gaps when there's a specific need, but it doesn't go far enough in terms of version management and configurability of dependencies.

softworkz commented 4 weeks ago

Oh, there's also one more thing we haven't talked about: Is it even possible to build all the VCPKG dependencies for UWP (local c runtime)? Generally, I'd assume "yes", but how about ffmpeg VCPKG itself?

Also not to forget HWA in ffmpeg. If ffmpeg-VCPKG cannot be built with HWA support, that would be another showstopper (and I I'm pretty sure it can't).

Scratch that - seems it can be built with HWA now.

softworkz commented 4 weeks ago

To be fair, I double checked and there seems to be an alternative to the global integration (vcpkg integrate - which I called a pest), where you import a props and targets file in the .vcxproj. Then you can also include a manifest (vcpkg.json) which specifies all dependencies and their versions and those will then be downloaded and maintained inside the project folder tree (locally instead of globally).

softworkz commented 4 weeks ago

Additional considerations regarding performance:

JunielKatarn commented 4 weeks ago

Regarding the MSS reference, if this library is used without using a playbackItem (valid use case I'm actually running into) then lifetime and ownership becomes an issue.

If this also happens on the winui branch, please open an issue with some details of your use case. I am sure we will find a fix for that. The change you proposed here, when used in a playback scenario, would actually lead to a circular reference with no way to release it.

Understood.

I will explore the new default branch and see if the issue is still there.

The scenario is simple: An app requires setting up a MediaPlayer. There is no usage of MediaPlaybackItem. Its functionality is taken care of by the app itself; the MediaPlayer expects a MediaStreamSource (that's where FFmpegInteropX comes in).

In summary, need to be able to obtain a MediaStreamSource from an IRandomAccess without the involvement of MediaPlaybackItem. In the current master branch this can't be done because:

  1. The MediaStreamSource is never created (CreateMediaStreamSource() is never called).
  2. Even if that method's call is added within FFmpegMediaSource, the instantiated MediaStreamSource gets lost immediately due to lack of ownership.

Something like this:

var ffmpegSource = await FFmpegInterop.FFmpegInteropMSS.CreateFromStreamAsync(stream);
var player = new MediaPlayer();
player.Source = Windows.Media.Core.MediaSource.CreateFromMediaStreamSource(ffmpegSource.GetMediaStreamSource());
JunielKatarn commented 4 weeks ago

Additional considerations regarding performance:

  • The ffmpeg codebase is largely optimized for being built with gcc. Compiling with llvm is generally close but not on par with gcc. Building with MSVC though, results in substantially less performant binaries
  • ffmpeg includes quite a number implementations for which there's an optimized version in x86 assembler. I'm not sure whether the ffmpeg VCPKG makes use of them, because this would require a 3rd party tool (like NASM)

I'll try to take a look at recent benchmarks. At least for pure audio processing, MSVC-built FFmpeg has worked perfectly fine for me.

JunielKatarn commented 4 weeks ago

Oh, there's also one more thing we haven't talked about: Is it even possible to build all the VCPKG dependencies for UWP (local c runtime)? Generally, I'd assume "yes", but how about ffmpeg VCPKG itself?

Yes. If you played around with this PR, building full UWP-friendly FFmpeg is as simple as

vcpkg.exe install ffmpeg:${architecture}-uwp

That's one of the motivations for this request.

JunielKatarn commented 4 weeks ago
  • No Configure Script Like @lukasf already mentioned: you cannot seriously compile ffmpeg without running its configure script. I don't think that the VCPKG does that (well - it simply cannot do it without having a Linux of MSYS2/MinGW environment) - it's rather building it with a specific configuration that you cannot change.

VCPKG is certainly not designed for working on forked code bases but for one-off (or few-off) customizations, they can be done through patches. This is an annoying but established process.

The config script, which I will be tweaking myself for my own purposes, can be modified this way to get the desired build parameters.

Building ffmpeg requires specific versions (or version ranges) for each dependency. Currently, the versions of the dependencies are precisely specified by including them as submodules, each pointing to an exact commit This means that you can check out and build successfully When using VCPKG, all this is getting lost. Instead, the dependency versions are depending on the versions of dependencies that you have currently loaded as VCPKG package, This means, the build would fail if the versions you have aren't the right ones

  • Loss of Isolation If you are using VCPKG for building different stuff which needs a different version of one of the dependencies (e.g. OpenSSL or GnuTLS, etc.), then you cannot build one or the other without loading the matching VCPKG version before

It is true that VCPKG has fixed sets of versions for each release, but stable releases are consistent. The OpenSSL version bundled with, say, FFmpeg 7.0, will work correctly with it.

It is also possible to set up a specific version "matrix", if needed. Not easy, but doable.

  • Risk of Interference VCPKG is an intrusive pest! Enabling it is an all-or-nothing decision. Once it is enabled, it pulls in ALL LIB PATHS OF ALL LOADED VCPKG packages - you cannot control this any further.

This is false. The idea of VCPKG is to have multiple instances for multiple purposes. To serve FFmpegInteropX, it only needs an instance that builds the known dependencies (zlib, openssl,... up to ffmpeg itself). No extra garbage. You take (and build) what you need. Then, as in this PR, the project can refer to its LIB and INCLUDE paths, and we're done. Agreed, it would not be a good idea to use a VCPKG instance which gets updated every week and is used to built completely unrelated stuff. That is not the case here.

softworkz commented 4 weeks ago

This is false. The idea of VCPKG is to have multiple instances for multiple purposes.

What I was referring to is the global integration using vcpkg integrate - that's what I had used at that time. But today I re-checked:

To be fair, I double checked and there seems to be an alternative to the global integration (vcpkg integrate - which I called a pest), where you import a props and targets file in the .vcxproj. Then you can also include a manifest (vcpkg.json) which specifies all dependencies and their versions and those will then be downloaded and maintained inside the project folder tree (locally instead of globally).

see here: Integration Methods

That would seem to me the best way, where explicit dependency versions are specified in a manifest that is specific and local to FFmpegInteropX.

softworkz commented 4 weeks ago

The config script, which I will be tweaking myself for my own purposes, can be modified this way to get the desired build parameters.

How is the config script being run? It needs a unix environment...

JunielKatarn commented 4 weeks ago

...I always ended up reverting back to AUTOCRLF.

We have had different experiences :) I always ended up disabling it, especially for multi-platform projects. Works like a charm as long as you use EditorConfig (which this project has set up).

IIRC, the problems are:

  • There is no such option in Git to make it preserve line endings as they are (unless using attributes to declare it as binary instead of text, but then losing the ability to diff properly), so without AUTOCRLF, it will turn all line endings into Unix style (LF)

Incorrect. Setting AUTOCRLF to false does exactly that. The problem always relied with the text editor. Most Windows editors forced/defaulted to CRLF. Modern editors (including Visual Studio 2017+ and Code) are EditorConfig-compatible which makes this a non-issue, unless you want to edit the sources on Windows 95's Notepad.

  • This causes some files to work no longer (when CRLF is expected)

Not a single file has this issue in FFmpegInteropX.

  • Many tools will still add CRLF on enter and then these lines will show up as changed in Git even though there are no other content changes in those lines

That's exactly what that aspect of this PR addresses. Most Visual Studio/MSBuild/NuGet files will be treated as CRLF by their respective tools. I think we should keep them like that in both the Git workspace and the Git commit database, regardless of the development OS.

For any other files that those tools would not mess up with, relying on EditorConfig allows us to decide all text aspects, including line endings.

This change ensures committed content is identical to edited content, respects the tools' EOL conventions, and spares developers from the annoying "the line endings will be converted..." Git warning.s

JunielKatarn commented 4 weeks ago

The config script, which I will be tweaking myself for my own purposes, can be modified this way to get the desired build parameters.

How is the config script being run? It needs a unix environment...

VCPKG for ffmpeg (and tons of other packages) spins up an MSYS environment. All under the hood. All self-contained. All tested.

@softworkz please let me emphasize, my proposal is not to outright start using VCPKG (I need to figure out the config patching first), but allow it as an opt-in alternative for custom builds that do not align with the "official" FFmpegInteropX dependency architecture (i.e. an audio-only instance). (See Directory.Build.Props in this PR).

JunielKatarn commented 4 weeks ago

What I was referring to is the global integration using vcpkg integrate - that's what I had used at that time. But today I re-checked:

I see! And couldn't agree more. I find the "integrate" thing despicable. I myself fought against it in a work-related project. Gladly succeeded.

But this proposal does not care about vcpkg integrate. It uses it for an initial dependency build, and grabs what it needs from the VCPKG installation directory with MSBuild logic. It's the inverse of the "integrate" workflow. It's "I'll grab what I need".

softworkz commented 4 weeks ago

I'll try to take a look at recent benchmarks. At least for pure audio processing, MSVC-built FFmpeg has worked perfectly fine for me.

It always works perfectly, that's not the problem, it's just slower. I'm doing all the (direct) ffmpeg development in Visual Studio and using the project generator from ShiftMediaProject. It even compiles assembler code with NASM, but it's still slower, so our production versions are built on MSYS2/MinGM with gcc (like FFmpegInteropX currently).

It's easy to hit limits. For example, I'm not able to get a 4k HDR video with 60fps to play fluently on Xbox. For 30fps it mostly works, but it depends on the input bitrate. So it's easy to get to the edge of performance and there's still a significant difference when HW acceleration is used. I know this very well because I had developed the DirectX12 QuickSync hw accel in ffmpeg for Intel and I developed in VS and frequently compared to builds via gcc on MSYS2.

JunielKatarn commented 4 weeks ago

It's easy to hit limits. For example, I'm not able to get a 4k HDR video with 60fps to play fluently on Xbox...

That's a shame. I don't intend to get chatty here, but I'm wondering if this project has switched to GCC to build FFmpeg, then.

softworkz commented 4 weeks ago

But this proposal does not care about vcpkg integrate. It uses it for an initial dependency build

What does that mean. Does it use the global VCPKG package store or not?

and grabs what it needs from the VCPKG installation directory with MSBuild logic. It's the inverse of the "integrate" workflow. It's "I'll grab what I need".

But it's still not isolated, so it may interfere with dependencies that are installed for different project, no?

Could you please take a look at the "manifest method" where all dependencies are kept isolated and locally in the project folder tree?

softworkz commented 4 weeks ago

It's easy to hit limits. For example, I'm not able to get a 4k HDR video with 60fps to play fluently on Xbox...

That's a shame. I don't intend to get chatty here, but I'm wondering if this project has switched to GCC to build FFmpeg, then.

AFAIR, FFmpegInteropX's ffmpeg is built on MSYS2/MinGW using gcc) - @lukasf - right? Or is it using MSVC? It's almost a year since I last built an ffmpeg for FFmpegInteropX..

Sorry, it's using msvc toolchain.

JunielKatarn commented 4 weeks ago

But this proposal does not care about vcpkg integrate. It uses it for an initial dependency build

What does that mean. Does it use the global VCPKG package store or not?

Not sure what you mean by "global VCPKG package store". The VCPKG instance that FFmpegInteropX will use will contain a LIB, include and DLL directory root where the exact dependencies it requires will be added.

Using MSBuild logic (See Directory.Build.props, if VCPKG is opted-in, then other sources for these dependencies (local directories, NuGet packages) will be ignored, and vice-versa. It's non-interfering because it's either-or.

and grabs what it needs from the VCPKG installation directory with MSBuild logic. It's the inverse of the "integrate" workflow. It's "I'll grab what I need".

But it's still not isolated, so it may interfere with dependencies that are installed for different project, no?

As I mentioned above, I'd argue it's isolated. And if it's not right now, for sure we can make it so. In the end, VCPKG will just create another set of directories with headers and libraries that can be imported where needed, when needed.

Could you please take a look at the "manifest method" where all dependencies are kept isolated and locally in the project folder tree?

Will do, and open a separate discussion for this topic to avoid bloating this PR further.

And as Lukas suggested, I'll split the PR and re-target to the new principal branch.

softworkz commented 4 weeks ago

Not sure what you mean by "global VCPKG package store". The VCPKG instance that FFmpegInteropX will use will contain a LIB, include and DLL directory root where the exact dependencies it requires will be added.

Please see here: https://learn.microsoft.com/en-us/vcpkg/concepts/classic-mode What I was talking about is what they are calling "Classic Mode".

image

They are about to deprecate "Classic Mode" (most likely due to the downsides I've mentioned) and they are now recommending to use the Manifest Mode.

As I mentioned above, I'd argue it's isolated. And if it's not right now, for sure we can make it so.

Yes, with "manifest mode"

In the end, VCPKG will just create another set of directories with headers and libraries that can be imported where needed, when needed.

That set of directories should be specific, local (inside) and isolated to FFmpegInteropX - that's what I said originally, and luckily it seems that "manifest mode" is exactly fulfilling those requirements. 😄

softworkz commented 4 weeks ago

Please also see here: https://learn.microsoft.com/en-us/vcpkg/consume/lock-package-versions

image

image

JunielKatarn commented 4 weeks ago

Please also see here: https://learn.microsoft.com/en-us/vcpkg/consume/lock-package-versions

image

image

I'll follow up on the eventual GH discussion. Regarding specific package versions, I was thinking if ffmpeg is the main dependency this project cares about and others' versions do not matter as much so long as they play along well, it would only be a matter of choosing the VCPKG release that contains the exact version we want. The general principle in VCPKG releases is, the latest available versions of all packages, so long as they build and run, and can be used together.

softworkz commented 4 weeks ago

Regarding specific package versions, I was thinking if ffmpeg is the main dependency this project cares about and others' versions do not matter as much so long as they play along well, it would only be a matter of choosing the VCPKG release that contains the exact version we want. The general principle in VCPKG releases is, the latest available versions of all packages, so long as they build and run, and can be used together.

LOL, such principle cannot be pertained by VCPKG because it depends on ffmpeg and individual dependency packages whether they can work together - not on the VCPKG package creators. From experience with media-autobuild_suite over the years, it happens several times each year that the build of ffmpeg fails due to changes in newer versions of dependency libs.

But it's not just about breaking builds. It's simply not possibly to work professionally without having controlled and reproducible builds. Once you have stabilized your application and do a release, you cannot have surprises anymore. For example when there's a bug to fix (e.g. in ffmpeg), you need to be sure that you get a build that is 100% identical to the previous build - except that single bugfix you made. You cannot have a build that uses all-latest libs and be subject to the potential mistakes of dozens of developers that might have been newly introduced meanwhile.

So, the abilitiy to specify exact versions for all dependency libs is mandatory as far as I see it.