-
Notifications
You must be signed in to change notification settings - Fork 420
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
FFmpeg: Update FFmpeg.AutoGen to 7.0.0 #6256
base: master
Are you sure you want to change the base?
Conversation
👍 macOS (x64):
👍 macOS (Rosetta):
👍 macOS (arm64):
Looks about as expected. Looks like |
👍 Windows (x64):
|
No hardware decoders load for me on Linux (either master or PR) but that's likely a configuration issue my side (or ffmpeg compile things™) |
Let's not.
Going by my above testing it seems to be fine as is. Haven't tested VM but that's a pretty esoteric use case that we'll cross if it ever comes up. None of our testing considers Rosetta right now, so I'd say it's also irrelevant (even though I tested it).
It may be safer, but I also don't expect users to be using FFmpeg directly themselves. |
Is this ready to be merged? You mention it needs a bit of work, but it seems to be working alright. |
This is expected (see #6025).
That's good, but I have a feeling this will break video playback on mobile platforms (those still bundle old, pre-5.0 FFmpeg libraries which might cause binary incompatibilities). Otherwise, I feel pretty good about the state of the PR. I'll probably do that one thing with the |
This should be fine now. I thought the mobile platforms might need some changes on the .NET side, but it looks like they'll work just fine as long as the native libraries follow the same conventions. |
👍 iOS (arm64):
👍 iOS simulator (x86_64):
(hardware acceleration never "works" on iOS simulator so ignoring) |
I just noticed that you removed the old iOS libraries from this branch, @frenzibyte. I would've preferred doing that in the auto-generated PR that will be created after #6255 is merged and new binaries are built (example of this PR: #6227). Android has old libraries too that should be removed, and I would like to keep it separated. Do you mind if we remove that change from this PR and include it in the binaries update? |
Sure, I pushed it here because I found there's no reason not to, as this PR is already meant to be merged after the FFmpeg libraries update. |
Fair enough. It's just my preference to keep it separated, so that merging only one PR would not result in having double or no library files. Thanks |
0434d6f
to
988c7fc
Compare
Closes #5051. There is still a VP9 bug (discussed in that issue), but there hasn't been any movement upstream so I assume this is not going to be fixed. I have added a rudimentary (untested) workaround for that issue in this PR.
Prerequisite:
FFmpeg.AutoGen had a large API change somewhere around 5.0 or 5.1.
The gist is that
ffmpeg.GetOrLoadLibrary
was removed in favour of theIFunctionResolver
API. The default resolvers are not usable in o!f1, so I added a simpleFFmpegFunctionResolver
class.Note:
OSArchitecture
check is a bit awkward, but seems to work as expectedFeel free to start testing / giving feedback, but this still needs a bit of work, and a lot of testing.
Testing:
Footnotes
The built-in
LibraryDependenciesMap
is wrong for the custom libraries used in o!f. It also requires fiddling with theRootPath
property and calls system APis (dlopen etc.) directly. UsingNativeLibrary
instead should be more ergonomic. ↩