add rewrite support using libopusenc

[dhewg]

This contains a small clean-up patch and adds support to rewrite .opus files. In my case I store voicemail recording as .opus files and send those out via email.

codecs.conf is parsed to allow setting two encoder settings:

[opus]
complexity=4
maxaveragebitrate=20000

[traud]

Puh. That is a big one.

1. The first change, that stack pointer thing. Is that something from you or created by some else?
2. [opus] as section collides with the codec module from Digium (yes, it has no type=opus but I am about the name). Your section is about not the codec but the format module. Can you change its name to [opus-file] or [opus-format]? I am not even sure this belongs to the configuration file codecs.conf. That seems to be the very first format module with a configuration file. What about a new configuration file like formats.conf?
3. The shared library libopusenc is not available in Debian, or I could not find it. Is that OpenWrt specific? I know its GitHub project. But where is that package coming from, just OpenWrt? Anyway, is there a way to make the whole thing conditional?
4. What exactly does re-write, when looked at from a high level? In other words: When do I need a format module with re-write exactly; what do you want to achieve on your OpenWrt router?

[traud]

For 4, you wrote in OpenWrt: ’I store voicemail recording as .opus files and send those out via email.’ From which source do you store them, are you recording within your Asterisk?

[dhewg]

1. By myself, why? Haven't observed a crash, but the code looked suspicious as it could
2. I can change it to whatever we want, the initial idea came from #4 . I'm fine either way, lemme know what you prefer
3. That's surprising and a bummer. It's not OpenWrt specific, it's from upstream itself. In fact opus-tools (so opusenc too) changed to using that library almost 4 years ago: https://github.com/xiph/opus-tools/commit/a638dfa37bd857e14b088e0ccade701dc6aafc79

[dhewg]

(Wrong key, continuing...)

3. I can make it conditional, but that's not going to improve the code readability due to ifdeffery
4. Yes, my asterisk instance gets an incoming alaw stream, and I use the voicemail module with format=opus with this patch to send out ogg/opus files, which are small and can nowadays be played back by ~anything (Think imap client on mobile while on holidays)

[dhewg]

3. wrt debian there's this bug report https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=910261

[dhewg]

Final comment for now, continuing the integer/float issue here:

libopus indeed provides public API functions to feed either input data, opus_multistream_encode and opus_multistream_encode_float (plus the plain variant without multistream).

libopusenc too has both public API functions: ope_encoder_write and ope_encoder_write_float, but uses opus_multistream_encode_float internally for both: https://github.com/xiph/libopusenc/blob/master/src/opusenc.c#L243. It converts integer pcm data to float for that: https://github.com/xiph/libopusenc/blob/master/src/opusenc.c#L794

The patches here only use ope_encoder_write.

I can try to look into making libopusenc use integer pcm data directly, but I'd consider that an implementation detail of that library. For this PR it still makes sense to make the complexity and bitrate configurable, right? But if/when libopusenc can be improved in that regard it indeed might perform better on soft-float devices, thanks for the hint!

[traud]

1. the code looked suspicious as it could

Mhm. First of all, that is not code created by me and I never used Opusfile before. Therefore, I have to research all that stuff myself first. Does the library store a pointer to the struct or copy the struct content? If I did not mis-read the source code of Opusfile, the struct content is copied. Therefore, heap or stack does not matter. However, I would be more happy to keep the struct on stack because a caller could create various combinations of those callbacks (yes,yes,null,null or yes,yes,yes,null or …). In other words: Can we drop that change? Or in other words: Why does it look suspicious exactly?

2. I'm fine either way, lemme know what you prefer

opus not. The referenced issue is about the codec module. Here, we are about the format module. Because you are the only one using it, you could even hard-code it. However, I would prefer a format.conf and there a section called opus.

3. I can make it conditional, but that's not going to improve the code readability due to ifdeffery

Debian was just one example. libopusenc is not part of libopus or libopusfile but a third part. Even if it does exist, it may not be present. Therefore, we have to make it conditional somehow. If you need help for the built-in tree part, please, say so.

5. converts integer pcm data to float

Interesting finding. Yes, you should at least report to libopusenc, as feature request. Then, one day someone adds it. Nevertheless, I wonder why Asterisk gives you non-float data. Does it convert the PCM stream internally already?

3. debian there's this bug report

Yes, two new packages are required, libopusenc and libopusenc-dev. Not sure why that stucks. Nevertheless, a complete different story.

[dhewg]

Why does it look suspicious exactly?

Exactly because it's a temporary pointer, but if you already checked that the cb pointers get copied then it obviously works either way.

Maybe I'm just more used to declare everything possible as static const, as that gets thrown in the .rodata elf section. Efficiency, attack surface, yada yada. Or maybe I just read kernel patches too often...

Anyway, I'll drop it and don't forsee issues as you verified it's not just working by accident as-is.

However, I would prefer a format.conf and there a section called opus

Alright, will adapt accordingly

Therefore, we have to make it conditional somehow. If you need help for the built-in tree part, please, say so.

Okay, I'll make it conditional with -DHAVE_LIBOPUSENC. And yeah, I'm not a fan of auto*, maybe you can look into that once I have the define in place?

Nevertheless, I wonder why Asterisk gives you non-float data. Does it convert the PCM stream internally already?

The module contains opus_f.format = ast_format_slin48; which I believe takes care of that aspect

[traud]

HAVE_LIBOPUSENC

Just, HAVE_OPUSENC without the ‘LIB’, please. I then take care of the autoconf part.

The module contains opus_f.format = ast_format_slin48; which I believe takes care of that aspect

That means, you get slin48? With PCM as source, I would have expected slin. Does Asterisk up- and down-sample?

[dhewg]

Sure, HAVE_OPUSENC then.

And yes, IIRC alaw in is 8khz, and 48khz is what opus want, so asterisk upsamples by a nice round factor of 6 (didn't verify how efficiently that is implemented)

[traud]

48 kHz is what opus want

Please, double-check that. In the codec module, I am feeding 8 kHz directly, if the source is 8 kHz. If I am not totally mistaken. That way, I do not have to upsample and Opus uses one of its 8 kHz formats internally. Or was it the sample rate? Ohh … normally I am on a complete different abstraction layer, I forget about such stuff. Please, check again, because all conversion stuff it eating resources on your OpenWrt device.

[dhewg]

If you feed it !=48khz it resamples itself: https://github.com/xiph/libopusenc/blob/master/src/opusenc.c#L432 To keep this code simple I would leave it like this (It's already 48khz only for reading opus files before this PR) We'd only shift the position in the chain where to resample.

[dhewg]

Updated:

• use formats.conf (I went with the plural version as the rest of asterisk uses)
• added HAVE_OPUSENC condition

[traud]

use formats.conf (I went with the plural version as the rest of asterisk uses)

Yes, correct. My fault.

added HAVE_OPUSENC condition

OK. However, the Makefile still requires/enables libopusenc. I am think about making that conditional … any idea for that without going for a full-blow configure script?

[traud]

I think, we leave the Makefile as is, except you have a great idea. If someone uses the Makefile and has libopusenc not installed, please, report! Then, we are going to find a solution together. By the way, to build and install libopusenc in Debian/Ubuntu, for example, you go for

sudo apt install build-essential autoconf automake libopus-dev libtool pkg-config
./autogen.sh
./configure --disable-doc --disable-examples --prefix=/usr
make
sudo make install

@dhewg the source passed all my (compile-time) tests. However, I found one issue in the struct. Can you confirm, that it still works that way? I cannot test run-time so easily. Otherwise, we have to find another approach to avoid compiler padding.

[dhewg]

I added a compile-time option for the Makefile to toggle this feature. And yes, this still works, just tested it ;) But I'm confused about that padding warning. Of course it's padded on 64bit systems, what's the warning supposed to warn against?

[traud]

Changed the code a bit towards my own and the Asterisk Coding Guidelines. Furthermore, I added the autotools stuff, so it builds in-tree. Finally, I tested and fixed the code Asterisk 13, again just compile-time, not runtime. The last remaining test is going to be iwyu. But not today.

I added a compile-time option for the Makefile to toggle this feature.

Great. Yepp, that is easy for a user.

What is the [padding] warning supposed to warn against?

That is a question for StackOverflow. I learned, automatic padding is not good because it is not cross-platform safe. The problem with an int, it is different in size on the underlying platform. This can lead to chaos when you transfer the (memory of such a) struct cross-platform and/or cross-compiler (in remote access, for example). But again, that is a question for a compiler or C language centric discussion board, for example, StackOverflow.

[dhewg]

Nice, looks all good to me!

[traud]

iwyu is helpful as final test especially in complex architectures like Asterisk. I took over this source file, never tested it at runtime, and did not touch it before. iwyu found three headers which were missed before this change here already. With the addition of libopusenc, just one header was missing. Furthermore, iwyu found an unused header. However, that got used with the addition of libopusenc now. Did I mention that I like iwyu?

Running the Debian/Ubuntu packaged iwyu is a bit tricky because you have to know and then install its version of Clang, the version that was used to build iwyu by the package maintainer. More about this here … Then, before you run iwyu, build Asterisk, and then go for

include-what-you-use -fblocks -DAST_MODULE=NULL -DAST_MODULE_SELF_SYM=__internal_format_ogg_opus_open_source_self -isystem/usr/include/opus -I$PWD/include ./formats/format_ogg_opus_open_source.c

Explanation: Clang needs the block-function extension for Asterisk, therefore -fblocks. Instead of AST_MODULE_SELF as for Asterisk 13, AST_MODULE_SELF_SYM must be set since Asterisk 16, normally __internal + filename + _self. Because the systems headers of Opus Codec are in a subfolder but those headers reference themselves without that subfolder, Clang needs a hint for that. Furthermore, the Asterisk headers are needed and that path must be absolute, why ever, therefore that $PWD. Finally, the file to be inspected must be given. Consequently, the Terminal has to be in the root of the Asterisk source tree.

Because we have a compile time switch in the source now, iwyu must be run twice, with an without defined. To test the latter, I simply removed the definition of HAVE_OPUSENC in the file include/asterisk/autoconfig.h and ran iwyu again.

[dhewg]

Nice, thanks!

[traud]

Was a pleasure. Because of this, even issues in other projects were found. For example, while code-reviewing, I had to look-up the documentation. On Opus-Codec.org, the HTML documentation was broken for libopusenc, for more than a year. Fixed now. And because that was a generic issue, more projects benefited from analyzing the cause. Lesson learned: You never know what you start with an issue on GitHub.

If you have other code or ideas, just open requests/issues. And do not forget to update and continue with your down-stream issue openwrt/telephony#693