[Home]

Summary:ASTERISK-21130: sip_pvt.dsp incorrect manipulation related to inband dtmfmode and faxdetect in SIPDtmfMode() app. and enable_dsp_detect()
Reporter:Roman S. (rolesok)Labels:
Date Opened:2013-02-18 06:37:38.000-0600Date Closed:2017-07-25 13:57:08
Priority:MinorRegression?
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:1.8.20.0 Frequency of
Occurrence		Constant
Related
Issues:
Environment:Attachments:( 0) asterisk-1.8.20.0-dsp-fix.diff
( 1) asterisk-1.8.20.0-dsp-fix.diff
Description:1. If {{sip_pvt.dsp}} is already inited (e.g. with {{faxdetect=yes}}) then it can't be changed with {{SIPDtmfMode()}} app.

{noformat}
30013 static int sip_dtmfmode(struct ast_channel *chan, const char *data)

30055         if ((ast_test_flag(&p->flags[0], SIP_DTMF) == SIP_DTMF_INBAND) ||
30056             (ast_test_flag(&p->flags[0], SIP_DTMF) == SIP_DTMF_AUTO)) {
30057                 enable_dsp_detect(p);

4598 static void enable_dsp_detect(struct sip_pvt *p)

4602         if (p->dsp) {
4603                 return;
{noformat}

Example:

{noformat}
sip.conf:
faxdetect = yes ; this enables dsp with DSP_FEATURE_FAX_DETECT flag
dtmfmode = rfc2833 ; no DSP_FEATURE_DIGIT_DETECT flag for dsp

extensions.conf:
...,SIPDtmfMode(inband)
{noformat}

{{SIPDtmfMode()}} doesn't set {{DSP_FEATURE_DIGIT_DETECT}} flag for dsp in {{enable_dsp_detect()}}.

2. If {{SIPDtmfMode()}} app. disables inband mode (e.g. switch to rfc2833) it also disables faxdetect (indeed any dsp feature).

{noformat}
30013 static int sip_dtmfmode(struct ast_channel *chan, const char *data)

30055         if ((ast_test_flag(&p->flags[0], SIP_DTMF) == SIP_DTMF_INBAND) ||
30056             (ast_test_flag(&p->flags[0], SIP_DTMF) == SIP_DTMF_AUTO)) {
30057                 enable_dsp_detect(p);
30058         } else {
30059                 disable_dsp_detect(p);
30060         }
{noformat}

This check (line 30055) should also contain {{SIP_PAGE2_FAX_DETECT_CNG}} in condition and keep dsp also for faxdetect.

Example:

{noformat}
sip.conf:
faxdetect = yes ; this enables dsp with DSP_FEATURE_FAX_DETECT flag
dtmfmode = inband ; dsp with DSP_FEATURE_DIGIT_DETECT flag

extensions.conf:
...,SIPDtmfMode(rfc2833)
{noformat}

{{SIPDtmfMode()}} disables any dsp feature including faxdetect (which indeed should be kept).

Thanks
Comments:By: Rusty Newton (rnewton) 2013-02-19 09:35:34.042-0600

Roman, do you want to submit your own fix as a patch so you will get credit in the commit?
By: Roman S. (rolesok) 2013-02-20 06:53:47.031-0600

Description for asterisk-1.8.20.0-dsp-fix.diff:

It shouldn't be an issue because {{SIPDtmfMode()}} app. can't call {{enable_dsp_detect()}} while sending/receiving fax. Other calls of {{enable_dsp_detect()}} are safe too:

{noformat}
 9 + *  \note Can (re)enable temporarily disabled with update_dsp_detect()
10 + *  function features, e.g. for sndfax_exec() and rcvfax_exec() */
{noformat}

{{enable_dsp_detect()}} still should be called because of {{ast_rtp_instance_dtmf_mode_set()}} and {{ast_dsp_set_digitmode()}} -- this can be full DSP initialization:

{noformat}
107 +           ast_test_flag(&p->flags[1], SIP_PAGE2_FAX_DETECT_CNG)) {
108                 enable_dsp_detect(p);
{noformat}

Should be used only to temp. update DSP features and keep related SIP flags -- e.g. for fax related tasks:

{noformat}
35 +/*! \brief Update detect features for already enabled DSP */
36 +static void update_dsp_detect(struct sip_pvt *p, int onoff, int digit, int fax)
{noformat}

AST_OPTION_FAX_DETECT operation should be separated from AST_OPTION_DIGIT_DETECT:

{noformat}
81 +       case AST_OPTION_FAX_DETECT:
82 +               if (ast_test_flag(&p->flags[1], SIP_PAGE2_FAX_DETECT_CNG)) {
{noformat}

Almost the same as above:

{noformat}
94 -               *cp = p->dsp ? 1 : 0;
95 +               *cp = (ast_dsp_get_features(p->dsp) & DSP_FEATURE_DIGIT_DETECT) ? 1 : 0;
{noformat}

DSP shouldn't be destroyed, but features should be disabled/reenabled only:

{noformat}
72 -                       if (*cp) {
73 -                               enable_dsp_detect(p);
74 -                       } else {
75 -                               disable_dsp_detect(p);
76 -                       }
77 +                       update_dsp_detect(p, *cp, 1, 0);
{noformat}
By: Richard Mudgett (rmudgett) 2013-02-20 11:09:58.840-0600

You need to sign the license agreement.
By: Roman S. (rolesok) 2013-02-20 11:34:54.326-0600

Done.
Also removed and reattached patch. But can't see it now. Is this OK?
By: Richard Mudgett (rmudgett) 2013-02-20 16:15:43.171-0600

I had been wondering why some attachments have not been showing up on issues.  It is because the contribution license on those files is pending.  The attachments should show on the issue when the license is approved.
By: Rusty Newton (rnewton) 2013-04-18 08:58:39.599-0500

Thanks for the patch, we see it now.
By: Rusty Newton (rnewton) 2017-07-25 13:57:08.153-0500

Closing this out since there has been no movement since 2013 and it was filed against a now EOL version of Asterisk. With no core development on chan_sip now, this patch is unlikely to get merged in.

If the patch is compatible with a currently supported version please request to reopen the issue and update it to reflect that information. At that point a community developer is welcome to walk the patch through the review process at any time.