[Asterisk-code-review] codec_negotiation: Implement SDP answer preferences (asterisk[master])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/14328 )

Change subject: codec_negotiation: Implement SDP answer preferences
......................................................................


Patch Set 3:

(3 comments)

https://gerrit.asterisk.org/c/asterisk/+/14328/3/configs/samples/pjsip.conf.sample 
File configs/samples/pjsip.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/14328/3/configs/samples/pjsip.conf.sample@875 
PS3, Line 875:                            ; remote - Include all codecs in the remote list that
             :                            ; are also in the local list preserving remote list
             :                            ; order. (default)
             :                            ; remote_merge - Include all codecs in BOTH lists
             :                            ; preserving the remote list order.  Codes in the
             :                            ; local list not in the remote list will be placed
             :                            ; at the end of the joint list.
             :                            ; remote_first - Include only the first codec in
             :                            ; the remote list.
> The wiki page defines the incoming_call_answer_pref option as having options which take into account the answer from the callee/called party. I don't think this is currently done, or at least I can not see where the information is communicated back and used.

The wording is kinda funny and unfortunately there's not enough room here to fully describe what's going on.  In this case, "remote" is Bob and his incoming answer was already resolved against his pending topology using outgoing_call_answer_pref (apply_negotiated_sdp_stream calls set_caps).  So what's passed to the core is the resolved list.  Alice's answer is created by resolving her pending topology with what came from the core (remote).  

Of course it's always possible I missed something but I've been auditing the process with debug statements.  Now that we can process a round-trip call, more tests are possible.

We'll need more/corrected documentation after the refactor.


https://gerrit.asterisk.org/c/asterisk/+/14328/3/include/asterisk/res_pjsip_session.h 
File include/asterisk/res_pjsip_session.h:

https://gerrit.asterisk.org/c/asterisk/+/14328/3/include/asterisk/res_pjsip_session.h@933 
PS3, Line 933:  * @param session
> We use \ everywhere else, why the use of @ here? Automagicness?

Oops, yeah I've been meaning to fix my Eclipse Doxygen templates for years.  Maybe I'll get to it before I due.  Will fix.


https://gerrit.asterisk.org/c/asterisk/+/14328/3/include/asterisk/res_pjsip_session.h@936 
PS3, Line 936: const char *ast_sip_session_get_name(struct ast_sip_session *session);
> Should the session be const as well?

Good point. Will fix.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/14328
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Iad188ae997bdcb5c28e2eb12c6bb2b732538ad45
Gerrit-Change-Number: 14328
Gerrit-PatchSet: 3
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Tue, 05 May 2020 13:29:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200505/44e802ae/attachment.html>