<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [x] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> This PR adds media forking support to the rtpengine module by exposing three functions:
rtpengine_copy_offer_f(): Sends a subscribe request to RTPEngine. rtpengine_copy_answer_f(): Sends a subscribe answer to RTPEngine. rtpengine_copy_delete_f(): Sends an unsubscribe request to RTPEngine. These functions enable interaction with RTPEngine for managing media subscriptions, allowing for scenarios like media duplication or forking. This feature is based on the OpenSIPS RTPEngine module and provides similar functionality for handling media operations. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/4071
-- Commit Summary --
* export subscribe operation functions
-- File Changes --
M src/modules/rtpengine/api.h (20) M src/modules/rtpengine/rtpengine.c (217) M src/modules/rtpengine/rtpengine.h (17) A src/modules/rtpengine/rtpengine_common.h (28)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/4071.patch https://github.com/kamailio/kamailio/pull/4071.diff
@Fr-Soltanzadeh pushed 1 commit.
e8b5c175aa2c2dd6385e04ba9c69c99164a05ab3 rtpengine: export subscribe operation functions
@rfuchs commented on this pull request.
Overall LGTM with some exceptions.
As for style - I would prefer to retain the "subscribe" schema of naming the functions instead of "copy" - took me a while to understand that this refers to making a copy of the media stream.
Also I think it may be worth refactoring `rtpp_function_call` and split it up into two parts, one of them taking an existing `dict` as argument, to avoid having the boilerplate `if(!extra_dict)` in the code. But that's not important and can be done later on separately.
- if(sess->callid)
+ bencode_dictionary_add_str(dict, "call-id", sess->callid); + else if(sess->msg) + bencode_dictionary_add_str(dict, "call-id", &sess->msg->callid->body); + if(sess->branch != RTPENGINE_ALL_BRANCHES) + bencode_dictionary_add_str(dict, "via-branch", viabranch); + if(to_tag && to_tag->len) + bencode_dictionary_add_str(dict, "to-tag", to_tag); + if(copy_flags & RTP_COPY_MODE_SIPREC) { + list = bencode_list(&bencbuf); + bencode_dictionary_add(dict, "flags", list); + bencode_list_add_string(list, "all"); + bencode_list_add_string(list, "siprec"); + } else if((copy_flags & RTP_COPY_LEG_BOTH) == RTP_COPY_LEG_BOTH) { + list = bencode_list(&bencbuf); + bencode_list_add_string(list, "all");
The `list` needs to be added into the dict as `flags` no?
- if(sess->branch != RTPENGINE_ALL_BRANCHES)
+ bencode_dictionary_add_str(dict, "via-branch", viabranch); + if(to_tag && to_tag->len) + bencode_dictionary_add_str(dict, "to-tag", to_tag); + if(copy_flags & RTP_COPY_MODE_SIPREC) { + list = bencode_list(&bencbuf); + bencode_dictionary_add(dict, "flags", list); + bencode_list_add_string(list, "all"); + bencode_list_add_string(list, "siprec"); + } else if((copy_flags & RTP_COPY_LEG_BOTH) == RTP_COPY_LEG_BOTH) { + list = bencode_list(&bencbuf); + bencode_list_add_string(list, "all"); + } else if(copy_flags & RTP_COPY_LEG_CALLER && sess->from_tag) { + bencode_dictionary_add_str(dict, "from-tag", sess->from_tag); + } else if(sess->to_tag) { + bencode_dictionary_add_str(dict, "from-tag", sess->to_tag);
The to-tag should be `to-tag` ?
@@ -3323,9 +3341,18 @@ static bencode_item_t *rtpp_function_call(bencode_buffer_t *bencbuf,
}
/* initialize some basic bencode items */ - ng_flags.dict = bencode_dictionary(bencbuf); + if(!extra_dict) { + ng_flags.dict = bencode_dictionary(bencbuf); + if(parse_by_module) { + ng_flags.flags = bencode_list(bencbuf); + } + } else { + ng_flags.dict = extra_dict; + ng_flags.flags = bencode_dictionary_get(ng_flags.dict, "flags");
Do you really need to create the `flags` here? Aren't the calling functions creating it?
@Fr-Soltanzadeh pushed 1 commit.
5f5c206e2e465ce2e4184364c8620323c7eab3ef rtpengine: export subscribe operation functions
@Fr-Soltanzadeh commented on this pull request.
- if(sess->callid)
+ bencode_dictionary_add_str(dict, "call-id", sess->callid); + else if(sess->msg) + bencode_dictionary_add_str(dict, "call-id", &sess->msg->callid->body); + if(sess->branch != RTPENGINE_ALL_BRANCHES) + bencode_dictionary_add_str(dict, "via-branch", viabranch); + if(to_tag && to_tag->len) + bencode_dictionary_add_str(dict, "to-tag", to_tag); + if(copy_flags & RTP_COPY_MODE_SIPREC) { + list = bencode_list(&bencbuf); + bencode_dictionary_add(dict, "flags", list); + bencode_list_add_string(list, "all"); + bencode_list_add_string(list, "siprec"); + } else if((copy_flags & RTP_COPY_LEG_BOTH) == RTP_COPY_LEG_BOTH) { + list = bencode_list(&bencbuf); + bencode_list_add_string(list, "all");
Yes, that's correct. Resolved.
@Fr-Soltanzadeh commented on this pull request.
- if(sess->branch != RTPENGINE_ALL_BRANCHES)
+ bencode_dictionary_add_str(dict, "via-branch", viabranch); + if(to_tag && to_tag->len) + bencode_dictionary_add_str(dict, "to-tag", to_tag); + if(copy_flags & RTP_COPY_MODE_SIPREC) { + list = bencode_list(&bencbuf); + bencode_dictionary_add(dict, "flags", list); + bencode_list_add_string(list, "all"); + bencode_list_add_string(list, "siprec"); + } else if((copy_flags & RTP_COPY_LEG_BOTH) == RTP_COPY_LEG_BOTH) { + list = bencode_list(&bencbuf); + bencode_list_add_string(list, "all"); + } else if(copy_flags & RTP_COPY_LEG_CALLER && sess->from_tag) { + bencode_dictionary_add_str(dict, "from-tag", sess->from_tag); + } else if(sess->to_tag) { + bencode_dictionary_add_str(dict, "from-tag", sess->to_tag);
If you mean that it should be like: ``` } else if(sess->to_tag) { bencode_dictionary_add_str(dict, "to-tag", sess->to_tag); ``` I don't think so. In the case that any of RTP_COPY_LEG_BOTH, RTP_COPY_MODE_SIPREC and RTP_COPY_LEG_CALLER flags are not in copy_flags, it means that it is copying rtp stream of the callee. The 'from-tag' for RTPEngine indicates which participant stream must be copied and here we want to copy callee's stream which is in sess->to_tag variable. 'to-tag' for RTPEngine means where to send copied rtp streams.
@Fr-Soltanzadeh pushed 1 commit.
b63d8d1badab990b5f3af42c61eedff701da74df rtpengine: export subscribe operation functions
@Fr-Soltanzadeh commented on this pull request.
@@ -3323,9 +3341,18 @@ static bencode_item_t *rtpp_function_call(bencode_buffer_t *bencbuf,
}
/* initialize some basic bencode items */ - ng_flags.dict = bencode_dictionary(bencbuf); + if(!extra_dict) { + ng_flags.dict = bencode_dictionary(bencbuf); + if(parse_by_module) { + ng_flags.flags = bencode_list(bencbuf); + } + } else { + ng_flags.dict = extra_dict; + ng_flags.flags = bencode_dictionary_get(ng_flags.dict, "flags");
Yes I need. Before this change, it was like: ``` if(parse_by_module) { ng_flags.flags = bencode_list(bencbuf); ng_flags.received_from = bencode_list(bencbuf); } ``` but now I have some flags in extra_dict, so I need to create flags in ng_flags from flags in extra_dict.
@Fr-Soltanzadeh pushed 1 commit.
95fd0134d97c5ee8b3cba8ed8861412697425998 rtpengine: export subscribe operation functions
Overall LGTM with some exceptions.
As for style - I would prefer to retain the "subscribe" schema of naming the functions instead of "copy" - took me a while to understand that this refers to making a copy of the media stream.
Thanks for the feedback, I’ve updated the function names to follow the "subscribe" schema, as you suggested.
@Fr-Soltanzadeh pushed 1 commit.
33faf97844ec372c22bf8801a256c49cc278bca9 rtpengine: export subscribe operation functions
@Fr-Soltanzadeh pushed 1 commit.
b33ec805d9e2e339a92711329b396a181127fb94 rtpengine: export subscribe operation functions
Thanks for the update. Aparently there is some format related error, can you investigate and force-push again?
@Fr-Soltanzadeh pushed 1 commit.
628a03f5b5f9edf442410674465bf80ac371e7ce rtpengine: export subscribe operation functions
Thanks. Can you also add some module documentation (in XML format) to this PR? If I understood the previous discussion correctly, the cfg functions should be probably also named ..subscribe.. instead of ..copy.. You don't need to add the README to the PR, it will created later automatically.
Thanks. Can you also add some module documentation (in XML format) to this PR? If I understood the previous discussion correctly, the cfg functions should be probably also named ..subscribe.. instead of ..copy.. You don't need to add the README to the PR, it will created later automatically.
The exported functions are intended to be used by other modules, rather than being exposed in the config file. Therefore, it seems that they may not need to be included in the module documentation at this stage. In a future PR, we can export these functions to the configuration file and update the documentation accordingly.
Thanks for the clarification, I thought there were already supposed to be used in the cfg (did not reviewed the code in detail yet). Ok, then lets wait for eventual more comments from @rfuchs and then merge it in the next days.
@rfuchs approved this pull request.
Merged #4071 into master.
-
Farzaneh Soltanzadeh -
Henning Westerholt
-
Richard Fuchs