Summary:	ASTERISK-27959: [patch] Asterisk 15.4.1 h264 fmtp negotiation problem
Reporter:	David Kuehling (dvdkhlng)	Labels:
Date Opened:	2018-07-10 21:51:39	Date Closed:	2018-12-17 08:27:42.000-0600
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Resources/res_format_attr_h264
Versions:	15.4.1	Frequency of Occurrence	Constant
Related Issues:
Environment:	Debian Stretch amd64	Attachments:
Description:	Hi, I'm trying to have a bunch of GXV3240, GXV3275 and GXV3140 telephones perform video-telephony via Asterisk.  The Asterisk 13 that's shipping with Debian did have severe problems correctly handling h264 fmtp negotiation, so I am currently investigating how much the latest upstream Asterisk release improves on that. Asterisk 15.4.1 is much better, now passing fmtp headers between all those video phones in the SDP.  However, I think there is one problem with how the {{profile-level-id}} attribute is handled: Looking at the logs, I can see that my GXV3240 phone sends a fmtp header for video: {noformat}    a=fmtp:99 profile-level-id=4D0016; packetization-mode=1 {noformat} When Asterisk now relays the request to the target of the call (a GXV3140), the profile-evel-id field gets dropped: {noformat}    a=fmtp:99 packetization-mode=1 {noformat} Which is a problem for some of these phones, as it makes them revert to very basic video codec parameters (the GXV3240 seem to go down to transmit QCIF when called without profile-level-id). Looking at the source code, it is quite obvious what happens here: There is a check in res_format_attr_h264.c that only outputs {{profile-level-id}}, if all its components are non-zero.  Howver, note how the GXV3240 has a zero for PROFILE_IOP (the middle byte): {code:java}    if (attr->PROFILE_IDC && attr->PROFILE_IOP && attr->LEVEL) { {code} To my untrained eye it looks like this should read {code:java}    if (attr->PROFILE_IDC || attr->PROFILE_IOP || attr->LEVEL) { {code} And patching asterisk accordingly makes it correctly pass the {{profile-level-id}}. Other phones (like the GVX3140) do not transmit any zeros in {{profile-level-id}}, so are not affected by the problem. However, note that I don't know anything about how fmtp negotiation is defined in the standard, so maybe my naive interpretation misses something here. cheers, David PS: will attach patch below
Comments:	By: Asterisk Team (asteriskteam) 2018-07-10 21:51:41.289-0500 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. By: David Kuehling (dvdkhlng) 2018-07-10 21:53:25.750-0500 single-line change that fixed the issue for me By: Kevin Harwell (kharwell) 2018-07-11 16:23:06.644-0500 [~dvdkhlng], thanks for the detailed report and diagnosis. Unfortunately we cannot accept patches without a [license agreement|https://wiki.asterisk.org/wiki/display/AST/Digium+License+Agreement] (thus why your patch was removed). If you'd like to submit a patch a [Digium License Agreement|https://issues.asterisk.org/jira/secure/DigiumLicense.jspa] will first need to be signed. If you do choose to go that route, and you'd like your contribution to be included faster, you should submit your patch for code review by the Asterisk Developer Community. To do so, please follow the Code Review [1] instructions on the wiki. Be sure to: * Verify that your patch conforms to the Coding Guidelines [2] * Review the Code Review Checklist [3] for common items reviewers will look for * If necessary, provide tests for the Asterisk Test Suite that verify the correctness of your patch [4] When ready, submit your patch and any tests to Gerrit [5] for code review. Thanks! [1] https://wiki.asterisk.org/wiki/display/AST/Code+Review [2] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines [3] https://wiki.asterisk.org/wiki/display/AST/Code+Review+Checklist [4] https://wiki.asterisk.org/wiki/display/AST/Asterisk+Test+Suite+Documentation [5] https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage By: Friendly Automation (friendly-automation) 2018-12-17 08:27:43.606-0600 Change 10812 merged by Friendly Automation: res_format_attr_h264.c: Make sure profile-level-id fmtp attribute is set [https://gerrit.asterisk.org/10812|https://gerrit.asterisk.org/10812] By: Friendly Automation (friendly-automation) 2018-12-17 08:28:48.363-0600 Change 10813 merged by Friendly Automation: res_format_attr_h264.c: Make sure profile-level-id fmtp attribute is set [https://gerrit.asterisk.org/10813|https://gerrit.asterisk.org/10813] By: Friendly Automation (friendly-automation) 2018-12-17 09:36:01.551-0600 Change 10814 merged by Joshua C. Colp: res_format_attr_h264.c: Make sure profile-level-id fmtp attribute is set [https://gerrit.asterisk.org/10814|https://gerrit.asterisk.org/10814]