Add support for bcg729 1.0.2+ #104
Comments
|
It would appear that the bcg729 API was changed in version 1.0.2 (specifically by commits BelledonneCommunications/bcg729@843c130 and BelledonneCommunications/bcg729@948d32a). Until Twinkle supports the new API, you should probably downgrade to 1.0.1, which seems to compile just fine. |
|
I have confirmed that everything compiles with bcg729 1.0.2 after adding the new arguments to If it is desirable to support both APIs, I have written a CMake test (42ec560) that does just that. If someone can provide me with the appropriate values, I'll gladly submit a PR. |
fbriere
changed the title
|
Incompatible API change in a point release? Sigh... I'm afraid allowing build against both version will be necessary - for some time, at least. |
(The master build is currently commented out due to LubosD#104; it can be enabled after that issue is fixed.)
(The master build is currently commented out due to LubosD#104; it can be enabled after that issue is fixed.)
Starting with version 1.0.2, bcg729 has changed its API to add support for G.729B, thus requiring us to adjust our function calls depending on which version is installed. When dealing with the new API, we merely need to add a few parameters to disable all G.729B features, namely: * On the decoder side: When `SIDFrameFlag` is not set, the decoder will behave just like before, decoding the payload as a standard G.729A voice frame (or concealing an erased frame). The other parameters, `rfc3389PayloadFlag` and `bitStreamLength`, are only of use when dealing with a SID frame sent as per RFC 3389, and are ignored if `SIDFrameFlag` is not set. * On the encoder side: When `enableVAD` is disabled, the encoder will behave just like before, producing only standard G.729A voice frames. The only API difference is the introduction of `*bitStreamLength`, to return the length of the encoded frame (0, 2 or 10 bytes). In our case, this will always be 10 bytes just like before; an assert() was added to guarantee this. Closes LubosD#104
|
I have just submitted PR #152 to fix this issue. Full disclosure: this code is untested. Of course, it compiles, and the CMake test to find which version is installed is working fine, but I couldn't test the part actually calling bcg729 functions. Now, the changes are so minimal to make any bug very unlikely. Still, if other users could give it i try just to confirm it works just fine, that would be appreciated. Thanks! |
Starting with version 1.0.2, bcg729 has changed its API to add support for G.729B, thus requiring us to adjust our function calls depending on which version is installed. When dealing with the new API, we merely need to add a few parameters to disable all G.729B features, namely: * On the decoder side: When `SIDFrameFlag` is not set, the decoder will behave just like before, decoding the payload as a standard G.729A voice frame (or concealing an erased frame). The other parameters, `rfc3389PayloadFlag` and `bitStreamLength`, are only of use when dealing with a SID frame sent as per RFC 3389, and are ignored if `SIDFrameFlag` is not set. * On the encoder side: When `enableVAD` is disabled, the encoder will behave just like before, producing only standard G.729A voice frames. The only API difference is the introduction of `*bitStreamLength`, to return the length of the encoded frame (0, 2 or 10 bytes). In our case, this will always be 10 bytes just like before; an assert() was added to guarantee this. Closes LubosD#104
Starting with version 1.0.2, bcg729 has changed its API to add support for G.729B, thus requiring us to adjust our function calls depending on which version is installed. When dealing with the new API, we merely need to add a few parameters to disable all G.729B features, namely: * On the decoder side: When `SIDFrameFlag` is not set, the decoder will behave just like before, decoding the payload as a standard G.729A voice frame (or concealing an erased frame). The other parameters, `rfc3389PayloadFlag` and `bitStreamLength`, are only of use when dealing with a SID frame sent as per RFC 3389, and are ignored if `SIDFrameFlag` is not set. * On the encoder side: When `enableVAD` is disabled, the encoder will behave just like before, producing only standard G.729A voice frames. The only API difference is the introduction of `*bitStreamLength`, to return the length of the encoded frame (0, 2 or 10 bytes). In our case, this will always be 10 bytes just like before; an assert() was added to guarantee this. Closes LubosD#104
Even better, they did not bother changing the SONAME, even though both versions's ABI are incompatible. Mixing them results in muted sound at best, and segfaults at worst. |
|
I've now had a chance to actually test G.729, and this seems to work fine. (Any confirmation would always be welcome, of course.) |
|
Hello all,
unfortunately, Twinkle does not seem to run under Manjaro.
At first, it does not accept any other settings for audio input or
audio output other than "ALSA default device". If there is any other
device specified, Twinkle complains that it cannot access that device
(no matter if it exists in the system or not).
Second: As soon as I dial any number, Twinkle emits a loud static noise
through the output device. The very same noise is transmitted by
Twinkle to the recipient of the call after the connection is
established, making a conversation impossible in both directions.
That, unfortunately, makes Twinkle unusable for me, which seems to be
independent of the discussed fix.
inxi-F:
Audio: Device-1: Intel 6 Series/C200 Series Family High Definition
Audio driver: snd_hda_intel Device-2: NVIDIA GF116 High Definition
Audio driver: snd_hda_intel Device-3: Logitech HD Webcam C525 type:
USB driver: snd-usb-audio,uvcvideo Sound Server: ALSA v: k5.2.13-1-
MANJARO
Kind regards,
DAC324
Am Sonntag, den 08.09.2019, 07:31 -0700 schrieb Frédéric Brière:
… I've now had a chance to actually test G.729, and this seems to work
fine. (Any confirmation would always be welcome, of course.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
[
{
***@***.***": "http://schema.org",
***@***.***": "EmailMessage",
"potentialAction": {
***@***.***": "ViewAction",
"target": "
#104?email_source=notifications\u0026email_token=ACH7D57ND7TGIG7NHHP3J2LQIUED5A5CNFSM4EPFVYK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6FREZI#issuecomment-529207909
",
"url": "
#104?email_source=notifications\u0026email_token=ACH7D57ND7TGIG7NHHP3J2LQIUED5A5CNFSM4EPFVYK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6FREZI#issuecomment-529207909
",
"name": "View Issue"
},
"description": "View this Issue on GitHub",
"publisher": {
***@***.***": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]
|
Unfortunately, the code still has not been fixed to compile with any G.729 version newer than 1.0.1. Even when being compiled with -DWITH_G729=Off, I still get that crackling - it does not seem to be dependent on G.729 as Twinkle chooses G.726 40 kbps for my test connection. So it looks like the problem is with the G.726 40 kbps codec installed in the system, and not G.729. By the way: Compiling from the (hopefully) master branch, I get the following version information: Am I doing something wrong, and is there a newer Twinkle branch already available? |
|
@DAC324 As I previously said, please open a separate issue for this unrelated problem. Thanks. |
Starting with version 1.0.2, bcg729 has changed its API to add support for G.729B, thus requiring us to adjust our function calls depending on which version is installed. When dealing with the new API, we merely need to add a few parameters to disable all G.729B features, namely: * On the decoder side: When `SIDFrameFlag` is not set, the decoder will behave just like before, decoding the payload as a standard G.729A voice frame (or concealing an erased frame). The other parameters, `rfc3389PayloadFlag` and `bitStreamLength`, are only of use when dealing with a SID frame sent as per RFC 3389, and are ignored if `SIDFrameFlag` is not set. * On the encoder side: When `enableVAD` is disabled, the encoder will behave just like before, producing only standard G.729A voice frames. The only API difference is the introduction of `*bitStreamLength`, to return the length of the encoded frame (0, 2 or 10 bytes). In our case, this will always be 10 bytes just like before; an assert() was added to guarantee this. Closes LubosD#104
Starting with version 1.0.2, bcg729 has changed its API to add support for G.729B, thus requiring us to adjust our function calls depending on which version is installed. When dealing with the new API, we merely need to add a few parameters to disable all G.729B features, namely: * On the decoder side: When `SIDFrameFlag` is not set, the decoder will behave just like before, decoding the payload as a standard G.729A voice frame (or concealing an erased frame). The other parameters, `rfc3389PayloadFlag` and `bitStreamLength`, are only of use when dealing with a SID frame sent as per RFC 3389, and are ignored if `SIDFrameFlag` is not set. * On the encoder side: When `enableVAD` is disabled, the encoder will behave just like before, producing only standard G.729A voice frames. The only API difference is the introduction of `*bitStreamLength`, to return the length of the encoded frame (0, 2 or 10 bytes). In our case, this will always be 10 bytes just like before; an assert() was added to guarantee this. Closes LubosD#104 (cherry picked from commit 46bee14)
cmake .. -DWITH_ALSA=On -DWITH_SPEEX=On -DWITH_ILBC=Off -DWITH_ZRTP=Off -DWITH_G729=On -DWITH_QT5=On
/home/admins/twinkle/src/audio/audio_decoder.cpp: In member function ‘virtual uint16 t_g729a_audio_decoder::decode(uint8*, uint16, int16*, uint16)’:
/home/admins/twinkle/src/audio/audio_decoder.cpp:550:50: error: invalid conversion from ‘int16* {aka short int*}’ to ‘uint8_t {aka unsigned char}’ [-fpermissive]
bcg729Decoder(_context, &payload[done], false, &pcm_buf[done * 8]);
^~~~~~~~~~~~~~~~~~
/home/admins/twinkle/src/audio/audio_decoder.cpp:550:68: error: too few arguments to function ‘void bcg729Decoder(bcg729DecoderChannelContextStruct*, uint8_t*, uint8_t, uint8_t, uint8_t, uint8_t, int16_t*)’
bcg729Decoder(_context, &payload[done], false, &pcm_buf[done * 8]);
^
In file included from /home/admins/twinkle/src/audio/audio_decoder.h:42:0,
from /home/admins/twinkle/src/audio/audio_decoder.cpp:20:
/usr/include/bcg729/decoder.h:68:24: note: declared here
BCG729_VISIBILITY void bcg729Decoder(bcg729DecoderChannelContextStruct decoderChannelContext, uint8_t bitStream[], uint8_t bitStreamLength, uint8_t frameErasureFlag, uint8_t SIDFrameFlag, uint8_t rfc3389PayloadFlag, int16_t signal[]);
^~~~~~~~~~~~~
/home/admins/twinkle/src/audio/audio_decoder.cpp: In member function ‘virtual uint16 t_g729a_audio_decoder::conceal(int16, uint16)’:
/home/admins/twinkle/src/audio/audio_decoder.cpp:565:48: error: invalid conversion from ‘int16* {aka short int*}’ to ‘uint8_t {aka unsigned char}’ [-fpermissive]
bcg729Decoder(_context, nullptr, true, pcm_buf);
^
/home/admins/twinkle/src/audio/audio_decoder.cpp:565:48: error: too few arguments to function ‘void bcg729Decoder(bcg729DecoderChannelContextStruct*, uint8_t*, uint8_t, uint8_t, uint8_t, uint8_t, int16_t*)’
In file included from /home/admins/twinkle/src/audio/audio_decoder.h:42:0,
from /home/admins/twinkle/src/audio/audio_decoder.cpp:20:
/usr/include/bcg729/decoder.h:68:24: note: declared here
BCG729_VISIBILITY void bcg729Decoder(bcg729DecoderChannelContextStruct *decoderChannelContext, uint8_t bitStream[], uint8_t bitStreamLength, uint8_t frameErasureFlag, uint8_t SIDFrameFlag, uint8_t rfc3389PayloadFlag, int16_t signal[]);
^~~~~~~~~~~~~
src/audio/CMakeFiles/libtwinkle-audio.dir/build.make:86: ошибка выполнения рецепта для цели «src/audio/CMakeFiles/libtwinkle-audio.dir/audio_decoder.cpp.o»
make[2]: *** [src/audio/CMakeFiles/libtwinkle-audio.dir/audio_decoder.cpp.o] Ошибка 1
CMakeFiles/Makefile2:233: ошибка выполнения рецепта для цели «src/audio/CMakeFiles/libtwinkle-audio.dir/all»
make[1]: *** [src/audio/CMakeFiles/libtwinkle-audio.dir/all] Ошибка 2
Makefile:129: ошибка выполнения рецепта для цели «all»
The text was updated successfully, but these errors were encountered: