Repository navigation
Support app managed SDP for signalling-plane B2BUA - #5255
wosrediinanatour wants to merge 1 commit into
Conversation
6867b06 to
e8c8f24
Compare
|
PTAL, @sauwming, @nanangizz, or @copilot ;-) |
|
@wosrediinanatour |
There was a problem hiding this comment.
🟡 Changes recommended
Later negotiation rounds still rewrite SDP before passthrough promotion, and flag rollback and Symbian export issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds SDP passthrough mode for signaling-only B2BUA/relay applications.
Changes:
- Adds passthrough negotiation APIs and PJSUA/PJSUA2 flags.
- Exposes the mode through
pjsuaCLI configuration. - Adds unit, SIPp integration, and CI coverage.
File summaries
| File | Description |
|---|---|
tests/pjsua/scripts-sipp/alt-pjsua-uas-sdp-passthrough.xml |
Adds UAS SIPp scenario. |
tests/pjsua/scripts-sipp/alt-pjsua-uas-sdp-passthrough.py |
Configures the UAS integration test. |
tests/pjsua/scripts-sipp/alt-pjsua-uac-sdp-passthrough.xml |
Adds UAC SIPp scenario. |
tests/pjsua/scripts-sipp/alt-pjsua-uac-sdp-passthrough.py |
Configures the UAC integration test. |
tests/pjsua/runall.py |
Registers the integration tests. |
pjsip/src/pjsua-lib/pjsua_media.c |
Selects passthrough during media renegotiation. |
pjsip/src/pjsua-lib/pjsua_call.c |
Propagates the call flag and handles differing media counts. |
pjsip/src/pjsip-ua/sip_inv.c |
Selects passthrough negotiation for invite sessions. |
pjsip/include/pjsua-lib/pjsua.h |
Defines the public PJSUA call flag. |
pjsip/include/pjsip-ua/sip_inv.h |
Stores passthrough state on invite sessions. |
pjsip-apps/src/swig/symbols.i |
Exposes the flag to PJSUA2 bindings. |
pjsip-apps/src/pjsua/pjsua_app.c |
Applies the option to calls. |
pjsip-apps/src/pjsua/pjsua_app_config.c |
Parses and documents the CLI option. |
pjsip-apps/src/pjsua/pjsua_app_common.h |
Stores application configuration. |
pjsip-apps/src/pjsua/pjsua_app_cli.c |
Applies the option to CLI calls. |
pjmedia/src/test/sdp_neg_test.c |
Tests initial passthrough negotiation. |
pjmedia/src/pjmedia/sdp_neg.c |
Implements passthrough state promotion. |
pjmedia/include/pjmedia/sdp_neg.h |
Declares and documents the API. |
.github/workflows/ci-linux.yml |
Runs the SIPp tests in CI. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| PJ_DECL(pj_status_t) pjmedia_sdp_neg_negotiate_passthrough(pj_pool_t *pool, | ||
| pjmedia_sdp_neg *neg); |
| neg->active_local_sdp = neg->neg_local_sdp; | ||
| neg->active_remote_sdp = neg->neg_remote_sdp; |
| if (call->inv) { | ||
| call->inv->sdp_passthrough = | ||
| (call->opt.flag & PJSUA_CALL_SDP_PASSTHROUGH) != 0; | ||
| } |
|
Whoops, I have requested Copilot review. @nanangizz , please let @wosrediinanatour know whether to act on the Copilot review, or to ignore it first for now. |
|
Thanks Florian. Looking back over #2770, #2900, #3320, #3336, #4155, #4473, #4598, #4923 and #4938, it is clear these all serve one project, and #4938 already accepted the signalling-plane B2BUA direction. So this is not a pushback on the goal — but this PR adds permanent public API ( What I think is right in this PRThe I do think the API shape needs to change — see question 1 — but something along these lines belongs in pjmedia regardless of how the pjsua half is decided. The pjsua half is the open question. An observation that may make the pjsua half much smaller#5068 recently added, for T.38, exactly the primitive a signalling-plane B2BUA needs — if (!is_stream_media(remote_sdp->media[mi])) {
stop_media_stream(call, mi, PJ_FALSE);
close_call_med_tp(call_med); /* release pjsua's transport */
call_med->type = PJMEDIA_TYPE_UNKNOWN;
call_med->state = PJSUA_CALL_MEDIA_NONE;
call_med->dir = PJMEDIA_DIR_NONE;
if (local_sdp->media[mi]->desc.port != 0)
got_media = PJ_TRUE; /* don't drop the call */
continue;
}together with Because that path marks the slot
The only reason it does not already cover your case is that After that generalisation I think what would still be missing is:
Three questions before choosing the shape1. SDP involvement — I think we should design for the most demanding case. My working assumption is that you need to be able to parse, validate and modify the SDP — rewriting That assumption changes the primitive. "Forward the SDP untouched" is not quite what is needed; what is needed is: pjmedia stops rewriting the media content, but the offer/answer state machine and the RFC 3264 Concretely, that suggests a negotiator-level mode — set once at creation — under which the input paths ( Does that match what you need, or do you need something beyond parse / validate / modify? 2. Would a call-level "all media is app-managed" work for you, building on #5068's mechanism above, rather than a flag inside the SDP negotiator? 3. If you do modify the body, who owns Remaining scopeOne more thing that would help me settle the shape rather than keep deciding increments: after this PR, what else do you still need before you have a working signalling-plane B2BUA on PJSUA2? A rough list is fine, it does not have to be designed yet. Every PR so far has been individually reasonable, which makes each easy to approve — but I have never seen the shape of the whole, and I would rather agree on the destination once. On the Copilot reviewIt landed while I was writing this. Since the pjsua-lib part is still under discussion, please only act on the pjmedia comments for now — I would rather you did not churn on code whose shape may still change:
I have some further code-level findings on the patch; happy to post them once the direction is clearer, so they land on the version we actually want. Not blocking, and not asking you to redesign anything yet. I would just like the picture before we commit to public API. |
|
Thank you for your comment. I really appreciate it! I haven't been aware of #5068, which looks quite interesting. We have had different use cases and, hence, our requirements shifted over time. We are currently working on a B2BUA, where originating and terminating part are not in our system. Thus, the B2BUA should not interfere:
But - you are right - maybe we could weaken "untouched". It would be OK to have a modified o= line, if the rest is untouched (relaxed requirements). 1. SDP involvement — I think we should design for the most demanding case.:
Both, SDP offer and answer are not in our hand. So, from our perspective, SDP is not modified, but rather given.
If the o= line is the only line that is modified, then it is OK. 2. Would a call-level "all media is app-managed" work for you...
Yes, and no. Yes for "all media is app-managed". According to "flag inside the SDP negotiator", you wrote in 1)
Hence, no. 3. If you do modify the body, who owns
Due to our relaxed requirements, o= line could be modified by the stack. Remaining scope
I completely agree and appreciate it very much! An observation that may make the pjsua half much smaller The whole section is quite interesting and gives a big shift, I will have to analyze. But - the direction looks very sound.
This was always the preferred way for me.
Good to know.
But instead using a flag for the whole lifetime, as discussed in other sections?
Good to know. |
|
Thanks, that clears up a lot. Noted: pure relay, offer and answer both given, and On question 2 you are right and my wording was wrong — it is not either/or. Both are needed: call-level "all media is app-managed" on the pjsua side (no transports, no streams, no lock-codec re-INVITE), plus a negotiator-level mode so the body that actually goes on the wire is not rewritten. And yes to your follow-up on item 3: a mode set once at negotiator creation and valid for its whole lifetime, not a per-call variant of One thing I want to be explicit about: I would like this feature to be as generic as possible. You mentioned your requirements have shifted before, and if this lands in pjsua-lib it has to serve signalling-plane B2BUAs in general, not a single deployment. "Media content untouched, Two things still open:
|
I fully agree!
In the past, "Unsupported media type (PJMEDIA_EUNSUPMEDIATYPE)", but probably not anymore with current version.
Agreed.
I would say, with all discussed points in place, there would be nothing remaining regarding signalling-plane Signaling-only B2BUA (https://www.rfc-editor.org/info/rfc7092/#section-3.1.2). SDP-Modifying Signaling-only (https://www.rfc-editor.org/info/rfc7092/#section-3.1.3) is not in our scope, neither is a Proxy-B2BUA (https://www.rfc-editor.org/info/rfc7092/#section-3.1.1). Additionally, a 3rd-party call control is something which should be enabled as well (e.g. https://www.rfc-editor.org/info/rfc3725/#section-4.1). Note that a PR about Late SDP is on the way. I hope I haven't misunderstood your question. Note that a future app could need something else/additionally, to my knowledge not in a foreseeable future. |
|
Thanks — that is the picture I needed. §3.1.2, with §3.1.3 covered by the contract anyway, plus 3PCC on top: bounded enough for me. The shape I would like to aim for: pjmedia — a negotiator-lifetime mode rather than a second PJ_DECL(pj_status_t) pjmedia_sdp_neg_set_passthrough(pjmedia_sdp_neg *neg,
pj_bool_t passthrough);Under the mode, pjsip-ua — keep something like your pjsua — call-level "all media is app-managed" (building on #5068), app-supplied SDP as a first-class input, and no media transport creation for such calls. On the Late SDP PR: late offers look orthogonal to me — message shape rather than body content — and both directions already have some support ( Could you rework this PR toward that shape? No rush. I will hold my remaining code-level comments until then, so they land on the right version. |
|
Looks good to me! Thank you for the discussion and guidance!
I also see it almost orthogonal and more a PJSUA2/PJSUA thing (explicitly acknowledge a call with a SDP). The development is done by my colleagues - I would like to keep it separate to make things easier (if it makes sense, we will be able to merge anytime) .
Yes, I plan to do it. Probably, most parts will be done completely from scratch. |
5746ab3 to
4ddcc86
Compare
sauwming
left a comment
sauwming
left a comment
There was a problem hiding this comment.
I reviewed this at 4ddcc86 against @nanangizz's final agreed design. That design has four parts: passthrough is a negotiator mode set once at creation; app-supplied SDP is a first-class input for offers and answers; o= is managed by the stack; and the passthrough path reuses the existing negotiate bookkeeping instead of copying it.
Already in line with the design: pjmedia_sdp_neg_set_passthrough(); modify_local_offer2() covering re-offers; the second offer/answer round in the unit test; the call-level PJSUA_CALL_MEDIA_APP_MANAGED flag built on #5068; the account-level media_app_managed; and the flag rollback on a failed re-INVITE.
The inline comments cover the remaining gaps, including two regressions for existing, non-app-managed users (the async re-INVITE path and the rollback path). Two further points are outside the diff, so they are listed here:
-
Incoming re-offers still get a pjsua-generated answer (
pjsua_call_on_rx_offer(),pjsua_call.c:6378). For app-managed calls, the answer comes frompjsua_media_channel_create_sdp()with no transports, so every m-line is port 0 and the address is0.0.0.0(pjsua_media.c:3088). It is then set withpjsip_inv_set_sdp_answer(), and passthrough sends it verbatim. An UPDATE with SDP, or a re-INVITE withoutasync, is therefore answered with all media rejected. Apjsua_call_set_sdp()made fromon_call_rx_offeris overwritten. Under the agreed design, the app's SDP should be the answer here. -
The initial INVITE has no first-class SDP input (
pjsua_call_make_call(), aroundpjsua_call.c:1038; pjsua2Call::makeCall()ignoresCallOpParam::sdp). The UAC's app-managed INVITE carries the placeholder SDP unless the app patches it inon_call_sdp_created, which is the workaround the design was meant to replace. On the UAS side, when the flag first arrives atpjsua_call_answer2()(stock pjsua--sdp-passthroughwithout the account flag), the full pjsua-generated answer is locked in passthrough. That answer lists codecs that are not in the offer and RTP ports thatchannel_updatethen closes, so it is not a valid RFC 3264 answer.
Tests: all four SIPp scenarios run only on alt_pjsua --custom-sdp. None of them exercises pjsua_call_set_sdp(), reinvite3()/update3(), or the pjsua2 reinvite/update SDP paths. The "no transport created" property of the account-level test is not asserted; the script says it was checked by hand.
PR title/description: these still describe the earlier PJSUA_CALL_SDP_PASSTHROUGH design. Please update them to match the current app-managed shape.
| pjmedia_av_sync *av_sync; /**< Media stream synchronizer */ | ||
| }; | ||
|
|
||
| PJ_INLINE(pj_bool_t) pjsua_call_media_is_app_managed( |
There was a problem hiding this comment.
Regression for normal calls. Because this also returns true for call->offer_app_managed, the short async re-INVITE window from #5068 now disables every media line in pjsua_media_channel_init() (enabled = PJ_FALSE at pjsua_media.c:2868). Before this PR, that window only affected the 488 check.
Scenario: a normal (non-app-managed) call receives an audio re-INVITE, and the app sets async=TRUE in on_call_rx_reinvite. apply_call_setting() runs with offer_app_managed=TRUE, so the existing audio transport is marked PJSUA_MED_TP_DISABLED. After offer_app_managed is reset, channel_update goes back to the type-based is_stream_media() and tries to update audio on a disabled transport. The audio is answered with port 0, or the stream fails. T.38 offers that mix audio and image hit the same path.
Suggest keeping offer_app_managed limited to the 488 decision, and using only call->opt.flag for the media-channel decision.
| pj_status_t status; | ||
|
|
||
| cancel_created_local_offer(call, old_state); | ||
| status = apply_call_setting_impl(call, old_opt, NULL, PJ_FALSE); |
There was a problem hiding this comment.
Regression for every reinvite/update caller. apply_call_setting_impl() with old_opt reinitialises the media channel of a CONFIRMED call. If old_opt still carries PJSUA_CALL_REINIT_MEDIA, which is not cleared after a successful reinvite/update, it also runs pjsua_media_channel_deinit().
Scenario: reinvite2(PJSUA_CALL_REINIT_MEDIA) succeeds, and the flag stays in call->opt. A later update2() or reinvite2() fails in pjsip_inv_update/reinvite/send_msg. The rollback then tears down all active streams and transports of the live call and creates new ones on ports that were never offered. Media goes dead with no renegotiation.
The rollback should restore only the flag/negotiator state it changed (the app-managed bit and the passthrough mode), not re-run the whole call-setting apply.
|
|
||
| app_config_init_call_setting(&opt); | ||
|
|
||
| pjsua_call_setting_default(&opt); |
There was a problem hiding this comment.
This replaces app_config_init_call_setting(&opt) with the older inline init, which undoes #5291. Auto-answered calls lose PJSUA_CALL_SET_MEDIA_DIR/media_dir, so pjsua --auto-answer=200 with a media-dir setting answers sendrecv.
Separately, the new flag is lost for command-line URI calls (pjsua --sdp-passthrough sip:uri) and for 'm' in the legacy UI. Those paths re-run app_config_init_call_setting(), which overwrites the flag that app_init() added.
Suggest keeping app_config_init_call_setting() here and setting PJSUA_CALL_MEDIA_APP_MANAGED inside it.
| * the negotiator directly, since it may already have been created. | ||
| */ | ||
| if (call->inv) { | ||
| call->inv->sdp_passthrough = |
There was a problem hiding this comment.
The agreed design sets the passthrough mode once, when the negotiator is created, and keeps it for the negotiator's whole lifetime. Here it is applied to an existing negotiator on every call-setting change. As a result, a reinvite/update can switch it on mid-call, and the rollback can switch it off again. The sdp_neg.h doc of pjmedia_sdp_neg_set_passthrough() itself warns against toggling.
Scenario: a normal pjsua-managed call is re-INVITEd with PJSUA_CALL_MEDIA_APP_MANAGED. The negotiator switches mode while keeping its existing PT maps and active SDP, is_stream_media() tears down the running streams, and PT bookkeeping may no longer match what was on the wire.
Suggest rejecting changes to PJSUA_CALL_MEDIA_APP_MANAGED after the call is created (return PJ_EINVALIDOP, or ignore the change) instead of re-syncing the negotiator.
| pjsip_dlg_dec_lock(dlg); | ||
| return status; | ||
| } | ||
| pjmedia_sdp_neg_set_passthrough(inv->neg, inv->sdp_passthrough); |
There was a problem hiding this comment.
This call (and the matching one in pjsip_inv_create_uas() at line 1945) has no effect. inv was just zero-allocated in the same function, so inv->sdp_passthrough is always PJ_FALSE here. The sip_inv.h doc ("set by the application before the session/negotiator is created") describes something that cannot be done.
As a result, pjsua has to patch the flag in after creation, in on_make_call_med_tp_complete (line 577), on_incoming (line 2333) and apply_call_setting (lines 814/853). A plain pjsip-ua user cannot get passthrough on the initial negotiator at all.
To match the agreed set-at-creation mode, suggest a creation-time option, for example a new pjsip_inv_option bit passed through the existing options argument of pjsip_inv_create_uac()/pjsip_inv_create_uas(). That would also remove the four copies in pjsua.
| /* Must have remote offer. */ | ||
| PJ_ASSERT_RETURN(neg->neg_remote_sdp, PJ_EBUG); | ||
|
|
||
| if (neg->passthrough) { |
There was a problem hiding this comment.
The agreed design keeps negotiate's state bookkeeping in one path, so that two copies don't have to be kept in sync by hand. This early-return block copies the version-bump logic and all the tail bookkeeping (state=DONE, answer_was_remote, and clearing initial_sdp_tmp/neg_*_sdp/has_remote_answer). Any later fix to negotiate()'s tail would have to be made twice.
Suggest swapping only process_answer()/create_answer()/assign_pt_and_update_map() for clones under if (neg->passthrough), then falling through to the shared code. The same applies to the copied tail in modify_local_offer2() (line 460).
| * net_type/addr_type/addr forcing, the media-line padding/ | ||
| * reordering, and the dynamic payload type remap - the | ||
| * application's re-offer is sent as supplied. | ||
| */ |
There was a problem hiding this comment.
In passthrough, the stack keeps bumping the o= version but no longer forces user/id/addr, both here and in answers (set_local_answer, line 714). A relayed body therefore gets a mixed origin: the far end's username and sess-id, combined with this leg's version counter.
Scenario: the B2BUA relays a re-offer whose far-end o= has a different sess-id or username from earlier bodies on this dialog. On this leg, o= changes sess-id mid-dialog while the version goes up by only one, which violates RFC 3264 §8. The peer may treat it as a new session.
The agreed contract was "o= managed by the stack". Suggest keeping the user/id/addr forcing from the previous local SDP, as in non-passthrough mode, and passing everything below o= through as-is. @nanangizz, please confirm this is the intended reading.
| } | ||
| call->call_hold_type = pjsua_var.acc[acc_id].cfg.call_hold_type; | ||
|
|
||
| /* Apply the account's "media_app_managed" default (if configured) as |
There was a problem hiding this comment.
Nit: several new comments are much longer than the project's coding style expects (keep comments minimal and brief). Examples are this 12-line block, the 16-line sdp_passthrough field doc in sip_inv.h:641, and the multi-paragraph comments in apply_call_setting_impl(), check_lock_codec() and pjsua_media_channel_init(). Many of them restate the code or the discussion history. Please trim them to a line or two where a comment is needed.
4ddcc86 to
9f07ee7
Compare
|
I tried to incorporate your feedback, @sauwming. I hope that everything is covered (I am sorry, if I have missed something). There is a new unit test in |
wosrediinanatour
changed the title
sauwming
left a comment
sauwming
left a comment
There was a problem hiding this comment.
This follow-up review covers 9f07ee7. As in my previous review of 4ddcc86, I'm checking against @nanangizz's final agreed design.
Fixed since the last review. Thanks!
- The passthrough mode is now set once when the negotiator is created, via
PJSIP_INV_SDP_PASSTHROUGH. - The copied
negotiate()bookkeeping is gone. - A failed reinvite/update now restores only
call->opt. - The async re-INVITE regression is fixed.
- #5291 is no longer undone in
pjsua_app.c. o=user/id/addr are now forced from the previous local SDP.make_call2/reinvite3/update3/set_sdpnow make app SDP a first-class input.
Verdict: not ready to merge yet. The new app-SDP paths still have several blocking issues. See the inline comments.
Blocking:
reject_received_offer()answers the wrong transaction. This can crash an app-managed outgoing (UAC) call, or tear down an incoming call during early dialog.- Incoming app-managed INVITEs are rejected with 488 in
verify_request()unless the app patches the SDP inon_call_sdp_created. - Hold, IP-change and NULL-SDP reinvite/update on app-managed calls send the placeholder SDP (port 0,
0.0.0.0). - App-supplied offers skip the
med_prov_cnt/PJSUA_MAX_CALL_MEDIAhandling, which can trigger an assert. - App-supplied SDP is not validated before it reaches the negotiator's
PJ_ASSERT_RETURN.
Should fix:
6. PJSUA2 has no way to answer an incoming re-offer on an app-managed call.
7. The new SDP inputs are accepted on normal (non-app-managed) calls.
8. There are no tests for the first-class app-SDP APIs.
Minor: long comments, and redundant state.
Outside the diff: the PR description still describes the old PJSUA_CALL_SDP_PASSTHROUGH design. The title has been updated, but please update the description to match the current app-managed shape.
| return PJ_SUCCESS; | ||
| } | ||
|
|
||
| static void reject_received_offer(pjsua_call *call) |
There was a problem hiding this comment.
Blocking. pjsua_call_answer2() answers inv->invite_tsx using the incoming-call state. That is the initial INVITE's transaction, not the transaction of the re-offer being rejected.
- (a) An app-managed outgoing call receives a re-INVITE, and the app sets no answer.
invite_tsxis now the re-INVITE's UAS transaction andlast_answeris NULL, soanswer2queues the answer withpj_list_push_back(&async_call.call_var.inc_call.answers). For an outgoing call that list was never initialised: it overlaysout_call.msg_data/local_sdp. The result is a NULL dereference, or corruption ofmsg_data. - (b) An app-managed incoming call in early dialog (180 sent) receives an UPDATE with SDP, which is common with preconditions.
answer2sends 488 to the initial INVITE, and the whole call is torn down.
This rejection shouldn't be needed. sip_inv.c already responds 488 when on_rx_offer leaves no answer set. Suggest removing reject_received_offer() and simply not setting an answer.
| } | ||
| } else { | ||
| } else if (!use_tmp_sdp) { | ||
| status = pjsua_media_channel_create_sdp(call->index, |
There was a problem hiding this comment.
Blocking. For app-managed calls, this still builds pjsua's placeholder answer (every m-line port 0), stores it as the local answer, and passes it to pjsip_inv_verify_request3(), which negotiates in normal mode. create_answer() finds no active media and returns PJMEDIA_SDPNEG_ENOMEDIA.
As a result, every incoming INVITE on an account with media_app_managed (or stock pjsua with --sdp-passthrough) is rejected with 488 before on_incoming_call fires, unless the app patches the SDP in on_call_sdp_created. The agreed design was meant to remove that workaround. A PJSUA2 app can therefore never answer with CallOpParam::sdp.
Even when verification passes, the placeholder is already stored as the local answer. The new REMOTE_OFFER guard in pjsua_call_answer2() then never triggers, and a plain answer(200) sends the all-port-0 body.
For app-managed calls, suggest skipping answer generation here and verifying without a local SDP. The SIPp tests miss this because --custom-sdp patches on_call_sdp_created.
| } else if (!sdp && | ||
| (call->opt.flag & PJSUA_CALL_NO_SDP_OFFER) == 0) | ||
| { | ||
| status = pjsua_media_channel_create_sdp(call->index, |
There was a problem hiding this comment.
Blocking. On an app-managed call, a NULL SDP here still generates the pjsua placeholder offer, which passthrough then sends verbatim. This covers reinvite2/update2, reinvite3/update3 with NULL, and set_hold via create_sdp_of_call_hold() above.
Scenario: after an IP change, pjsua_acc.c:5864/5876 calls pjsua_call_update()/reinvite() on every call. The same happens if the app calls setHold() or reinvite() without CallOpParam::sdp. The B2BUA leg then offers every m-line at port 0 with c=0.0.0.0, and the far end drops all media.
For app-managed calls, a NULL SDP should reuse the active local SDP (pjmedia_sdp_neg_send_local_offer()), or be rejected with PJ_EINVALIDOP.
| @@ -4677,75 +4708,88 @@ pj_status_t pjsua_media_channel_update(pjsua_call_id call_id, | |||
| /* Reset audio_idx first */ | |||
There was a problem hiding this comment.
Blocking. App-supplied offers (make_call2, reinvite3, update3, set_sdp) skip pjsua_media_channel_create_sdp(). That function is the only place that raises med_prov_cnt to the SDP's m-line count and enforces PJSUA_MAX_CALL_MEDIA.
Scenario: a B2BUA relays audio+video (or two audio m-lines) through make_call2() with default settings (aud_cnt=1, vid_cnt=0). med_prov_cnt is 1 while local_sdp->media_count is 2, so the pj_assert(call->med_prov_cnt >= local_sdp->media_count) just above (line 4706) aborts in debug builds. In release builds, m-lines beyond med_cnt are silently ignored.
Suggest applying the same count adjustment and max check to app-supplied SDP, and returning an error if the SDP exceeds PJSUA_MAX_CALL_MEDIA.
| dlg->pool, msg_data); | ||
| } | ||
| if (sdp) { | ||
| call->async_call.call_var.out_call.local_sdp = |
There was a problem hiding this comment.
Blocking. The app SDP is cloned without validation. pjmedia_sdp_neg_create_w_local_offer() (reached from pjsip_inv_create_uac(), and from pjsua_call_set_sdp() when there is no negotiator yet) checks it with PJ_ASSERT_RETURN(pjmedia_sdp_validate(...) == PJ_SUCCESS).
A relayed far-end SDP that parses but fails strict validation aborts the process in debug builds. Examples: no session-level or media-level c= line, or a dynamic PT without rtpmap. Please validate with pjmedia_sdp_validate() in make_call2/reinvite3/update3/set_sdp, and return the error to the caller.
| @@ -810,17 +835,27 @@ void Call::setHold(const CallOpParam &prm) PJSUA2_THROW(Error) | |||
|
|
|||
| void Call::reinvite(const CallOpParam &prm) PJSUA2_THROW(Error) | |||
There was a problem hiding this comment.
Should fix. PJSUA2 has no way to supply an answer to an incoming re-offer synchronously. pjsua_call_set_sdp() has no wrapper, and OnCallRxOfferParam carries no SDP, yet app-managed calls now reject any re-offer that has no app answer.
A PJSUA2 B2BUA with mediaAppManaged=true that receives an UPDATE with SDP, or a re-INVITE it doesn't take async in onCallRxReinvite, therefore rejects every such offer with 488. On outgoing legs this also reaches the reject_received_offer() issue above. Suggest adding an SDP field to OnCallRxOfferParam (or a Call::setSdp() wrapper).
| * Set/replace the call's local SDP directly, bypassing pjsua's own SDP | ||
| * generation (on_call_sdp_created()). | ||
| */ | ||
| PJ_DEF(pj_status_t) pjsua_call_set_sdp(pjsua_call_id call_id, |
There was a problem hiding this comment.
Should fix. This API, like make_call2/reinvite3/update3 and an app answer in on_call_rx_offer, is accepted on calls that are not app-managed. On those calls pjsua keeps its own transports and streams and negotiates in normal mode.
Scenario: make_call2(sdp) without PJSUA_CALL_MEDIA_APP_MANAGED. The offer's ports and crypto/ICE attributes don't match call_med->tp, but apply_med_update() still runs pjmedia_transport_media_start() and pjmedia_stream_info_from_sdp() against them. The result is one-way or dead media, or an SRTP/ICE start failure that drops the call.
Suggest requiring the flag (PJ_EINVALIDOP otherwise), or at least documenting the risk clearly.
| return 0; | ||
| } | ||
|
|
||
| static int test_app_managed_mode_immutability(void) |
There was a problem hiding this comment.
Should fix. This is the only new pjsua unit test, and it covers flag immutability and answer rollback. Nothing tests make_call2, set_sdp, reinvite3/update3, a UAS answer with app SDP, or an incoming UPDATE/re-offer on an app-managed call. All the SIPp scenarios depend on --custom-sdp patching on_call_sdp_created.
That is why the blocking issues in this review (the 488 on incoming INVITEs, the placeholder offer on hold/IP change, the UPDATE/UAC reject crash, and the med_prov_cnt assert) pass CI undetected. Please add tests that drive the first-class SDP APIs without the on_call_sdp_created workaround.
| * to the application. Video is included regardless of PJMEDIA_HAS_VIDEO, so a | ||
| * build without video keeps reporting an offered video line as unsupported | ||
| * media instead of silently treating it as media the application owns. | ||
| /* Is the given SDP media line one that pjsua itself implements a stream |
There was a problem hiding this comment.
Minor: the long comments flagged in the previous review are still present. Examples:
- 11 lines here above
is_stream_media() - 13 in
pjsua_media_channel_update() - 6 in
check_lock_codec() - 5 in
pjsua_media_channel_init() - 4 in
pjsua_media_channel_create_sdp()
Several restate the code or the design discussion. The coding style asks for minimal, brief comments; please trim these to a line where a comment is needed.
| pj_str_t siprec_metadata; /**< SIPREC metadata update | ||
| pending notification, | ||
| internal. */ | ||
| pj_bool_t sdp_passthrough; /**< SDP is app-managed. */ |
There was a problem hiding this comment.
Minor: this public field just repeats inv->options & PJSIP_INV_SDP_PASSTHROUGH. The bit is set in two places and this field is read in six, so they must be kept in sync. Suggest dropping the field and checking the option bit directly.
Related redundancies:
- The
set_answerpath inon_rx_offersets the app answer a second time withpjsip_inv_set_sdp_answer(neg_local). That clones it twice and overwritesinitial_sdp_tmpagain. restore_call_setting()is a one-line wrapper.
|
Thx @sauwming for the good review. This time I addressed each with an own commit, so that it is easier to review. The commits should be squashed eventually. Note that I added two additional fixes (commits). I have also updated the PR's description. |
6b95cff to
48928e8
Compare
75585ac to
93523c4
Compare
sauwming
left a comment
sauwming
left a comment
There was a problem hiding this comment.
This follow-up review covers 93523c4. As before, I'm checking against @nanangizz's final agreed design.
Fixed since the 09-30 review. Thanks!
reject_received_offer()is gone.verify_request()no longer rejects incoming app-managed INVITEs.- Hold is refused on app-managed calls, and reinvite/update with a NULL SDP reuse the active SDP.
- App SDP is validated, and its m-line count is checked.
- PJSUA2 can answer a re-offer through
OnCallRxOfferParam::answer. - The new SDP APIs have tests.
- The callee-confirmation race fix in the latest commit looks good.
Verdict: not ready to merge yet. See the inline comments. Two blocking items are on lines outside the diff, so they are listed here:
-
The 200 answering an incoming INVITE with no SDP still carries a placeholder offer (
on_answer_call_med_tp_complete(),pjsua_call.c:3107). An app-managed account receives an INVITE without SDP (the 3PCC flow), and the app callspjsua_call_answer(200)without callingpjsua_call_set_sdp()first.inv->negis NULL, so the #1526 path runspjsua_media_channel_init(). This function then builds an SDP with every m-line at port 0 andc=0.0.0.0, and it is sent as the offer. The new guard inpjsua_call_answer2()only rejects the REMOTE_OFFER state, not a NULL negotiator. The PR description says missing app SDP fails explicitly, so this should returnPJ_EINVALIDOPinstead. -
An incoming Replaces on an app-managed call gets a 200 with no SDP answer (
process_incoming_call_replace(),pjsua_call.c:1323).verify_request()no longer sets a local answer for app-managed calls, so the negotiator is still in REMOTE_OFFER whenpjsip_inv_answer(200)runs. The 200 goes out with no body, which breaks offer/answer, and the replaced call is hung up anyway, so both legs end up broken. The app has no hook to supply the answer. Either give it one, or reject Replaces on app-managed calls.
The branch also needs squashing before merge: it has 13 commits, including a fixup!.
| switch (pjmedia_sdp_neg_get_state(inv->neg)) { | ||
| case PJMEDIA_SDP_NEG_STATE_LOCAL_OFFER: | ||
| PJ_LOG(4,(inv->obj_name, | ||
| "pjsip_inv_update: using pending local offer")); |
There was a problem hiding this comment.
Blocking: regression for all pjsip users. On master, pjsip_inv_update() returns PJ_EINVALIDOP for any negotiator state other than DONE. With this change, in LOCAL_OFFER it overwrites the caller's offer with the pending local offer and sends that instead. Nothing checks whether that offer is already outstanding in another transaction.
Scenario: on an ordinary call, a re-INVITE carrying an offer is outstanding, or the initial INVITE got a 180 with no SDP. The app calls pjsua_call_update() (or a pjsip user calls pjsip_inv_update() with a new offer). An UPDATE then goes out repeating the offer that is still pending. That breaks RFC 3311: the far end answers 491/500, or two answers race. The newly generated SDP and transports are silently discarded.
Please restore the previous PJ_EINVALIDOP behaviour for LOCAL_OFFER, or limit the new path to the specific app-managed case it was added for.
|
|
||
| /* Create offer */ | ||
| if ((call->opt.flag & PJSUA_CALL_NO_SDP_OFFER) == 0) { | ||
| offer = call->async_call.call_var.out_call.local_sdp; |
There was a problem hiding this comment.
Blocking. On an app-managed call with no SDP supplied, this still falls through to pjsua_media_channel_create_sdp(). With no transports, the INVITE then goes out with every m-line at port 0 and o=/c= set to 0.0.0.0.
This can come from pjsua_call_make_call(), or from make_call2() with sdp == NULL. It is also what stock pjsua --sdp-passthrough does in cmd_make_single_call. The only way around it is patching the SDP in on_call_sdp_created, which is the workaround the agreed design was meant to remove.
Please fail with PJ_EINVALIDOP when the call is app-managed and no SDP was supplied (unless PJSUA_CALL_NO_SDP_OFFER is set).
| } | ||
|
|
||
| if (call->med_prov_cnt < local_sdp->media_count) | ||
| call->med_prov_cnt = local_sdp->media_count; |
There was a problem hiding this comment.
Blocking. This raises med_prov_cnt for every call, but the extra media_prov[] slots were never initialised. make_call2() with an SDP skips pjsua_media_channel_init(), which is what copies call->media into media_prov.
Scenario: pjsua_call_make_call2() with a 2-m-line app SDP. media_prov[1] is still zeroed from reset_call(), and it is later memcpy'd into call->media[]. That leaves call_med->call = NULL (dereferenced by pjsua_set_media_tp_state()), idx = 0, and strm.a.conf_slot = 0 instead of PJSUA_INVALID_ID (slot 0 is the sound device). ssrc and tp_auto_del are also lost.
For ordinary calls, the pj_assert below can no longer fire, so a local SDP with more m-lines than provisional media is silently accepted.
Suggest initialising the new slots the way pjsua_media_channel_init() does, and limiting the bump to app-managed calls.
| !neg->has_remote_answer, | ||
| PJMEDIA_SDPNEG_EINSTATE); | ||
|
|
||
| neg->initial_sdp = neg->initial_sdp_tmp; |
There was a problem hiding this comment.
Blocking: new public API. initial_sdp is restored from initial_sdp_tmp unconditionally. When WAIT_NEGO was entered through pjmedia_sdp_neg_create_w_remote_offer(initial, remote), initial_sdp_tmp is NULL, so the caller-supplied initial SDP is lost.
Scenario: a pjsip user creates a UAS invite session with local_sdp (the negotiator starts in WAIT_NEGO with initial_sdp set and initial_sdp_tmp NULL), then calls cancel_local_answer(). A later set_local_answer(NULL), or pjsip_inv_answer() without SDP, hits PJ_ASSERT_RETURN(neg->initial_sdp, PJMEDIA_SDPNEG_ENOINITIAL).
The case where set_local_answer() was called with no earlier initial SDP is fine: going back to NULL is the correct revert there. The create_w_remote_offer() path needs to keep its initial SDP, for example by recording at creation that there is nothing to restore.
|
|
||
| if (maudcnt + mvidcnt + mtxtcnt == 0 && | ||
| !(call->offer_app_managed && sdp_has_active_media(rem_sdp))) | ||
| !(pjsua_call_media_is_app_managed(call) && |
There was a problem hiding this comment.
Should fix. App-managed calls still reach this 488 when the remote offer has no active m-lines. An offer whose m-lines are all port 0 (for example, the far end removing all streams) is rejected before the app can relay it.
This is the "validation releases the call" case raised earlier in the design discussion. For app-managed calls, suggest skipping this check entirely and leaving the decision to the app.
| goto on_return; | ||
| } | ||
|
|
||
| if (pjsua_call_media_is_app_managed(call)) { |
There was a problem hiding this comment.
Should fix. For app-managed calls, returning NULL here means an incoming offerless re-INVITE is always answered with the old active local SDP: sip_inv falls back to pjmedia_sdp_neg_send_local_offer(). The app is never consulted.
That breaks RFC 3725 third-party call control, which is within this PR's stated scope. The controller sends a re-INVITE without SDP and expects the other leg's offer in the 200, but gets the stale SDP. pjsua_call_set_sdp() can't help: the negotiator is DONE and the 200 is built synchronously.
Suggest giving the app a way to supply the offer here, for example an SDP out-param on on_call_rx_reinvite/on_call_tx_offer, or async handling.
| * | ||
| * @return PJ_SUCCESS on success, or the appropriate error code. | ||
| */ | ||
| PJ_DECL(pj_status_t) pjsua_call_make_call2( |
There was a problem hiding this comment.
Should fix before the API is public. pjsua_call_make_call2() (7 positional arguments), pjsua_call_reinvite3() and pjsua_call_update3() each extend an existing signature by one more positional parameter.
pjproject prefers param structs for extensibility. Otherwise the next extension, such as the late-SDP options planned for the follow-up PR, would need make_call3/reinvite4/update4 on top of the existing 2/3 versions. Suggest a param struct (with a _default() initialiser) carrying sdp and room to grow. This is much cheaper to change now than after release.
There was a problem hiding this comment.
Fully agree - like in PJSUA2.
|
Addressed each comment via one commit. Next: squashing together. |
f5b06a3 to
a7e67c9
Compare
|
Latest change tries to fix race condition for tests on MacOS. PTAL, @sauwming |
sauwming
left a comment
sauwming
left a comment
There was a problem hiding this comment.
This follow-up review covers a7e67c9. As before, I'm checking against @nanangizz's final agreed design.
Fixed since the 10-02 review. Thanks!
- The
pjsip_inv_update()LOCAL_OFFER path is now limited to passthrough. make_call2/reinvite3/update3now take a param struct (pjsua_call_op_param).- The
media_provpadding is limited to app-managed calls and copied from initialised slots. cancel_local_answer()keeps the initial SDP.- The 488 check is skipped for app-managed offers, and incoming Replaces is rejected.
- The commits are squashed.
- The
pjsua_call_test-1273 race is fixed, and CI is green.
Verdict: not ready to merge yet. Two blocking items remain, plus several should-fix items (inline). Two of them are on lines outside the diff, so they are listed here:
-
Blocking: PJSUA2 crashes on an offerless re-INVITE (
Endpoint::on_call_rx_reinvite(),endpoint.cpp:2022). The new offerless re-INVITE path for app-managed calls passesoffer == NULLtoon_call_rx_reinvite(pjsua_call.c:6552), but the PJSUA2 trampoline callsprm.offer.fromPj(*offer)unconditionally. A PJSUA2 app withmediaAppManaged=truethat receives a re-INVITE without SDP (the RFC 3725 3PCC flow this PR targets) dereferences NULL. Please guard it, leavingprm.offerempty, and document that the offer may be NULL in bothon_call_rx_reinviteandOnCallRxReinviteParam. -
Should fix: a rejected re-offer is still answered with 200 (
pjsua_call_on_rx_offer(),pjsua_call.c:6422). If the app callspjsua_call_set_sdp()insideon_call_rx_offerand then sets*codeto a non-200 value, this path returns without cancelling the local answer. The negotiator stays in WAIT_NEGO, andsip_invbuilds a 200 with that answer, so the offer the app wanted to reject is accepted. Please callpjmedia_sdp_neg_cancel_local_answer()here, as theapply_call_setting()failure path already does.
| */ | ||
| if (!call->med_ch_cb && | ||
| (call->opt_inited || (code==183 || code/100==2)) && | ||
| if (!call->med_ch_cb && |
There was a problem hiding this comment.
Blocking. The "Application SDP required" guard just above covers only 2xx. For an app-managed incoming INVITE with no SDP, a 183 (or any 1xx when opt is passed, so opt_inited is set) still enters this #1526 block. on_answer_call_med_tp_complete() then builds the port-0 / c=0.0.0.0 placeholder SDP and sets it as the local offer with pjsip_inv_set_local_sdp().
Scenario: app-managed account, INVITE without SDP, and the app calls pjsua_call_answer(183) or answer2(180, &opt). The 183 carries the placeholder. A later answer_with_sdp(200, real_sdp) fails with EINSTATE because the negotiator is already in LOCAL_OFFER, and a plain answer(200) passes the guard and sends the placeholder offer.
Suggest skipping this block entirely for app-managed calls, and returning PJ_EINVALIDOP for 183 as well as 2xx when no app SDP has been set.
Nit: this line and the next were re-indented to 5 spaces. Please restore the 4-space indentation, which also removes an unrelated diff hunk.
| goto on_error; | ||
| } | ||
| PJ_LOG(4,(inv->obj_name, | ||
| "pjsip_inv_update: using pending local offer")); |
There was a problem hiding this comment.
Should fix. In passthrough mode, this replaces the caller's offer with the pending neg-local offer. Nothing checks whether that offer is already outstanding in another transaction.
Scenario: on an app-managed confirmed call, pjsua_call_reinvite3() sends offer A, and the negotiator is in LOCAL_OFFER waiting for the 200. The app then calls pjsua_call_update(). create_unsupplied_offer returns the active SDP, this path picks the pending offer A, and an UPDATE carrying A goes out while the re-INVITE is still in progress. The far end answers 491/500, or two answers race (RFC 3311 §5.1).
Suggest returning PJ_EINVALIDOP (or PJSIP_EPENDINGTX) when an INVITE/UPDATE carrying the offer is still pending, and using the pending offer only when it has not been sent yet.
| return status; | ||
| } | ||
|
|
||
| status = pjsip_inv_set_local_sdp(call->inv, sdp); |
There was a problem hiding this comment.
Should fix: behaviour change for ordinary calls. pjsua_call_answer_with_sdp() now uses pjsip_inv_set_local_sdp() for every call, not just app-managed ones. On an ordinary call answering an INVITE with no SDP, this creates the negotiator with the app's SDP as the offer. answer2 then skips the #1526 media-channel init, so no transports or media_prov slots are set up.
Scenario: a non-app-managed call receives an INVITE without SDP, and the app calls pjsua_call_answer_with_sdp(200, sdp), which the updated docs now describe as supported. When the ACK brings the answer, pjsua_media_channel_update() hits pj_assert(med_prov_cnt >= local_sdp->media_count) with med_prov_cnt == 0. A release build ends up with med_cnt = 0 and no media.
Suggest keeping the previous behaviour for non-app-managed calls, or running the media-channel init first.
| replaced_call = (pjsua_call*) | ||
| replaced_dlg->mod_data[pjsua_var.mod.id]; | ||
| if (replaced_call->opt.flag & PJSUA_CALL_MEDIA_APP_MANAGED) { | ||
| PJ_LOG(3,(THIS_FILE, "Rejecting Replaces for app-managed " |
There was a problem hiding this comment.
Should fix. This checks only the replaced call's PJSUA_CALL_MEDIA_APP_MANAGED flag. The new call becomes app-managed through the account default (apply_acc_media_app_managed_default()), whatever the replaced call is. The check also runs after on_call_replace_request(2) has already told the app that the replace was accepted.
Scenario: media_app_managed is switched on with pjsua_acc_modify() (which this PR supports) while a normal call exists, and an INVITE/Replaces for that call arrives. The replaced call has no flag, so the request isn't rejected. The new call is app-managed, verify_request sets no answer, and process_incoming_call_replace() sends a 200 with no SDP answer. That is the same broken-both-legs outcome as before.
Suggest checking whether the new call will be app-managed (account default or replaced-call flag), and doing it before on_call_replace_request.
| if (pjsua_var.ua_cfg.cb.on_call_rx_reinvite) { | ||
| (*pjsua_var.ua_cfg.cb.on_call_rx_reinvite)( | ||
| call->index, NULL, rdata, | ||
| NULL, &async, &code, &opt); |
There was a problem hiding this comment.
Should fix. &opt is passed to on_call_rx_reinvite, but whatever the app changes there is discarded: call->opt is never updated from opt. On the offer path, pjsua_call_on_rx_offer() does apply it via apply_call_setting(call, &opt, offer). An app that, for example, changes the media direction for an offerless re-INVITE has the change silently dropped.
Also, pjsua_call_cleanup_flag(&call->opt) above clears the transient flags directly on call->opt, before the app has decided anything.
Suggest applying opt (or at least documenting that it is ignored on this path), and cleaning up a local copy instead of call->opt.
| @@ -2597,12 +2584,8 @@ pj_status_t pjsua_media_channel_init(pjsua_call_id call_id, | |||
| mtxtidx, &mtxtcnt, &mtottxtcnt); | |||
|
|
|||
| if (maudcnt + mvidcnt + mtxtcnt == 0 && | |||
There was a problem hiding this comment.
Should fix: behaviour change for ordinary calls. Dropping the sdp_has_active_media() condition widens the #5068 T.38 exception. pjsua_call_media_is_app_managed() is also true for offer_app_managed, so any async re-offer on an ordinary call now skips the "no supported media → 488" check. That includes offers whose m-lines are all port 0, or that have no media at all.
Before this PR, such an offer was rejected with 488 here. Now apply_call_setting() succeeds and the call proceeds with an offer that neither pjsua nor the app may be able to answer meaningfully.
Suggest limiting the skip to call->opt.flag & PJSUA_CALL_MEDIA_APP_MANAGED and keeping the active-media check for the offer_app_managed case.
| #define TEST_TIMEOUT_MSEC 8000 | ||
| #define TEST_POLL_MSEC 50 | ||
| #define MEDIA_SETTLE_MSEC 500 | ||
| #define IOQUEUE_CLEANUP_MARGIN_MSEC 200 |
There was a problem hiding this comment.
Minor:
- 24 of the lines changed by the macro substitution now exceed 80 columns, mostly
wait_until(..., TEST_TIMEOUT_MSEC)(e.g. lines 1421, 1498, 1518, 1676). Please re-wrap them. - Replacing the timeouts in tests this PR doesn't otherwise touch (SIPREC, hold, rtcp-mux, dir-narrowing) adds unrelated churn. Consider limiting it to the new tests, or moving it into a separate commit.
ce799e7 to
89f5cce
Compare
|
Fixed crash in tests. Fixed the branch-added tests while leaving pjsua_call_test(void) and the inherited test scenarios unchanged.
PTAL |
sauwming
left a comment
sauwming
left a comment
There was a problem hiding this comment.
This follow-up review covers 89f5cce. As before, I'm checking against @nanangizz's final agreed design.
Fixed since the 10-05 review. Thanks!
- No placeholder SDP in 183 responses.
- The pending-offer UPDATE is fixed.
- The Replaces check now uses the account default and runs before
on_call_replace_request. optis applied on offerless re-INVITEs.- PJSUA2 no longer crashes on a NULL offer.
- A rejected re-offer now cancels the local answer.
- The T.38 488 check is narrowed back.
- The
put_frame()crash is fixed (media_provinitialised inreset_call()), and CI is green.
Verdict: not ready to merge yet. One blocking regression remains for ordinary calls (inline, pjsua_call.c:6477), plus several should-fix items. Two of them are on lines outside the diff, so they are listed here:
-
App-managed outgoing calls still open the sound device (
pjsua_call_make_call2(),pjsua_call.c:1087). Thepjsua_set_snd_dev()condition has noPJSUA_CALL_MEDIA_APP_MANAGEDcheck, andaud_cntdefaults to 1, so app-managed calls still open the sound device (or start the null-sound clock) even though pjsua never creates a stream for them. A signalling-only B2BUA on a server without an audio device failsmake_call2(). This is also the path that led to the earlierput_frame()crash. Please skip this block for app-managed calls. -
Memory grows over long app-managed calls (
pjsip_inv_set_local_sdp(),sip_inv.c:3223). On a confirmed call,pjsip_inv_set_local_sdp()callsmodify_local_offer2()oninv->pool, which lives as long as the session. That is pre-existing, but this PR makes it a per-re-offer path:pjsua_call_set_sdp()andanswer_with_sdp()on confirmed calls go through it for every relayed re-offer, or every answered offerless re-INVITE. A long B2BUA call then grows memory without bound.pjsip_inv_reinvite()andpjsip_inv_update()useinv->pool_provfor the same operation; please do the same here.
| goto on_return; | ||
| } | ||
|
|
||
| if (async) { |
There was a problem hiding this comment.
Blocking: regression for ordinary calls. This async early return now comes before pjsua_media_channel_create_sdp() (line 6489) for every call. On master, create_sdp ran first and the async return came after it. As a result, ordinary (non-app-managed) async re-INVITEs no longer run pjmedia_transport_encode_sdp(), and on_call_sdp_created no longer fires.
Scenario: an ordinary SRTP or ICE call; the peer sends a re-INVITE, and the app sets async = PJ_TRUE in on_call_rx_reinvite, then answers later with pjsua_call_answer_with_sdp(). On master, the SRTP answerer picked its crypto policy and ICE set its remote-offer state during encode_sdp. Now that step is skipped, so media_start in pjsua_media_channel_update() runs on stale or missing transport state: SRTP keys don't update, or ICE start fails. Apps that capture the generated answer in on_call_sdp_created also stop receiving it.
Please apply the early return only to PJSUA_CALL_MEDIA_APP_MANAGED calls. The test comment at pjsua_call_test.c:767 ("PJSUA still builds an answer") will then be accurate again.
| return status; | ||
| } | ||
|
|
||
| status = pjsip_inv_set_local_sdp(call->inv, sdp); |
There was a problem hiding this comment.
Should fix (still open from the 10-05 review). pjsip_inv_set_local_sdp() is used here for ordinary calls too, which changes existing behaviour.
- A non-app-managed call receives an INVITE without SDP, and the app calls
answer_with_sdp(200, sdp). A negotiator is created in LOCAL_OFFER, soanswer2skips the Assertion when receiving INVITE with no SDP and video is deactivated (thanks Bogdan Krakowski for the report) #1526 media-channel init andmed_prov_cntstays 0. When the ACK brings the answer,pjsua_media_channel_update()hits themed_prov_cntassert, or in a release build the call has no media. - If
answer2fails after this point (e.g.optaddsMEDIA_APP_MANAGEDandapply_call_settingrejects it), the rollback below usespjmedia_sdp_neg_cancel_offer(). That moves a negotiator that had no active SDP to DONE. A laterpjsua_call_answer(200)then skips both the Assertion when receiving INVITE with no SDP and video is deactivated (thanks Bogdan Krakowski for the report) #1526 path and the app-managed guard, and sends a 200 with no offer.
Suggest keeping the previous behaviour for non-app-managed calls. For the rollback, return the negotiator to its prior state (NULL/no negotiator) instead of calling cancel_offer.
| pjsip_method_cmp(&tsx->method, &pjsip_update_method)==0) | ||
| { | ||
| if (tsx->state == PJSIP_TSX_STATE_CALLING) { | ||
| inv->update_tsx = tsx; |
There was a problem hiding this comment.
Should fix: affects all calls. update_tsx is set for every outgoing UPDATE, including ones without SDP, and pjsip_inv_reinvite() now refuses whenever it is set. It is also cleared only after the state handler and the on_tsx_state callbacks have run.
Scenario: on an ordinary call, a session-timer refresh UPDATE (no SDP), or a pjsua_call_update() with NO_SDP_OFFER, is outstanding, and the app calls pjsua_call_set_hold() or pjsua_call_reinvite(). Both now return PJ_EINVALIDOP, which they did not before. RFC 3311 only forbids this while an UPDATE carries an offer. Likewise, an app that sends a re-INVITE from on_call_tsx_state/on_call_media_state when the UPDATE's 200 arrives gets PJ_EINVALIDOP, because update_tsx is still set at that point.
Please track the UPDATE only when it carries an SDP offer, and clear it before the callbacks run. Nit: the 5-line comment above can be cut down to one line.
| */ | ||
| static pj_bool_t inv_offer_tsx_pending(const pjsip_inv_session *inv) | ||
| { | ||
| return inv->invite_tsx != NULL || inv->update_tsx != NULL; |
There was a problem hiding this comment.
Should fix. Any invite_tsx is counted as carrying the pending offer, including the UAS initial INVITE transaction during the early dialog.
Scenario: an app-managed UAS in the early dialog (after a reliable 183 with SDP) calls pjsua_call_set_sdp() and then pjsua_call_update() with no SDP. That UPDATE is the only way to re-offer before the call is confirmed. invite_tsx is the incoming INVITE, which does not carry our offer, yet the UPDATE is refused with PJ_EINVALIDOP. The negotiator then stays in LOCAL_OFFER, and the peer's own re-offers get 491.
Please count only UAC INVITE transactions here.
| if (code != PJSIP_SC_OK) | ||
| async = PJ_FALSE; | ||
|
|
||
| if (!async && code == PJSIP_SC_OK) { |
There was a problem hiding this comment.
Should fix. On an app-managed call, an offerless re-INVITE gets 488 by default unless the app implements on_call_rx_reinvite and goes async. Without this handler, sip_inv would re-offer the active local SDP, which is what the outgoing NULL-SDP path already does.
Scenario: a peer sends an offerless re-INVITE (an RFC 3725 3PCC flow, or an offerless session refresh) to an app-managed call whose app does not go async. The peer gets 488 instead of a 200 re-offering the active SDP, so the refresh fails and the peer may tear down the call. Separately, the pjsua.h/call.hpp docs now say the callback fires for offerless re-INVITEs in general, but ordinary calls never get it with offer == NULL.
Suggest defaulting to re-offering the active local SDP when the app doesn't go async, and making the docs say this applies to app-managed calls only.
Also, in the apply_call_setting() failure path just below, PJSIP_ERRNO_TO_SIP_STATUS turns a non-SIP status such as PJ_ETOOMANY into a 599 response. Please map such errors to a proper SIP code (e.g. 488 or 500). The 6-line comment there can also be cut down to one line.
| } | ||
| } | ||
|
|
||
| if ((code == PJSIP_SC_PROGRESS || code/100 == 2) && |
There was a problem hiding this comment.
Should fix. This guard rejects code 183 when the negotiator is NULL or in REMOTE_OFFER, so an app-managed call cannot send a 183 without SDP.
Scenario: a signalling-only B2BUA relays a downstream 183 Session Progress that has no body (legal for an unreliable 183) via pjsua_call_answer(183), and gets PJ_EINVALIDOP.
The goal was to stop the placeholder SDP going out in a 183. That is better done by skipping the #1526 SDP generation for app-managed calls, so the 183 simply carries no body. The guard itself can then go back to covering 2xx only, or 2xx plus a reliable 183.
| puts (" --custom-sdp=STR Replace generated SDP with this string."); | ||
| puts (" Use \\r\\n or \\n as line separators. The full SDP is replaced as-is."); | ||
| #endif | ||
| puts (" --sdp-passthrough Keep and forward the local/remote SDP as-is during"); |
There was a problem hiding this comment.
Should fix. --sdp-passthrough is available in standard pjsua builds (PJSUA_MEDIA_HAS_PJMEDIA=1), but those builds have no way to supply app SDP. With the option on, MEDIA_APP_MANAGED is set on every call and on the account default, so every outgoing call fails with "Application SDP offer required" and auto-answer 200 fails with "Application SDP required". The usage text, meanwhile, promises that the SDP is forwarded as-is.
Please put the option under #if !PJSUA_MEDIA_HAS_PJMEDIA, like --custom-sdp just above. It is also not written back by write_settings().
1a25683 to
4908c00
Compare
|
PTAL! I incorporated your comments. (May I ask how you are doing the reviews? They are really good - and I would like to improve my own "internal" reviews....) |
|
I'm using Claude, then manually check the result |
4908c00 to
63c1cda
Compare
|
Fixed three additional findings: • Outgoing sample calls now supply custom SDP when app-managed mode comes from either the account or call settings, consistently across CLI, legacy, and startup paths. |
Enable signalling-plane B2BUAs to relay SDP while preserving valid offer/answer state and stack-managed origin semantics. API changes and extensions: - Add an application-managed mode and local-answer cancellation API to the SDP negotiator, plus a PJSIP invite creation option. - Add PJSUA/PJSUA2 app-managed account and call settings, extensible operation parameters, and first-class SDP input for calls, answers, re-INVITEs, and UPDATEs. Major architectural changes: - Fix application-managed mode when the invite session creates its negotiator and preserve stack ownership of SDP o= identity and versioning. - Bypass PJSUA media transports for app-managed calls and require explicit application SDP where negotiation needs it.
63c1cda to
46a3d6f
Compare
sauwming
left a comment
sauwming
left a comment
There was a problem hiding this comment.
This follow-up review covers 46a3d6f. As before, I'm checking against @nanangizz's final agreed design.
Fixed since the 10-06 review. Thanks!
- The async early return is now limited to app-managed calls.
- The sound device is no longer opened for app-managed calls.
- The
answer_with_sdp()behaviour change and its rollback are fixed. update_tsxnow tracks only UPDATEs that carry SDP, and is cleared before the callbacks run.- The 183 guard is now limited to reliable 183.
--sdp-passthroughis now gated on!PJSUA_MEDIA_HAS_PJMEDIA.pjsip_inv_set_local_sdp()now allocates frompool_prov.- Several of the earlier unresolved threads look addressed in the code. Please resolve the ones you consider done.
CI: GitHub Actions hasn't run on this head yet (no workflow runs exist for 46a3d6f), and the Bitrise pr job failed. Please check the Bitrise failure.
Verdict: not ready to merge yet. Four blocking items remain (inline). One of them is a regression for every pjsip user (sip_inv.c:3895).
Should fix, outside the diff:
- App-managed calls still get phantom media slots (
pjsua_media_channel_init(),pjsua_media.c:2672). When the media channel is re-initialised with no remote SDP (reinvite3,update3, an offerless re-INVITE), app-managed calls still run theaud_cnt/vid_cnt/txt_cnt"add new media" logic.channel_updatetypes the app-managed slots as UNKNOWN, somtotaudcntis 0 and an extra audio slot is added each time. With a relayed SDP ofPJSUA_MAX_CALL_MEDIAm-lines and the defaultaud_cnt = 1,reinit_med_cntcomes to max+1.reinvite3/update3then fail withPJ_ETOOMANY, and an offerless re-INVITE gets 488. Smaller calls get a spurious extramedia_provslot. App-managed calls should skip this media-count adjustment entirely.
Minor:
pjsua_call_reinvite3()andpjsua_call_update3()repeat the same pending-offer check, NO_SDP_OFFER check,validate_managed_sdp()call and four rollback sites.sip_inv.calso repeatspjmedia_sdp_neg_set_passthrough(inv->neg, (inv->options & PJSIP_INV_SDP_PASSTHROUGH) != 0)at 7 negotiator-creation sites. A smallinv_create_neg_*()helper insip_inv.cand a shared prepare/rollback helper inpjsua_call.cwould stop a future creation site (e.g. in the planned late-SDP PR) from silently missing passthrough.- Please update the Teluu copyright end year to 2026 in the modified files that carry one (e.g.
Copyright (C) 2008-2011 Teluu Inc.inpjsua_call.c,sip_inv.c,sdp_neg.c,pjsua.h,call.hpp,endpoint.cpp).
| * still be carrying an unanswered offer that this re-INVITE would | ||
| * otherwise duplicate/overlap (RFC 3311 Section 5.1). | ||
| */ | ||
| if (inv_offer_tsx_pending(inv)) |
There was a problem hiding this comment.
Blocking: regression for every pjsip user. On master, pjsip_inv_reinvite() refuses whenever inv->invite_tsx is set. inv_offer_tsx_pending() now ignores a UAS invite_tsx, so a new re-INVITE is allowed while an incoming re-INVITE is still being answered.
Scenario: a confirmed ordinary call receives a re-INVITE that the app answers async (on_call_rx_reinvite with async=TRUE). Meanwhile pjsua_call_reinvite() runs, triggered by the app, the IP-change handler, lock-codec or the session timer. pjsip_inv_reinvite() now continues: in REMOTE_OFFER it sets the active SDP as the local answer to the pending offer, and pjsip_inv_invite() sends a second INVITE, overwriting inv->invite_tsx. The app's later pjsua_call_answer2() fails with "no incoming INVITE", the incoming re-INVITE never gets a final response, and the INVITEs overlap (RFC 3261 §14.1, 491). In debug builds, pj_assert(inv->invite_tsx==NULL || tsx==inv->invite_tsx) in inv_on_state_confirmed fires.
The 10-06 comment asked for UAC-only counting in pjsip_inv_update() only. Since the helper is shared, please restore the plain if (inv->invite_tsx != NULL) return PJ_EINVALIDOP; here, and keep the UAC-only rule in pjsip_inv_update().
| */ | ||
| static pj_bool_t inv_offer_tsx_pending(const pjsip_inv_session *inv) | ||
| { | ||
| return (inv->invite_tsx && inv->invite_tsx->role == PJSIP_ROLE_UAC) || |
There was a problem hiding this comment.
Blocking. This helper only looks at UAC INVITE and UPDATE transactions. It misses an offer that is still outstanding in a UAS response: a 2xx waiting for ACK, or a reliable 18x waiting for PRACK. As a result, the passthrough LOCAL_OFFER branch of pjsip_inv_update() resends that pending offer.
Scenario: an app-managed call receives an offerless re-INVITE and answers it async with pjsua_call_answer_with_sdp(), so the 200 carries our offer and the negotiator stays in LOCAL_OFFER until the ACK arrives. If pjsua_call_update() runs before the ACK, create_unsupplied_offer() returns the active SDP, pjsip_inv_update() takes the passthrough LOCAL_OFFER path, and this helper returns false because invite_tsx is UAS. An UPDATE then goes out carrying the same offer the ACK is about to answer. That is exactly the overlap RFC 3311 §5.1 forbids, and which the comment says this check prevents.
Please also treat a UAS invite_tsx that sent a response carrying the offer (still waiting for ACK/PRACK) as pending in the pjsip_inv_update() path.
| if ((code/100 == 2 || | ||
| (code == PJSIP_SC_PROGRESS && | ||
| (call->inv->options & PJSIP_INV_REQUIRE_100REL))) && | ||
| (call->opt.flag & PJSUA_CALL_MEDIA_APP_MANAGED) && |
There was a problem hiding this comment.
Blocking. This guard rejects only when the negotiator is NULL or in REMOTE_OFFER. After an async offerless re-INVITE on an app-managed call, the negotiator is DONE, so answering without SDP is allowed and the 200 goes out with no offer.
Scenario: an app-managed call receives a re-INVITE without SDP. The app sets async=TRUE in on_call_rx_reinvite, then calls pjsua_call_answer2(call, NULL, 200). The guard passes because the state is DONE, process_answer() finds no LOCAL_OFFER, and the 200 is sent with no body. That violates RFC 3261 §14.2 / RFC 3264: the ACK cannot carry an answer, and 3PCC (RFC 3725) flows stall. The documented "re-offer the active local SDP" default only applies on the synchronous path.
Please make it fail here as well (an offerless re-INVITE pending and the negotiator not in LOCAL_OFFER), or apply the same re-offer-active-SDP default on the async path.
| allow_asym, &active); | ||
|
|
||
| if (neg->passthrough) { | ||
| active = pjmedia_sdp_session_clone(pool, neg->neg_local_sdp); |
There was a problem hiding this comment.
Blocking. In passthrough mode, any answer is accepted (here, and in the local-answer branch) without checking that its m-line count matches the offer's. active_local_sdp and active_remote_sdp can therefore end up with different media_count.
Scenario: the far end answers a relayed 2-m-line offer with 1 m-line, or the app answers a 3-m-line offer with 2. Negotiation succeeds, a wrong-sized answer goes on the wire (RFC 3264 §6), and both active SDPs are stored with different counts. check_lock_codec() already had to be patched for this. Any pjsip user of PJSIP_INV_SDP_PASSTHROUGH, or any pjsua code that walks local->media[i]/remote->media[i] together, can read past the shorter array.
Please add a cheap media_count check against the offer (fail with PJMEDIA_SDPNEG_EMISMEDIA) in both passthrough branches. validate_app_sdp() doesn't check this either.
| pjmedia_av_sync *av_sync; /**< Media stream synchronizer */ | ||
| }; | ||
|
|
||
| PJ_INLINE(pj_bool_t) pjsua_call_media_is_app_managed( |
There was a problem hiding this comment.
Should fix (from an earlier, still-open thread). This still ORs in offer_app_managed, the temporary per-offer T.38 exception from #5068, mixing it with the per-call app-managed mode. It is harmless today only because that flag is cleared before is_stream_media() and verify_request() run.
Any future path that calls channel_update(), verify_request() or is_stream_media() while offer_app_managed is set (inside apply_call_setting() during an async re-offer) would treat an ordinary call as fully app-managed and drop all its streams. Please use only the PJSUA_CALL_MEDIA_APP_MANAGED flag here, and keep offer_app_managed limited to the 488 decision.
| status = pjmedia_sdp_parse(pool, dup_sdp.ptr, dup_sdp.slen, | ||
| &answer); | ||
| if (status == PJ_SUCCESS) | ||
| status = pjsua_call_set_sdp(call_id, answer); |
There was a problem hiding this comment.
Should fix. On an ordinary (non-app-managed) call, pjsua_call_set_sdp() returns PJ_EINVALIDOP, and the code below then forces statusCode to 488. The OnCallRxOfferParam::answer doc presents it as a general way to answer synchronously.
So a PJSUA2 app that fills prm.answer.wholeSdp in onCallRxOffer for a normal call gets every re-INVITE/UPDATE offer rejected with 488, with only a level-1 log. Please either document answer as app-managed only, or ignore it (with a warning) for normal calls instead of rejecting the offer.
| #if !PJSUA_MEDIA_HAS_PJMEDIA | ||
| if ((app_config.auto_answer == 183 || | ||
| app_config.auto_answer/100 == 2) && | ||
| (opt.flag & PJSUA_CALL_MEDIA_APP_MANAGED) && |
There was a problem hiding this comment.
Should fix. The auto-answer custom-SDP path checks only the call-setting flag, which is set only by --sdp-passthrough. It doesn't check the account's media_app_managed default, unlike app_make_call(), which checks both.
Scenario: alt_pjsua --acc-media-app-managed --custom-sdp=... --auto-answer=200 without --sdp-passthrough. opt lacks the flag, so plain pjsua_call_answer2() runs, apply_call_setting() ORs in the account flag, and the answer fails with "Application SDP required". Outgoing calls work, but incoming auto-answer fails. Please check pjsua_var.acc[acc_id].cfg.media_app_managed here too.
Motivation and context
This extension is intended for the following use cases:
Non-goals:
After the discussion below, we settled for
API changes and extensions
pjmedia_sdp_neg_set_passthrough()andPJSIP_INV_SDP_PASSTHROUGH.PJSUA_CALL_MEDIA_APP_MANAGEDand the account-levelmedia_app_manageddefault, including the corresponding PJSUA2mediaAppManagedconfiguration.pjsua_call_make_call2()pjsua_call_reinvite3()pjsua_call_update3()pjsua_call_set_sdp()CallOpParam::sdpto outgoing calls, re-INVITEs, andUPDATEs.
OnCallRxOfferParam::answer.Architectural changes
every negotiator created during that session.
o=. The stack retains ownership of theo=identity and version.active-SDP promotion, version handling, pool management, and cleanup.
RTCP, ICE, or STUN processing.
media-count limits.
all-rejected placeholder SDP.
can be sent with either re-INVITE or UPDATE.
behavior.
Test coverage
pjmedia/src/test/sdp_neg_test.ccovers real-world passthrough offers andanswers, re-offers, re-answers,
o=handling, version updates, contentpreservation, and state cleanup.
pjsip/src/test/pjsua_call_test.ccovers:make_call2(),set_sdp(),reinvite3(), andupdate3().set_sdp()followed by UPDATE.tests/pjsua/scripts-sipp/cover UAC and UASpassthrough, account-managed incoming calls without PJSUA media transports,
early UPDATE, and multi-codec re-INVITE behavior.
tests/pjsua/runall.pyand Linux CI.