Add callback to notify succesfull completion of conf/vid_conf operation#4446
Add callback to notify succesfull completion of conf/vid_conf operation#4446trengginas merged 8 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces asynchronous callbacks to notify the completion of audio and video conference operations, addressing potential staleness in port information due to asynchronous execution. Key changes include:
- Adding callback methods (on_conf_op_completed and on_vid_conf_op_completed) in Endpoint.
- Setting up the corresponding callback functions in both audio and video conference subsystems.
- Updating documentation and API comments to indicate that operations now execute asynchronously.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pjsip/src/pjsua2/endpoint.cpp | Added conversion methods and callback handlers for conf operations. |
| pjsip/include/pjsua2/endpoint.hpp | Introduced callback parameter structures and virtual callbacks. |
| pjsip/src/pjsua-lib/pjsua_vid.c | Added setter for video conference operation callback. |
| pjsip/src/pjsua-lib/pjsua_aud.c | Added setter for audio conference operation callback. |
| pjsip/include/pjsua-lib/pjsua.h | Updated documentation to indicate asynchronous behavior. |
| pjmedia/src/pjmedia/vid_conf.c | Updated operation dispatch and callback invocation in video conf. |
| pjmedia/src/pjmedia/conference.c | Updated operation dispatch and callback invocation in audio conf. |
| pjmedia/include/pjmedia/vid_conf.h | Defined types and documentation for video conference operations. |
| pjmedia/include/pjmedia/conference.h | Defined types and documentation for audio conference operations. |
| pjsip-apps/src/pjsua/pjsua_app.c | Added commented examples of using the new callbacks. |
Comments suppressed due to low confidence (1)
pjsip/include/pjsua-lib/pjsua.h:8093
- The term 'succesfull' is misspelled; consider correcting it to 'successful'.
* This operation executes asynchronously, use the callback set from \a on_conf_op_completed to receive notification upon succesfull
pjmedia/src/pjmedia/vid_conf.c
Outdated
| pjmedia_vid_conf_op_info info = { 0 }; | ||
|
|
||
| pj_log_push_indent(); | ||
| info.op_type = PJMEDIA_VID_CONF_OP_DISCONNECT_PORTS; |
There was a problem hiding this comment.
In the op_update_port function, the callback is reporting a disconnect operation type instead of an update; change it to PJMEDIA_VID_CONF_OP_UPDATE_PORT to correctly indicate an 'update port' operation.
| info.op_type = PJMEDIA_VID_CONF_OP_DISCONNECT_PORTS; | |
| info.op_type = PJMEDIA_VID_CONF_OP_UPDATE_PORT; |
| pj_mutex_unlock(conf->mutex); | ||
|
|
||
| /* Process op */ | ||
| switch(type) { |
There was a problem hiding this comment.
Why not invoke the callbacks from this function instead? E.g: after the switch and just copy the op type & op param to info, and as mentioned before, adding status/result into the info may be better.
There was a problem hiding this comment.
Some operations are called from other method. e.g.: op_disconnect_ports() are called from op_remove_port() or op_remove_port() from pjmedia_conf_destroy().
There was a problem hiding this comment.
If I understand this correctly, then this callback is called for operations that are explicitly invoked by users.
Operation that is not explicitly called will not get notified by this callback.
There was a problem hiding this comment.
Yes, no callback there. Or just add some callback invocations in conf_destroy(). Either way sounds okay to me, also better to mention this in the docs.
pjsip/include/pjsua2/endpoint.hpp
Outdated
| * (e.g., startTransmit/stopTransmit), the first AudioMedia serves | ||
| * as the source. | ||
| */ | ||
| AudioMediaVector2 opData; |
There was a problem hiding this comment.
Not very sure about a couple things here:
- using vector, perhaps simply leave it as port ids? Note: app can use
AudioMediaHelperto cast it to AudioMedia (better also mention this in this docs perhaps) and there may be also port number-1. - using term
Confin the naming (perhaps this is the first in pjsua2?), what about usingMediainstead? In pjsua2, the focus tends to shift from Conf to Media/port (seeadjustRx/TxLevelvspjmedia_conf_adjust_rx_leveldocs).
There was a problem hiding this comment.
It is no longer AudioMediaVector but still a vector, unsigned vector :)
The param is not an array of a specific type, what if in the future there is a new field that is not an int/unsigned.
If union is a problem (in SWIG perhaps), IMO, using a flat struct/class is better than a vector, e.g:
struct OpParam {
unsigned mediaId;
unsigned targetMediaId; /* Only applicable for op: ... */
int adjustLevel; /* Only applicable for op: ... */
};
There was a problem hiding this comment.
When using union is chosen, please make sure it works with SWIG (java, python, C#).
There was a problem hiding this comment.
actually MediaEventData is using union (ref). Tested here using java, and it works
…on (pjsip#4446) * Add callback to notify succesfull completion of conf/vid_conf operation * Fix incorrect op_type * Modification based on comments * Missed to set status * modification based on comments * modification based on comments * Modification based on comments * Add comments and call the callback from conf_destroy()
Since #3928, conference/video conference is done asynchronously.
The changes introduce other implications, consider the scenario:
pjsua_conf_connect())pjsua_conf_get_port_info()).After the patch, the information from (
pjsua_conf_get_port_info()) might not be the latest, since the operation is notcompleted.
This PR will add a callback to notify when the operation is completed.