Move FFmpegInteropLibs.props into project and remove Inconsistency

[softworkz]

The inclusion of the FFmpegInteropLibs.props inside <ImportGroup Label="PropertySheets"> wasn't quite right.

It caused an internal error with the consequence that the C++/WinRT property page was not shown (or not always visible). This PR fixes the issue, allowing it to be visible always:

Then there was the FFmpegInteropX.FFmpegUWP.targets file included but its content then excluded with a condition. This PR removes that inclusion and the flag and adds the 'ReferenceCopyLocal' path explicitly in the project. That way, include files, lib files and dll files come all from the same source (./Output/..) instead of possibly mixing content from the output folder and the FFmpeg nuget package under ./packages

[lukasf]

The first changeset is fine.

The second changeset is wrong. The intention of this flag was to prevent copying all the ffmpeg dlls into the FFmpegInteropX output folder while we build the lib. We only need the include files form FFmpegInteropX, but not the dlls, and we also do not want the dlls to be propagated to consuming libs (if project references are used).

If you make a new PR with only the first changeset, then we can merge that.

[softworkz]

Okay, then I'll remove the dll copying.

But why include the targets file, then? And then create a flag to make it non-effective? Why not remove targets and then the flag is not needed (like I did here)?

The intention of this flag was to prevent copying all the ffmpeg dlls into the FFmpegInteropX output folder while we build the lib.

[lukasf]

Because we need all the other parts of the target file (include directories and libs). It does not only contain the copy local part.

[softworkz]

Because we need all the other parts of the target file (include directories and libs). It does not only contain the copy local part.

But the includes are already coming through the .props file. Essentially you're doubling them up and are currently including both: The includes from the Output folder (via the props file) and the includes from the package (via the targets file).

[softworkz]

Ho ho ho ho hoooooo!

Now I suddenly come to realize... couldn't this even be the reason why many are experiencing crashes with custom ffmpeg versions?

When header files from the official FFmpeg package are taken on build and then used with the binaries from a custom build - then that's just a blueprint recipe for crashes to be seen.

[softworkz]

Now I suddenly come to realize... couldn't this even be the reason why many are experiencing crashes with custom ffmpeg versions?

Confirmed!

Remember the crash I reported when accessing the ffmpeg dictionary for retrieving the title? I had commented it out (which was probably a bad idea, because that's just like closing the eyes from a problem). Now it doesn't crash anymore!

I'm pretty sure that this is also the reason for the crashes which others have experienced: The vcxproj was including headers and libs from two sources: The output folders and the FFmpeg nuget package under ./packages - which is typically the original FFmpeg package. When taking headers or libs from one source and later using it with dlls from the other source (and there's no ABI compatibility), then it's no wonder when crashes are happening.

I have updated this PR now and removed the ReferenceCopyLocalPaths section and I've also added the missing ResourceCompile element, leaving the inclusion of the targets file removed.

Of course, in general two possible options exist:

• Compile against an ffmpeg build in the ./Output/FFmpegUWP folder OR
• Compile against the nuget package in the ./packages/... folder

But it can't be both at the same time. Now, which one should it be?

I see it from the perspective of someone being new to the project: If it would use the nuget package and someone like me comes and builds its own version of ffmpeg, then it would be anything but obvious that it's also needed to replace package in the packages folder - after building the custom ffmpeg but before building FFmpegInteropX.

Having a "blank" state - where you can't build FFmpegInteropX before you haven't built ffmpeg seems to be more reasonable to me. Of course this could be made switchable again, but that makes the vcxproj even more cluttered and the whole story more confusing, but that's eventually a choice of yours to be made.

[brabebhin]

I think this was originally approach 1, when you'd have to build ffmpeg by yourself. It's also how Microsoft did it. The nuget was introduced at a later date.

I think the only time we use a custom ffmpeg build is when updating the ffmpegUWP package to a new ffmpeg version, and that takes some doing indeed.

It would be cool to dynamically pick 1 over 2 if it exists. It would make things easier for everyone I guess.

[softworkz]

It would be cool to dynamically pick 1 over 2 if it exists. It would make things easier for everyone I guess.

From a background of being new to the project, I can say that this is the worst possible thing that you could do to a newcomer.

Automatic things are always nice but only for those who know about it. But when you don't know this, it can drive you into the highest heights of madness 😆 Even when you know about it, it's difficult, because you can never be confident with which input the lib has been built. Just following example from the way I've been working with this:

• I make some changes which I want to compare with an earlier state, or I want to try a different route and want to keep my work-in-progress
• Then I create another source tree (from the git repo) in another folder
• I make some changes there and compile - but I forgot to copy my ffmpeg build outputs
• If it would switch automatically, then it would take the ffmpeg from the nuget package and I will not notice that this happened and I'll get a result which behave differently again and costs me another few hours to find out

When I know it, then it doesn't hurt me of course, but if not...

[brabebhin]

We could document it ofc. We fully lack docs in this topic anyways.

[softworkz]

We could document it ofc. We fully lack docs in this topic anyways.

Docs are good - when it's being read, and then this is a detail so special, that you might have read it without making sense out of it and when you get there, you don't remember. Making it automatic is almost the same as it was, and as it turns out, this has probably been a core reason for the crashes (not the only, but one of) which cost me a large amounts of time and burnt a lot of money. There are always such things in sw development, but saving others from running into known potential pitfalls provides mutual benefit among developers.

Bottom line is: I got bitten by this, aaarrrggggghhhh! 😜

[brabebhin]

By docs i mean some summary descriptions on what is going on, so you can jump start. Nothing too fancy, so people can quickly read it.

[lukasf]

Actually, I totally forgot that we had these definitions in the .props file as well. The intention was to use the definitions from the NuGet file. We should remove the redundand local definitions and use NuGet only. Otherwise, we have to keep things up-to-date in two places and we might not even notice if one of the two gets out of sync. Plus, it might lead to obscure bugs such as the one you noticed @softworkz.

[lukasf]

I simply deleted the FFmpegInteropLibs.props. Build works fine, everything is already defined in the nuget packages. #388

[softworkz]

I simply deleted the FFmpegInteropLibs.props. Build works fine, everything is already defined in the nuget packages. #388

Sure. I favored the other way because now the risk still exists that somebody who builds a custom ffmpeg doesn't realize that FFmpegInteropX is built against the original package instead of the custom build.

But well, it's a detail.