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

[fdk-aac] make patent-encumbered HE-AAC optional #16007

Merged
merged 2 commits into from
Aug 12, 2021

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Feb 2, 2021

By default, use a fork of fdk-aac (which was originally written
by Fraunhofer for the Android Open Source Project) which has
support for the patent-encumbered HE-AAC, HE-AACv2, and xHE-AAC
profiles removed. This fork is used by Fedora and Arch Linux to
provide support for most AAC use cases without patent licensing
fees and to permit combining it with GPL software. The upstream
fdk-aac with support for all AAC profiles can be built with the
new 'he-aac' option for this port.

Fedora Legal's opinion, from
https://bugzilla.redhat.com/show_bug.cgi?id=1501522#c112 :
The Fedora Project is aware that the Free Software Foundation
has stated that the Fraunhofer FDK AAC license is GPL
incompatible, specifically, because of Clause 3.

We believe that the fdk-aac software codec implementation that we
wish to include in Fedora is no longer encumbered by AAC patents.
This fact means that Clause 3 in the FDK AAC license is a "no op",
or to put it plainly, if no patents are in play, there are no
patent licenses to disclaim. For this (and only this) specific
implementation of fdk-aac, we believe that the FDK AAC license is
GPL compatible.

@Be-ing Be-ing force-pushed the fdk_aac_heaac_feature branch 2 times, most recently from 9e0de16 to 24700e9 Compare February 2, 2021 22:15
@Be-ing Be-ing force-pushed the fdk_aac_heaac_feature branch 3 times, most recently from 0cec233 to 1ca5423 Compare February 3, 2021 02:55
@NancyLi1013 NancyLi1013 added the category:port-update The issue is with a library, which is requesting update new revision label Feb 3, 2021
@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Feb 3, 2021
@Be-ing Be-ing marked this pull request as draft February 3, 2021 18:16
@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 3, 2021

I have opened a merge request to add CMake support to the fork without HE-AAC. This allowed removing the modified CMakeLists.txt and related files from this port. Also, I merged updates from upstream into the fork without HE-AAC, so now this port will build the same code with or without the he-aac option, except of course for the HE-AAC support.

@Be-ing Be-ing force-pushed the fdk_aac_heaac_feature branch from 1ca5423 to 3a73461 Compare February 3, 2021 18:40
@Be-ing Be-ing marked this pull request as ready for review February 3, 2021 18:49
@Be-ing Be-ing marked this pull request as draft February 3, 2021 18:54
@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 3, 2021

I have asked upstream to tag a new release. Let's wait a bit for them to respond before merging this.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 3, 2021

@Sibras I am wondering if this PR might present issues building ffmpeg with the fdk-aac option. Should that build fdk-aac with the new he-aac option introduced in this PR?

@NancyLi1013 NancyLi1013 added depends:upstream-changes Waiting on a change to the upstream project and removed info:reviewed Pull Request changes follow basic guidelines labels Feb 4, 2021
@Sibras
Copy link
Contributor

Sibras commented Feb 4, 2021

@Sibras I am wondering if this PR might present issues building ffmpeg with the fdk-aac option. Should that build fdk-aac with the new he-aac option introduced in this PR?

Yes this may cause some issues. Since this PR changes the default behavior of fdk perhaps make the he-aac option default to on (so optionally disable) to keep that libs functionality consistent.

Also the ffmpeg project doesn't recognize the removal of HE-AAC parts as making it GPL compatible so no matter if its included or not ffmpeg with fdk still requires the non-redistributable license.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 4, 2021

ffmpeg is the only port in vcpkg that depends on fdk-aac, so I think it would be sufficient to change the ffmpeg port to depend on fdk-aac[he-aac] instead of just fdk-aac for ffmpeg's fdk-aac option.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 4, 2021

perhaps make the he-aac option default to on

Let's default to not putting users in a questionable legal situation.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 4, 2021

From https://bugzilla.redhat.com/show_bug.cgi?id=1501522#c29 :

Christian, supporting HE-AAC encoding is the raison d'être of fdk-aac. Otherwise FFmpeg's aac codec is good enough for all other use cases. We already have gstreamer1-libav in RPMFusion covering these, so having a crippled fdk-aac in Fedora is no gain. See https://trac.ffmpeg.org/wiki/Encode/AAC for more information. Unless we're looking at several years until unstripped fdk-aac can be included in Fedora, I think it's not worth the effort to add support for this stripped version to FFmpeg and other multimedia software and we should just wait.

So there wouldn't be much of a point to building ffmpeg with fdk-aac using the fdk-aac fork without HE-AAC. That would be redundant with the AAC codec FFMPEG already has. In Mixxx we have already implemented encoding using fdk-aac, so we'll be using the fork without HE-AAC in our Windows and macOS packages. (If we were starting code for AAC encoding now, I'd suggest using FFMPEG instead, but nevertheless we currently have fdk-aac support implemented.)

@Be-ing Be-ing force-pushed the fdk_aac_heaac_feature branch from 3a73461 to fe9b02a Compare August 1, 2021 17:34
@Be-ing Be-ing marked this pull request as ready for review August 1, 2021 17:34
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 1, 2021

Rebased on #18549.

@Be-ing Be-ing force-pushed the fdk_aac_heaac_feature branch from fe9b02a to c6ced68 Compare August 1, 2021 17:36
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 261c458af6e3eed5d099144aff95d2b5035f656b -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/f-/fdk-aac.json b/versions/f-/fdk-aac.json
index d7aee95..941d525 100644
--- a/versions/f-/fdk-aac.json
+++ b/versions/f-/fdk-aac.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "b217056128a666ddab48cf0bd9d78098987aaa39",
+      "git-tree": "7c806b3497f78ee26a48f9aa1716afd3198e2021",
       "version-semver": "2.0.2",
       "port-version": 1
     },

ports/fdk-aac/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

Have you tested the feature he-aac on your local? @Be-ing

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 3, 2021

Yes, this port works both with and without the he-aac feature. And the ffmpeg port works with its fdk-aac feature.

@Be-ing Be-ing force-pushed the fdk_aac_heaac_feature branch from e251612 to 22488e8 Compare August 3, 2021 08:11
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout a863c84812089836a3c0f2f215ab3e2579fc8acf -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/f-/fdk-aac.json b/versions/f-/fdk-aac.json
index beccee5..d874a00 100644
--- a/versions/f-/fdk-aac.json
+++ b/versions/f-/fdk-aac.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "f218c0d4301af1cb8348d75a8c5b2caabad4b20b",
+      "git-tree": "1ee90ec9cebb3eebb36f0a81a2415f0121c7a8da",
       "version-semver": "2.0.2",
       "port-version": 1
     },

@Be-ing Be-ing force-pushed the fdk_aac_heaac_feature branch from 22488e8 to fbeb8c6 Compare August 3, 2021 08:14
Be-ing added 2 commits August 3, 2021 04:26
By default, use a fork of fdk-aac (which was originally written
by Fraunhofer for the Android Open Source Project) which has
support for the patent-encumbered HE-AAC, HE-AACv2, and xHE-AAC
profiles removed. This fork is used by Fedora and Arch Linux to
provide support for most AAC use cases without patent licensing
fees and to permit combining it with GPL software. The upstream
fdk-aac with support for all AAC profiles can be built with the
new 'he-aac' option for this port.

Fedora Legal's opinion, from
https://bugzilla.redhat.com/show_bug.cgi?id=1501522#c112 :
The Fedora Project is aware that the Free Software Foundation
has stated that the Fraunhofer FDK AAC license is GPL
incompatible, specifically, because of Clause 3.

We believe that the fdk-aac software codec implementation that we
wish to include in Fedora is no longer encumbered by AAC patents.
This fact means that Clause 3 in the FDK AAC license is a "no op",
or to put it plainly, if no patents are in play, there are no
patent licenses to disclaim. For this (and only this) specific
implementation of fdk-aac, we believe that the FDK AAC license is
GPL compatible.

Also:
* remove restriction on dynamic linking. Upstream has a .def
file which is used by CMake.
* replace deprecated vcpkg functions
There is no point to this feature without HE-AAC because FFmpeg has
its own AAC codec that does not support HE-AAC.
@Be-ing Be-ing force-pushed the fdk_aac_heaac_feature branch from fbeb8c6 to 9a8bddb Compare August 3, 2021 09:26
@NancyLi1013 NancyLi1013 removed depends:upstream-changes Waiting on a change to the upstream project requires:author-response labels Aug 3, 2021
@Be-ing Be-ing changed the title [fdk-aac] make patent-encumbered HE-AAC optional; update version [fdk-aac] make patent-encumbered HE-AAC optional Aug 3, 2021
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 5, 2021

Merge?

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Aug 6, 2021
@NancyLi1013
Copy link
Contributor

Waiting for merge now. @Be-ing

"description": "A standalone library of the Fraunhofer FDK AAC code",
"homepage": "https://github.com/mstorsjo/fdk-aac"
"port-version": 1,
"description": "A standalone Third-Party Modified Version of the Fraunhofer FDK AAC Codec Library for Android. Uses a fork without HE-AAC, HE-AACv2, or xHE-AAC support to avoid patent licensing and GPL compatibility issues when built without the he-aac option.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"description": "A standalone Third-Party Modified Version of the Fraunhofer FDK AAC Codec Library for Android. Uses a fork without HE-AAC, HE-AACv2, or xHE-AAC support to avoid patent licensing and GPL compatibility issues when built without the he-aac option.",
"description": "The Fraunhofer FDK AAC Codec Library. By default, uses a fork without HE-AAC, HE-AACv2, or xHE-AAC support to avoid patent licensing and GPL compatibility issues. For the original, un-forked version, use the \"he-aac\" feature.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Hmmmm.... the "for Android" part is the really unfortunate part. I do want this to say something to the effect of "by default uses the fork" rather than always saying it uses the fork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that was clear already but if you have a suggestion for better wording please share.

@BillyONeal
Copy link
Member

I'm a bit concerned about changing what we consider an authoritative source for an already shipping port; under normal circumstances we would require adding a new port (for example, fdk-aac-stripped) instead. However, because this is supposedly the same library with a feature removed, it might be OK. How often does upstream fdk-aac change and are you going to be on the hook to apply such changes in a timely manner?

@ras0219-msft
Copy link
Contributor

It looks like this is at least supported by the fedora project: https://src.fedoraproject.org/rpms/fdk-aac-free

@BillyONeal
Copy link
Member

It looks like this is at least supported by the fedora project: https://src.fedoraproject.org/rpms/fdk-aac-free

@Be-ing Can you confirm that this is expected to be "supported" tracking upstream closely and apply my suggested change to the description? (I tried to apply that for you but this PR was created without the "allow maintainers to make edits" option checked) Then we'll merge.

@NancyLi1013 NancyLi1013 added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Aug 12, 2021
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 12, 2021

Yes, this will be maintained. Alpine Linux is considering to package the fork too. Now that I fixed the merge conflicts and CMake build in the fork, maintaining the fork should be a simple matter of merging upstream when there is a release.

@NancyLi1013 NancyLi1013 requested a review from BillyONeal August 12, 2021 08:59
@BillyONeal BillyONeal merged commit d306ab4 into microsoft:master Aug 12, 2021
@BillyONeal
Copy link
Member

Thanks for your contribution!

@Be-ing Be-ing deleted the fdk_aac_heaac_feature branch August 13, 2021 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants