Skip to content
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_{4,6,7}: Darwin clean‐ups and improvements #351188

Merged
merged 4 commits into from
Oct 26, 2024

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Oct 25, 2024

This brings the functionality of our mpv package on macOS more in line with their official CI binaries while cleaning up a bunch of cruft that is no longer required after #346043. See the commit messages for details.

The flag removals are technically breaking changes past the freeze, although I highly doubt anyone was ever toggling them. If preferred, we could instead leave the flags around but warn about their impending removal when they’re set, and ditch them after branch‐off. I’d definitely rather not keep around pointless feature flags that don’t incur any additional dependencies in 25.05, though.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

All of these are supplied in the standard environment as part of the
SDK now, so there’s no closure difference at build or runtime and
no reason to turn them off.
Upstream is pretty good about using availability checks, and there
are runtime‐checked features for macOS 10.13, 11, and 12 that
are only available if we build with a modern SDK. This impacts,
e.g. hardware‐accelerated video decoding in mpv.

FFmpeg should still continue to build and run on all our supported
macOS releases, with runtime functionality being no worse than before
on older versions.
@emilazy
Copy link
Member Author

emilazy commented Oct 25, 2024

Actually apparently we’re going based on anywhere‐on‐Earth time so the removals are totally okay.

@emilazy emilazy requested review from Atemu and jopejoe1 October 25, 2024 14:47
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Oct 25, 2024
@emilazy emilazy changed the base branch from staging-next to staging October 25, 2024 16:44
@emilazy
Copy link
Member Author

emilazy commented Oct 25, 2024

Retargeted to staging as this will of course cause a bunch of rebuilds.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Diff LGTM. If you say this works the same on Darwin, that sounds good to me.

@emilazy
Copy link
Member Author

emilazy commented Oct 26, 2024

It works better, at least for hardware‐accelerated decoding on new OS versions :)

It should behave the same way on old macOS releases but it’s always possible that upstream may have missed some availability checks. That would be a bug we can fix (and upstream the fix for) if it arises, but should be much less relevant once we bump the minimum supported version for 25.05 anyway.

@Atemu
Copy link
Member

Atemu commented Oct 26, 2024

Speaking of which, we use SDK 15 here, wouldn't that already exclude macOS < 15 from running the binaries?

@emilazy
Copy link
Member Author

emilazy commented Oct 26, 2024

No; the SDK controls what functionality you can access, and the minimum version (“deployment target”) controls what functionality is assumed to be available. With the new SDK pattern, apple-sdk_* determines the former and darwinMinVersionHook determines the latter. If you try to use APIs that are not present in your minimum version, you get a warning in C‐family languages and an error in Swift. Some packages are badly‐written and use autoconf‐style static checks to determine API availability, so we default to the SDK corresponding to our minimum supported version by default to reduce the likelihood of programs failing to start on old OS versions, but software that is correctly written for macOS will do conditional runtime availability checks for features they use that are not present in the minimum OS version they support.

In the case of FFmpeg, they even have checks for versions below our current x86_64-darwin baseline of 10.12:

emily@yuyuko ~/D/ffmpeg ((66d61918))> rg '@available|__builtin_available'
libavfilter/vf_yadif_videotoolbox.m
199:    if (@available(macOS 10.11, iOS 8.0, *)) {
279:    if (@available(macOS 10.11, iOS 8.0, *)) {
315:    if (@available(macOS 10.11, iOS 8.0, *)) {
383:    if (@available(macOS 10.11, iOS 8.0, *)) {

libavcodec/videotoolboxenc.c
1207:    if (__builtin_available(macOS 10.13, *)) {
1629:        if (__builtin_available(macOS 10.10, *)) {

libavfilter/metal/utils.m
33:    if (@available(macOS 10.15, iOS 11, tvOS 14.5, *)) {

libavutil/hwcontext_videotoolbox.c
442:        if (__builtin_available(macOS 10.11, iOS 9, *))
455:        if (__builtin_available(macOS 10.13, iOS 11, tvOS 11, watchOS 4, *))
468:        if (__builtin_available(macOS 10.11, iOS 9, *))
480:        if (__builtin_available(macOS 10.13, iOS 11, tvOS 11, watchOS 4, *))
494:        if (__builtin_available(macOS 10.13, iOS 11, *))
501:        if (__builtin_available(macOS 10.11, iOS 9, *))
511:        if (__builtin_available(macOS 10.12, iOS 10, *))
517:        if (__builtin_available(macOS 10.13, iOS 11, *))
527:        if (__builtin_available(macOS 10.13, iOS 11, tvOS 11, watchOS 4, *))
548:        if (__builtin_available(macOS 12.0, iOS 15.0, tvOS 15.0, *))
624:    if (__builtin_available(macOS 10.8, iOS 10, *)) {

libavcodec/videotoolbox.c
953:#if defined(MAC_OS_X_VERSION_10_9) && !TARGET_OS_IPHONE && (MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_9) && AV_HAS_BUILTIN(__builtin_available)
955:        if (__builtin_available(macOS 10.9, *)) {
961:#if defined(MAC_OS_VERSION_11_0) && !TARGET_OS_IPHONE && (MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_11_0) && AV_HAS_BUILTIN(__builtin_available)
962:    if (__builtin_available(macOS 11.0, *)) {

However, you did remind me that despite supporting deploying to old versions, FFmpeg doesn’t properly build with the ancient 10.12 SDK we currently default to on x86_64-darwin; they use a typedef that isn’t present and that we previously had to patch around. This is quite a common case; there is a lot of software like MoltenVK that supports quite old OS releases but expects a modern SDK to build with. I’ve now removed that patch, further reducing the Darwin‐related cruft 🎉

@emilazy emilazy merged commit a162f7e into NixOS:staging Oct 26, 2024
10 of 11 checks passed
@emilazy emilazy deleted the push-vsokvwstkvrv branch October 26, 2024 13:31
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/title-the-darwin-sdks-have-been-updated/55295/1

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

Successfully merging this pull request may close these issues.

3 participants