[asterisk-bugs] [JIRA] (ASTERISK-21981) Pass-through support for Opus and VP8 formats
Matt Jordan (JIRA)
noreply at issues.asterisk.org
Tue Jul 9 09:41:03 CDT 2013
[ https://issues.asterisk.org/jira/browse/ASTERISK-21981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=207854#comment-207854 ]
Matt Jordan commented on ASTERISK-21981:
----------------------------------------
Some cleanup/findings from a cursory glance:
# I don't see a {{format_opus}} or a {{format_vp8}} in the patch. Do those exist? If so, you have to {{svn add}} them in order for them to get included in the diff.
# Remove WARNING messages that aren't warnings and commented out code, such as {{/* ast_rtcp_send_h261fur(p->vrtp); */}}. If things need a *TODO* that's fine, but commented out code without context as a tendency to create bugs. Example:
{noformat}
+ /* Only use this for WebRTC users */
+ struct ast_format_cap *fcap = ast_channel_nativeformats(ast);
+ struct ast_format vp8;
+ ast_format_set(&vp8, AST_FORMAT_VP8, 0);
+ if(ast_format_cap_iscompatible(fcap, &vp8)) {
+ sip_pvt_lock(p);
+ if (p->vrtp) {
+ ast_log(LOG_WARNING, "chan_sip, sending RTCP FIR to WebRTC user\n");
+ /* FIXME Fake RTP write, this will be sent as an RTCP packet */
+ struct ast_frame fr;
+ fr.frametype = AST_FRAME_CONTROL;
+ fr.subclass.integer = AST_CONTROL_VIDUPDATE;
+ res = ast_rtp_instance_write(p->vrtp, &fr);
+ }
+ sip_pvt_unlock(p);
+ } else {
+ transmit_info_with_vidupdate(p);
+ /* ast_rtcp_send_h261fur(p->vrtp); */
+ }
{noformat}
# Parsing out Opus parameters in the SDP is fine, but WARNING on each one is probably not a good thing :-)
{noformat}
+ if (sscanf(fmtp_string, "maxplaybackrate=%30u", &value) == 1) {
+ ast_log(LOG_WARNING, "Got Opus maxplaybackrate=%d\n", value);
+ /* TODO: actually handle this */
+ found = TRUE;
+ }
{noformat}
# SDP attribute parsing for specific codecs actually shouldn't be done in {{chan_sip}}. In Asterisk 10 (and more so in Asterisk 11), we added the ability for specific resource modules to provide contextual parsing of SDP parameters to customize handling of codecs. This is done for H.263 in {{res_format_attr_h263}}, H.264 in {{res_format_attr_h264}}, SILK in {{res_format_attr_silk}}, etc. I'd imagine you would want to put this code in a {{res_format_attr_opus}}, then register the module as an attribute format handler/provider. The SDP parsing in {{chan_sip}} will automatically find and call the callbacks to parse the SDP and pass the information to a codec module. This also reduced code duplication in other channel drivers, as they don't have re-write SDP parsing or codec manipulation.
# This also applies to SDP generation - a specific format module can add things to an SDP. For an example, see {{silk_sdp_generate}} in {{res_format_attr_silk}}.
# I am concerned about this:
{noformat}
+/* Opus: copied from opus_decoder.c */
+static int opus_samples(unsigned char *data, int len) {
{noformat}
Unfortunately, if possible, it would be better to remove this particular function. I'm not sure what that does to the overall patch however.
# Sending a fast picture update over RTCP doesn't have to be limited to just VP8.
Specific points:
{quote}
The Opus SDP parameters negotiation (draft-ietf-payload-rtp-opus-00) in chan_sip.c is currently only a placeholder. Asterisk ignores what the caller provides, and when originating a call it currently sets a few parameters that may be unneeded as Asterisk isn't going to actually terminate the Opus stream. Considering the passthrough Opus support, what may be useful to improve the patch would be to store the parameters from a caller in the SIP structure, and copy them in the SDP to provide to the callee: in fact, Opus has some end-to-end functionality (e.g., FEC) that may be useful when supported by both parties. I'm not sure what the best behaviour would be when Asterisk is actually going to originate a call: I guess the correct thing to do would be to set no SDP parameter at all (caller and callee may fine tune this later in a re-INVITE).
{quote}
As noted above, the SDP parameter negotiation should all be moved into a separate module. This has a few benefits:
# It keeps it out of {{chan_sip}} (Yay!), which reduces code bloat there and makes it usable in any channel driver that uses SDP, i.e., {{chan_pjsip}}
# It can be extended and built on much easier
# Supporting 'passing through' parameters between both call legs can be done in the format attribute module - which is how we do it with H.264 and other pass through formats. I'd certainly prefer to keep such logic out of {{chan_sip}} and the {{sip_pvt}} if possible. The way that's accomplished is to use the {{ast_format_attr}} struct to pass the information through.
{quote}
Following the suggestion by Matt on the mailing list, I kept the computation of the number of samples in an Opus RTP packet in frame.c. It may be worth pointing out, though, that that method is, as explicitly documented in the code, copied from opus_decoder.c: libopus has a BSD license, is it compliant with the Asterisk license or would a different approach to the computation be needed to have it an acceptable inclusion?
{quote}
Yeah, I'm a bit concerned by it. If possible, we might want to just remove it. If not, we can do a more in-depth look at it.
{quote}
Considering the VP8 patch also includes a small modification to the RTP engine to support the transmission of a RTCP FIR message (to request a key frame), I also modified chan_sip.c in order to have it add a new SDP attribute (a=rtcp-fb:* ccm fir) whenever VP8 is involved. This is especially needed in WebRTC, as for instance Chrome ignores RTCP FIR messages if support for it hasn't been previously negotiated. I'm not sure this RTCP FIR message is, in the current state of the patch, also transparently forwarded by Asterisk when received from a leg: what is the current behaviour of Asterisk with respect to this when acting in passthrough mode?
{quote}
# A fast picture update frame generated as a result of an RTCP FIR message is put onto the channel that owns the RTCP session. That *should* get passed through the bridge, if the bridging technology supports passing the frame through.
# Since you've gone ahead and put support for RTCP FIR into the RTCP code (which is a good thing), I'd make it non-specific to VP8.
{quote}
About the RTCP FIR message, as I anticipated this is used to request the transmission of a key frame. As such, I modified the behaviour of AST_CONTROL_VIDUPDATE in chan_sip.c to have it transmit a RTCP FIR message rather than a SIP INFO in the VP8 case. Olle expressed some concerns about this approach on the mailing list (http://lists.digium.com/pipermail/asterisk-dev/2013-May/060386.html). As I said in a reply, I wrote it that way to actually minimize the impact of my addition on the default behaviour, but of course this may need to be addressed properly.
{quote}
It need to be configurable.
> Pass-through support for Opus and VP8 formats
> ---------------------------------------------
>
> Key: ASTERISK-21981
> URL: https://issues.asterisk.org/jira/browse/ASTERISK-21981
> Project: Asterisk
> Issue Type: New Feature
> Security Level: None
> Components: Formats/General
> Reporter: Tzafrir Cohen
> Attachments: asterisk_opus+vp8_passthrough.patch
>
>
> Following discussions on the asterisk-dev mailing list, I create a bug for Lorenzo Miniero to merge the parts of his WebRTC codecs patch that can be merged.
> See:
> http://lists.digium.com/pipermail/asterisk-dev/2013-May/060388.html
> http://lists.digium.com/pipermail/asterisk-dev/2013-May/060419.html
> (If this is a duplicate: sorry for the noise. I failed to find it)
> Including this patch should allow for a review in time before the Asterisk 12 deadline. I marked this as "major" as I think this is an important feature missing in Asterisk right now. Supporting the formats is the least we can do.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.asterisk.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
More information about the asterisk-bugs
mailing list