FFmpeg Win64: Introduce optional shared DLL method

[ghost]

I thought it would be a good first step for future framedumping improvements.

• Switch to a different method of using FFmpeg, requires runtime DLLs, no custom-build integration.
• DelayedLoading ensures DLLs are optional, only required when dumping frames.
• Updated to FFmpeg 4.0.2 (includes, etc)
• Rename Externals/ffmpeg folder to Externals/ffmpeg-win64 to make that more obvious
• Missing FFmpeg LICENSE.txt and README.txt files added (FFmpeg's license compliance recommendation point 2 states that using dynamic linking to DLLs is the preferred way of using it in another program )
• UPDATE1: Added FFmpeg text to Dolphin's about box (Qt) as recommended in point 10 of the FFmpeg license compliance checklist.
• UPDATE2: Update1 revert
• UPDATE3: Auto downloading added
• UPDATE4: Update 3 revert
• UPDATE5: Warning message FFmpeg install guide replaced with a link to the Dolphin Wiki.
• UPDATE6: Wiki page under construction.

Detailed explanation here:
Dolphin Forums - Framedumping Research

Version 1.0 of the warning message

...

Version 5 of the warning message

[RisingFog]

Could you branch out the External changes into a different commit?

[RisingFog]

I'm a +1 on this, and it has always been something that I wanted to have done with ffmpeg on Dolphin.

This should cut out not only custom building the dll files, but also distributing them and opens up the possibility of allowing any supported codec being used for frame dumping.

[ghost]

@RisingFog I just squashed them before uploading this branch, I had them separate, I've read some stuff and I guess it confused me and I squashed too early. I still have the old branches and backup copies, but you're suggesting I can just split this here into two commits? I didn't know that's possible so I'll go read up on that.

That's said, I have a bit of a question regarding my rename to the ffmpeg-win64 - the include files (or how they're properly called) as in relation to platform specifics, are those really only used for Windows? But they do come from the same windows-specific package as the .lib and .dll

[RisingFog]

@Zexaron They're only required for Windows, Linux/macOS grabs the dependencies on compile from the OS directly (if the dependencies are installed/available).

[ghost]

@RisingFog Just to clear out, If I deleted the Externals\ffmpeg\include folder, would the solution compile successfully for Linux/macOS on the buildbot here, for example? If that's not true then the -win64 suffix may be less optimal, in that case there's a counter argument, which is that the folder came from the win64 zip package and so it would be natural to retain that suffix to notify the source where it came from, even tho the includes might be usable by other OS. (if this theory is true). So it kinda loops back in favor of keeping the suffix.

@RisingFog I've read up on commit splitting, it's not the simplest thing and I don't want to mess this PR up, so may take some time because I should do a push test to another non-pr branch too. I'm aware that more commits is easier for reviewing, but in this case does it also matter when merging? I would then just keep amending changes (not many of them) and do a commit split later once I figure it out better, fine?

[BhaaLseN]

Splitting is fairly easy; open git gui, check "amend last commit" and remove everything that isn't the folder rename - then commit it with a new message. Everything else thats left is the other changes.

[ghost]

@BhaaLseN Thanks for the tip. Unfortunately I've unticked the GUI component when installing the 2.17 version a few months ago. I already plan to include it when I do a reinstall, but probably not a good idea to do that in the middle of this PR, IMO.

[RisingFog]

@Zexaron It would compile on linux/macOS without that folder.

[BhaaLseN]

I wasn't aware you could actually not install git gui. Do you mean you're doing this all from cli with no gui whatsoever? In that case: git reset HEAD~1 and re-git add the files related to the renaming.

[ghost]

Fixed all of these points but commit split would have to wait until tomorrow at earliest, as it would probably stretch deep into the night.

@BhaaLseN It's a bit of a long story, a bit offtopic but in short, in the beginning last year I thought GitForWindows was it's own thing (some unreliable random source) but then official git-scm links to it, but I thought there were two for windows. I used it a bit before, but I settled with Bash, and after a fresh Win10 install 3 months ago I remember unticking it in the installer, since then I wanted to try the GUI again, but I couldn't find it in Program Files. However I forgot to confirm with a search, which I did right now, and I found git-gui.exe, why on earth is it not in the root directory like git bash and cmd. So maybe there's 2 different GUIs after all, in one program. No need to worry, I'll figure it all out tomorrow, too late today.

[shuffle2]

IMO it would be nicer to just have a yes/no click-through which would download and extract the needed files (maybe drop back to the instructions given in the current state if it fails). But that can be done in a different PR (if anyone wants to implement it).

[ghost]

@shuffle2 That was the original plan indeed. The choice is to use this as is, temporairly, or wait until that is done separately and then merge both at a later date?

EDIT: Commit split complete, hopefully this even better.

EDIT2: Wondering how checking/crash prevention is done in non-Windows OS ... Current Diff

UPD3: Diff Link Updated

[ghost]

@RisingFog Those .lib dependencies are still required due to the way I implemented delayed loading support. Simply specifying the DLL filenames under /DELAYLOAD uses the VS helper function which does this under the hood automatically, it's hidden, so you won't see it in the source. Otherwise much more code changes and effort would have been required with manual implementation of the LoadLibrary/GetProcAddress.

[RisingFog]

@Zexaron Alright, I didn't see that the new lib files were added in the diff. Ignore my review then for that.

[ghost]

@RisingFog Oh ok, maybe if you tick the "Approve Changes", might get rid of them.

[JosJuice]

@Zexaron No, reviews don't work that way.

[shuffle2]

Is there a specific reason to use delayloading instead of just LoadLibrary/GetProcAddress?

[ghost]

@shuffle2 AFAIK delayloading just uses the helper function which does the LoadLibrary/GetProcAddress automatically, but the point is to have it delay loaded, not startup loaded, in other words, optional, if it's that easy just adding some DLL filenames to a setting it's well worth it.

[ghost]

I wanted to rebase, just had a BSOD while in interactive rebase, why is my Win10 doing this to me now. Index file got corrupt for the whole Dolphin repo, sorry this is going to take some time and I'm busy with other health stuff. I might have to first fix the BSODs I had a few, it could be HW failing too.

There was no action happening, it was just in the middle of the rebase waiting for manual edits, index shuldn't be that easily corruptible, this is some serious issue if just an untidy shutdown of bash/cmd/mintty would cause this. The index file shows NULNULNUL ... and same thing happened on an untidy shutdown before without any BSODs.

[Techjar]

I've had that happen before as well, my guess is it has something to do with disk write caching. I don't see how just an unclean shutdown of the terminal would corrupt the git index, as it doesn't store any state in the terminal session.

[ghost]

Looks like thing are going a bit better with my PC stability, I've imaged my Win10 off the Samsung 860 EVO 250GB to a similarly fresh 850 EVO 250GB which I had laying around meant to put a fresh install of Win7 on, and put it on another sata port as well as different sata cable, been working for 2 days now, but not so fast, still have to do more checks/test and just giving it some time, including using linux on a third 840 Pro SSD and putting that on the first port and the same cable, if linux has a kernel panic then it has to be the port or the sata cable or something in the controller/motherboard, ruling out the 860 as the culprit.

I know it's not directly relevant to this PR, but what I'm trying to get to is the schedule, if this PR won't get merged temporairly before the auto-ffmpeg-downloader PR is done (any volunteers?), then there's no point for me to rush here.

[RisingFog]

Is there any update on this PR?

[ghost]

@RisingFog I'm still around, althought unfortunately still dealing with the PC hardware troubleshooting and maintenance, various side-chores have opened up, had to be done first, other PCs got problems, I've had 5 hardware failures in less than 6 months, 3 warranties, after no issues for like 4 years, it's just bad coincidence, and I simply can't get to fixing the dolphin git repo until I get my PC back - I'm closer, I think one of the newer SSDs is randomly locking up after less than 2 months of use, since it's happeing on other SATA ports as well, now I have to test it on another PC, but the nature of the problem is that it takes time, each configuration case I have to play around a bit and wait, so iteration is slow, but also becuase I have a lot of life stuff going on with the home improvement stuff, new appliances etc.

It's still up in the air for this PR, if there's an update, it would come in another PR, the updater/downloader for ffmpeg. But ofcourse I will resolve the conflicts in this PR ASAP.
I wouldn't want to mess with this one adding the updater here.

Plus, I was also going to update git and maybe learn/familiarize with one of the GUIs, so there's all of that as well.

[delroth]

I can't think of a situation where you'd like ffmpeg delay-loading elsewhere than on Windows. On Linux it's not like people randomly go and install shared libraries but don't expect to have to recompile to use them. And there's the whole mess of ABI compatibility if you start wanting to make something generic... which I don't think is worth trying to solve.

[stenzek]

This is the situation I was thinking of - make libavcodec an optional dependency, which could be added later without recompiling. But indeed, header versions/ABI differences could make it an issue.

That said, I'm still not sold on the linker-based method, due to the points above, but I'm not gonna bikeshed too much about it :P

[ghost]

And I thought being windows-specific was a good thing, but it's not the delayed-loading that makes it platform specific right, it's the libaries them selfs.

The reasons for delayed-loading is for either or both of the two main issues I saw pretty early on, avoiding shipping ffmpeg with every release of dolphin and avoiding forcing all users to have it installed even if they're not going to use any functionality that requires FFmpeg.

Now if everyone doesn't care about the shipping size or the hosting space, there would be no need for delayed-loading nor user installation ... otherwise this PR doesn't add anything and actually relieves around 1.5 MB, with average 4 releases per day, 2 TB of saved space in a year.

[ghost]

The wiki page has been set and it's under construction, in the meantime I added a temporary installation text from the original warning message.

https://wiki.dolphin-emu.org/index.php?title=FFmpeg

[ghost]

Warning for localization: trailing spaces here on purpose for a quick button width fix so that the border doesn't touch the text, otherwise I think a lot more code would be needed to adjust the width, unless if there's a shorter way that someone knows how to adjust the width without having to define the whole dialog element-by-element.

9f2adb2#diff-8fa06c6cfb14789472b0c27be3da88b0R986

[JosJuice]

If you want to warn localizers, make an i18n comment in the code.

[ghost]

Done, althought it's a bit awkward positioning, would the ^^ trick work out ?

13b6e92#diff-8fa06c6cfb14789472b0c27be3da88b0R987

[JosJuice]

The i18n comment has to be right above the string, not below. And using arrows won't make sense to localizers.

You should also make sure the comment refers to both the leading and trailing spaces, not just the trailing one.

[ghost]

Corrections applied.

[JosJuice]

No, the i18n comment has to be on the line right above the string, not 7 lines before.

[Rukario]

Would it be alright if the "Copy to URL to clipboard" button is to be changed into just "Open Wiki" and it will open the URL in user-defined default browser, much like the "Wiki" button you see in context menu from game list? If you like this idea, may I also suggest drop the blue text, the button got it covered.

[ghost]

I'd hope to have at least one redundancy, but whatever. Install instruction is a bit more critical if the user can't get to them IMO. We'll see what people think, or they may just like it how it is, but as far as me goes I'm fine with this as well.

The i18n line should be fixed now, I thought that wouldn't build. The wiki's also beginning to be shaped, but the primary thing, the install instructions, are more or less good for practical use as of now.

[ghost]

Many linux distros are using ffmpeg 4.0.2 for their testing/unstable build - ffmpeg is updated here as well to match, includes, libs, and wiki.

@Rukario There that should make everyone happy now !!!

It's was actually a good idea, it got rid of the hack I had to make to widen the button.

[ghost]

@spycrab Okay, I put it in UICommon.cpp as it made most sense atm IMO.

[vadosnaprimer]

Won't piping over to whatever version of ffmpeg is available on the system make it easier than linking a shared lib? That would allow to just use whatever commands are supported by the given ffmpeg build, nothing would have to be explicitly loaded, no internal dependencies/symbols to resolve (I hope I'm understanding this correctly).

[JosJuice]

The Zexaron account doesn't exist anymore, meaning this PR can't be updated, so...

[vadosnaprimer]

Could you still comment on my suggestion?

[JosJuice]

It's not really the area of Dolphin I focus on, but... I guess it has the disadvantage of making things harder to set up for people who just want a quick and easy dump (not TASVideos.org quality) but doesn't really have disadvantages other than that? The disadvantage doesn't sound too bad to me considering that only a small subset of users use frame dumping, but I can't speak for what the other devs think.

[vadosnaprimer]

If ffmpeg is not found, an error message with a link makes it rather trivial imo. On the other hand, having it in PATH permanently means you only download it once and all dolphin installations and portables can use it. It's also nice to have a couple default ffmpeg commands and a way to change them.

[ghost]

Intent to revive this on my own, anyone objecting?