@Fr-Soltanzadeh pushed 1 commit.

ccfbf1324398ae1cbc3ebb6542eb0a5ae0a03cfc fix clang format error

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/f5f7c913d4f008b9937677ae2c2d66baa0... ```sanity_load_api``` function is defined in ```sanity``` module and is called in modules using the apis.

...

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/f5f7c913d4f008b9937677ae2c2d66baa0...

`sanity_load_api` function is defined in `sanity` module 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.

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.

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.

...

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.

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.

sorry but, can you please rebase instead of merge? We don't want any merge commit in PR

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.

As PR https://github.com/kamailio/kamailio/pull/3956 has been merged, please close this PR.