Summary:	ASTERISK-21981: Pass-through support for Opus and VP8 formats
Reporter:	Tzafrir Cohen (tzafrir)	Labels:
Date Opened:	2013-07-01 01:18:03	Date Closed:	2013-08-23 10:50:01
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Formats/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) asterisk_opus+vp8_passthrough_20130718.patch ( 1) asterisk_opus+vp8_passthrough.patch
Description:	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.
Comments:	By: Rusty Newton (rnewton) 2013-07-01 18:32:01.085-0500 Please have Lorenzo attach patches here or link to a reviewboard issue if he is on reviewboard already. By: Lorenzo Miniero (lminiero) 2013-07-08 03:21:27.108-0500 I've signed my agreement some days ago but I still haven't received any feedback... until then, I'm unable to attach any patch, sorry. By: Lorenzo Miniero (lminiero) 2013-07-08 08:49:04.111-0500 Ok, I'm finally attaching the new patch: asterisk_opus+vp8_passthrough.patch This new patch only provides passthrough support for Opus and VP8. The transcoding module for Opus and the placeholder format module for VP8 have both been removed, as they're unneeded. A couple of notes about the patch:     * 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).     * 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?     * 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?     * 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. I've tested the patch briefly with both PhonerLite (Opus) and Linphone (VP8) by placing a simple call to an Echo test application and it seems to work fine, but more testing would of course be suggested. By: Matt Jordan (mjordan) 2013-07-09 09:39:47.272-0500 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. 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 needs to be configurable. By: Lorenzo Miniero (lminiero) 2013-07-10 04:34:49.996-0500 Hi Matt, about your points: {quote} 1. 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. {quote} There are no format files for either of them as of now. Those would apply to support for .opus and raw .vp8 files, wouldn't they? Opus files are basically .ogg files that contain Opus frames rather than Vorbis ones, and so to support them I guess the existing format_ogg_vorbis one could be a good starting point. By the way, what do you mean by "svn add"? As of now I'm not aware of any SVN folder for this work, I just uploaded the patch. {quote} 2. Remove WARNING messages that aren't warnings and commented out code. If things need a TODO that's fine, but commented out code without context as a tendency to create bugs. Example: {quote} If you mean the commented ast_rtcp_send_h261fur(p->vrtp) in the code, that was already there in the original chan_sip.c. I only added an if to send a RTCP FIR in the VP8 case, and do what was already there if not. I agree about the WARNING messages, I had them there only because they made it easier for me to debug. {quote} 3. Parsing out Opus parameters in the SDP is fine, but WARNING on each one is probably not a good thing :-) {quote} Yes, you're right, as I said for the previous point it was just an easy way for me to immediately visualize what was happening and check I was doing the right thing. Considering SDP parsing should move somewhere else, as you suggested, they can probably be completely removed. {quote} 4. 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. 5. 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. {quote} I didn't know about this, thanks for the explaination! I'll go through the files you mention and prepare a resource module for Opus. I guess the same won't be needed for VP8, since as you said the RTCP FIR negotiation shouldn't be VP8 specific. {quote} 6. I am concerned about this: {quote} Yes, I guessed so. As I said, I don't think it will be needed for passthrough purposes, as from an RTP point of view, what Asterisk sees passing by are always 48 kHz frames, even when they're not. I needed something like this when transcoding, as the actual framerate was important there. {quote} 7. Sending a fast picture update over RTCP doesn't have to be limited to just VP8. {quote} Ok, then I guess we can negotiate the parameter via SDP and, in case our peer supports it, send a keyframe request using RTCP FIR instead of a SIP INFO. This could be the "configurable" you mentioned at the end of your coomment. Would such a negotiation need to be defined somewhere outside of chan_sip as well, or considering it would be codec agnostic can it be defined there instead? By: Lorenzo Miniero (lminiero) 2013-07-18 09:04:17.727-0500 I've worked on changing the patch according to Matt's points. So far I've implemented a new resource module to handle Opus-related SDP, as Matt asked. I still haven't touched the FIR-related code, as I've a couple of doubts related to the new module. I implemented a new resource module: * res_format_attr_opus.c (opus.h contains the keys) basing on the existing CELT and SILK modules, as they shared a couple of attributes (Opus is based on those two codecs anyway). When testing it, though, I've encountered a couple of situations that puzzled me: # According to draft-ietf-payload-rtp-opus, almost all of the parameters you can negotiate are put in a single fmtp attribute (e.g., "a=fmtp:101 stereo=1; sprop-stereo=1"). My module takes care of this by making use of strstr to check for a parameter in order to parse it. The problem is that in Asterisk, if fmtp parameters are separated by whitespaces (as in the example I put before) only the first one is passed to the parser. Removing the whitespace ("a=fmtp:101 stereo=1;sprop-stereo=1") passes the whole string to the parser successfully. Is this a known issue/limitation? I guess this can be blamed on the sscanf format that is used to parse the fmtp in chan_sip.c in the first place. # I then tried a simple scenario, where an Opus compliant softphone (Alice) calls another one (Bob) through Asterisk. I've noticed that my module parses the parameters correctly, but Asterisk doesn't make use of the parameters provided by Alice in order to generate the parameters for the call towards Bob: the defaults I placed there are still generated. This makes sense if we consider that it's Alice's opus_attr structure that is updated when parsing, while to generate the SDP for Bob a different opus_attr (related to that leg) is going to be used, but shouldn't the original opus_attr for Alice going to be used as a basis to generate the offer to send Bob in the current Asterisk architecture for passthrough codecs? Or am I missing something in the format modules specification that may help in that sense? Apart from that, I've removed all the unneeded warning messages, and also the infamous opus_samples method. As of now, when querying the number of samples via ast_codec_get_samples, the method always returns 960 for Opus: this assumes 20ms of audio (the same incorrect assumption was already there as well for CELT) considering that the sampling rate in Opus is always advertised as 48kHz. I'm attaching the updated patch as asterisk_opus+vp8_passthrough_20130718.patch. By: Matt Jordan (mjordan) 2013-07-23 11:31:59.573-0500 {quote} According to draft-ietf-payload-rtp-opus, almost all of the parameters you can negotiate are put in a single fmtp attribute (e.g., "a=fmtp:101 stereo=1; sprop-stereo=1"). My module takes care of this by making use of strstr to check for a parameter in order to parse it. The problem is that in Asterisk, if fmtp parameters are separated by whitespaces (as in the example I put before) only the first one is passed to the parser. Removing the whitespace ("a=fmtp:101 stereo=1;sprop-stereo=1") passes the whole string to the parser successfully. Is this a known issue/limitation? I guess this can be blamed on the sscanf format that is used to parse the fmtp in chan_sip.c in the first place. {quote} Yup, that's a bug. With the way the sscanf is written, the "%s" specifier will hit the first whitespace character and stop. That can be pretty easily fixed to scan a particular number of characters or until we hit a null termination. {quote} I then tried a simple scenario, where an Opus compliant softphone (Alice) calls another one (Bob) through Asterisk. I've noticed that my module parses the parameters correctly, but Asterisk doesn't make use of the parameters provided by Alice in order to generate the parameters for the call towards Bob: the defaults I placed there are still generated. This makes sense if we consider that it's Alice's opus_attr structure that is updated when parsing, while to generate the SDP for Bob a different opus_attr (related to that leg) is going to be used, but shouldn't the original opus_attr for Alice going to be used as a basis to generate the offer to send Bob in the current Asterisk architecture for passthrough codecs? Or am I missing something in the format modules specification that may help in that sense? {quote} Hm. That isn't what's supposed to happen :-) The format attributes presented by Alice (which are stored on her {{ast_channel}}) should be pulled over to the Bob channel during {{ast_request}}. When the INVITE request is sent in {{sip_call}}, the format attributes should be added to the SDP - but those format attributes should have the settings from Alice by virtue of {{ast_request}. It's possible that it is something subtle in the format attribute copying or in the SDP generation - a more in depth code review may help with that. I definitely think this is ready for Review Board :-)