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

Add fdk-aac-free #61

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Add fdk-aac-free #61

merged 1 commit into from
Apr 26, 2022

Conversation

knackebrot
Copy link
Contributor

Fedora maintains a fork of libfdk-aac stripped of all AAC modes except for MPEG-2 AAC-LC, patents of which should now be expired. Their legal team greenlighted it, so it should be fine to redistribute such binaries. I'd like this encoder to replace the default LAME encoder that is in Jellyfin, because:

  1. AAC is far more common on the Internet (less potential browser bugs)
  2. better compression
  3. Don't know if this is also in the mpegts muxer, but mp3 with variable bitrate couldn't seek accurately on some platforms. For Jellyfin, we could simply use fdk-aac VBR preset 5 or 4 instead of a fixed bitrate and save some bandwidth without sacrificing quality.

If this gets merged I'm planning to create a PR to the main Jellyfin repository to swap LAME for FDK-AAC.

Related links:
https://bugzilla.redhat.com/show_bug.cgi?id=1501522
https://frederickding.gitlab.io/HandBrake/

@nyanmisaka
Copy link
Member

nyanmisaka commented Feb 7, 2021

The default audio encoder in JF 10.7 has been changed to AAC encoder.
jellyfin/jellyfin-web#2026

We also prefer to libfdk_aac encoder if it's available in a custom ffmpeg build.
However, no additional adjustments were made to its options.
jellyfin/jellyfin#2715

Can we remove --enable-nonfree before adding that greenlighted FDK AAC library?

BTW since we do not force users to use jellyfin-ffmpeg, there is a high probability that there is no libfdk_aac encoder in a custom redistributable ffmpeg.

@nyanmisaka
Copy link
Member

Please use debian patch to modify ffmpeg sources.
60c4241

@knackebrot
Copy link
Contributor Author

knackebrot commented Feb 7, 2021

I've just found out that fdk-aac might not be worth it, at least at the bitrates we use. On my x86_64 server, it encodes at ~ 15x vs LAME's 93x. That's a 6x slowdown for audio that is indistinguishable, and at a similar bitrate. On my aarch64 phone it's 10x vs 55x. On devices with a weak FPU the difference might be slimmer, though. Still, LAME doesn't support surround sound, so here we must use aac.

@joshuaboniface
Copy link
Member

Do we still want this @nyanmisaka @knackebrot ?

@knackebrot
Copy link
Contributor Author

knackebrot commented Feb 20, 2022

I'd like to have this although it's noticeably slower (maybe due to its reliance on integer math). It won't be an issue for most users, only those with lots of concurrent encodes, and I still stand behind my opinion that FFmpeg AAC is not very good.

@knackebrot
Copy link
Contributor Author

Hi @nyanmisaka @joshuaboniface
There were some changes in the upstream repository and now the abysmal performance i've been seeing is gone and it performs similarly to ffmpeg's aac encoder:

ffmpeg -i "02 - Gouche.flac" -vn -c:a libfdk_aac -b:a 256k gouche.m4a

size=    8181kB time=00:04:20.27 bitrate= 257.5kbits/s speed=74.7x
ffmpeg -i "02 - Gouche.flac" -vn -c:a aac -b:a 256k gouche-ff.m4a

size=    8192kB time=00:04:20.27 bitrate= 257.9kbits/s speed=76.5x

I've built and tested it on win64 and ubuntu focal and would like to see it merged.

Copy link
Member

@nyanmisaka nyanmisaka left a comment

Choose a reason for hiding this comment

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

LGTM. I want to do some testing before merging.

debian/patches/0035-remove-fdk-aac-from-nonfree.patch Outdated Show resolved Hide resolved
@nyanmisaka
Copy link
Member

IIRC in transcoding profile, aac encoder is preferred in jellyfin-web but mp3lame is preferred in other client such as android.

fdk_aac is preferred to aac in the server side too if it exists.

@nyanmisaka nyanmisaka merged commit 0dc42ed into jellyfin:jellyfin Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants