[WIP] AAC/HE-AAC encoder (powered by libfdk-aac) #1387

Palakis · 2017-11-21T07:29:49Z

Features:

Started as an internal PR on my fork. Original conversation here: https://github.com/Palakis/mixxx/pull/4

NOTE: Compiling and using this requires a version of libshout modified to support AAC. A ready-to-build source Debian package is provided in lib/ and a build is provided here: https://launchpad.net/~palakis/+archive/ubuntu/libshout-aac

daschuer · 2017-11-21T16:30:53Z

Do you plan this to be ready before our beta deadline? This is a great often requested feature.

Since it requires the patched libshout version, we could make it conditional.
The best solution would be to detect the required libs at runtime. Will it be possible or should this be
Scons switch?

JosepMaJAZ · 2017-11-26T15:15:49Z

On Windows, it cannot be built, because of libshout.
Currently, the windows libs use version 2.4.1 (it's curious because it has source even for opus, but not for aac)

I see that under the lib subfolder, you included a tar for version 2.3.1 that includes a patch to add aac support. We will need to apply that patch into the windows libs so that it can be built previous to merging this (else windows builds will become broken).
I'm going to take a look to see if the patch can be applied without problems and can be built with that. Then, I would submit a PR over the buildserver.

[CXX] src\engine\sidechain\shoutconnection.cpp
shoutconnection.cpp
src\engine\sidechain\shoutconnection.cpp(329): error C2065: 'SHOUT_FORMAT_AAC': undeclared identifier
scons: *** [win64_build\engine\sidechain\shoutconnection.obj] Error 2
scons: building terminated because of errors.

daschuer

Thank you very much for bringing this back to live.
I have left some comments and ideas for improvements.
It would be great if we can finish this by the end of this month to include it in the 2.3 beta.
If not, never mind, than we can shift it to 2.4

daschuer · 2019-01-18T13:53:32Z

src/encoder/encoderfdkaac.cpp

+      m_pAacDataBuffer(nullptr),
+      m_aacInfo() {
+
+    if (pFormat == ENCODING_AAC) {


This can become a switch case

daschuer · 2019-01-18T13:55:33Z

src/encoder/encoderfdkaac.cpp

+const mixxx::Logger kLogger("EncoderFdkAac");
+}
+
+EncoderFdkAac::EncoderFdkAac(EncoderCallback* pCallback, QString pFormat)


pFormat: we use the p prefix for pointers.
In this case you can use a constant reference:
const QString format

daschuer · 2019-01-18T13:58:36Z

build/depends.py

@@ -682,6 +682,11 @@ def configure(self, build, conf):
        if not conf.CheckLib(['libmp3lame', 'libmp3lame-static']):
            raise Exception("Could not find libmp3lame.")

+class FdkAac(Dependence):
+    def configure(self, build, conf):
+        if not conf.CheckLib(['libfdk-aac']):


This must be removed. It would be required without our dynamic loading code.

daschuer · 2019-01-18T14:02:36Z

src/encoder/encoderfdkaac.cpp

+#endif
+
+    for (const auto& libname : libnames) {
+        m_library = new QLibrary(libname, 0);


This can become a unique_ptr and new can be replaced my std::make_unique

daschuer · 2019-01-18T14:05:34Z

src/encoder/encoderfdkaac.cpp

+            kLogger.debug() << "Successfully loaded encoder library " << libname;
+            break;
+        } else {
+            kLogger.warning() << "Failed to load " << libname << ", " << m_library->errorString();


This is IMHO not a warning because we may find a library later. Can we give the warning after the loop, listing the all probed locations?

daschuer · 2019-01-19T16:02:16Z

src/encoder/encoderfdkaac.cpp

+}
+
+void EncoderFdkAac::flush() {
+    // At this point there may still be samples in the FIFO buffer.


please also drop a word why.

daschuer · 2019-01-19T16:05:38Z

src/encoder/encoderfdkaac.h

+    QString buttWindowsFdkAac();
+
+    // libfdk-aac common AOTs
+    static const int AOT_AAC_LC = 2; // AAC-LC


please add a comment where these values come from.
We normally initialize all variables in the C++ file.

daschuer · 2019-01-19T16:08:37Z

.travis.yml

 before_install:
  # Virtual X, needed for analyzer waveform tests
  - if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then export DISPLAY=:99.0         ; fi
  - if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then sh -e /etc/init.d/xvfb start ; fi
-  - if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew install scons portaudio libsndfile libogg libvorbis portmidi taglib libshout protobuf flac ffmpeg qt chromaprint rubberband libmodplug libid3tag libmad mp4v2 faad2 wavpack opusfile lilv lame sound-touch; fi
+  - if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then brew install scons portaudio libsndfile libogg libvorbis portmidi taglib libshout protobuf flac ffmpeg qt chromaprint rubberband libmodplug libid3tag libmad mp4v2 faad2 wavpack opusfile lilv lame sound-touch fdk-aac; fi


I think we don't need this, because we dynamic load the library.

do we have tests that exercise AAC encoding? won't those fail w/o the library being installed?

daschuer · 2019-01-19T16:08:56Z

appveyor.yml

@@ -50,7 +50,7 @@ for:

  install:
    - sudo apt-get update
-    - sudo apt-get -y install gdb libavformat-dev libchromaprint-dev libfaad-dev libflac-dev libid3tag0-dev libmad0-dev libmodplug-dev libmp3lame-dev libmp4v2-dev libopus-dev libopusfile-dev libportmidi-dev libprotobuf-dev libqt5opengl5-dev libqt5sql5-sqlite libqt5svg5-dev librubberband-dev libshout3-dev libsndfile1-dev libsqlite3-dev libtag1-dev libupower-glib-dev libusb-1.0-0-dev libwavpack-dev portaudio19-dev protobuf-compiler qt5-default qtscript5-dev libqt5x11extras5-dev scons qtkeychain-dev liblilv-dev libsoundtouch-dev
+    - sudo apt-get -y install gdb libavformat-dev libchromaprint-dev libfaad-dev libflac-dev libid3tag0-dev libmad0-dev libmodplug-dev libmp3lame-dev libmp4v2-dev libopus-dev libopusfile-dev libportmidi-dev libprotobuf-dev libqt5opengl5-dev libqt5sql5-sqlite libqt5svg5-dev librubberband-dev libshout3-dev libsndfile1-dev libsqlite3-dev libtag1-dev libupower-glib-dev libusb-1.0-0-dev libwavpack-dev portaudio19-dev protobuf-compiler qt5-default qtscript5-dev libqt5x11extras5-dev scons qtkeychain-dev liblilv-dev libsoundtouch-dev libfdk-aac-dev


I think we don't need this, because we dynamic load the library.

daschuer · 2019-01-19T16:09:58Z

build/depends.py

@@ -1507,7 +1514,7 @@ def depends(self, build):
                FidLib, SndFile, FLAC, OggVorbis, OpenGL, TagLib, ProtoBuf,
                Chromaprint, RubberBand, SecurityFramework, CoreServices, IOKit,
                QtScriptByteArray, Reverb, FpClassify, PortAudioRingBuffer, LAME,
-                QueenMaryDsp]
+                QueenMaryDsp, FdkAac]


can be removed.

daschuer · 2019-01-19T16:14:19Z

This will make a big difference for our radio DJs :-)

rryan

thanks for working on this!

rryan · 2019-01-21T02:20:09Z

src/encoder/encoder.h

+                    QString fileExtIn = QString::null) :
+                label(labelIn), internalName(nameIn), lossless(losslessIn),
+                    fileExtension(fileExtIn){
+                if(fileExtension == QString::null) {


please use .isEmpty() instead of == QString::null

rryan · 2019-01-21T02:20:35Z

src/encoder/encoder.h

-            Format(QString labelIn, QString nameIn, bool losslessIn) :
-                label(labelIn), internalName(nameIn), lossless(losslessIn) {}
+            Format(QString labelIn, QString nameIn, bool losslessIn,
+                    QString fileExtIn = QString::null) :


It might be simpler to use QString() instead of QString::null

rryan · 2019-01-21T02:27:18Z

src/encoder/encoderfdkaac.cpp

+      m_pInputFifo(nullptr),
+      m_pFifoChunkBuffer(nullptr),
+      m_readRequired(0),
+      m_aacEnc(),


I don't think it will be initialized unless it's explicitly in this list, will it?

https://godbolt.org/z/2VaJx5
vs.
https://godbolt.org/z/Dn8zTE

rryan · 2019-01-21T02:28:02Z

src/encoder/encoderfdkaac.cpp

+#elif __WINDOWS__
+    // Give top priority to libfdk-aac copied
+    // into Mixxx's installation folder
+    libnames << "libfdk-aac-1.dll";


is that filename a commonly distributed Windows DLL? Why the -1 ?

rryan · 2019-01-21T02:40:44Z

src/encoder/encoderfdkaac.cpp

+    // Last resort choices: try versions with unusual names
+    libnames << "libfdk-aac.dll";
+    libnames << "libfdkaac.dll";
+#elif __APPLE__


Please add libfdk-aac here too so that if it's present in the search paths it will work.

rryan · 2019-01-21T02:51:42Z

src/encoder/encoderfdkaac.cpp

+
+    // Actual encoder init, validates settings provided above
+    int result = aacEncEncode(m_aacEnc, nullptr, nullptr, nullptr, nullptr);
+    if (result != AACENC_OK) {


is there any API we can call that gives a human readable error message ?

rryan · 2019-01-21T02:52:21Z

src/encoder/encoderfdkaac.cpp

+        return -1;
+    }
+
+    // This initializes the encoder handle but not the encoder itself.


Do we need error checking for any of the function calls below? Or is the aacEncEncode the only way we can know if these methods failed?

Palakis · 2019-01-21T10:25:35Z

Thanks! I can address the aforementioned issues on short notice, but the main blocking point is aac support in libshout. Or maybe libshout has a "transparent" mode where it just passes through whatever's given to it.

JosepMaJAZ · 2019-01-21T13:03:37Z

Just a quick note. The aac support on libshout is intentionally not part of the library. They don't accept the patch into the source because they don't want to support non-free codecs. Support for mp3 came originally from its popularity. Finally, no, there is no "pass through" mode so that Mixxx could itself add the missing blocks. So i believe that in this case we need to use our own libshout if aac encoding is enabled. (As in another setting in scons like faad) El dl., 21 gen. 2019 , 11:25, Stéphane Lepin <[email protected]> va escriure:

…

Thanks! I can address the aforementioned issues on short notice, but the main blocking point is aac support in libshout. Or maybe libshout has a "transparent" mode where it just passes through whatever's given to it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1387 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJdFf1mJOtfdLuzEp4y4C98DloKbnFp4ks5vFZWogaJpZM4QlZgc> .

daschuer · 2019-01-21T17:29:11Z

Here is the link to our discussion we had 6 Years ago.

https://icecast-dev.xiph.narkive.com/yIVmJeBE/streaming-aac-with-libshout

For me it would be fine to merge this without a supporting libshout. Instead we can link the AAC patch in the wiki.
At least a step forward.

JosepMaJAZ · 2019-01-21T19:02:37Z

"Link the patch in the wiki"? I'm not sure I quite understand that. Do you mean: "we don't support it, but hey... this is open source and you can modify it with this patch" ?
We have a "lib" subfolder in Mixxx where we have multiple libraries for different reasons. This is just one more case where we might want to have our own library instead of relying on what the OS (or the build env) provides.
Given the small pace at which libshout changes, I don't see a problem with that. And just like some distros don't build aac playback support when building Mixxx, they could build without the AAC encoding/streaming support on libshout.

daschuer · 2019-01-21T22:36:53Z

Yes that makes sense. We just need a PR. The patch is here xiph/Icecast-libshout#8

Palakis · 2019-01-25T09:53:21Z

@daschuer I won't have enough time to finish this by the end of this month. Would be best to postpone it to 2.4.

daschuer · 2019-01-25T13:27:44Z

Ok, nevermind.

daschuer · 2019-02-24T20:06:40Z

I have just found libshout-idjc-2.3.1 it looks like they have renamed mp3.c to mpeg.c and added AAC capability. At least I can select AAC encoding in idjc. Can we use this libary in Mixxx?

Palakis · 2019-02-24T22:06:49Z

@daschuer

Can we use this library in Mixxx?

It seems so, for Linux at leasr. I need to check how it can be used on Windows and macOS

daschuer · 2019-02-24T22:08:50Z

On Windows and MacOs we ship our own libraries so we can use it there as well.

daschuer · 2019-08-16T13:41:54Z

Thank you for you recent changes.
I think this will really make a difference for Mixxx.

I wonder how building will work on the various Linux distros. Do w need to maintain the non idjc version?

If yes, we need to make it conditional in scons. In a way that it fails by default if the idjc package is required but not available or so.

I am currently working on a solution for Mixxx 2.2.3 of https://bugs.launchpad.net/bugs/1833225
That 2.4.2 and 2.4.3 are braking broadcast ing. It should fall back to libshout 2.4.1 in the source tree if the is provided version suffers the bug.

Maybe we can ship for 2.3.0 the idjc version in the source tree?

Palakis · 2019-08-16T15:24:09Z

@daschuer Thanks!

I wonder how building will work on the various Linux distros. Do w need to maintain the non idjc version?

What do you mean by that?

If yes, we need to make it conditional in scons. In a way that it fails by default if the idjc package is required but not available or so.

Will do!

I am currently working on a solution for Mixxx 2.2.3 of https://bugs.launchpad.net/bugs/1833225
That 2.4.2 and 2.4.3 are braking broadcast ing. It should fall back to libshout 2.4.1 in the source tree if the is provided version suffers the bug.
Maybe we can ship for 2.3.0 the idjc version in the source tree?

By 2.4.2 and 2.4.3, you mean versions of libshout or libshout-idjc? Regarding shipping libshout-idjc in the source tree, is it something that's been done or is being done for another dependency?

daschuer · 2020-01-18T17:39:18Z

Here is a version without conflicts and libshout-idcj: https://github.com/Palakis/mixxx/pull/5
Please have a look.

github-actions · 2020-10-20T00:22:21Z

This PR is marked as stale because it has been open 90 days with no activity.

Be-ing · 2021-02-25T02:54:58Z

Superseded by #3615

Be-ing · 2021-02-25T02:57:14Z

Thank you for starting this @Palakis. Sorry it has taken so long to get it merged and released. We have spent the past few months investing a ton of effort to make our build infrastructure more maintainable so adding new dependencies is no longer a big deal. Since you have started this there is now a fork of fdk-aac without HE-AAC support to avoid patent issues that Fedora and Arch Linux package. We are now using it in our Windows and macOS builds.

Be-ing added the broadcasting label Dec 27, 2017

Be-ing added this to the 2.2.0 milestone Dec 27, 2017

Be-ing modified the milestones: 2.2.0, stalled Jun 23, 2018

mixxxdj deleted a comment Oct 18, 2018

Stéphane L and others added 4 commits January 17, 2019 09:26

fdk-aac encoder

c892b75

build: fix missing FdkAac

2d48d5b

encoderfdkaac: refactor array usage

3e44f7e

travis(osx): install fdk-aac

6f0fd94

daschuer modified the milestones: stalled, 2.3.0 Jan 19, 2019

daschuer requested changes Jan 19, 2019

View reviewed changes

rryan requested changes Jan 21, 2019

View reviewed changes

build: libfdk-aac not needed

42fd1a5

daschuer modified the milestones: 2.3.0, 2.4.0 Jan 25, 2019

Palakis added 12 commits August 16, 2019 10:49

Merge branch 'master' into fdkaac-encoder

99281ac

ci(ubuntu): use libshout-idjc

4ed8cf0

build: use libshout-idjc

87fab8d

broadcastmanager: fix libshout include

1991353

encoder: fix syntax error

1f4292e

build(depends): fix fdkaac build issue

4e8a638

encoderfdkaac: remove usage of QDesktopServices

4a1e497

encoderfdkaac: fix sample conversion

9cbc025

shoutconnection: fix libshout include

d80c66f

shoutconnection: fix syntax errors

befa58c

dlgprefbroadcadt: fix libshout include

7a37800

version.cpp: fix libshout include

4a26349

This was referenced Aug 16, 2019

[2.3.x unix] Add libshout-idjc mixxxdj/buildserver#77

Merged

[2.3.x-windows] Add libshout-idjc mixxxdj/buildserver#78

Closed

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 20, 2020

Be-ing changed the base branch from master to main October 23, 2020 23:53

daschuer mentioned this pull request Jan 9, 2021

AAC/HE-AAC encoder for recording and broadcast #3543

Closed

ronso0 marked this pull request as draft February 14, 2021 21:59

Be-ing closed this Feb 25, 2021

mixxxbot mentioned this pull request Aug 22, 2022

AAC Streaming Support #5796

Closed

daschuer added broadcast Bugs pertaining to streaming radio broadcaster use-case and removed broadcasting labels Oct 4, 2022

[WIP] AAC/HE-AAC encoder (powered by libfdk-aac) #1387

[WIP] AAC/HE-AAC encoder (powered by libfdk-aac) #1387

Conversation

Palakis commented Nov 21, 2017 • edited Loading

daschuer commented Nov 21, 2017

JosepMaJAZ commented Nov 26, 2017 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jan 19, 2019

rryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Palakis commented Jan 21, 2019

JosepMaJAZ commented Jan 21, 2019 via email

daschuer commented Jan 21, 2019

JosepMaJAZ commented Jan 21, 2019

daschuer commented Jan 21, 2019

Palakis commented Jan 25, 2019

daschuer commented Jan 25, 2019

daschuer commented Feb 24, 2019

Palakis commented Feb 24, 2019

daschuer commented Feb 24, 2019

daschuer commented Aug 16, 2019

Palakis commented Aug 16, 2019

daschuer commented Jan 18, 2020

github-actions bot commented Oct 20, 2020

Be-ing commented Feb 25, 2021

Be-ing commented Feb 25, 2021 • edited Loading

Palakis commented Nov 21, 2017 •

edited

Loading

JosepMaJAZ commented Nov 26, 2017 •

edited

Loading

Be-ing commented Feb 25, 2021 •

edited

Loading