kamailio
rtpengine: export api
Pre-Submission Checklist
- [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:
- [x] PR should be backported to stable branches
- [x] Tested changes locally
- [ ] Related to issue #XXXX (replace XXXX with an open issue number)
Description
Rtpengine module doesn't export api to be used by other modules. Api for four of module functions are added.
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?
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/f5f7c913d4f008b9937677ae2c2d66baa01dcf0e/src/modules/sanity/api.h#L44
sanity_load_api function is defined in sanity module and is called in modules using the apis.
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.
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/f5f7c913d4f008b9937677ae2c2d66baa01dcf0e/src/modules/sanity/api.h#L44
sanity_load_apifunction is defined insanitymodule and is called in modules using the apis.
You are right, there are different patters on how this API is exposed. Thanks for the fixes regarding the format, but I think unfortunately its still failing. Did you executed in the main kamailio directory the clang-format tool? This is usually the easiest way to fix it.
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.
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.
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.
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:
- Re-implementing rtpengine functionality in the new module would result in redundant code, which isn't ideal. Leveraging existing modules promotes maintainability.
- Similar approaches, like SIPREC in OpenSIPS, use existing RTP related modules effectively.
- Regarding performance concerns, the module assumes recording is handled by each node individually, minimizing any impact on Kamailio.
I see, but SIPREC in Opensip is a general approach and uses the SIPREC protocol. Is your approach the same?
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.
What i mean the strong dependency between the siprec module and the rtpengine daemon service. Using pure code in the siprec module can add these capabilities when you use other rtp engines (like as lrkproxy or rtpproxy) in the solution. However, we have to wait for the work to be done.
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.
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.
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.
sorry but, can you please rebase instead of merge? We don't want any merge commit in PR
sorry but, can you please rebase instead of merge? We don't want any merge commit in PR
Yes, done. Thank you.
Checks are all fine now, thanks. Lets wait for a bit more time for eventual feedback from @rfuchs, otherwise it will be merged soon.
Thanks, it was manually merged (due to the commit message not conforming to the standard) in commit 3d002d561e82b8bb.
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.
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.
Yes. That was my mistake. I fixed it in a new PR: https://github.com/kamailio/kamailio/pull/3956
As PR https://github.com/kamailio/kamailio/pull/3956 has been merged, please close this PR.