Summary:ASTERISK-21976: Set more than one codec in dialplan execution using SIP_CODEC (adapted chan_sip:try_suggested_codec)
Reporter:Dennis Guse (dennis.guse)Labels:
Date Opened:2013-06-28 08:03:17Date Closed:2013-08-21 08:47:06
Versions:SVN Frequency of
Environment:FreeBSD (AMD64) 9.1 using Asterisk provided via ports (at the moment 11.4)Attachments:( 0) patch-channels-chan__sip.c-393919
Description:For video calls, we would like to set the codecs in the dialplan using
SIP_CODEC. However, if SIP_CODEC is set, all codecs except the ONE set are disallowed and thus either audio or video is available.

Attached is a patch for 11.4 that allows SIP_CODEC to contain a list of codecs , e.g. "gsm,h264".

Patch against: 11.4: https://docs.google.com/file/d/0ByFooYVveHXdNng4WThoNV8zLVk/edit?usp=sharing

Do you have any feedback?

Authors: Dennis Guse (dennis.guse@qu.tu-berlin.de) und Frank Haase (fra.haase@googlemail.com).
Comments:By: Matt Jordan (mjordan) 2013-06-28 08:52:11.587-0500

A few comments here:

# Patches cannot be supplied inline or in comments on the issue. They must be attached in unified diff format to this issue after signing a license contributor agreeement.
# Improvements and new features are only allowed against trunk, and will not be applied to release branches (particularly LTS releases). Please write the patch against Asterisk trunk.

I've removed the code in the comment section on the issue and put this into Waiting for Feedback - thanks!

By: Dennis Guse (dennis.guse) 2013-06-28 10:08:40.854-0500

Patch against trunk (393126)

and contributor agreement submitted.

By: Dennis Guse (dennis.guse) 2013-07-02 02:40:34.912-0500

Contributor assignment acknowledged.
Patch ported to TRUNK.

By: Richard Mudgett (rmudgett) 2013-07-02 10:50:58.415-0500

Please attach the patch to the issue itself instead of pasting a link to an external entity.  Posting patches to external entities most likely will result in the external entity deleting the post after so many days.
Attach the patch in "diff -u" format by selecting More Actions -> Attach files in the menu above.

By: Dennis Guse (dennis.guse) 2013-07-02 12:45:17.716-0500

Patch is now attached!

By: Dennis Guse (dennis.guse) 2013-07-04 09:33:45.044-0500

Patch attached

By: Rusty Newton (rnewton) 2013-07-09 09:02:37.708-0500

Thanks. Here is the guidelines for future reference. It covers patch submission: https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines

By: Matt Jordan (mjordan) 2013-07-09 09:04:16.955-0500

While it's fine to have a patch on the issue for a released branch - which can be useful for users of Asterisk - since this is an improvement/new feature, it must be written against Asterisk trunk to be included in the next version. Please make sure your patch applies cleanly to trunk and attach it to the issue.

Some [coding guideline|https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines] problems:

# {{const char *codecList;}} => should be {{codec_list}}. Asterisk doesn't use camelCase or CamelCase.
# All {{if}} statements should have brackets. Older portions of the codebase didn't always follow this rule, but new code should.

Minor nitpick:

# Reduce indentation here:
+ ast_getformatbyname(ast_strip(codec), &fmt);
+ if (fmt.id) {
by using logging a NOTICE and {{continue}} if {{fmt.id}} is 0.

By: Dennis Guse (dennis.guse) 2013-07-10 02:50:57.733-0500

Fixed code to comply with coding style guidelines (variables names, if {}).

+ made ast_strip use more clear (Sorry, didnt saw your comment).

+ use continue, if fmt.id == 0

By: Dennis Guse (dennis.guse) 2013-07-10 02:52:45.399-0500

Code adapted to comply with coding style

By: Rusty Newton (rnewton) 2013-07-16 14:12:32.062-0500

You need to hit "Send back" to change the status once you have provided feedback. Thanks!

By: Matt Jordan (mjordan) 2013-07-31 19:43:16.142-0500

Posted to review board at https://reviewboard.asterisk.org/r/2728/