Skip to content

Issue/104 bcg729 api #139

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.

Sign up for GitHub

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

Closed
tonich-sh wants to merge 4 commits into from
Closed

Issue/104 bcg729 api #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f43b9f4
Test building with bcg729 on Travis CI
fbriere Feb 10, 2018
6fa1755
Add support for new bcg729 API introduced in 1.0.2 [incomplete]
fbriere Feb 6, 2018
0b0f972
Implemented new api (1.0.2+) for bcg729 codec
Mar 29, 2019
5386847
Enabled bcg729-master tests
Mar 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ env:
- FLAGS="-DWITH_QT5=ON -DWITH_ALSA=ON -DWITH_GSM=ON -DWITH_SPEEX=ON -DWITH_ZRTP=ON"
# (qttools5-dev-tools is explicitly included because of Debian bug #835295)
- PACKAGES="libasound2-dev libgsm1-dev libspeex-dev libspeexdsp-dev libzrtpcpp-dev qtdeclarative5-dev qttools5-dev qttools5-dev-tools"

matrix:
# Test various compiler versions
- PACKAGES_ADD="g++-4.9" MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9"
Expand All @@ -15,9 +16,15 @@ env:
# The version of uCommon available on trusty (6.0.7) will cause a build
# failure when compiling with GCC 7 or Clang.
#- PACKAGES_ADD="g++-7" MATRIX_EVAL="CC=gcc-7 && CXX=g++-7"

# Test with all options disabled
- FLAGS="-DWITH_QT5=OFF -DWITH_ALSA=OFF -DWITH_GSM=OFF -DWITH_SPEEX=OFF -DWITH_ZRTP=OFF" PACKAGES=""

# Test building with bcg729
- BUILD_BCG729="master" FLAGS="-DWITH_QT5=OFF -DWITH_ALSA=OFF -DWITH_G729=ON" PACKAGES="git ca-certificates pkg-config libtool automake autoconf"
# Also test the old pre-1.0.2 API (issue #104)
- BUILD_BCG729="1.0.1" FLAGS="-DWITH_QT5=OFF -DWITH_ALSA=OFF -DWITH_G729=ON" PACKAGES="git ca-certificates pkg-config libtool automake autoconf"

# See https://docs.travis-ci.com/user/languages/cpp/#C11-C%2B%2B11-%28and-Beyond%29-and-Toolchain-Versioning
before_install:
- eval "${MATRIX_EVAL}"
Expand All @@ -28,6 +35,8 @@ install:
- sudo apt-get -y install bison cmake flex libccrtp-dev libmagic-dev libreadline-dev libsndfile1-dev libucommon-dev libxml2-dev linux-libc-dev $PACKAGES $PACKAGES_ADD

script:
# Download and build bcg729 if necessary
- if [ "$BUILD_BCG729" ]; then git clone git://git.linphone.org/bcg729.git && (cd bcg729 && git checkout "$BUILD_BCG729" && ./autogen.sh && ./configure && make && sudo make install); fi
- mkdir _build
- cd _build
- cmake -DCMAKE_INSTALL_PREFIX=../_install $FLAGS ..
Expand Down
4 changes: 4 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ if (WITH_G729)
if (G729_FOUND)
message(STATUS "bcg729 OK")
set(HAVE_BCG729 TRUE)

if (NOT G729_OLD_API)
set(HAVE_BCG729_API_1_0_2 TRUE)
endif (NOT G729_OLD_API)

include_directories(${G729_INCLUDE_DIR})
else (G729_FOUND)
Expand Down
28 changes: 28 additions & 0 deletions cmake/FindG729.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,42 @@
INCLUDE(CMakePushCheckState)
INCLUDE(CheckCSourceCompiles)

FIND_PATH(G729_INCLUDE_DIR bcg729/decoder.h)
FIND_LIBRARY(G729_LIBRARY NAMES bcg729)

IF(G729_INCLUDE_DIR AND G729_LIBRARY)
SET(G729_FOUND TRUE)

# The bcg729 API was changed in 1.0.2 (see #104); since there is no
# apparent way to determine the actual installed version, this checks
# whether we are dealing with the old or new API.
CMAKE_PUSH_CHECK_STATE()
SET(CMAKE_REQUIRED_INCLUDES "${INCLUDE_DIRECTORIES};${G729_INCLUDE_DIR}")
SET(CMAKE_REQUIRED_LIBRARIES "${G729_LIBRARY}")
SET(CMAKE_REQUIRED_QUIET TRUE)
# Try to compile something using the old (pre-1.0.2) API
CHECK_C_SOURCE_COMPILES("
#include <bcg729/encoder.h>
#include <bcg729/decoder.h>

int main() {
/* This function requires an argument since 1.0.2 */
initBcg729EncoderChannel();
return 0;
}
" G729_OLD_API)
CMAKE_POP_CHECK_STATE()
ENDIF(G729_INCLUDE_DIR AND G729_LIBRARY)

IF(G729_FOUND)
IF (NOT G729_FIND_QUIETLY)
MESSAGE(STATUS "Found bcg729 includes: ${G729_INCLUDE_DIR}/bcg729/decoder.h")
MESSAGE(STATUS "Found bcg729 library: ${G729_LIBRARY}")
IF (G729_OLD_API)
MESSAGE(STATUS "Using the old (pre-1.0.2) bcg729 API")
ELSE (G729_OLD_API)
MESSAGE(STATUS "Using the new (post-1.0.2) bcg729 API")
ENDIF (G729_OLD_API)
ENDIF (NOT G729_FIND_QUIETLY)
ELSE(G729_FOUND)
IF (G729_FIND_REQUIRED)
Expand Down
23 changes: 21 additions & 2 deletions src/audio/audio_decoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,15 +542,30 @@ uint16 t_g729a_audio_decoder::get_ptime(uint16 payload_size) const
uint16 t_g729a_audio_decoder::decode(uint8 *payload, uint16 payload_size,
int16 *pcm_buf, uint16 pcm_buf_size)
{
#ifdef HAVE_BCG729_API_1_0_2
int frame_size;
uint8 *encoded_data_ptr = payload;
int16 *decoded_data_ptr = pcm_buf;
uint32 new_len = 0;

@fbriere fbriere Jul 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The type of new_len (uint32) does not match the return type of this function (uint16).

for (uint16 done = 0; done < payload_size && new_len < pcm_buf_size; done += frame_size)

@fbriere fbriere Jul 8, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The second half of this condition will interrupt the loop if we're about to write past the end of payload (as is its goal). However, these functions are intended to encode/decode their entire input, and have no way to tell the caller they only did part of the job.

The previous version avoided this problem with the use of assertions, to ensure that payload was big enough; I think you would have to do the same. (The asserted expressions will end up a bit more complicated, though.)

{
uint8 is_sid = (payload_size - done < 8) ? 1 : 0;

@fbriere fbriere Jul 8, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, why 8? Not that it won't work (any expression returning true on 2 and false on 10+ will do the job), but I was wondering if it had a special significance that I was unaware of.

frame_size = (is_sid == 1) ? 2 : 10;
bcg729Decoder(_context, encoded_data_ptr, payload_size, false, is_sid, false, decoded_data_ptr);

@fbriere fbriere Jul 8, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I already made mention of the use of payload_size as bitStreamLength. On second thought, I guess it would make sense if the entire payload was only a single SID frame sent per RFC 3389. Which is not the case here, of course. (Personally. I'd much rather recommend sticking to a simple 0.)

encoded_data_ptr += frame_size;
decoded_data_ptr += 80;
new_len += 80;
}
return new_len;
#else
assert((payload_size % 10) == 0);
assert(pcm_buf_size >= payload_size*8);

for (uint16 done = 0; done < payload_size; done += 10)
{
bcg729Decoder(_context, &payload[done], false, &pcm_buf[done * 8]);
}

return payload_size * 8;
#endif
}

bool t_g729a_audio_decoder::valid_payload_size(uint16 payload_size, uint16 sample_buf_size) const
Expand All @@ -562,7 +577,11 @@ uint16 t_g729a_audio_decoder::conceal(int16 *pcm_buf, uint16 pcm_buf_size)
{
assert(pcm_buf_size >= 80);

#ifdef HAVE_BCG729_API_1_0_2
bcg729Decoder(_context, nullptr, false, true, false, false, pcm_buf);
#else
bcg729Decoder(_context, nullptr, true, pcm_buf);
#endif
return 80;
}

Expand Down
22 changes: 20 additions & 2 deletions src/audio/audio_encoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,11 @@ uint16 t_g726_audio_encoder::encode(int16 *sample_buf, uint16 nsamples,
t_g729a_audio_encoder::t_g729a_audio_encoder(uint16 payload_id, uint16 ptime, t_user *user_config)
: t_audio_encoder(payload_id, ptime, user_config)
{
#ifdef HAVE_BCG729_API_1_0_2
_context = initBcg729EncoderChannel(false);
#else
_context = initBcg729EncoderChannel();
#endif
}

t_g729a_audio_encoder::~t_g729a_audio_encoder()
Expand All @@ -448,13 +452,27 @@ uint16 t_g729a_audio_encoder::encode(int16 *sample_buf, uint16 nsamples,
assert (payload_size >= (nsamples/8));

silence = false;

#ifdef HAVE_BCG729_API_1_0_2
uint16 new_len = 0;
int16 *decoded_data_ptr = sample_buf;
uint8 *encoded_data_ptr = payload;
uint8 frame_size;
int loops = (int)nsamples / 80;
for (uint16 done = 0; done < loops; done ++)
{
bcg729Encoder(_context, decoded_data_ptr, encoded_data_ptr, &frame_size);
encoded_data_ptr += frame_size;
decoded_data_ptr += frame_size * 8;

@fbriere fbriere Jul 8, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a mistake, as decoded_data_ptr should always be incremented by 80, regardless of frame_size. (Of course, as long as enableVAD has been unset, frame_size will be 10 and everything will work fine. But if Annex B features are ever enabled, this will no longer be the case.)

new_len += frame_size;
}
return new_len;
#else
for (uint16 done = 0; done < nsamples; done += 80)
{
bcg729Encoder(_context, &sample_buf[done], &payload[done / 8]);
}

return nsamples / 8;
#endif
}

#endif
1 change: 1 addition & 0 deletions twinkle_config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#cmakedefine HAVE_ILBC
#cmakedefine HAVE_ZRTP
#cmakedefine HAVE_BCG729
#cmakedefine HAVE_BCG729_API_1_0_2
#cmakedefine HAVE_GSM

#cmakedefine HAVE_UNISTD_H
Expand Down