Summary:	ASTERISK-21150: UDPTL Error Correction Scheme Negotiation Issue, Asterisk copies the Error Correction Scheme from T38 offer from remote peer even if UDPTL error correction scheme is set to NONE for that peer in sip.conf
Reporter:	NITESH BANSAL (nbansal)	Labels:
Date Opened:	2013-02-22 07:17:12.000-0600	Date Closed:	2013-09-18 14:14:17
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_sip/T.38
Versions:	11.4.0	Frequency of Occurrence
Related Issues:
Environment:	x86_64 Debian Squeeze Distro	Attachments:	( 0) do-not-use-ec-from-offer-if-configured-ec-is-none.patch
Description:	Scenario: Asterisk receives a T38 invite which proposes some error correction scheme, Asterisk copies the error correction scheme from the offer and puts it in 200 OK. This behaviour does not change even if UDPTL error correction scheme is set to NONE for this peer. Expected Output: Asterisk should respond with no error correction scheme if this peer has t38_udptl error correction scheme set to NONE. Below is the reference from T38 spec: """"T38FaxUdpEC is negotiated only when using UDPTL as the transport. This parameter can have one of three values (see also Table D.2): t38UDPNoEC, t38UDPRedundancy, or t38UDPFEC. If the answering endpoint supports the offered error correction mode, then it shall return the same value in the answer, otherwise a different value shall be returned""""
Comments:	By: NITESH BANSAL (nbansal) 2013-02-22 07:20:24.666-0600 I have made a patch for this. Please review this patch. Regards, Nitesh Bansal By: Matt Jordan (mjordan) 2013-02-27 16:33:40.584-0600 Nitesh - why wouldn't we want to do something like the following for any error correction scheme: * If the inbound INVITE request's error correction scheme matches the configuration option, simply put the value in the 200 OK and carry on * If the inbound INVITE request's error correction scheme does not match, issue a WARNING or a NOTICE and respond with the configured value I'm not sure why we'd want to only do this for NONE and not for all of the various values. On the same vein, I have a feeling that we use the configuration value for outbound requests but never really intended to use it as a way to sanitize inbound requests. This may have some nasty repercussions from people who expected us to just echo whatever the endpoint told us. What is the drawback for being permissive and echoing what the endpoint wants? By: NITESH BANSAL (nbansal) 2013-03-01 09:54:18.770-0600 Hi Matt, According to me, if EC scheme is set to NONE is sip.conf, it means that you explicitly state that no error correction scheme should be used. Whereas in case of FEC/Redundancy, i think it is best to keep the code as it is, because if EC is cofigured to FEC or Redundancy, it means that you want to use some kind of error correction if possible. I don't think anyone would configure EC to NONE in their sip.conf and still want to use ERROR CORRECTION depending on the offer from peer, the use case which we had was also related to testing a T38 call without any error correction, so we cpnfigured EC to NONE in our sip.conf. Regards, Nitesh Bansal By: Jonathan Rose (jrose) 2013-09-09 12:43:01.288-0500 This patch has a bit of a flaw. If the SDP is being processed against a pvt that has already had the t38 state set to T38_ENABLED then once we reach the following line: if (error_correction_scheme_prior_offer_from_peer != ast_udptl_get_error_correction_scheme(p->udptl) && \ UDPTL_ERROR_CORRECTION_NONE == error_correction_scheme_prior_offer_from_peer) error_correction_scheme_prior_offer_from_peer will be used uninitialized which could cause this condition's evaluation to be a bit unpredictable. Aside from that, I personally don't see anything wrong with what this patch is trying to accomplish. It seems sensible. But we need to work out whether or not it's appropriate to have a presumptive setting to initialize the error correction scheme variable with or else whether we should postpone the fetch until we have actually reached the point where it will be needed. (For instance, have a flag that gets raised by what is currently setting the error correction variable and then retrieve it before using it when that flag is raised). I'll wait a bit on this to see if you have an opinion on which would be more appropriate. By: NITESH BANSAL (nbansal) 2013-09-10 05:34:48.278-0500 Hi, I have adapted the patch. By: Mark Michelson (mmichelson) 2013-09-10 16:35:24.068-0500 I just had a look at the patch on reviewboard, and thought I'd comment here. My initial thoughts on the matter were pretty much exactly the same as Matt's that he made back in February. 1) My interpretation of the sip.conf option is that you are configuring what Asterisk believes the peer capabilities are. This means that if we are offering T.38 to the peer, then that is the error correction we should use. For incoming T.38 reinvites, just go with the error correction that was offered, as it's less likely to cause issues with the peer. This would be in agreement with the snippet of T.38 you pasted in the description. This is because Asterisk is the answering endpoint in this scenario and Asterisk supports all three error correction schemes. Since Asterisk supports the offered scheme, then it should answer with the offered scheme. 2) If my interpretation of 1) is not correct, then I think that it is arbitrary to only apply this logic for "none" and not other error correction options. I read your explanation, but I'm not buying it. It makes the configuration setting much more difficult to explain by having the value sometimes overrule what the incoming reinvite offers and sometimes not. One thing that has not been mentioned anywhere in this issue is what actual real-world problems have been encountered because of Asterisk's current behavior. If the problem is strictly hypothetical, I don't think the current behavior should be changed. If there have been interoperability issues, then that's a different story. My expectation would be Asterisk would interoperate much *better* with the current behavior since Asterisk is responding with an error correction scheme that is known to be supported since the scheme was offered in the inbound reinvite. By: NITESH BANSAL (nbansal) 2013-09-16 07:34:56.457-0500 Hi Matt, The logic behind this patch was that we wanted to test our faxes without error correction. Without this patch, there is no way to disable error correction in asterisk. Well, i would say choice is yours, we did not have any inter op issues as such, but i still believe that it is nice to have a way to disable error correction in someway. Regards, Nitesh Bansal By: Jonathan Rose (jrose) 2013-09-18 14:14:17.347-0500 Per Mark's explanation, I'm closing this issue since this configuration option isn't intended to force error correction in any way for incoming calls. If the ability to do this ever becomes necessary and useful for any reason in the future, please file a new issue report for a new feature request. As such, you would probably want a separate configuration option which doesn't interfere with this one.