16 Dec
2024
16 Dec
'24
4:18 p.m.
<!-- 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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/4071(a)github.com&gt;
16 Dec
16 Dec
8:14 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@Fr-Soltanzadeh pushed 1 commit.
e8b5c175aa2c2dd6385e04ba9c69c99164a05ab3 rtpengine: export subscribe operation functions
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4071/files/300c68b06c768fab3d3171…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4071/before/300c68b06c768fab3d3171a1918068482653933d/after/e8b5c175aa2c2dd6385e04ba9c69c99164a05ab3(a)github.com>
17 Dec
17 Dec
12:06 a.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@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?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#pullrequestreview-2506057003
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/review/2506057003(a)github.com>
4:59 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@Fr-Soltanzadeh pushed 1 commit.
5f5c206e2e465ce2e4184364c8620323c7eab3ef rtpengine: export subscribe operation functions
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4071/files/e8b5c175aa2c2dd6385e04…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4071/before/e8b5c175aa2c2dd6385e04ba9c69c99164a05ab3/after/5f5c206e2e465ce2e4184364c8620323c7eab3ef(a)github.com>
5:08 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#discussion_r1887948047
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/review/2507927376(a)github.com>
5:22 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#discussion_r1887958646
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/review/2507944418(a)github.com>
5:26 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@Fr-Soltanzadeh pushed 1 commit.
b63d8d1badab990b5f3af42c61eedff701da74df rtpengine: export subscribe operation functions
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4071/files/5f5c206e2e465ce2e41843…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4071/before/5f5c206e2e465ce2e4184364c8620323c7eab3ef/after/b63d8d1badab990b5f3af42c61eedff701da74df(a)github.com>
5:36 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#discussion_r1887970342
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/review/2507963010(a)github.com>
8:50 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@Fr-Soltanzadeh pushed 1 commit.
95fd0134d97c5ee8b3cba8ed8861412697425998 rtpengine: export subscribe operation functions
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4071/files/b63d8d1badab990b5f3af4…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4071/before/b63d8d1badab990b5f3af42c61eedff701da74df/after/95fd0134d97c5ee8b3cba8ed8861412697425998(a)github.com>
9:16 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#issuecomment-2548054542
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/c2548054542(a)github.com>
9:23 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@Fr-Soltanzadeh pushed 1 commit.
33faf97844ec372c22bf8801a256c49cc278bca9 rtpengine: export subscribe operation functions
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4071/files/95fd0134d97c5ee8b3cba8…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4071/before/95fd0134d97c5ee8b3cba8ed8861412697425998/after/33faf97844ec372c22bf8801a256c49cc278bca9(a)github.com>
9:51 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@Fr-Soltanzadeh pushed 1 commit.
b33ec805d9e2e339a92711329b396a181127fb94 rtpengine: export subscribe operation functions
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4071/files/33faf97844ec372c22bf88…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4071/before/33faf97844ec372c22bf8801a256c49cc278bca9/after/b33ec805d9e2e339a92711329b396a181127fb94(a)github.com>
9:55 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
Thanks for the update. Aparently there is some format related error, can you investigate
and force-push again?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#issuecomment-2548140270
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/c2548140270(a)github.com>
10:32 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@Fr-Soltanzadeh pushed 1 commit.
628a03f5b5f9edf442410674465bf80ac371e7ce rtpengine: export subscribe operation functions
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4071/files/b33ec805d9e2e339a92711…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/4071/before/b33ec805d9e2e339a92711329b396a181127fb94/after/628a03f5b5f9edf442410674465bf80ac371e7ce(a)github.com>
18 Dec
18 Dec
2:50 a.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#issuecomment-2548825091
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/c2548825091(a)github.com>
4:04 a.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#issuecomment-2549052015
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/c2549052015(a)github.com>
4:11 a.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#issuecomment-2549069482
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/c2549069482(a)github.com>
10:37 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
@rfuchs approved this pull request.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#pullrequestreview-2511576638
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/review/2511576638(a)github.com>
10:38 p.m.
New subject: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)
Merged #4071 into master.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#event-15701966584
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/issue_event/15701966584(a)github.com>
47
days inactive
49
days old
18 comments
3 participants
participants (3)
-
Farzaneh Soltanzadeh -
Henning Westerholt
-
Richard Fuchs