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-headless: uplift #354952

Merged
merged 8 commits into from
Nov 23, 2024
Merged

ffmpeg-headless: uplift #354952

merged 8 commits into from
Nov 23, 2024

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Nov 10, 2024

In #354936 we got a nice overview from the upstream jellyfin-ffmpeg maintainer over features that actually matter for real-world use-cases. This enables most of them in ffmpeg-headless by default.

Total diff:

/nix/store/m5ghr321rsxpfl001c5fdh56y9f42lpd-ffmpeg-headless-7.1-bin	 233072648
/nix/store/d0dl773klln23daxy1f24xcpbgch184a-ffmpeg-headless-7.1-bin	 296402944

Looking at a more realistic diff with something else in the closure however:

nix-build --expr 'with import ./. { }; buildEnv { name = "foo"; paths = [ ffmpeg-headless pipewire ]; }'

/nix/store/0qbvh2bkjswbxhf2n1c75ddjj8dv9446-foo	 656619816
/nix/store/pg0jhvijgnxs3hbvjk4nr7c1cbsni9fy-foo	 663084912

I think <9MiB is an acceptable increase.

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.

/nix/store/m5ghr321rsxpfl001c5fdh56y9f42lpd-ffmpeg-headless-7.1-bin	 233072648
/nix/store/rjimh83l9pdpmd7qvanvfhkwr13cmbz5-ffmpeg-headless-7.1-bin	 234001624
/nix/store/rjimh83l9pdpmd7qvanvfhkwr13cmbz5-ffmpeg-headless-7.1-bin	 234001624
/nix/store/pk5bgvay9yb1hl6pcda2fs9d33s7gws0-ffmpeg-headless-7.1-bin	 234002056
/nix/store/pk5bgvay9yb1hl6pcda2fs9d33s7gws0-ffmpeg-headless-7.1-bin	 234002056
/nix/store/4gmnzizak5ca1skhf9r9m2m0jcbik8ap-ffmpeg-headless-7.1-bin	 236116824
/nix/store/4gmnzizak5ca1skhf9r9m2m0jcbik8ap-ffmpeg-headless-7.1-bin	 236116824
/nix/store/nqardac1g5h774bprz145fsl4qm2p92l-ffmpeg-headless-7.1-bin	 236173272
/nix/store/nqardac1g5h774bprz145fsl4qm2p92l-ffmpeg-headless-7.1-bin	 236173272
/nix/store/a9a7n7w0dhcjay5vi41xy685m6md1hkw-ffmpeg-headless-7.1-bin	 294694384

This may look like quite an increase but if you combine it with a noteworthy package such as pipewire, the increase is only ~5MiB:

/nix/store/cs43xb7x8bqy0igaip0vrsgjij5aafc9-foo	 657507664
/nix/store/xcgc1k9mn8f9fqns9832dnnlhnxm6dka-foo	 661376384

With actual closures, I believe this would be even less.
/nix/store/a9a7n7w0dhcjay5vi41xy685m6md1hkw-ffmpeg-headless-7.1-bin	 294694384
/nix/store/2wxab02vqydrmm19m4wiffz1ic3v7528-ffmpeg-headless-7.1-bin	 295755456
/nix/store/2wxab02vqydrmm19m4wiffz1ic3v7528-ffmpeg-headless-7.1-bin	 295755456
/nix/store/0km9wmmxs4aasna3jvx6ksigbi5qlas8-ffmpeg-headless-7.1-bin	 296508848
@Atemu Atemu requested review from emilazy and jopejoe1 November 10, 2024 09:34
@Atemu Atemu marked this pull request as draft November 10, 2024 09:36
@Atemu
Copy link
Member Author

Atemu commented Nov 10, 2024

(Drafting because this should only land after breaking features on staging are unblocked.

@Atemu Atemu added the 2.status: wait for branch‐off Waiting for the next Nixpkgs branch‐off label Nov 10, 2024
@Atemu Atemu mentioned this pull request Nov 10, 2024
13 tasks
@emilazy
Copy link
Member

emilazy commented Nov 10, 2024

Wait, how is this breaking? Although staging has branched off already, so no need to wait really.

@Atemu Atemu marked this pull request as ready for review November 10, 2024 17:23
@Atemu
Copy link
Member Author

Atemu commented Nov 10, 2024

That's a good point, not sure what I was thinking.

This pulls in clang as a build input but as it turns out this actually _reduces_
closure size slightly:

/nix/store/0km9wmmxs4aasna3jvx6ksigbi5qlas8-ffmpeg-headless-7.1-bin	 296508848
/nix/store/d0dl773klln23daxy1f24xcpbgch184a-ffmpeg-headless-7.1-bin	 296402944

Fixes NixOS#344114
@Atemu
Copy link
Member Author

Atemu commented Nov 22, 2024

Added cuda-llvm which surprisingly decreases closure size.

I'd like to move ahead with this one now. Any objections to the 60.3965 MiB stand-alone and 6.1656 MiB composite closure size increase?

Copy link
Member

@jopejoe1 jopejoe1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 22, 2024
@emilazy
Copy link
Member

emilazy commented Nov 22, 2024

No closure size worries from me. The only thing here that doesn’t seem like obviously a good idea is OpenMPT; Arch and Fedora don’t enable it and it’s unclear to me what their security track record is like. I’ll defer to your judgement on that one, but maybe we could wait and see if anyone really wants it? (I have to wonder how many people use Jellyfin to play tracker modules.)

@ofborg ofborg bot requested review from Bot-wxt1221 and jopejoe1 November 23, 2024 03:09
@wegank wegank added the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Nov 23, 2024
@Atemu
Copy link
Member Author

Atemu commented Nov 23, 2024

I'm not entirely sure what it's useful for because it seems that's for stuff from before I was born but if it's important enough for jellyfin to activate, it probably is for us too. The Wikipedia page also says that it's expected of media players to support this format, so I think it would be the least surprising behaviour if e.g. mpv (using ffmpeg/libav) supported it.

@Atemu Atemu merged commit 4ed371b into NixOS:staging Nov 23, 2024
40 of 41 checks passed
@Atemu Atemu deleted the ffmpeg-headless-uplift branch November 23, 2024 12:34
@emilazy
Copy link
Member

emilazy commented Nov 23, 2024

The official mpv binaries don’t support it, and Debian and Fedora don’t ship support, so I’m not sure how widespread that expectation is. They’re quite elaborate sequencer instruction files rather than “audio” files per se; I expect that tracker enthusiasts mostly used specialized players for the task. I worry about exploits like this when turning on obscure formats by default.

Looks like they put out a lot of security updates, but at least we seem to be keeping on top of them.

Atemu added a commit to Atemu/nixpkgs that referenced this pull request Nov 24, 2024
With NixOS#354952, the regular ffmpeg-headless
should now have all the deps required for frigate to make use of hardware
acceleration on all hardware.

This is progress towards NixOS#271863

This reverts commit 7e33e47.

This may be surprising as it's quite soon after
7e33e47 was merged but we decided in
NixOS#357717 (comment) to merge it
quickly to unbreak master and backport to 24.11. Non-full ffmpeg can be pursued
in master using a simple revert which is where we are now.
@alyssais
Copy link
Member

This broke building ffmpeg for musl. Fix is #360234.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants