-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
mpv: clean‐ups and Darwin improvements #350674
Conversation
Oh, this is satisfying. Swift already propagates its corresponding SDK, so we don’t need the patch.
mpv currently doesn’t even build successfully on macOS without Swift, and there’s no use of Swift outside of the platform.
Using `--vo=gpu` or `--vo=gpu-next` with MoltenVK is now supported and has replaced the previous `cocoa-cb` as the default.
This is now detected correctly as part of the SDK, as it should be.
Does nothing, removed upstream in 2012(!): <mpv-player/mpv@1fde09d>.
Does nothing, removed upstream in 2012(!): <mpv-player/mpv@1fde09d>.
Does nothing, removed upstream in 2012(!): <mpv-player/mpv@6a26b4a>.
This is required as part of X11 support since the Meson transition.
Removed upstream: <mpv-player/mpv@200992f>.
It builds fine on macOS.
Upstream’s Meson defaults, upstream’s CI binaries, and Arch don’t enable this.
This prints out a big warning about how you shouldn’t be using it. Upstream CI builds and Arch don’t enable it. There are other backends of questionable worth that we don’t expose options for, so let’s ditch this one.
No reason for FreeBSD to miss out on the fun.
423c415
to
888c460
Compare
Since we’re doing auto‐detection anyway, this is unnecessary.
On macOS, many things won’t work properly unless mpv is executed from the app bundle, such as spatial audio with `--ao=avfoundation`. This ensures that those features work reliably and also avoids duplicating the entire `mpv` binary.
888c460
to
12ce8fd
Compare
@AndersonTorres @fpletz would be great to get this reviewed before the freeze tonight if you have the time ^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides, lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the cleanups. Much appreciated! ❤️
@@ -151,18 +111,13 @@ stdenv'.mkDerivation (finalAttrs: { | |||
hash = "sha256-BOGh+QBTO7hrHohh+RqjSF8eHQH8jVBPjG/k4eyFaaM="; | |||
}; | |||
|
|||
patches = [ | |||
# Fix build with Darwin SDK 11 | |||
./0001-fix-darwin-build.patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this file removed from Nixpkgs repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intended to, but it’s possible it got lost in rebasing. Will open a PR soon if nobody beats me to it (currently racing against time for the 24.11 freeze).
This ports mpv over to the new SDK format from #346043 (so much less cruft! yay!), enables the new upstream default of Vulkan on Darwin, and cleans up a bunch of flags and dependencies, including removing flags that had no effect, broke the build to toggle, or are just obsolete, and adjusting some defaults to be less Linux‐specific or more inline with upstream and other distros.
There are still things to investigate with the macOS build (e.g. VideoToolbox seems to support fewer pixel output formats compared to their official CI binaries), but I wanted to get this batch with the theoretically‐breaking changes posted before the freeze.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.