17 Aug
2024
17 Aug
'24
3:27 a.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 -->
- [* ] Commit message has the format required by CONTRIBUTING guide
- [* ] Commits are split per component (core, individual modules, libs, utils, ...)
- [* ] Each component has a single commit (if not, squash them into one commit)
- [* ] 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)
- [* ] 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 -->
- [ ] PR should be backported to stable branches
- [ ] Tested changes locally
- [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
<!-- Describe your changes in detail -->
Rtpengine module doesn't export api to be used by other modules. Api for four of
module functions are added.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3948
-- Commit Summary --
* export api in rtpengine module
* Merge branch 'master' into Feature/1-rtpengine-export-api-in-module
-- File Changes --
A src/modules/rtpengine/api.h (65)
M src/modules/rtpengine/rtpengine.c (22)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3948.patch
https://github.com/kamailio/kamailio/pull/3948.diff
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/3948(a)github.com&gt;
17 Aug
17 Aug
3:56 a.m.
Thanks for the PR, good addition. Out of curiosity, which modules do you anticipate will
use the rtpengine API?
Regarding the PR, please fix the clang format checks by running clang-format on your
files. I have also one question about the rtpengine_load_api(..) function, it should be
placed in the module actually using the rtpengine API, and not in the module exporting the
API, right?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2294711186
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2294711186(a)github.com>
5:43 a.m.
@Fr-Soltanzadeh pushed 1 commit.
ccfbf1324398ae1cbc3ebb6542eb0a5ae0a03cfc fix clang format error
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/3948/files/a02e01ae24c31b692e615e…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/3948/before/a02e01ae24c31b692e615e5a3a06d19597a9a2fa/after/ccfbf1324398ae1cbc3ebb6542eb0a5ae0a03cfc(a)github.com>
6:55 a.m.
@Fr-Soltanzadeh pushed 1 commit.
c9dea2ef47208d74b90b74739baf7befc323e78f fix clang format error
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/3948/files/ccfbf1324398ae1cbc3ebb…
You are receiving this because you are subscribed to this thread.
Message ID:
<kamailio/kamailio/pull/3948/before/ccfbf1324398ae1cbc3ebb6542eb0a5ae0a03cfc/after/c9dea2ef47208d74b90b74739baf7befc323e78f(a)github.com>
8:16 a.m.
Thanks for the PR, good addition. Out of curiosity,
which modules do you anticipate will use the rtpengine API?
I want to implement ```siprec``` module and I need some of ```rtpengine``` module
functionalities.
Regarding the PR, please fix the clang format checks by running clang-format on your
files. I have also one question about the rtpengine_load_api(..) function, it should be
placed in the module actually using the rtpengine API, and not in the module exporting the
API, right?
As in:
https://github.com/kamailio/kamailio/blob/f5f7c913d4f008b9937677ae2c2d66baa…
```sanity_load_api``` function is defined in ```sanity``` module and is called in modules
using the apis.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2294830125
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2294830125(a)github.com>
9:52 a.m.
As I understood from your message, you are going to implement a new module called siprec
for audio recording by rtpengine capabilities in Kamailio. Is it right?
If yes, so your implementation will have a close dependency on rtpengine module.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2294851416
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2294851416(a)github.com>
9:52 a.m.
Regarding the
PR, please fix the clang format checks by running clang-format on your files. I have also
one question about the rtpengine_load_api(..) function, it should be placed in the module
actually using the rtpengine API, and not in the module exporting the API, right?
As in:
https://github.com/kamailio/kamailio/blob/f5f7c913d4f008b9937677ae2c2d66baa…
`sanity_load_api` function is defined in `sanity` module and is called in modules using
the apis.
18 Aug
18 Aug
6:54 a.m.
As I understood from your message, you are going to
implement a new module called siprec for audio recording by rtpengine capabilities in
Kamailio. Is it right? If yes, so your implementation will have a close dependency on
rtpengine module.
Yes, the new module will have a close dependency on it.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2295201024
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2295201024(a)github.com>
7:48 a.m.
In my opinion, consider the considerations of strong module dependencies in the design.
For example, you have used the capabilities of another module in your code and called it
recently, while it might be better to design and implement this capability in the new
module itself.
Another important point is that this heavy dependency can even reduce the performance of
the Rrtpengine module in normal mode. Suppose you have several rtpengine nodes separately
from the Kamailio server. In this case, you will not receive the media packets on the
Kamailio server itself. How does the new module want to access the packets on the other
server? I think each nodes separately performs the recording operation and saves it to its
own file system.
However, this development can be a good contributions for Kamailio.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2295216432
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2295216432(a)github.com>
8:07 a.m.
In my opinion, consider the considerations of strong
module dependencies in the design. For example, you have used the capabilities of another
module in your code and called it recently, while it might be better to design and
implement this capability in the new module itself. Another important point is that this
heavy dependency can even reduce the performance of the Rrtpengine module in normal mode.
Suppose you have several rtpengine nodes separately from the Kamailio server. In this
case, you will not receive the media packets on the Kamailio server itself. How does the
new module want to access the packets on the other server? I think each nodes separately
performs the recording operation and saves it to its own file system. However, this
development can be a good contributions for Kamailio.
Its a bit difficult to comment on the performance differences of different architectural
approaches without seeing it implemented and running somewhere. Regarding the rtpengine
call recording, this is indeed executed on the individual nodes where the rtpengine is
executed.
If the mentioned new module needs to communicate with the rtpengine, its is in my opinion
a good design choice to use the existing module and not duplicating the same code again on
its own. I don't think this will cause any issue for the performance of the rtpengine
module (for its own usage), if the communication path from the Kamailio server to the
rtpengine daemons is working fine.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2295221499
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2295221499(a)github.com>
10:46 a.m.
In my opinion, consider the considerations of strong
module dependencies in the design. For example, you have used the capabilities of another
module in your code and called it recently, while it might be better to design and
implement this capability in the new module itself. Another important point is that this
heavy dependency can even reduce the performance of the Rrtpengine module in normal mode.
Suppose you have several rtpengine nodes separately from the Kamailio server. In this
case, you will not receive the media packets on the Kamailio server itself. How does the
new module want to access the packets on the other server? I think each nodes separately
performs the recording operation and saves it to its own file system. However, this
development can be a good contributions for Kamailio.
Thank you for the feedback. I agree with @henningw, to address your concerns:
1) Re-implementing rtpengine functionality in the new module would result in redundant
code, which isn't ideal. Leveraging existing modules promotes maintainability.
2) Similar approaches, like SIPREC in OpenSIPS, use existing RTP related modules
effectively.
3) Regarding performance concerns, the module assumes recording is handled by each node
individually, minimizing any impact on Kamailio.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2295269544
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2295269544(a)github.com>
19 Aug
19 Aug
5:42 a.m.
I see, but SIPREC in Opensip is a general approach and uses the SIPREC protocol. Is your
approach the same?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296006175
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296006175(a)github.com>
6:02 a.m.
In my opinion,
consider the considerations of strong module dependencies in the design. For example, you
have used the capabilities of another module in your code and called it recently, while it
might be better to design and implement this capability in the new module itself. Another
important point is that this heavy dependency can even reduce the performance of the
Rrtpengine module in normal mode. Suppose you have several rtpengine nodes separately from
the Kamailio server. In this case, you will not receive the media packets on the Kamailio
server itself. How does the new module want to access the packets on the other server? I
think each nodes separately performs the recording operation and saves it to its own file
system. However, this development can be a good contributions for Kamailio.
Its a bit difficult to comment on the performance differences of different architectural
approaches without seeing it implemented and running somewhere. Regarding the rtpengine
call recording, this is indeed executed on the individual nodes where the rtpengine is
executed.
If the mentioned new module needs to communicate with the rtpengine, its is in my opinion
a good design choice to use the existing module and not duplicating the same code again on
its own. I don't think this will cause any issue for the performance of the rtpengine
module (for its own usage), if the communication path from the Kamailio server to the
rtpengine daemons is working fine.
6:05 a.m.
Unfortunately there are still clang-format errors in the PR. If you are not able to solve
them let us know, it can be also merged manually.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296055216
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296055216(a)github.com>
7:06 a.m.
I see, but SIPREC in Opensip is a general approach and
uses the SIPREC protocol. Is your approach the same?
Not exactly the same but yes.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296182344
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296182344(a)github.com>
7:20 a.m.
Unfortunately there are still clang-format errors in
the PR. If you are not able to solve them let us know, it can be also merged manually.
It seems that the problem is that checking clang-format is commit by commit and since my
first commit had clang-format error, the problem was still there. Now I squashed all my
last commits(include merging with master) and hope to be effective.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296222476
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296222476(a)github.com>
7:25 a.m.
sorry but, can you please rebase instead of merge? We don't want any merge commit in
PR
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296232597
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296232597(a)github.com>
8:08 a.m.
sorry but, can you please rebase instead of merge? We
don't want any merge commit in PR
Yes, done, thank you.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296312645
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296312645(a)github.com>
8:40 a.m.
Checks are all fine now, thanks. Lets wait for a bit more time for eventual feedback from
@rfuchs, otherwise it will be merged soon.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296367557
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296367557(a)github.com>
8:58 a.m.
No objections from my POV
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296400629
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296400629(a)github.com>
9:33 a.m.
Thanks, it was manually merged (due to the commit message not conforming to the standard)
in commit 3d002d561e82b8bb.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296465727
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296465727(a)github.com>
9:33 a.m.
Closed #3948.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#event-13929747287
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/issue_event/13929747287(a)github.com>
11:45 a.m.
I think this PR is completely wrong as it was done, because the API exports the functions
for kamailio.cfg, which take pseudo-variables in parameters that are fixed-up at startup,
thus at runtime the parameters are expected to be pv-string-parsed pointer, not pointer to
a string value. I expect crashes on minimal tests as it is done right now. Or the user of
the API functions, as they were done, have to call the corresponding fixup functions and
provide the result to the functions, afterwards also free the fixed-up result (which are
not set right now), otherwise will be memory leaks.
Probably the API should export the corresponding kemi functions, which work directly with
`str*` parameters.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296756994
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296756994(a)github.com>
1:32 p.m.
Reopened #3948.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#event-13933037948
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/issue_event/13933037948(a)github.com>
1:32 p.m.
Yes, you are right, I and also others missed it unfortunately. The author probably did
only minimal tests. @Fr-Soltanzadeh can you create a new PR that changes the API to use
the KEMI functions as suggested? If there are more questions, let us know.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2296978539
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2296978539(a)github.com>
26 Aug
26 Aug
12:26 p.m.
Yes. That was my mistake. I fixed it in a new PR:
https://github.com/kamailio/kamailio/pull/3956
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2310482101
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2310482101(a)github.com>
3 Sep
3 Sep
3:07 a.m.
As PR https://github.com/kamailio/kamailio/pull/3956 has been merged, please close this
PR.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#issuecomment-2325673614
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/c2325673614(a)github.com>
3:07 a.m.
Closed #3948.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3948#event-14109652428
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3948/issue_event/14109652428(a)github.com>
16
days inactive
33
days old
27 comments
6 participants
participants (6)
-
Daniel-Constantin Mierla -
Farzaneh Soltanzadeh
-
Henning Westerholt
-
Mojtaba Esfandiari.S
-
Richard Fuchs
-
Victor Seva