Lyra support #3949
Lyra support #3949
Conversation
|
The version selection is very tricky indeed, can only work for specific Lyra and Bazel version. I tried Lyra 1.3.2 with Bazel 5.3.2 and it didn't work, while 1.3.0 and 5.0.0 was a success. The option The detection also failed: |
|
How about our SDP specs (e.g: codec name, supported parameters (mandatory & optional)), perhaps it is a good idea to describe it here too? |
Just fixed the |
Ah, I thought the python script is under the Executing the script, the lib generation in |
pjmedia/src/pjmedia-codec/lyra.cpp
Outdated
| attr->info.pt = (pj_uint8_t)id->pt; | ||
| attr->info.channel_cnt = 1; | ||
|
|
||
| idx = attr->info.pt - PJMEDIA_RTP_PT_LYRA_8; |
There was a problem hiding this comment.
Note that PT can be rewritten, using it in index calculation here without range check may or may not be harmful (some old codecs may still use it though). Perhaps better avoiding this if there is a more reliable way (e.g: comparing clock rate perhaps)?
pjmedia/src/pjmedia-codec/lyra.cpp
Outdated
| pjmedia_frame frames[]) | ||
| { | ||
| unsigned count = 0; | ||
| unsigned frm_size = lyra_cfg.bit_rate / (50 * 8); |
There was a problem hiding this comment.
Is the lyra_cfg.bit_rate always the same between local & remote?
There was a problem hiding this comment.
Currently yes. We can add the information on the SDP, however it would be proprietary.
There was a problem hiding this comment.
I mean does Lyra mandate that both sides must use the same bitrate? If not, then assuming both using same bit rate does not sound very safe, even for communication among PJSIP endpoints, as the bitrate seems to be configurable.
If Lyra does not provide packetizing APIs (haven't checked myself), IMO a safer approach would be not allowing multiple frames per packet, this is simple but perhaps not a good idea for low bitrate codecs. Another approach may be the parse just returns the whole encoded Lyra payload in the first frame and zero length data for any next frames (haven't checked if this is possible).
There was a problem hiding this comment.
The issue is that the decoder expects a single frame when decoding. It will be an issue if the payload has multiple frames.
I think using a SDP setting is the correct way, although currently, it would be proprietary.
There was a problem hiding this comment.
Using proprietary SDP is fine to me (without standard/rfc, the codec name is also kind of proprietary?). So the decoder will tell remote via its SDP (as usually SDP is about publishing decoder capability) about its expected bitrate when multiple frames per packet is used by the encoder?
There was a problem hiding this comment.
Just added the bitrate setting on the SDP.
a=rtpmap:96 lyra/16000
a=fmtp:96 bitrate=3200
The bitrate information is needed for parsing, regardless multiple frames used/not.
There was a problem hiding this comment.
I think we need to clarify in the docs:
- whether the bitrate in the SDP is requested by receiver/decoder or an information from sender/encoder,
- what will happen if SDP offer-answer use different bitrates.
| auto decoded = lyra_data->dec->DecodeSamples(samples_to_request); | ||
| if (!decoded.has_value()) { | ||
| PJ_LOG(4, (THIS_FILE, "Decode failed!")); | ||
| return PJMEDIA_CODEC_EFAILED; |
|
Btw, in case you manage to find (and test against) any other SIP phone/B2BUA/server supporting Lyra, I think it would be great to be mentioned here too. |
The bazel build process will download all the required third party on its To clean up the build output, you can use: I tried rebuilding from a fresh folder of lyra source and the header files are there. |
|
Okay, cleaning and rebuilding Lyra solved the issue.
Also, perhaps provide more detailed instruction on how to perform the copying and specifying the path for Android? |
- Add bitrate config on SDP, allowing different bitrate on local and remote - Fix some compile warning - Set [--with-lyra]/model_coeffs as default to PJMEDIA_CODEC_LYRA_DEFAULT_MODEL_PATH
| @@ -83,6 +83,10 @@ enum pjmedia_audio_pt | |||
| PJMEDIA_RTP_PT_G7221_RSV1, /**< G722.1 reserve */ | |||
| PJMEDIA_RTP_PT_G7221_RSV2, /**< G722.1 reserve */ | |||
| PJMEDIA_RTP_PT_OPUS, /**< OPUS */ | |||
| PJMEDIA_RTP_PT_LYRA_8, /**< LYRA @ 8Kbps */ | |||
pjsip/include/pjsua2/media.hpp
Outdated
| */ | ||
| struct CodecLyraConfig | ||
| { | ||
| unsigned bit_rate; /**< The expected bit rate from remote. */ |
There was a problem hiding this comment.
just a note, the naming convention in pjsua2 is bitRate, instead of bit_rate. But since Opus is already wrong, I guess we can leave it as is.
| # pragma warning(disable: 4100) // Possible loss of data | ||
| #endif | ||
|
|
||
| #include "lyra_encoder.h" |
There was a problem hiding this comment.
Can we can suppress these warnings?
In file included from /Users/ming/teluu/tools/lyra-1.3.0/lyra_encoder.h:25:
In file included from /Users/ming/teluu/tools/lyra-1.3.0/include/com_google_absl/absl/types/span.h:67:
/Users/ming/teluu/tools/lyra-1.3.0/include/com_google_absl/absl/meta/type_traits.h:301:36: warning: builtin __has_trivial_destructor is deprecated; use __is_trivially_destructible instead [-Wdeprecated-builtins]
: std::integral_constant<bool, __has_trivial_destructor(T) &&
^
/Users/ming/teluu/tools/lyra-1.3.0/include/com_google_absl/absl/meta/type_traits.h:350:36: warning: builtin __has_trivial_constructor is deprecated; use __is_trivially_constructible instead [-Wdeprecated-builtins]
: std::integral_constant<bool, __has_trivial_constructor(T) &&
^
/Users/ming/teluu/tools/lyra-1.3.0/include/com_google_absl/absl/meta/type_traits.h:494:17: warning: builtin __has_trivial_assign is deprecated; use __is_trivially_assignable instead [-Wdeprecated-builtins]
bool, __has_trivial_assign(typename std::remove_reference<T>::type) &&
^
/Users/ming/teluu/tools/lyra-1.3.0/include/com_google_absl/absl/meta/type_traits.h:559:8: warning: builtin __has_trivial_copy is deprecated; use __is_trivially_copyable instead [-Wdeprecated-builtins]
(__has_trivial_copy(ExtentsRemoved) || !kIsCopyOrMoveConstructible) &&
^
/Users/ming/teluu/tools/lyra-1.3.0/include/com_google_absl/absl/meta/type_traits.h:560:8: warning: builtin __has_trivial_assign is deprecated; use __is_trivially_assignable instead [-Wdeprecated-builtins]
(__has_trivial_assign(ExtentsRemoved) || !kIsCopyOrMoveAssignable) &&
| //PJ_LOG(4, (THIS_FILE, "Codec recover")); | ||
|
|
||
| /* output_buf_len is unreferenced when building in Release mode */ | ||
| PJ_UNUSED_ARG(output_buf_len); |
- Update some doc - Change field setting name - Remove un-needed code - Suppress compile warning
| lyra_data->enc_bit_rate != 9200)) | ||
| { | ||
| lyra_data->enc_bit_rate = PJMEDIA_CODEC_LYRA_DEFAULT_BIT_RATE; | ||
| } |
There was a problem hiding this comment.
Perhaps better to also get the decoder bitrate from SDP as the app is allowed to modify the SDP?
| pkt_size -= frm_size; | ||
|
|
||
| ++count; | ||
| } |
There was a problem hiding this comment.
Perhaps it is better to be a bit more lenient when remote does not follow bitrate param in SDP, e.g: when count==1 and frm_size < pkt_size, set the frames[0].size = pkt_size?
There was a problem hiding this comment.
The condition count==1 and frm_size < pkt_size is still within the loop.
shouldn't it be:
pj_size_t orig_pkt_size = pkt_size;
while (pkt_size >= frm_size && count < *frame_cnt) {
frames[count].type = PJMEDIA_FRAME_TYPE_AUDIO;
frames[count].buf = pkt;
frames[count].size = frm_size;
frames[count].timestamp.u64 = ts->u64 +
lyra_data->samples_per_frame * count;
pkt = ((char*)pkt) + frm_size;
pkt_size -= frm_size;
++count;
}
if (pkt_size != 0 && count == 1) {
frames[0].size = orig_pkt_size;
}There was a problem hiding this comment.
Sure, the idea is to avoid truncating the payload if it contains only one frame.
| * endpoint will send data at 3200 bitrate, while the local endpoint | ||
| * will send data at 6000 bitrate. | ||
| */ | ||
| unsigned bit_rate; |
There was a problem hiding this comment.
Describe the valid & default bitrates too?
And perhaps also clockrates?
pjmedia/src/pjmedia-codec/lyra.cpp
Outdated
| lyra_factory.param[2].pt = PJMEDIA_RTP_PT_LYRA_32; | ||
| lyra_factory.param[2].clock_rate = 32000; | ||
|
|
||
| lyra_factory.param[3].enabled = 0; |
There was a problem hiding this comment.
How to enable the disabled clock_rates?
- Add comments - Add compile time setting to enable certain samplerate - Get decoder bitrate from SDP - More lenient on codec parse
This PR will add support for lyra codec (https://github.com/google/lyra).
Requirement:
Notes and limitations:
rtpmapSDP attribute will follow the specification defined in rfc8866. It will also specify the bitrate setting as an fmtp param. e.g.:x64andReleasemode only.Building Lyra:
Build commands for Win, Mac, Linux
Build commands for Android
Setup include and lib folder
Bazel will download and build the required third party code used by Lyra. We can use this file libgen.py.zip to generate a single static library and header files required by pjsip.
Run the script from the Lyra source folder.
python3 libgen.pyFor windows, add
--platform=winto the command.Add Lyra support
For GNU targets (Mac, Linux, Android):
Run
configureorconfigure-androidcommand and specify the Lyra source folder using--with-lyraoption../configure --with-lyra=[lyra_src_folder]You should see this on the output if everything is in order.
checking lyra usability... yescontinue building the library
Configuring the codec:
pjmedia_codec_lyra_config::bit_rate/CodecLyraConfig::bit_rate.The value represents the decoder bitrate requested by the receiver. Endpoints can be configured with different bitrates. For example, the local endpoint might be set to a bitrate of 3200, while the remote endpoint is set to 6000. In this scenario, the remote endpoint will send data at 3200 bitrate, while the local endpoint will send data at 6000 bitrate
lyra_config.binarypb,lyragan.tflite,quantizer.tflite,soundstream_encoder.tflite).Fortunately, Lyra provided those files on their source folder (
model-coeffs). You can copy those files to a new folder and specify it topjmedia_codec_lyra_config::model_path/CodecLyraConfig::model_path.CodecLyraConfig::model_path. Check here as reference.