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.

Sign up for GitHub

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

Jump to bottom

ffmpeg: add avs2/3 options #337010

Open
wants to merge 5 commits into
base: staging
Choose a base branch
from
Open

ffmpeg: add avs2/3 options #337010

wants to merge 5 commits into from
+195 −0

Conversation

jopejoe1
Copy link
Member

@jopejoe1 jopejoe1 commented Aug 24, 2024 •
edited
Loading

Description of changes

Adds support for de-/encoding of avs2 and decoding of avs3.

Can be tested with the following commands.

ffmpeg -f lavfi -i testsrc -t 10 -c:v avs2 /tmp/avs2.mkv
ffmpeg -i /tmp/avs2.mkv -c:v hevc /tmp/decoded-avs2.mkv
ffmpeg -i /tmp/avs3-sample.mkv -c:v hevc /tmp/decoded-avs3.mkv

Samples can be found at https://github.com/xatabhk/avs2-avs3-video-samples

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.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Aug 24, 2024
@ofborg ofborg bot requested review from justinas, emilazy and Atemu August 24, 2024 14:12
@jopejoe1 jopejoe1 changed the title ffmpeg: add avs options ffmpeg: add avs2/3 options Sep 1, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024
@jopejoe1 jopejoe1 removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 10, 2024
FliegendeWurst
FliegendeWurst requested changes Oct 27, 2024
View reviewed changes
pkgs/by-name/xa/xavs2/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/xa/xavs2/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/da/davs2/package.nix Outdated Show resolved Hide resolved
Atemu
Atemu reviewed Oct 27, 2024
View reviewed changes
Copy link
Member

@Atemu Atemu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Forgot to submit this review... ^^'

pkgs/by-name/da/davs2/package.nix Show resolved Hide resolved
pkgs/by-name/da/davs2/package.nix Show resolved Hide resolved
pkgs/by-name/da/davs2/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/xa/xavs2/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/xa/xavs2/package.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from Atemu October 30, 2024 09:11
Atemu
Atemu reviewed Oct 30, 2024
View reviewed changes
pkgs/by-name/xa/xavs2/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/da/davs2/package.nix Outdated Show resolved Hide resolved
emilazy
emilazy reviewed Oct 30, 2024
View reviewed changes
pkgs/by-name/xa/xavs2/package.nix
Comment on lines +40 to +50
postInstall = lib.optionalString (!stdenv.hostPlatform.isStatic) ''
rm $lib/lib/*.a
'';
Copy link
Member

Choose a reason for hiding this comment

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

Do we normally delete static libraries entirely like this? I thought isStatic was about whether executables were linked statically.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it was about both static libraries and executables.

Atemu
Atemu approved these changes Oct 30, 2024
View reviewed changes
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 and I think everything past this point is isn't effort well spent.

pkgs/by-name/da/davs2/package.nix Show resolved Hide resolved
pkgs/by-name/xa/xavs2/package.nix Show resolved Hide resolved
@ofborg ofborg bot requested review from Atemu and emilazy October 31, 2024 03:33
@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Oct 31, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@jopejoe1 jopejoe1 removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 14, 2024
@ofborg ofborg bot requested a review from Bot-wxt1221 November 15, 2024 08:30
@Atemu Atemu removed their request for review November 15, 2024 21:52
@wegank wegank removed 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 labels Nov 15, 2024
@ofborg ofborg bot requested a review from Atemu November 16, 2024 09:57
@Atemu Atemu removed their request for review November 16, 2024 09:58
Scrumplex
Scrumplex reviewed Nov 20, 2024
View reviewed changes
pkgs/by-name/da/davs2/package.nix
cd build/linux

# When setting this over `env.AS` it gets ignored
export AS=${lib.getExe buildPackages.nasm}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add nasm to nativeBuildInputs instead, and then unset AS here. This worked in my local testing.

Note that nasm will probably only work for x86/x86_64 systems, so perhaps guard this behind stdenv.hostPlatform.isx86

The reason why env.AS didn't work, is that stdenv sets its own value during build/

Copy link
Member

Choose a reason for hiding this comment

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

Prior art:

nixpkgs/pkgs/by-name/x2/x264/package.nix

Lines 34 to 36 in f17c1d5

preConfigure = lib.optionalString stdenv.hostPlatform.isx86 ''
# `AS' is set to the binutils assembler, but we need nasm
unset AS

pkgs/by-name/xa/xavs2/package.nix
Comment on lines +37 to +38
# When setting this over `env.AS` it gets ignored
export AS=${lib.getExe buildPackages.nasm}
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@FliegendeWurst FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Dec 4, 2024
@SigmaSquadron SigmaSquadron removed the awaiting_changes (old Marvin label, do not use) label Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Atemu Atemu Atemu approved these changes

@FliegendeWurst FliegendeWurst FliegendeWurst requested changes

@Scrumplex Scrumplex Scrumplex left review comments

@justinas justinas Awaiting requested review from justinas

@emilazy emilazy Awaiting requested review from emilazy

@Bot-wxt1221 Bot-wxt1221 Awaiting requested review from Bot-wxt1221

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jopejoe1 @Scrumplex @FliegendeWurst @emilazy @Atemu @wegank @SigmaSquadron