Implementation_of_replace_port_to_support_conf_async_port

[PesalaDeSilva]

Signed-off-by: Pesala Silva pesala.silva@ifs.com

Summary

Add optional early conference slot reservation for PJSUA calls (null-port
placeholder) and implement pjmedia_conf_replace_port to swap that slot to
the real stream after SDP/stream setup, without performing the heavy work on
arbitrary threads. Queued operations run on the conference media tick; old
port destruction is deferred for safety.

Motivation

Some integrations need a stable conf_slot ID while the call is still being
set up (e.g. before media is fully active). PJSIP 2.15-era behaviour can defer
slot assignment until channel update; this patch preserves an early slot and
replaces the port in place when audio is ready.

Changes (high level)

• pjmedia: pjmedia_conf_replace_port + implementation, op queue, deferred
  destroy list; small hardening in conf get/put frame paths.
• pjsua: pjsua_call_preallocate_conf_port(); pjsua_aud_channel_update
  uses replace when a placeholder slot exists, with add/remove fallback.

[Copilot]

Pull request overview

Adds an early "placeholder" conference-slot reservation for PJSUA calls so callers can hold a stable conf_slot ID while media setup is still in progress, plus a new pjmedia_conf_replace_port API on the conference bridge that swaps the placeholder for the real stream port via an op queue, with deferred destruction of the old port.

Changes:

• New pjsua API pjsua_call_preallocate_conf_port() and inline placeholder allocation in pjsua_aud_channel_update with a replace_port-then-fallback-to-add_port strategy.
• New pjmedia_conf_replace_port (queued) + pjmedia_conf_replace_port_impl (worker) with an op_list/op_mutex/destroy_list mechanism drained from the master get_frame tick.
• Hardening of get_frame/put_frame against NULL master/port and a renamed mutex string.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.

File	Description
pjmedia/include/pjmedia/conference.h	Adds op-type enum, internal conf_op / port_destroy_item types, static-function declarations, and pjmedia_conf_replace_port[_impl] to the public header.
pjmedia/src/pjmedia/conference.c	Implements queued replace_port, deferred port destroys, op-queue drain in get_frame, NULL guards in get_frame/put_frame; introduces an op_mutex field that is never created.
pjsip/include/pjsua-lib/pjsua.h	Declares the new pjsua_call_preallocate_conf_port API.
pjsip/src/pjsua-lib/pjsua_aud.c	Implements pre-allocation API and uses replace_port from pjsua_aud_channel_update, with placeholder also auto-allocated when no caller opted in.

Comments suppressed due to low confidence (7)

pjmedia/src/pjmedia/conference.c:881

• conf->op_mutex is declared and used (locked/unlocked in pjmedia_conf_replace_port, in get_frame to drain ops, and destroyed in pjmedia_conf_destroy), but it is never created with pj_mutex_create_recursive() / pj_mutex_create_simple() anywhere in pjmedia_conf_create(). With the field left as NULL from PJ_POOL_ZALLOC_T, the first call to pj_mutex_lock(conf->op_mutex) will dereference a NULL pointer and crash. An op_mutex creation step must be added during conference creation (and on failure, pjmedia_conf_destroy must clean up the rest correctly).

    conf->op_list = PJ_POOL_ZALLOC_T(pool, struct conf_op);
    pj_list_init(conf->op_list);
    conf->op_pending = PJ_FALSE;

    //conf->destroy_list = PJ_POOL_ZALLOC_T(pool, struct port_destroy_item);
    pj_list_init(conf->destroy_list);

pjmedia/include/pjmedia/conference.h:361

• These three functions are declared static in a public header. static declarations in a header give each translation unit that includes the header its own (file-local) declaration but no definition, producing "used but never defined" / unused-static-function warnings (which become errors under -Wall -Werror used by this project) and linker problems. These helpers are private to conference.c and should not be declared in the public header at all — move the forward declarations into conference.c. Additionally, conf_handle_op_ is declared twice on lines 358 and 361.

static void conf_handle_op_(pjmedia_conf* conf, struct conf_op* op);
static void conf_defer_port_destroy_(pjmedia_conf* conf, pjmedia_port* p);
static void conf_run_deferred_destroys_(pjmedia_conf* conf);
static void conf_handle_op_(pjmedia_conf* conf, struct conf_op* op);

pjmedia/include/pjmedia/conference.h:580

• pjmedia_conf_replace_port is declared with PJ_DEF in a public header. PJ_DEF is intended for the definition in the .c file (it expands to the export/visibility attributes for the function body); using it on a declaration in a header will produce duplicate-symbol / redefinition issues for every translation unit that includes this header. Use PJ_DECL(pj_status_t) here, matching the declaration of pjmedia_conf_replace_port_impl immediately above and the convention used elsewhere in this header (e.g. pjmedia_conf_add_port).

PJ_DEF(pj_status_t) pjmedia_conf_replace_port(pjmedia_conf *conf,
                                              pj_pool_t *pool,
                                              pjmedia_port *strm_port,
                                              unsigned slot);

pjmedia/include/pjmedia/conference.h:575

• pjmedia_conf_replace_port_impl should not be part of the public API — it is an internal helper executed from the conference op queue. Exposing it via PJ_DECL in conference.h lets external callers invoke it directly on arbitrary threads, bypassing the queue/serialization that the PR is specifically introducing. Make it static inside conference.c and remove this declaration.

PJ_DECL(pj_status_t) pjmedia_conf_replace_port_impl(pjmedia_conf *conf,
                                pj_pool_t *pool,
                                pjmedia_port *strm_port,
                                unsigned            slot);

pjmedia/src/pjmedia/conference.c:846

• The mutex name passed to pj_mutex_create_recursive is being changed from "conf" to "conf_op", but this is still the main conf->mutex (the new op-queue mutex is conf->op_mutex, a separate field that is never created in this PR — see the other comment). Renaming the existing mutex here is misleading for diagnostics and unrelated to the rest of the change. Please revert this rename and instead use "conf_op" as the name of the newly-introduced conf->op_mutex.

    status = pj_mutex_create_recursive(pool, "conf_op", &conf->mutex);

pjsip/src/pjsua-lib/pjsua_aud.c:917

• pjmedia_conf_replace_port is asynchronous — it queues the op and returns PJ_SUCCESS before the swap actually executes on the conference tick. That means this fallback ("replace_port failed, falling back to add_port") can only trigger on early validation failures (bad slot, allocation failure) and will NOT catch any failure inside pjmedia_conf_replace_port_impl (resample create, buffer alloc, channel mismatch, etc.). If the deferred impl later fails the slot is silently left in a broken state and call_med->strm.a.conf_slot still points at the placeholder. Consider passing a result callback via op->cb so the call-media layer can recover (e.g. remove the broken slot and add a fresh one) if the async replace fails.

            if (call_med->strm.a.conf_slot != PJSUA_INVALID_ID) {
                /* Slot pre-allocated with placeholder; replace it with real stream */
                status = pjmedia_conf_replace_port(pjsua_var.mconf,
                    call->inv->pool,
                    call_med->strm.a.media_port,
                    (unsigned)call_med->strm.a.conf_slot);
                if (status != PJ_SUCCESS) {
                    PJ_LOG(2, (THIS_FILE, "replace_port failed, falling back to add_port: %d", status));
                    pjsua_conf_remove_port(call_med->strm.a.conf_slot);
                    call_med->strm.a.conf_slot = PJSUA_INVALID_ID;
                    /* Fall through to add_port */
                }
            }

pjsip/src/pjsua-lib/pjsua_aud.c:815

• This block unconditionally pre-allocates a null placeholder port whenever conf_slot == PJSUA_INVALID_ID, i.e. for every call that did not explicitly call pjsua_call_preallocate_conf_port(). That defeats the original control flow (which only added the real stream port to the conference once) and now creates a null port + adds a slot + immediately attempts to replace it on every audio channel update. This is wasted work and an extra pjmedia_conf_add_port/replace_port pair for callers that never wanted the early-slot feature. The placeholder should only be created when the caller has actually opted in (e.g. only keep the replace_port path when conf_slot was already pre-allocated, and fall through to plain add_port otherwise), as the surrounding comment in pjsua_call_preallocate_conf_port already implies.

        /* Pre-allocate conference slot with null port for PJSIP 2.15 async conf.
        * replace_port will swap it with the real stream later, keeping the same
        * slot so switch connections (e.g. 2->4) route real audio.
        */
        if (call_med->strm.a.conf_slot == PJSUA_INVALID_ID) {
            pjmedia_port* null_port = NULL;
            unsigned null_slot;
            pj_status_t pre_status;
            pj_str_t null_name = pj_str("call-null-placeholder");
            unsigned clock_rate = si->fmt.clock_rate;
            unsigned channel_count = si->fmt.channel_cnt;
            unsigned samples_per_frame = PJMEDIA_SPF(clock_rate, 20000, channel_count);

            pre_status = pjmedia_null_port_create(
                call->inv->pool,
                clock_rate,
                channel_count,
                samples_per_frame,
                16,
                &null_port);
            if (pre_status == PJ_SUCCESS) {
                pre_status = pjmedia_conf_add_port(pjsua_var.mconf,
                    call->inv->pool,
                    null_port,
                    &null_name,
                    &null_slot);
                if (pre_status == PJ_SUCCESS) {
                    call_med->strm.a.conf_slot = (int)null_slot;
                }
                else {
                    pjmedia_port_destroy(null_port);
                }
            }
        }

[sauwming]

@PesalaDeSilva , we have replied via email for discussions and feedback regarding this PR, so please check it as well

[PesalaDeSilva]

Hi @sauwming,
thank you for the feedback. I have checked the email and addressed all the Copilot review comments on the PR. Here is a summary of what was fixed:

Bug fixes:

• _c_onf->op_mutex was never created — added pj_mutex_create_recursive() for it in pjmedia_conf_create()
• conf->mutex name was incorrectly renamed to "conf_op" — reverted to "conf"; the new conf->op_mutex now correctly uses "conf_op"
• Stray #endif after put_frame() — removed
• tx_buf was allocated from caller's pool instead of slot_pool — fixed to use slot_pool
• destroy_list was allocated without pj_list_init nearby — alloc and init now happen together
• PJSUA_LOCK held across pjmedia_conf_add_port / pjmedia_conf_replace_port (3 places) — fixed with unlock/work/relock_ pattern

Design fix:

• Removed the unconditional null-port pre-allocation inside pjsua_aud_channel_update — placeholder is now only created when the caller explicitly opts in via pjsua_call_preallocate_conf_port()

Code quality:

• Internal types (conf_op, port_destroy_item, enum, callback typedef) moved out of the public header into conference.c
• Static function declarations removed from the public header
• Debug trace logs (pj_thread_this, LOCKED/UNLOCKED/START messages) removed
• Doxygen /** block with @param and @return added for pjsua_call_preallocate_conf_port in pjsua.h

Style:

• Pointer style, brace placement, // → /* */ comments, and typos ("embeded", "detatching") all corrected

Noted / intentionally deferred:

• The async replace_port fallback limitation: on deferred failure, on_error_unlock safely restores the null placeholder to the slot (no crash, no dangling pointer — audio is silence). Full async recovery via op->cb is acknowledged as a future improvement and documented in a code comment.

Please let me know if anything else needs to be addressed.

I wanted to confirm — I believe I have not missed any email regarding this PR. If there was a specific email with additional feedback or discussion points that I may have overlooked, could you please resend it or summarize the key points here on the PR? I want to make sure I have addressed everything correctly.

[sauwming]

Here is @nanangizz's feedback sent via email dated 26 May:
"
There seem to be several issues with the PR as described in the
copilot/AI review and some are quite major (e.g: rewriting the
async-op mechanism, exposing internal conference types in the public
header). Some approaches could also be improved, for example,
using a null-port placeholder causes unnecessary recreation of
objects (resampler, null-port itself, etc) on every renegotiation.

We understand the feature is important for your integration, and we
think it can be useful to other apps as well. So we are considering
implementing the feature ourselves. The outline:

• Configurable in account/call settings, replacing
  pjsua_call_preallocate_conf_port(), so no pre-created placeholder
  port is needed. Slot ID and all per-slot state (connections, mute,
  adj_level, etc.) survive renegotiation. Default off, to avoid
  backward-compatibility issues.
• New pjmedia_conf_detach_port() which mainly sets conf_port->port
  to NULL, and pjmedia_conf_replace_port() to re-attach the slot to a
  new port.
• We'll cover both the conference and parallel-conference backends,
  video conference can be a follow-up.
  "

So yes, there are still significant missing gaps.